Add fetchpriority support for link header [chromium/src : main]
Yoav Weiss (Gerrit)
Attention is currently required from: Patrick Meenan, Takashi Toyoshima.
5 comments:
File content/browser/loader/navigation_early_hints_manager.cc:
Patch Set #4, Line 596:
return net::MEDIUM;So `as=stylesheet fetchpriority=high` would have a lower priority than `as=stylesheet`? That doesn't seem right..
Patch Set #4, Line 601:
case network::mojom::LinkAsAttribute::kFont:Preloaded fonts are not currently render-blocking, so marking them as "highest" seems excessive. There are plans to experiment with making them render blocking for short periods of time, which may make this more appropriate.
File services/network/public/mojom/request_priority.mojom:
Patch Set #3, Line 8:
// It corresponds to the "internal priority" of a fetch."It also corresponds"?
File third_party/blink/renderer/platform/loader/link_header_test.cc:
Patch Set #4, Line 28:
const char* fetch_priority;Aside: we should just convert all this to WPTs and drop those unittests..
File third_party/blink/web_tests/http/tests/linkHeader/link-header-preload-fetchpriority.php:
Patch Set #4, Line 1:
<?phpDon't we have a WPT way to do this? (e.g. through H2 priority reflection)
To view, visit change 4228311. To unsubscribe, or for help writing mail filters, visit settings.
Takashi Toyoshima (Gerrit)
Attention is currently required from: Patrick Meenan, Yoav Weiss.
Patch set 4:Code-Review +1
1 comment:
File content/browser/loader/navigation_early_hints_manager.cc:
Patch Set #4, Line 601:
case network::mojom::LinkAsAttribute::kFont:Preloaded fonts are not currently render-blocking, so marking them as "highest" seems excessive. […]
Ah, this is due to my comment. As we already used a wrong priority for Font, I suggested keeping the same wrong implementation in this CL, and another follow-up CL will fix the priority to see how our perfbots detect it.
Probably, submitting this first, then fixing the priority later will be a better order as we can easily revert the priority change part if perfbots complain... WDYT?
To view, visit change 4228311. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Patrick Meenan.
1 comment:
File content/browser/loader/navigation_early_hints_manager.cc:
Patch Set #4, Line 601:
case network::mojom::LinkAsAttribute::kFont:
Ah, this is due to my comment. […]
Makes sense!! I was just surprised by it 😊
To view, visit change 4228311. To unsubscribe, or for help writing mail filters, visit settings.
Patrick Meenan (Gerrit)
Attention is currently required from: Yoav Weiss.
4 comments:
Patchset:
Yoav@ thanks. Fixed it so this CL won't lower the priority of HIGHEST resources when fetchpriority is set to high.
I'll work on a separate CL to actually fix the base priorities. Preloaded fonts should be the same as HTML-preloaded fonts and, arguably, preloaded stylesheets should be the same as preloaded scripts/fonts and not the same priority as the HTML. For that matter, regular early-body CSS should also be the same priority so that it is sequenced in parser order relative to blocking scripts (since they block each other) and only @import stylesheets should be HIGHEST (probably a few CL's and experiments before it all lands).
File content/browser/loader/navigation_early_hints_manager.cc:
Patch Set #4, Line 596:
return net::MEDIUM;So `as=stylesheet fetchpriority=high` would have a lower priority than `as=stylesheet`? That doesn't […]
Thanks. Fixed it so font and stylesheet remain HIGHEST when set to "high". I'll take on fixing the actual priorities in a separate CL.
File services/network/public/mojom/request_priority.mojom:
Patch Set #3, Line 8:
// It corresponds to the "internal priority" of a fetch."It also corresponds"?
Done
File third_party/blink/web_tests/http/tests/linkHeader/link-header-preload-fetchpriority.php:
Patch Set #4, Line 1:
<?php
Don't we have a WPT way to do this? (e.g. […]
Not yet. We will (sort of) when I add the priority header but it will still probably need to be a browser test for explicit priority values since those aren't spec'd. We will probably be able to wpt that high and low for a given resource type result in different priorities.
To view, visit change 4228311. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Patrick Meenan.
Patch set 5:Code-Review +1
1 comment:
Patchset:
LGTM
To view, visit change 4228311. To unsubscribe, or for help writing mail filters, visit settings.
Patrick Meenan (Gerrit)
Attention is currently required from: Patrick Meenan.
Patch set 5:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Add fetchpriority support for link header
This plumbs support for the "fetchpriority" attribute of the link
http header when used with rel=preload.
The support is plumbed for both regular HTTP responses as well as 103
early-hints which are processed separately.
Bug: 1405738
Change-Id: I0880bb9879d5a7b853f23d4092dbfea8fb540dcd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4228311
Reviewed-by: Yoav Weiss <yoav...@chromium.org>
Reviewed-by: Takashi Toyoshima <toyo...@chromium.org>
Commit-Queue: Patrick Meenan <pme...@chromium.org>
Reviewed-by: danakj <dan...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1103485}
---
M content/browser/loader/navigation_early_hints_manager.cc
M content/browser/loader/navigation_early_hints_manager.h
M content/browser/loader/navigation_early_hints_manager_unittest.cc
M services/network/public/cpp/link_header_parser.cc
M services/network/public/cpp/link_header_parser_unittest.cc
M services/network/public/mojom/link_header.mojom
M services/network/public/mojom/request_priority.mojom
M third_party/blink/renderer/core/loader/link_load_parameters.cc
M third_party/blink/renderer/core/loader/preload_helper.cc
M third_party/blink/renderer/platform/loader/link_header.cc
M third_party/blink/renderer/platform/loader/link_header.h
M third_party/blink/renderer/platform/loader/link_header_test.cc
M third_party/blink/renderer/platform/network/http_parsers.cc
A third_party/blink/web_tests/http/tests/linkHeader/link-header-preload-fetchpriority-expected.txt
A third_party/blink/web_tests/http/tests/linkHeader/link-header-preload-fetchpriority.php
15 files changed, 373 insertions(+), 115 deletions(-)