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
Performance control of embedded content #911
base: main
Are you sure you want to change the base?
Performance control of embedded content #911
Changes from 6 commits
9918ead
e8fe860
360c2c5
db0d5ff
57ee232
d7ee3d0
bcad7e9
b9110f2
2878ba2
797c0e0
820e6a3
6ca66f1
240131b
94ed1f8
89e1266
13ae7b8
08db464
8e92d81
0938264
a366a0a
1042bbb
79c8fe9
904b816
235ed21
34ccf2d
f239784
6d94c66
055b3cf
d1462c6
da9c878
5775239
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 might swap the order of these two points, since the second goal is required to be achieved for the first goal to be achieved.
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 have it in that order to lead with and highlight the main driving factor, which is to improve end user experience. And you are right that the second goal is necessary to achieve the first. But I wanted to put emphasis on the end user impact.
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.
Should this imply early-script, as well?
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.
Early-script refers to constraints during load vs. Script refers to constraints while running/post-load. Will add to explainer to clarify this.
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.
Sounds good, perhaps more specific naming could help in creating clearer scoping of these, since
script
generically feels like a superset ofEarly-script
based on the naming aloneThere 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.
Yes, the specific names need to be worked on at some point. We're looking to start the discussion with the problem and solution we're proposing, and work the details like names as we get a better picture/direction. Do you think that's reasonable? Or should we find better names now to make the read easier?
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 table above was a bit hard to follow. I think the text in this section is a bit clearer to start with, so I might swap those sections
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 wonder if we need this table at all, and can just move this one to a non-goal?
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 purpose of this table to to ensure alignment on what should be standardized for the various layers of configuration. This may influence the API. Any feedback on how that could be better conveyed?
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.
Should this be the user agent as opposed to the 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.
The intention is to enable documents to set restrictions on embedded content. Each document would be able to choose which policies to apply to which content, through Document Policy mechanism. While this could be leveraged by the User Agent in some "mode", we're focusing on exposing the capability as a Web Platform API for this explainer.
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.
Should we have a keyword to target all of them? Is that a use case we think will be common?
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'd prefer not to have a keyword for all as the categories might change as the API evolves. We still need to get a better understanding of how that would work.
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.
Since the example above includes multiple, I wonder if the example should only include one, and show how to use multiple in this section instead
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.
Added a single-category example to the general API portion. This section is intended to provide the reasoning behind some of the choices shown in the proposed API shape.
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.
Curious why this one doesn't have more details like the others?
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.
We know we need to think through any interactions but haven't done it yet, so it's currently more a placeholder.
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.
Might be worth explaining these in more detail
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.
These are explained in the linked NSM document. Should we describe the gist of it here, or more explicitly ask to read through the linked doc for the details?
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.
Might be worth explaining why this is a problem and/or how this is solved if so.
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.
Also brought in from NSM explainer. For the solution, we're looking for input on whether the 2-depth limit would be a reasonable constraint or if we'd need to find some sort of trade off.
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.
This section could use a bit more clarity and detail
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 following was determined for such approach" -> "the following outline the reasons for moving away from such an approach"
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.
Resolved.
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.
Missing subject to the sentence
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.
Resolved.
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.
"to the iframe"
Also should we update this to
<iframe>
?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.
Removed item from the list.
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'm not sure additional standards work is a good reason to go with one approach and not another, so I'm not sure if that is worth listing
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 reasoning here was that even if we wanted to go with the custom mechanism option, we'd need to re-do a lot of the work that Document Policy already does. In particular, it seems like HTML spec changes are more challenging to get through, so defining our own would add that while not providing anything fundamentally better compared to Document Policy. I can remove this item.