Skip to content
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

Add the Priority Hints changes to the html spec #8470

Merged
merged 29 commits into from
Feb 17, 2023

Conversation

pmeenan
Copy link
Contributor

@pmeenan pmeenan commented Nov 3, 2022

This applies the specified changes from the Priority Hints spec to html for #7150.

The main changes are:

  • Add a new fetchpriority attribute and fetchPriority IDL attribute to the img, link, script and iframe elements.
  • Plumb the attribute through to the underlying fetch for the resource associated with a given element.

/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 )

@pmeenan
Copy link
Contributor Author

pmeenan commented Nov 3, 2022

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.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests

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...

source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@pmeenan
Copy link
Contributor Author

pmeenan commented Nov 4, 2022

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.

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...

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:

Screen Shot 2022-11-04 at 10 21 28 AM

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:

Screen Shot 2022-11-04 at 10 22 33 AM

pmeenan added a commit to pmeenan/fetch that referenced this pull request Nov 11, 2022
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/
pmeenan added a commit to pmeenan/fetch that referenced this pull request Nov 22, 2022
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/
pmeenan added a commit to pmeenan/fetch that referenced this pull request Dec 2, 2022
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/
pmeenan added a commit to pmeenan/fetch that referenced this pull request Dec 2, 2022
- 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 pmeenan force-pushed the priority-hints branch 2 times, most recently from b8b4c2e to c85da3a Compare December 2, 2022 17:59
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
annevk added a commit to pmeenan/fetch that referenced this pull request Dec 8, 2022
- 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]>
annevk added a commit to whatwg/fetch that referenced this pull request Dec 15, 2022
- 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]>
@pmeenan
Copy link
Contributor Author

pmeenan commented Jan 11, 2023

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.

@pmeenan
Copy link
Contributor Author

pmeenan commented Jan 18, 2023

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.

domenic and others added 3 commits January 25, 2023 17:32
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.
@pmeenan
Copy link
Contributor Author

pmeenan commented Jan 30, 2023

@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".

@domfarolino
Copy link
Member

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?

@pmeenan
Copy link
Contributor Author

pmeenan commented Feb 1, 2023

@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".

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found editorial issues. It looks like the iframe propagation is done correctly now, but it would be good to double check once all the other things are in order.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@pmeenan
Copy link
Contributor Author

pmeenan commented Feb 9, 2023

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.

Screenshot 2023-02-09 at 2 11 37 PM

I switched fetchpriority to priority in the spec text is to set the priority used when processing the iframe attributes with a link to the fetch standard which matches how referer policy links out.

I'm wondering if it would be better to say that it is used to set the request fetch priority in the document state (which then sets the priority at fetch time) like the note below mentions. It's technically more accurate about the direct effect of what it sets but setting the priority is the dev-facing end-state.

@annevk
Copy link
Member

annevk commented Feb 14, 2023

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.

@annevk annevk requested a review from domenic February 14, 2023 09:54
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.

I can think of three possible ways that fetchpriority="" could impact iframe fetches. It could impact:

  1. Changes to the src="" attribute
  2. Navigations of any sort, e.g. location.href = "https://other.example.com/" inside the iframe or frames[0].location.href = "..." from outside.
  3. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if what is specified for referrer policy actually matches implementations. That does seem like rather weird behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@pmeenan
Copy link
Contributor Author

pmeenan commented Feb 16, 2023

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).

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woohoo, merging! Please remember to remove the iframe WPTs and update MDN, if you haven't already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants