Add the Priority Hints changes to the html spec #8470
Conversation
|
One thing I didn't see working when building locally was that the attribute description did not appear to populate for fetchpriority like it does for all of the other attributes. Best as I can tell, it gets it from the glossary table but it didn't populate in any of the elements where the attribute was added. |
There was a problem hiding this comment.
Can you be sure to add tests for script and iframe as well?
Can you add tests that when a service worker intercepts requests (from all four elements), the request has the correct .priority?
One thing I didn't see working when building locally was that the attribute description did not appear to populate for fetchpriority like it does for all of the other attributes.
Were you running the full build.sh? It's done via a somewhat wonky setup, but it should work. The PR preview does not work because it's not running the full build.sh...
|
Thanks for the thorough review. First pass of the easy cleanups are done, going to look closer at the ones that are going to require a bit more thinking. I'll submit a set of WPT's through the Chromium flow and link them here after they have landed.
Yes, with the full build.sh, the generated local file doesn't have a description attached to any of the fetchpriority content attributes. They all look like this:
The glossary table at the end which is where it looks like the content attribute descriptions for the other attributes are pulled from looks like it is populated correctly:
|
These changes when combined with similar changes to HTML in whatwg/html#8470 obsolete the existing Priority Hints specification https://wicg.github.io/priority-hints/
These changes when combined with similar changes to HTML in whatwg/html#8470 obsolete the existing Priority Hints specification https://wicg.github.io/priority-hints/
These changes when combined with similar changes to HTML in whatwg/html#8470 obsolete the existing Priority Hints specification https://wicg.github.io/priority-hints/
- Renames the existing implementor-internal "priority" attribute on Request to "internal priority". - Adds a new "priority" interface to RequestInit and Request for specifying an explicit priority hint. These changes when combined with similar changes to HTML in whatwg/html#8470 obsolete the existing Priority Hints specification https://wicg.github.io/priority-hints/ This fixes whatwg#1319
pmeenan
force-pushed
the
priority-hints
branch
2 times, most recently
from
b8b4c2e
to
c85da3a
Compare
There was a problem hiding this comment.
I noticed that you defined this for iframe, but you haven't actually defined how it's passed through to the navigate algorithm and ultimately ends up in Fetch.
"process the iframe attributes" initializes a variable that is then thrown away. (I see now that @domenic also noted this and apparently this isn't limited to the iframe element.)
- Renames the request priority concept to internal priority and "adds" priority for usage by APIs. - Adds a new priority member to RequestInit for specifying a priority hint. These changes combined with whatwg/html#8470 obsolete the Priority Hints specification: https://wicg.github.io/priority-hints/. Tests: TODO. Fixes whatwg#1319. Co-authored-by: Anne van Kesteren <[email protected]>
- Renames the request priority concept to internal priority and "adds" priority for usage by APIs. - Adds a new priority member to RequestInit for specifying a priority hint. These changes combined with whatwg/html#8470 obsolete the Priority Hints specification: https://wicg.github.io/priority-hints/. Tests: https://chromium-review.googlesource.com/c/chromium/src/+/4097472. Fixes #1319. Co-authored-by: Anne van Kesteren <[email protected]>
|
Sorry about the delay, just getting back to this now that fetch has merged. I think I cleaned up most of the dangling variables, formatting and references. The big gaping hole still to fill is plumbing the iframe attribute through the navigation logic to the underlying fetch. Will follow-up shortly when I get that bit sorted out. |
|
OK, I think it is plumbed through the iFrame case now and I don't think I missed any of the linkages along the way. It mirrors the path of referrer policy and adds a fetch priority to the document state that is used on document navigation. It defaults to auto but can be explicitly set in the iFrame case. |
Also: * Correct a few instances in the spec where "nested navigable"/"content navigable" was used, but "child navigable" was more correct, including in the names of the create and destroy algorithms. * Export "navigable container" and "content navigable" (the latter scoped to "navigable container"). Closes whatwg#8713.
This adds "has `Link` processing" to the link types table, which currently only has "Yes" values for preload and preconnect. Closes whatwg#4224, and removes the reference to it in the spec. That issue was about the general lack of specification for the Link header, which has been resolved. whatwg#8741 tracks potentially adding Link header support for rel=stylesheet, but there's no need to retain notices in the spec about the possibility of this nonstandard feature. (The note was also quite incomplete, as it didn't talk about things like the cascade, document.styleSheets, etc.)
As discussed in whatwg#8743. Needed by WebGPU and WebCodecs.
|
@domfarolino Thanks for taking a look. Looks like there was also another use of script options in the timers section. Both are now explicitly setting it to "auto". |
|
Per the second paragraph of #8470 (review), I just want to confirm that the module script inheritance of priority hints is indeed correct and intended? |
|
@domfarolino Ah, no. Fixed, thanks. In the spec discussions we determined that the fetch priority wouldn't cascade so it is now wiped and set to "auto". |
|
Thanks. I cleaned up the whitespace and wrapping (and tweaked where some paragraphs broke to make it much easier to spot errors). I think I got them all. I tweaked the language around setting the priority when processing the iframe attributes (remaining comments above) but there's one part of it I'm wavering on.
I switched fetchpriority to priority in the spec text I'm wondering if it would be better to say that it is used to set the |
|
Thanks! I suppose it would be good to combine those sentences in some fashion. Only "The fetchpriority attribute is a fetch priority attribute." has normative meaning. Perhaps adding "also" in the note would help or perhaps the "purpose" bits should generally be in notes, though that would be a larger change. Maybe @domenic can have a final look at this. |
There was a problem hiding this comment.
Made it halfway through before I have to run for the night; will try to pick this back up tomorrow. I found and fixed some nits, and one substantive question. Also, we need to fix the attribute not showing up in the element definitions; it's not clear why that's broken but I'll try poking at it.
|
|
||
| <dt><span data-x="link options fetchpriority">fetchpriority</span></dt> | ||
| <dd>the state of <var>el</var>'s <code data-x="attr-link-fetchpriority">fetchpriority</code> | ||
| content attribute</dd> |
There was a problem hiding this comment.
Do we want to allow setting this on Link headers as well? If so we need to modify "apply link options from parsed header attributes".
source
Outdated
| @@ -90197,6 +90328,10 @@ interface <dfn interface>BeforeUnloadEvent</dfn> : <span>Event</span> { | |||
| document.</p> | |||
| </li> | |||
|
|
|||
| <li><p>A <dfn data-x="document-state-request-fetch-priority">request fetch priority</dfn>, which | |||
| is a <span>fetch priority attribute</span>, initially <code | |||
| data-x="attr-fetchpriority-auto-state">auto</code>.</p></li> | |||
There was a problem hiding this comment.
Hmm.
I can think of three possible ways that fetchpriority="" could impact iframe fetches. It could impact:
- Changes to the
src=""attribute - Navigations of any sort, e.g.
location.href = "https://other.example.com/"inside the iframe orframes[0].location.href = "..."from outside. - Navigations specifically to the history entry that was loaded.
You've chosen (3), which seems surprising. I'd expect (1). For example, with your (3), you get semantics like the following:
iframe.fetchPriority = "high";
iframe.src = "https://example.com/";
await waitForLoadEvent(iframe);
iframe.fetchPriority = "low";
iframe.contentWindow.reload(); // uses "high", not "low"
await waitForLoadEvent(iframe);
iframe.contentWindow.location.href = "https://other.example.com/"; // uses "low"
await waitForLoadEvent(iframe);
iframe.contentWindow.back(); // uses "high", not "low"However, I will note that your choice matches what we've done for referrer policy... hmm. Thoughts welcome from @domfarolino @jakearchibald.
There was a problem hiding this comment.
I wonder if what is specified for referrer policy actually matches implementations. That does seem like rather weird behavior.
There was a problem hiding this comment.
For referrer policy it makes sense, we spent a lot of time thinking about it, and I think Dom updated Chromium to match. If you fetched it with a given referrer (policy) the first time, you should keep that. For priority I'm less sure, since it's performance-ish and feels like it's meant more to apply to the container.
There was a problem hiding this comment.
Fair, but presumably referrer policy also applies to 1? I think that's the thing that stood out as weird to me.
It's a good reminder though that these kind of fetch attributes don't translate well to iframe as navigation is just a much more complicated endeavor.
There was a problem hiding this comment.
We also stick srcdoc on the history entry, and seem to be leaning towards doing the same for sandbox #6809 (comment), although that isn't how it currently works in browsers.
srcdoc isn't quite the same, since setting it triggers a navigation.
I think I agree with @annevk here that there's no perfect choice.
There was a problem hiding this comment.
Fair, but presumably referrer policy also applies to 1? I think that's the thing that stood out as weird to me.
Right, the current value of the referrerpolicy="" attribute will apply to src="" changes. I can see how I made things confusing by contrasting (3) and (1); this PR does indeed do (1) + (3). I meant that (1) alone would be more expected to me.
So I think the question is: is fetchpriority="" more like loading="", i.e. only applies to (1), or is it more like referrerpolicy="", i.e. applies to (1) + (3)? I tend toward the former, but don't insist on it. I can certainly see the case that it's more like the latter.
@pmeenan, what are your thoughts?
There was a problem hiding this comment.
I'm starting to think that we should remove the attribute from iFrames entirely and leave it for subresources. Chrome hasn't implemented it yet and as I think about it more, trying to schedule the document fetch may not be the right level of scheduling for an iFrame.
We might be better off thinking of iFrame scheduling on its own and explore something like "defer" or other concepts that more align with lazy-loading but with more explicit control. That way it could work for scheduling the iFrame creation independent of document src (injected html, external url, srcdoc, etc).
It's quite possible that trying to prioritize an iFrame's document fetch against subresources on the page that contains it could be completely ineffective since it lives in a different document context and may end up partitioned into separate cache and networking pools depending on the browser-level sandboxing.
Our hacky script is sensitive to 1- vs. 2-space indents, amaaaazing
|
I stripped out the attribute from iFrame's (as well as all of the plumbing) and added the processing for link headers. Since it's not implemented anywhere for iFrames yet and it likely won't work as people would expect, it's better to handle iFrame scheduling as its own thing (off to update the explainer and WPT tests now). |
This applies the specified changes from the Priority Hints spec to html for #7150.
The main changes are:
fetchpriorityattribute andfetchPriorityIDL attribute to theimg,link,scriptandiframeelements./embedded-content.html ( diff )
/images.html ( diff )
/index.html ( diff )
/indices.html ( diff )
/infrastructure.html ( diff )
/links.html ( diff )
/scripting.html ( diff )
/semantics.html ( diff )
/timers-and-user-prompts.html ( diff )
/urls-and-fetching.html ( diff )
/webappapis.html ( diff )