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
Define top level origin of a document #4966
Define top level origin of a document #4966
Changes from 34 commits
16041a9
62eb0c6
b4981d0
f484159
e7f6f3e
da502cf
3b5fa11
52e2d54
f83487d
8d095e0
890c4d2
3bceaf6
7b38330
78fc264
fbf9e59
2c35783
d464974
aa0e97f
e135536
50ebf05
d239858
7f63f1e
5915bb0
318b1f1
9d4d898
1a22388
ab45aff
fcb7d6a
1451bcf
0d3ca3a
a2b7b8f
067bba7
2a84963
ba3f160
89e1fe2
bcdd0e9
7fe83d1
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.
Same here, it seems using the container document would be sufficient and require less going up the tree (though it might end up with more words I suppose).
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.
A question on how does "browsing context's top-level browsing context" work? If it's such that every browsing context has a reference to its top-level browsing context then we are not really going up the tree here, but just accessing that reference? Searching other places that invoke "browsing context's top-level browsing context", I see examples like 1, 2, 3, etc.
I am not sure how container document would be better, since container document will lead to the parent and we'll have to continue to iterate till the last parent, 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.
I agree; I don't see why container document would be better. Top-level browsing context is defined straightforwardly in a way that bottoms out in container document; it's a nice abstraction on top that saves us some typing. (Explicitly: TLBC uses ancestor which uses child browsing context which uses container document.) I don't see why we would avoid using that abstraction when it gives us exactly what we want.
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 think grabbing the active document of the top-level browsing context is the problematic aspect here. It might well be changing due to race conditions. Container document could become null I suppose, but that seems safer than a different document.
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 don't see how a race condition could be involved. This step is atomic. If you're worried about things spontaneously changing during a single step, both approaches seem equally problematic.
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.
It's currently atomic because the spec is rather blind to site isolation. It also seems nicer to me if top-level origin works recursively.