-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Spec] Allow setting automatic beacon data from cross-origin subframes. #203
Conversation
spec.bs
Outdated
@@ -2173,6 +2171,11 @@ event|event-level beacon=] when a fenced frame initiates a successful [=navigate | |||
/fenced-frame/automatic-beacon-cross-origin-no-opt-in.https.html | |||
/fenced-frame/automatic-beacon-use-ancestor-data.https.html | |||
</wpt> | |||
|
|||
TODO: Add the following tests: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is blocking merge right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. This isn't ready for review yet. I just uploaded this so I could see a visual diff.
I'm also running into issues with the tests not being found. I tried running bikeshed update --skip-manifest
and got this error:
FATAL ERROR: Bikeshed currently only knows how to handle WPT v8 manifest data, but got v9. Please report this to the maintainer!
I think I have the latest release installed (4.2.8) so adding any new WPTs to the spec might be blocked until that gets patched.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now, thanks! Assuming it compiles after the Bikeshed fix you're waiting on goes in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to defer to @VergeA's review here since the annals of the cross-origin automatic beacon work is slightly beyond me at this point, but from looking at the PR nothing major sticks out.
This PR modifies the automatic beacon algorithms to support data being set from frames that are cross-origin to the fenced frame config's mapped URL. More specifically, this makes the following modifications:
setReportEventDataForAutomaticBeacons()
is modified to allow data to be set from cross-origin subframes, but only if the data'scrossOriginExposed
parameter is set totrue
.snapshot automatic beacon mapping
type is introduced that contains 2 mappings. The first mapping is the same as the existingautomatic beacon data map
. The second mapping only contains automatic beacon data whosecrossOriginExposed
parameter is set to true. This is needed since, as part of this change, cross-origin subframes that trigger automatic beacons will now use the first cross-origin exposed data it finds up the frame tree, versus the first data it finds regardless of whether it is cross-origin exposed or not (which it wouldn't be able to use).attempt to send an automatic beacon
algorithm now checks either the "all data mapping" or the "cross-origin only mapping" based on whether the automatic beacon initiator is cross-origin to the fenced frame config's mapped URL. This will ensure that if an ancestor has usable data, it won't accidentally grab data from a closer ancestor that can't be used. This also has a side effect of letting us remove the|should send beacon with data|
variable, as the same behavior is now obtained by simply checking the cross-origin only data mapping.get the automatic beacon data mapping to use
is renamed and modified to read data into both the regular mapping and the cross-origin only mapping.Preview | Diff