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
Elaborate on FencedFrameConfig #67
Elaborate on FencedFrameConfig #67
Changes from 1 commit
253c468
7a8c42c
6c88d0e
652b055
eb4b785
fe49697
d4314ce
b0230c8
82d464d
95e5cda
be5bb1f
166bff4
0d24a58
06ee8f5
ac16848
98aa564
214e86b
e86d78a
4a2d08e
3269b83
cb828dc
d417e29
218fdd1
51b819e
767a341
f388f39
fb0c9d9
82cba61
eff0092
f8463bb
e9cd240
a69e4d0
ba85f30
1118d60
900d922
36f45db
6a0a43b
1fbcbd4
640b841
208b004
9755313
656609f
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.
I put this as a TODO here that we should follow-up on quickly, but feel free to address this before we land if you want. Basically I think inside this section we should introduce a new member called "fenced frame config instance" or whatever is similar to what we have on
DocumentLoader
where this can live. That way here and elsewhere we have a concrete thing to initialize, assign, and address wherever we need.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.
Agree. Where should it live? If we didn't have to deal with urn iframes I think it could just be attached to the fenced content navigable or whatever, but we probably have to attach it to the root.
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.
That reminds me: this part about fenced frame config mappings is probably wrong for urn iframes (though it is wrong in the same way that our implementation is "wrong"):
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 fenced navigable is indeed a traversable navigable, per https://wicg.github.io/fenced-frame/#fenced-navigable-container-fenced-navigable. That's the root, essentially
content::Page
/content::FencedFrame
so the member should probably live there, and iframes can climb the tree to reference it. That's basically what the implementation does, 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.
The config mapping does go in the Page in the implementation, but that is semi-broken for urn iframes because it means that you can load configs that shouldn't really be in scope (e.g., you can in principle take a single urn and then create arbitrarily deep nested urn iframes using that urn).
The config instance goes in the root FTN in the implementation because it's more important for that degeneracy not to exist there. But when urn iframes are gone, it would go in the FrameTree. (Maybe the config mapping should also go there.)
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.
Ah yeah... hmm OK I think for now we should make it a normal member of "navigable" (basically FTN) with some strong Note saying that once URN iframes are gone this will exclusively live on "traversable navigable".