Add render blocking status to Performance Resource Timing [chromium/src : main]
Yoav Weiss (Gerrit)
Attention is currently required from: Abin Paul.
4 comments:
Patchset:
Looks great! :) A few initial comments
File third_party/blink/renderer/core/timing/performance_resource_timing.cc:
Patch Set #5, Line 447:
if (RuntimeEnabledFeatures::RenderBlockingStatusEnabled())Add parenthesis? (they are not required, but I prefer them when they're not colliding with surrounding code that doesn't add them)
File third_party/blink/renderer/core/timing/performance_resource_timing.idl:
Patch Set #5, Line 54:
// See: https://github.com/abinpaul1/resource-timing/blob/render-blocking-status-explainer/Explainer/Render_Blocking_Status.mdMaybe move the explainer to the Resource Timing repo?
File third_party/blink/renderer/platform/runtime_enabled_features.json5:
Patch Set #5, Line 1997:
status: "test"I think we can make this "experimental" to enable folks to play around with it.
To view, visit change 3709521. To unsubscribe, or for help writing mail filters, visit settings.
Blink W3C Test Autoroller (Gerrit)
Attention is currently required from: Abin Paul.
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/34628.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Yoav Weiss (Gerrit)
Attention is currently required from: Abin Paul.
1 comment:
Patchset:
The test failures seem relevant. You probably need to update the webexposed expectations
To view, visit change 3709521. To unsubscribe, or for help writing mail filters, visit settings.
Abin Paul (Gerrit)
Attention is currently required from: Yoav Weiss.
2 comments:
File third_party/blink/renderer/core/timing/performance_resource_timing.cc:
Patch Set #5, Line 447:
if (RuntimeEnabledFeatures::RenderBlockingStatusEnabled())
Add parenthesis? (they are not required, but I prefer them when they're not colliding with surroundi […]
Added
File third_party/blink/renderer/platform/runtime_enabled_features.json5:
Patch Set #5, Line 1997:
status: "test"I think we can make this "experimental" to enable folks to play around with it.
Changed
To view, visit change 3709521. To unsubscribe, or for help writing mail filters, visit settings.
Abin Paul (Gerrit)
Attention is currently required from: Hao Liu, Yoav Weiss.
1 comment:
Patchset:
Fixed the test failures
To view, visit change 3709521. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Abin Paul, Hao Liu.
12 comments:
Patchset:
Thanks!! A few comments on the tests, mostly nits on their style
File third_party/blink/web_tests/external/wpt/resource-timing/render-blocking-status-link.html:
Nit: spurious new line
Patch Set #8, Line 15:
<link rel=stylesheet href='resources/empty_style.css?stylesheet-head'>In terms of style, it'd be nice to have consistency around single/double quotes for all the attributes
Patch Set #8, Line 21:
<link rel=stylesheet id="link-head-remove-attr" blocking="render" href='resources/empty_style.css?stylesheet-head-blocking-render-remove-attr'>Can you break up long lines to try and get them to fit in 80 chars?
Patch Set #8, Line 27:
<body>Can you explicitly close the <head> above this?
Nit: spurious new line
Patch Set #8, Line 138:
'stylesheet-head-dynamic-innerHTML' : 'non-blocking',Can you put this in the same order as the resources? (so higher up)
It's also not immediately obvious to me why this is "non blocking". Have you looked into it? Could it be a bug? If it isn't, a comment pointing to the spec indicating why this is correct would be useful
Patch Set #8, Line 141:
'stylesheet-head-dynamic-dom' : 'non-blocking',Same here, I'd naively expect this to be blocking..
File third_party/blink/web_tests/external/wpt/resource-timing/render-blocking-status-script.html:
Patch Set #8, Line 9:
<script src="resources/empty_script.js?script-head"></script>Nit: You could add a comment above this line, indicating that this is the start of test cases..
Spurious new line
Patch Set #8, Line 46:
//Dynamic explicitly async scriptNit: space before comment
Patch Set #8, Line 49:
non_async_script.async = false;Worthwhile to point to the relevant spec indicating that this is correct
To view, visit change 3709521. To unsubscribe, or for help writing mail filters, visit settings.
Abin Paul (Gerrit)
Attention is currently required from: Abin Paul, Hao Liu, Yoav Weiss.
Patch set 9:Commit-Queue +1
11 comments:
File third_party/blink/web_tests/external/wpt/resource-timing/render-blocking-status-link.html:
Nit: spurious new line
Shall I keep it because now it separates the test case elements from the other elements?
Patch Set #8, Line 15:
<link rel=stylesheet href='resources/empty_style.css?stylesheet-head'>
In terms of style, it'd be nice to have consistency around single/double quotes for all the attribut […]
Done
Patch Set #8, Line 21:
<link rel=stylesheet id="link-head-remove-attr" blocking="render" href='resources/empty_style.css?stylesheet-head-blocking-render-remove-attr'>Can you break up long lines to try and get them to fit in 80 chars?
I've tried to break up lines that seemed long but the longer ones now still tend to lie in between 80-100 chars. Should I try again with a hard limit of 80 chars for the entire document?
Patch Set #8, Line 27:
<body>Can you explicitly close the <head> above this?
Done
Nit: spurious new line
Removed
Patch Set #8, Line 138:
'stylesheet-head-dynamic-innerHTML' : 'non-blocking',
Can you put this in the same order as the resources? (so higher up) […]
https://html.spec.whatwg.org/multipage/links.html#link-type-stylesheet
says it is implicitly potentially render-blocking only if it was inserted by node document's parser, so I think its non-blocking when inserted by scripts. Am I understanding it right?
I have tried to capture this in a comment before the start of the test cases that adds dynamic stylesheets.
Patch Set #8, Line 141:
'stylesheet-head-dynamic-dom' : 'non-blocking',Same here, I'd naively expect this to be blocking..
I believe this case will also be explained with the comment being added above.
File third_party/blink/web_tests/external/wpt/resource-timing/render-blocking-status-script.html:
Patch Set #8, Line 9:
<script src="resources/empty_script.js?script-head"></script>Nit: You could add a comment above this line, indicating that this is the start of test cases..
Added.
Spurious new line
Removed.
Patch Set #8, Line 46:
//Dynamic explicitly async scriptNit: space before comment
Done
Patch Set #8, Line 49:
non_async_script.async = false;Worthwhile to point to the relevant spec indicating that this is correct
Done
To view, visit change 3709521. To unsubscribe, or for help writing mail filters, visit settings.
Abin Paul (Gerrit)
Patchset:
Build was failing because of a change that went in to the mainline. Fixed it now.
To view, visit change 3709521. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Abin Paul, Hao Liu.
Patch set 11:Code-Review +1
5 comments:
Patchset:
LGTM!!
File third_party/blink/web_tests/external/wpt/resource-timing/render-blocking-status-link.html:
Shall I keep it because now it separates the test case elements from the other elements?
Cool
Patch Set #8, Line 21:
<link rel=stylesheet id="link-head-remove-attr" blocking="render" href='resources/empty_style.css?stylesheet-head-blocking-render-remove-attr'>
I've tried to break up lines that seemed long but the longer ones now still tend to lie in between 8 […]
Looks better, thanks!
Patch Set #8, Line 138:
'stylesheet-head-dynamic-innerHTML' : 'non-blocking',
https://html.spec.whatwg.org/multipage/links.html#link-type-stylesheet […]
Great, thanks!
Patch Set #8, Line 141:
'stylesheet-head-dynamic-dom' : 'non-blocking',
I believe this case will also be explained with the comment being added above.
cool
To view, visit change 3709521. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Abin Paul, Hao Liu, Mike West.
1 comment:
Patchset:
Adding mkwst@ for the .mojom change
To view, visit change 3709521. To unsubscribe, or for help writing mail filters, visit settings.
Mike West (Gerrit)
1 comment:
File third_party/blink/public/mojom/timing/resource_timing.mojom:
Patch Set #11, Line 124:
bool render_blocking_status;Adding this bool LGTM.
To view, visit change 3709521. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Abin Paul, Hao Liu.
Patch set 11:Commit-Queue +1
1 comment:
File third_party/blink/renderer/core/timing/performance_resource_timing.idl:
Patch Set #5, Line 54:
// See: https://github.com/abinpaul1/resource-timing/blob/render-blocking-status-explainer/Explainer/Render_Blocking_Status.mdMaybe move the explainer to the Resource Timing repo?
As https://github.com/w3c/resource-timing/pull/330 would require some time to land, I'm fine with landing this as is, and changing the link once it does.
To view, visit change 3709521. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Abin Paul, Hao Liu.
Patch set 11:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Add render blocking status to Performance Resource Timing
This CL introduces a renderBlockingStatus field to Performance Resource
Timing object. This field is behind a Runtime Enabled Flag.
Bug: 1337256
Change-Id: I0175fb0bce5e3e2aa8eeebd8cff3d2c9f920e3db
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3709521
Commit-Queue: Yoav Weiss <yoav...@chromium.org>
Reviewed-by: Yoav Weiss <yoav...@chromium.org>
Reviewed-by: Mike West <mk...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1020810}
---
M AUTHORS
M third_party/blink/public/mojom/timing/resource_timing.mojom
M third_party/blink/renderer/core/loader/cross_thread_resource_timing_info_copier.cc
M third_party/blink/renderer/core/timing/performance.cc
M third_party/blink/renderer/core/timing/performance_resource_timing.cc
M third_party/blink/renderer/core/timing/performance_resource_timing.h
M third_party/blink/renderer/core/timing/performance_resource_timing.idl
M third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
M third_party/blink/renderer/platform/loader/fetch/resource_fetcher.h
M third_party/blink/renderer/platform/loader/fetch/resource_timing_info.h
M third_party/blink/renderer/platform/runtime_enabled_features.json5
A third_party/blink/web_tests/external/wpt/resource-timing/render-blocking-status-link.html
A third_party/blink/web_tests/external/wpt/resource-timing/render-blocking-status-script.html
A third_party/blink/web_tests/external/wpt/resource-timing/resources/empty_style.css
M third_party/blink/web_tests/external/wpt/resource-timing/resources/fake_responses.py
A third_party/blink/web_tests/external/wpt/resource-timing/resources/importer.css
A third_party/blink/web_tests/external/wpt/resource-timing/resources/importer.js
A third_party/blink/web_tests/external/wpt/resource-timing/resources/importer_async.js
A third_party/blink/web_tests/external/wpt/resource-timing/resources/importer_dynamic.css
A third_party/blink/web_tests/external/wpt/resource-timing/resources/importer_print.css
M third_party/blink/web_tests/external/wpt/resource-timing/tojson.html
M third_party/blink/web_tests/platform/generic/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
M third_party/blink/web_tests/platform/generic/webexposed/global-interface-listing-dedicated-worker-expected.txt
M third_party/blink/web_tests/platform/generic/webexposed/global-interface-listing-expected.txt
M third_party/blink/web_tests/platform/generic/webexposed/global-interface-listing-shared-worker-expected.txt
25 files changed, 507 insertions(+), 6 deletions(-)
Blink W3C Test Autoroller (Gerrit)
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/34628