Skip to content
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

Add clearOriginJoinedAdInterestGroup to spec.bs #844

Merged

Conversation

MattMenke2
Copy link
Contributor

@MattMenke2 MattMenke2 commented Oct 5, 2023

This adds the explainer changes from explainer PR #829 / issue #475 to the spec.

Note that I have no idea how to preview this before creating a pull request, so there are likely bugs that I'll need to fix.


Preview | Diff

This adds the explainer changes from explainer PR WICG#829 / issue  WICG#475 to the spec.
Some cleanups - more likely to follow.
Fix error?
@MattMenke2
Copy link
Contributor Author

Ok, I think this is good for review now. I'll do another pass tomorrow, just in case, but happy with what I have.

@qingxinwu qingxinwu added the spec Relates to the spec label Oct 6, 2023
Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically LGTM % some nits! Nice work

spec.bs Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % one comment about queueing tasks

spec.bs Outdated Show resolved Hide resolved
spec.bs Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great % two small questions

spec.bs Show resolved Hide resolved
spec.bs Outdated
<h3 id="clearoriginjoinedAdInterestGroups">clearOriginJoinedAdInterestGroups()</h3>
{{Window/navigator}}.{{Navigator/clearOriginJoinedAdInterestGroups()}} removes a user from
[=interest groups=] whose [=interest group/joining origin=] is the current top level page's
origin.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if this is called in a fenced frame? Why does "top level page" mean there? Basically I'm trying to see if we can link to one of these:

so we can make this more rigorous.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When run in a fenced frame, it behaves exactly like leaveAdInterestGroup() does when passed a non-empty group: It throws. Note that I exactly copied the verbiage for that, so if one is wrong, they both are. I assume the throwing is covered by the 'not allowed to use the "join-ad-interest-group" policy-controlled feature' bit, but could be wrong.

I copied "top level page's origin" from the definition of "joining origin" ("The top level page origin from where the interest group was joined.") - reusing terminology used when defining joining origin when mentioning joining origin seems reasonable to me. If that terminology is wrong, then that definition needs to be updated as well. The doc repeatedly refers to "top-level origin", which may be the term we want? I've replaced it here, but left joining origin's definition alone.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. With a new mode of fenced frames being introduced soon-ish that will allow arbitrary permission policies to be enabled inside of a fenced frame (I think), we should figure out what this API should do in that context outside of the usual permission policy restrictions. Basically I'm just trying to figure out if the behavior reached beyond the fenced frame and into the outermost main frame to grab the "top-level" origin, or if it is scoped to the fenced frame.

If you could point me to the part of the spec where we run the comparison between the "all interest group joining origins" and "the top level page's origin" (you know, to determine whether the user should be removed from the IG or not) then I think that'd help me answer this definitively.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The text for setting joining origin in joinAdInterestGroup is: "Set interestGroup’s joining origin to this's relevant settings object's top-level origin."

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK thanks, so the top-level origin is scoped to a fenced frame here, and does not transcend the boundary (i.e., does not "jump" the fence).

In that case, I think the test we should use here is:

...removes a user from [=interest groups=] whose [=interest group/joining origin=]
is the associated {{Navigator}}'s [=relevant settings object=]'s
[=environment/top-level origin=]

(Which should link to https://html.spec.whatwg.org/multipage/webappapis.html#relevant-settings-object and https://html.spec.whatwg.org/multipage/webappapis.html#concept-environment-top-level-origin respectively, of course)

Does that make sense / sound good?

Copy link
Contributor Author

@MattMenke2 MattMenke2 Oct 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, done.

Unless we're letting these fenced frames set 1P cookies for the fenced frame's origin, I suspect this behavior is not what we want, long term, since this is much akin to setting 1P cookies for the embedded frame (albeit 1P cookies that can only be accessed in an auction - using the top-level-page's joining origin is more like partitioned cookies), but don't think this PR is the place to iron that out.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's interesting. Let me just cc @shivanigithub just for extra exposure, but I agree that it shouldn't have a bearing on this PR specifically.

Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, I guess I made these comments but forgot to submit my review the other day. Sorry

spec.bs Show resolved Hide resolved
spec.bs Outdated
<h3 id="clearoriginjoinedAdInterestGroups">clearOriginJoinedAdInterestGroups()</h3>
{{Window/navigator}}.{{Navigator/clearOriginJoinedAdInterestGroups()}} removes a user from
[=interest groups=] whose [=interest group/joining origin=] is the current top level page's
origin.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. With a new mode of fenced frames being introduced soon-ish that will allow arbitrary permission policies to be enabled inside of a fenced frame (I think), we should figure out what this API should do in that context outside of the usual permission policy restrictions. Basically I'm just trying to figure out if the behavior reached beyond the fenced frame and into the outermost main frame to grab the "top-level" origin, or if it is scoped to the fenced frame.

If you could point me to the part of the spec where we run the comparison between the "all interest group joining origins" and "the top level page's origin" (you know, to determine whether the user should be removed from the IG or not) then I think that'd help me answer this definitively.

Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just need to fix up the merge conflicts and then I think we're all set.

@MattMenke2
Copy link
Contributor Author

Conflicts resolved.

@JensenPaul JensenPaul merged commit dd1e801 into WICG:main Oct 26, 2023
2 checks passed
github-actions bot added a commit that referenced this pull request Oct 26, 2023
SHA: dd1e801
Reason: push, by JensenPaul

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to morlovich/turtledove that referenced this pull request Oct 27, 2023
SHA: dd1e801
Reason: push, by morlovich

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec Relates to the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants