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

Introduce user context subscriptions #836

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

OrKoN
Copy link
Contributor

@OrKoN OrKoN commented Dec 20, 2024

This feature will allow scoping events per user context to get events for all newly created browsing contexts within a user context.


Preview | Diff

@OrKoN OrKoN force-pushed the orkon/user-context-subscriptions branch 4 times, most recently from 35c8ebc to 2476be6 Compare December 20, 2024 15:49
@OrKoN OrKoN marked this pull request as ready for review December 20, 2024 15:52
@OrKoN OrKoN requested a review from sadym-chromium December 20, 2024 15:52
@OrKoN OrKoN force-pushed the orkon/user-context-subscriptions branch from 2476be6 to 5887e58 Compare December 20, 2024 16:00
Copy link
Member

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

Just a few minor comments, otherwise this looks good, thanks!

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved

<div algorithm>

To <dfn export>remove user context subscriptions</dfn>:
Copy link
Member

Choose a reason for hiding this comment

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

So, this does create undefined behaviour, because it makes it observable whether a specific user context was removed or not, which is implementation defined (I guess technically that was already the case since it affects what's in storage). It isn't a blocker here, but I wonder if we should define that any user context created via createUserContext actually is removed if it goes from 1 to 0 browsing contexts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure I get why this creates undefined behavior. I added this algorithm to define a clean-up step to avoid accumulating subscriptions if the client does not unsubscribe (unsubscribe requests with the ID of removed subscriptions can still be issued). I think it is not observable to the local end. Could you please clarify?

It isn't a blocker here, but I wonder if we should define that any user context created via createUserContext actually is removed if it goes from 1 to 0 browsing contexts.

In Chrome, the user context stays there if all browsing contexts in it are removed.

Copy link
Member

Choose a reason for hiding this comment

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

I guess it's actually not externally visible whether or not this cleanup happened, so it's fine.

The problem I'm thinking of is actually already an issue. Say you create a user context, create a tab in that user context, then close the tab. Now an implementation is permitted to remove the user context. But whether or not you do that is visible if you then try to create another new tab in that user context.

It kind of makes sense for gecko because you can delete the user context in the browser UI. So if someone actually interacts with the browser and does that all bets are off. But maybe the spec should be clearer that it's not expected that user contexts (or their associated storage) are removed without user interaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the same situation can happen in Chrome if the user closes a user context via the UI. I think it makes sense to clarify that user contexts should not get removed randomly by the implementation, but removing them as the result of a direct user action is okay.

OrKoN and others added 2 commits December 20, 2024 18:39
Co-authored-by: jgraham <[email protected]>
Co-authored-by: jgraham <[email protected]>
@OrKoN OrKoN force-pushed the orkon/user-context-subscriptions branch from f4ad643 to 58d2241 Compare December 20, 2024 17:41
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
Co-authored-by: jgraham <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants