Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: main
Are you sure you want to change the base?
Introduce user context subscriptions #836
Changes from 4 commits
5887e58
1bc84b5
58d2241
1a68a27
5a8c14c
4823337
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.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 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?
In Chrome, the user context stays there if all browsing contexts in it are removed.
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 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.
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.
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.