-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 browsing context groups #4350
Conversation
(This does not fix all issues identified in #4198, but it does introduce the necessary infrastructure to fix them and should be strictly better than the status quo. In particular, we will still need to define agent allocation. I'd suggest we file a follow-up for that.) |
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 all the groundwork has made this a nicely self-contained change; cool. All my feedback seems to be "editorial", but I am fairly concerned about the clarity implications of the remove/discard distinction and the potential null-ness of group, so looking forward to your answers there.
This is ready for another/final look. This should also remove "and the user agent itself has a strong reference to its top-level browsing contexts" as that's now more clearly defined, but waiting for #4368 to land first to make rebasing easier and less likely to introduce mistakes. |
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.
Will give this the approve bit, because I don't want to block over another weekend. But, I'd like to do one more review if possible and our timelines line up, both with regard to my latest question, and to allow you to remove the text you mention now that #4368 has landed.
source
Outdated
<span>browsing context</span> is <span data-x="a browsing context is | ||
discarded">discarded</span>. <a href="https://github.com/whatwg/html/issues/4361">Issue | ||
#4361</a> sketches a setup that could improve this situation.</p> | ||
</li> |
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.
Is there a reason we don't just return false if either group is null?
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.
Well, the responsible browsing context might also be null, right? But in general, they need to remain in the group they were in when they were created. So it seems better to explicitly not deal with something becoming null until we've addressed putting them in the appropriate groups better at which point it'll all follow from first principles.
The grouping concepts unit of related browsing contexts and unit of similar-origin browsing contexts were not accurate, due to browsing contexts being able to hold a sequence of (potentially cross-site) documents. Fixes #4198.
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.
LGTM again. I'm content with the XXX because as we discovered in https://freenode.logbot.info/whatwg/20190222#c2016315, discarded BCs are fairly complicated, and the right answer is not obvious. I look forward to fixing that.
* "Unit of related browsing contexts" was removed from the HTML spec in 2019 (whatwg/html#4350) * "Report task end time" moved to LONG-ANIMATION-FRAMES and has been renamed "Record task end time"
* "Unit of related browsing contexts" was removed from the HTML spec in 2019 (whatwg/html#4350) * "Report task end time" moved to LONG-ANIMATION-FRAMES and has been renamed "Record task end time" Closes: #108
The grouping concepts unit of related browsing contexts and unit of similar-origin browsing contexts were not accurate, due to browsing contexts being able to hold a sequence of (potentially cross-site) documents.
Fixes #4198.
/browsers.html ( diff )
/infrastructure.html ( diff )
/webappapis.html ( diff )
/window-object.html ( diff )