Prototype milestone attribute support for scripts [chromium/src : main]

12 views

Skip to first unread message

Yoav Weiss (Gerrit)

unread,

Jun 1, 2021, 3:54:54 AM6/1/21

to blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org

Attention is currently required from: Yoav Weiss, Dominic Farolino, Yutaka Hirano.

Yoav Weiss uploaded patch set #24 to this change.

Prototype milestone attribute support for scripts

Proposal doc: https://docs.google.com/document/d/15k6sLw3hscfsD1BD51FJ_qWLIOVziS33a3Ld-kB-G3w/edit#

Change-Id: I0c2fe30e1b6cb99d5b4ed09e2aec7350faafdf28
---
M third_party/blink/public/mojom/fetch/fetch_api_request.mojom
M third_party/blink/renderer/core/dom/document.cc
M third_party/blink/renderer/core/dom/document.h
M third_party/blink/renderer/core/dom/increment_load_event_delay_count.cc
M third_party/blink/renderer/core/html/html_attribute_names.json5
M third_party/blink/renderer/core/html/html_script_element.cc
M third_party/blink/renderer/core/html/html_script_element.h
M third_party/blink/renderer/core/html/html_script_element.idl
M third_party/blink/renderer/core/html/keywords.json5
M third_party/blink/renderer/core/html/link_style.cc
M third_party/blink/renderer/core/html/parser/html_preload_scanner.cc
M third_party/blink/renderer/core/html/parser/preload_request.cc
M third_party/blink/renderer/core/html/parser/preload_request.h
M third_party/blink/renderer/core/loader/build.gni
M third_party/blink/renderer/core/loader/font_preload_manager.cc
M third_party/blink/renderer/core/loader/image_loader.cc
D third_party/blink/renderer/core/loader/importance_attribute.cc
M third_party/blink/renderer/core/loader/link_loader.cc
A third_party/blink/renderer/core/loader/loading_attributes.cc
R third_party/blink/renderer/core/loader/loading_attributes.h
M third_party/blink/renderer/core/loader/modulescript/module_tree_linker.cc
M third_party/blink/renderer/core/loader/preload_helper.cc
M third_party/blink/renderer/core/paint/paint_timing.cc
M third_party/blink/renderer/core/script/dynamic_module_resolver.cc
M third_party/blink/renderer/core/script/html_parser_script_runner.cc
M third_party/blink/renderer/core/script/mock_script_element_base.h
M third_party/blink/renderer/core/script/pending_script.cc
M third_party/blink/renderer/core/script/pending_script.h
M third_party/blink/renderer/core/script/script_element_base.h
M third_party/blink/renderer/core/script/script_loader.cc
M third_party/blink/renderer/core/script/script_loader.h
M third_party/blink/renderer/core/script/script_runner.cc
M third_party/blink/renderer/core/script/script_runner.h
M third_party/blink/renderer/core/script/script_scheduling_type.h
M third_party/blink/renderer/core/svg/svg_script_element.h
M third_party/blink/renderer/core/workers/worker_or_worklet_global_scope.cc
M third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
M third_party/blink/renderer/platform/loader/fetch/resource_request.cc
M third_party/blink/renderer/platform/loader/fetch/resource_request.h
M third_party/blink/renderer/platform/loader/fetch/script_fetch_options.cc
M third_party/blink/renderer/platform/loader/fetch/script_fetch_options.h
M third_party/blink/renderer/platform/runtime_enabled_features.json5
A third_party/blink/web_tests/http/tests/inspector-protocol/resources/milestone-frame.html
A third_party/blink/web_tests/http/tests/inspector-protocol/timeline/milestone-priority-expected.txt
A third_party/blink/web_tests/http/tests/inspector-protocol/timeline/milestone-priority.js
M third_party/blink/web_tests/webexposed/element-instance-property-listing-expected.txt
M third_party/blink/web_tests/webexposed/global-interface-listing-expected.txt
A third_party/blink/web_tests/wpt_internal/milestone/milestone-added-after-milestone.html
A third_party/blink/web_tests/wpt_internal/milestone/resources/current.js
A third_party/blink/web_tests/wpt_internal/milestone/resources/empty.css
A third_party/blink/web_tests/wpt_internal/milestone/resources/inject-milestone-script.py
A third_party/blink/web_tests/wpt_internal/milestone/resources/inject-style.js
A third_party/blink/web_tests/wpt_internal/milestone/script-execution-order-with-no-paint.html
A third_party/blink/web_tests/wpt_internal/milestone/script-execution-order.html
54 files changed, 547 insertions(+), 44 deletions(-)

To view, visit change 2698428. To unsubscribe, or for help writing mail filters, visit settings.

Yutaka Hirano (Gerrit)

unread,

Jun 2, 2021, 2:29:21 AM6/2/21

to Yoav Weiss, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org, Hiroshige Hayashizaki, Yutaka Hirano, Dominic Farolino, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Nate Chapin, Stephen Chenney

Attention is currently required from: Yoav Weiss, Dominic Farolino.

5 comments:

Patchset:
- Patch Set #24:
  I'd like this change to be linked with a crbug issue.
File third_party/blink/renderer/core/script/script_runner.h:
- Patch Set #24, Line 126: Vector<mojom::blink::FetchMilestone> milestones_vector_ = {
  You can use a simple array. This member can be static.
File third_party/blink/renderer/core/script/script_runner.cc:
- Patch Set #24, Line 382: PotentiallyBeginLifecycleUpdate
  Shouldn't this be done after executing the script?
File third_party/blink/renderer/core/script/script_scheduling_type.h:
- Patch Set #24, Line 76: kMilestone
  Can we have some comments here?

File third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc:

Patch Set #24, Line 475:

  if (milestone == mojom::blink::FetchMilestone::kMilestoneBeforeFirstPaint) {
    priority = ResourceLoadPriority::kVeryHigh;
  } else if (milestone ==
             mojom::blink::FetchMilestone::kMilestoneAfterFirstPaint) {
    priority = ResourceLoadPriority::kMedium;
  }

I think it's better to use switch here, assuming you will add more values in the future.

To view, visit change 2698428. To unsubscribe, or for help writing mail filters, visit settings.

Dominic Farolino (Gerrit)

unread,

Jun 3, 2021, 6:24:20 PM6/3/21

to Yoav Weiss, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org, Hiroshige Hayashizaki, Yutaka Hirano, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Nate Chapin, Stephen Chenney

Attention is currently required from: Yoav Weiss, Yutaka Hirano.

9 comments:

Patchset:

I looked at most of the implementation so far, but no tests. I have a couple of questions:
 - Can a ScriptRunner ever get notified about the same milestone twice?
 - Can a ScriptRunner ever get notified about a *lesser* milestone than its latest?
 
If the answer to any of the above is:
 - "No", can add DCHECKs that assert this
 - "Yes", can we add documentation in ScriptRunner explaining that it's OK if we see a given milestone multiple times, and here's why etc etc

File third_party/blink/renderer/core/dom/document.cc:
- Patch Set #24, Line 6615: // TODO(yoav): Only run this if we're not aiting on styles
  typo: "waiting"
File third_party/blink/renderer/core/html/keywords.json5:
- Patch Set #24, Line 76: // milestong attribute
  typo
File third_party/blink/renderer/core/script/script_runner.h:

- Patch Set #24, Line 126: Vector<mojom::blink::FetchMilestone> milestones_vector_ = {
  You can use a simple array. This member can be static.

- I'm really wondering if we can just get rid of this altogether? We seem to only use it so that we can iterate over all of the enum values, but can we just do that directly, something like https://stackoverflow.com/a/261986/3947332 ? It would also have the upside of keeping all of the values in one place instead of having to remember to add new relevant ones here
File third_party/blink/renderer/core/script/script_runner.cc:
- Patch Set #24, Line 69: case ScriptSchedulingType::kMilestone: {
  The block is here just to limit the scope of the `milestone` variable right?
- Patch Set #24, Line 74: ++render_blocking_scripts_;
  I imagine other elements that will use this attribute will want to make a similar check... could we add a method to the loading attributes file like `IsRenderBlockingMilestone(milestone)` that would return true for all render-blocking milestones, assuming there will be more than just this one? Well, even if it is just this one forever, I think it might be nicer that way, but in that case I'm agnostic
- Patch Set #24, Line 81: pending_milestone_scripts_.Set(milestone, scripts_queue);
  Wouldn't it just be easier to initialize the `scripts_queue` as an empty GC'd HeapDeque? Or are you worried about the unnecessary allocation? Or is there something I'm not realizing about the initialization that happens in the variable declaration.
- Patch Set #24, Line 263: for (auto milestone : milestones_vector_) {
  See my comment in script_runner.h. Is it possible to just iterate over all of the actual mojo milestone values?
File third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc:
- Patch Set #24, Line 209: // TODO(yoav): Make sure milestones play nice with priority hints.
  I'd be curious to hear more about what this means specifically (maybe as as comment).

To view, visit change 2698428. To unsubscribe, or for help writing mail filters, visit settings.

Kouhei Ueno (Gerrit)

unread,

Jun 6, 2021, 10:52:24 PM6/6/21

to Yoav Weiss, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org, Hiroshige Hayashizaki, Yutaka Hirano, Dominic Farolino, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Nate Chapin, Stephen Chenney

Attention is currently required from: Yoav Weiss, Yutaka Hirano.

1 comment:

Patchset:
- Patch Set #24:
  Highlight:
  - I love the idea of milestone attribute, and the attribute plumbing logic looks good.
  Lowlight:
  - If we are to do this, I'd prefer to have an explicit lifecycle manager somewhere. Since the document is the one driving this, I wonder if we can make them new states of https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/document_lifecycle.h and have the script loader (or the per-document resource loading coordinator (not sure if we have one or not) listen to that). I'm not suggesting to land all design at once, but I'd like to see the end design we are aiming for, and have this CL be an intermediate step toward that.

To view, visit change 2698428. To unsubscribe, or for help writing mail filters, visit settings.

Kinuko Yasuda (Gerrit)

unread,

Jun 6, 2021, 11:28:33 PM6/6/21

to Yoav Weiss, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org, Kouhei Ueno, Hiroshige Hayashizaki, Yutaka Hirano, Dominic Farolino, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Nate Chapin, Stephen Chenney

Attention is currently required from: Yoav Weiss, Yutaka Hirano.

1 comment:

Patchset:
- Patch Set #24:
  Some meta comment: It'd be good to share the brief plan about how you want to move this forward and get inputs on it. Per offline chat it sounded like you rather want something with which developers can poke around to explore the possibility than trying to build something complete. I think it can make sense, but as far as it wants to land I recommend building a rough consensus on what factors/inputs we will look for, and what actions may be taken based on that (e.g. abandon and delete the code, adjust and iterate, start more formal discussion towards shipping etc)
  (Also I suppose you're going to send an intent to prototype before landing this one?)

To view, visit change 2698428. To unsubscribe, or for help writing mail filters, visit settings.

Yoav Weiss (Gerrit)

unread,

Jun 7, 2021, 4:24:13 PM6/7/21

to blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org, Kinuko Yasuda, Kouhei Ueno, Hiroshige Hayashizaki, Yutaka Hirano, Dominic Farolino, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Nate Chapin, Stephen Chenney

Attention is currently required from: Yutaka Hirano.

1 comment:

Patchset:
- Patch Set #24:
  Thanks all for reviewing, and apologies for my slowness. Here's a tentative plan I'd love your opinions on:
  - I sent an I2P for this and land it behind a flag (addressing all your comments, obviously)
  - We run this by the WebSDK & WEC teams and potentially the broader community to test potential benefits.
  - Assuming reception is positive and results are promising, your team could take on to ship a less-prototype-y implementation, if y'all would be interested
  Does that make general sense?

To view, visit change 2698428. To unsubscribe, or for help writing mail filters, visit settings.

Dominic Farolino (Gerrit)

unread,

Jun 7, 2021, 6:17:47 PM6/7/21

to Yoav Weiss, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org, Kinuko Yasuda, Kouhei Ueno, Hiroshige Hayashizaki, Yutaka Hirano, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Nate Chapin, Stephen Chenney

Attention is currently required from: Yoav Weiss, Yutaka Hirano.

1 comment:

Patchset:
- Patch Set #24:
  Thanks all for reviewing, and apologies for my slowness. […]
  Chiming in here. If TOK is OK with that plan, I'm happy to help as much as I can with coordinating that with them.

To view, visit change 2698428. To unsubscribe, or for help writing mail filters, visit settings.

Kouhei Ueno (Gerrit)

unread,

Jun 8, 2021, 12:36:44 AM6/8/21

to Yoav Weiss, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org, Kinuko Yasuda, Hiroshige Hayashizaki, Yutaka Hirano, Dominic Farolino, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Nate Chapin, Stephen Chenney

Attention is currently required from: Yoav Weiss, Dominic Farolino, Yutaka Hirano.

1 comment:

Patchset:
- Patch Set #24:
  Chiming in here. […]
  The first two bullets LGTM. I can't comment on the last one :)

To view, visit change 2698428. To unsubscribe, or for help writing mail filters, visit settings.

Yoav Weiss (Gerrit)

unread,

Jun 8, 2021, 3:18:26 PM6/8/21

to blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org

Attention is currently required from: Yoav Weiss, Dominic Farolino, Yutaka Hirano.

Yoav Weiss uploaded patch set #25 to this change.

Prototype milestone attribute support for scripts

Proposal doc: https://docs.google.com/document/d/15k6sLw3hscfsD1BD51FJ_qWLIOVziS33a3Ld-kB-G3w/edit#

Change-Id: I0c2fe30e1b6cb99d5b4ed09e2aec7350faafdf28

Bug: 1217733

To view, visit change 2698428. To unsubscribe, or for help writing mail filters, visit settings.

Hiroshige Hayashizaki (Gerrit)

unread,

Jun 8, 2021, 5:35:59 PM6/8/21

to Yoav Weiss, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org, Kinuko Yasuda, Kouhei Ueno, Yutaka Hirano, Dominic Farolino, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Nate Chapin, Stephen Chenney

Attention is currently required from: Yoav Weiss, Dominic Farolino, Yutaka Hirano.

13 comments:

Commit Message:
- Patch Set #23, Line 9: Change-Id: I0c2fe30e1b6cb99d5b4ed09e2aec7350faafdf28
  Could you provide a tracking bug, spec issue, design doc, etc?
  Done
Patchset:
- Patch Set #25:
  I thought before-first-paint scripts are evaluated anytime && as soon as possible before the first paint (as well as blocking the first paint), but according to
  - The initial value of `ScriptRunner::latest_milestone_` (It seems before-first-paint scripts are not evaluated until `ScheduleReadyMilestoneScripts(kMilestoneBeforeFirstPaint)` or `ScheduleReadyMilestoneScripts(kMilestoneAfterFirstPaint)` is called, i.e. are blocked until those calls)
  - The comments and code of Document::DidRemoveAllPendingStylesheets() and Document::SchedulePaintMilestoneScripts() (It seems before-first-paint scripts wait for blocking stylesheets? It adds a new interaction between parser and non-parser-inserted scripts though...in the current spec only parser-inserted scripts can wait for parser-blocking stylesheets)
  - HTMLParserScriptRunner::ExecuteScriptsWaitingForParsing() might ensure ordering between before-first-paint/after-first-paint and defer scripts?
  It seems my understanding was insufficient.
  Could you explain and document (in the design doc and/or within the code/comments) the (ideal and current) semantics of before-first-paint and after-first-paint?
File third_party/blink/public/mojom/fetch/fetch_api_request.mojom:
- Patch Set #25, Line 109: enum FetchMilestone {
  Could you add some comments and a link to crbug.com/1217733?
File third_party/blink/renderer/core/dom/document.cc:

- Patch Set #24, Line 6615: // TODO(yoav): Only run this if we're not aiting on styles

- ditto; link to the crbug
File third_party/blink/renderer/core/loader/modulescript/module_tree_linker.cc:
- Patch Set #25, Line 468: mojom::blink::FetchMilestone::kMilestoneNone);
  So we set the milestone-based loading priority only to the top-level script, right?
  Could you add a comment here (probably one similar to the comment about "importance" at Lines 457-)?
File third_party/blink/renderer/core/script/html_parser_script_runner.cc:
- Patch Set #25, Line 461: // TODO(yoav): specify this. Also, schedule a task to only run this after
  ditto; link to the crbug
- Patch Set #25, Line 463: if (document_) {
  If we don't have this call, what would occur?
  Is this to ensure before/after-first-paint scripts to be evaluated before defer scripts? What would be ideal state (especially when the parsing is completed before the first paint)?
File third_party/blink/renderer/core/script/script_loader.cc:
- Patch Set #25, Line 827: // TODO(yoav): Specify this and move this to the right place based on the
  Could you include crbug.com/1217733 to provide information (which indirectly provides the link of the design doc)? e.g. TODO(crbug.com/1217733)
File third_party/blink/renderer/core/script/script_runner.h:
- Patch Set #25, Line 72: void ScheduleReadyMilestoneScripts(mojom::blink::FetchMilestone milestone);
  I feel the logic would be much clearer if we have two separate methods:
  - ScriptRunner::ScheduleReadyMilestoneScripts() that schedules all milestone scripts with <= `latest_milestone_`, and
  - ScriptRunner::AdvanceMilestone(milestone) that potentially changes `latest_milestone_` and calls ScheduleReadyMilestoneScripts().
  I expect most of the current call sites of ScheduleReadyMilestoneScripts() is changed to AdvanceMilestone(), except for the one at ScriptRunner::NotifyScriptReady().
- Patch Set #25, Line 118: render_blocking_scripts_
  I feel maintaining this count separately from `pending_milestone_scripts_` (IIUC this should be equal to something like `pending_milestone_scripts_[kMilestoneBeforeFirstPaint].size()`, right?) is error prone. (IIRC a similar counter for async/in-order scripts caused bugs when scripts are moved between documents and thus was removed)
  How about making IsRenderBlocking() iterate over `pending_milestone_scripts_` and removing this counter?
File third_party/blink/renderer/core/script/script_runner.cc:
- Patch Set #25, Line 316: void ScriptRunner::MovePendingScript(ScriptRunner* new_runner,
  The milestone script case should be added here.
  In the current patch set, milestone scripts moved to another Document are evaluated on the original Document (while they shouldn't evaluated).
  Probably I'll add related tests in `third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/moving-between-documents/`.
File third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc:

- Patch Set #24, Line 209: // TODO(yoav): Make sure milestones play nice with priority hints.

- Could you add a link to crbug.com/1217733?
File third_party/blink/renderer/platform/loader/fetch/script_fetch_options.cc:
- Patch Set #25, Line 62: params.MutableResourceRequest().SetFetchMilestone(milestone_);
  Could you add a link to crbug.com/1217733?

To view, visit change 2698428. To unsubscribe, or for help writing mail filters, visit settings.

Yoav Weiss (Gerrit)

unread,

Jun 8, 2021, 11:55:55 PM6/8/21

to blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org, Kinuko Yasuda, Kouhei Ueno, Hiroshige Hayashizaki, Yutaka Hirano, Dominic Farolino, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Nate Chapin, Stephen Chenney

Attention is currently required from: Dominic Farolino, Hiroshige Hayashizaki, Yutaka Hirano.

5 comments:

Commit Message:

- Patch Set #23, Line 9: Change-Id: I0c2fe30e1b6cb99d5b4ed09e2aec7350faafdf28
  Could you provide a tracking bug, spec issue, design doc, etc?

- Added a proposal doc and a bug!
File third_party/blink/renderer/core/dom/document.cc:

- Patch Set #24, Line 6615: // TODO(yoav): Only run this if we're not aiting on styles

- typo: "waiting"
  Fixed!
File third_party/blink/renderer/core/dom/increment_load_event_delay_count.cc:
- Patch Set #23, Line 12: #include <base/debug/stack_trace.h>
  nit: not needed in the final CL?
  Oops!
File third_party/blink/renderer/core/html/keywords.json5:

- Patch Set #24, Line 76: // milestong attribute
  typo

- Fixed!
File third_party/blink/renderer/core/script/script_runner.h:

- Patch Set #24, Line 126: Vector<mojom::blink::FetchMilestone> milestones_vector_ = {

- I'm really wondering if we can just get rid of this altogether? We seem to only use it so that we ca […]
  I tried to avoid casting the enum values to ints. A static array sounds like a reasonable compromise, but let me know what you think.

To view, visit change 2698428. To unsubscribe, or for help writing mail filters, visit settings.

Kinuko Yasuda (Gerrit)

unread,

Jun 9, 2021, 5:32:50 AM6/9/21

to Yoav Weiss, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org, Kouhei Ueno, Hiroshige Hayashizaki, Yutaka Hirano, Dominic Farolino, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Nate Chapin, Stephen Chenney

Attention is currently required from: Kouhei Ueno, Yoav Weiss, Dominic Farolino, Hiroshige Hayashizaki, Yutaka Hirano.

1 comment:

Patchset:
- Patch Set #24:
  The first two bullets LGTM. […]
  Sorry that I hadn't responded to this! The plan sounds good to me, the ownership / last one can be probably discussed in parallel.

To view, visit change 2698428. To unsubscribe, or for help writing mail filters, visit settings.

Dominic Farolino (Gerrit)

unread,

Jun 11, 2021, 8:57:10 PM6/11/21

to Yoav Weiss, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org, Kinuko Yasuda, Kouhei Ueno, Hiroshige Hayashizaki, Yutaka Hirano, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Nate Chapin, Stephen Chenney

Attention is currently required from: Kouhei Ueno, Yoav Weiss, Hiroshige Hayashizaki, Yutaka Hirano.

1 comment:

File third_party/blink/renderer/core/script/script_runner.h:

- Patch Set #24, Line 126: Vector<mojom::blink::FetchMilestone> milestones_vector_ = {

- I tried to avoid casting the enum values to ints. […]
  My comment is more around having a separate list at all, and it possibly getting out of sync with the mojo enum, or that not being very clearly intentional if it does on purpose. I guess that means I'd like to see this array go away and be cast to an iterable directly, but not sure if that's super weird or what others think. It just feels more error prone to maintain a separate list than to cast and iterate.
  If we must maintain a separate list, and we think that is OK because the scripting code is intentionally interested in a strict subset of the actual mojo values, then I'd prefer to pull this list out into an array in maybe `loading_attributes.h` where we define a list of `values_that_scripts_care_about` or something... not sure

To view, visit change 2698428. To unsubscribe, or for help writing mail filters, visit settings.

Yutaka Hirano (Gerrit)

unread,

Jun 14, 2021, 1:45:12 AM6/14/21

to Yoav Weiss, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org, Kinuko Yasuda, Kouhei Ueno, Hiroshige Hayashizaki, Yutaka Hirano, Dominic Farolino, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Nate Chapin, Stephen Chenney

Attention is currently required from: Kouhei Ueno, Yoav Weiss, Hiroshige Hayashizaki.

2 comments:

File third_party/blink/renderer/core/html/parser/html_preload_scanner.cc:
- Patch Set #26, Line 831: mojom::blink::FetchMilestone milestone_;
  = kNone;
File third_party/blink/renderer/core/script/script_runner.h:
- Patch Set #26, Line 127: static constexpr mojom::blink::FetchMilestone const milestones_[] = {
  kMilestones

To view, visit change 2698428. To unsubscribe, or for help writing mail filters, visit settings.

Yoav Weiss (Gerrit)

unread,

Jun 29, 2021, 3:48:48 PM6/29/21

to blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org, Kinuko Yasuda, Kouhei Ueno, Hiroshige Hayashizaki, Yutaka Hirano, Dominic Farolino, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Nate Chapin, Stephen Chenney

Attention is currently required from: Kouhei Ueno, Dominic Farolino, Hiroshige Hayashizaki.

8 comments:

File third_party/blink/public/mojom/fetch/fetch_api_request.mojom:

- Patch Set #25, Line 109: enum FetchMilestone {

- Could you add some comments and a link to crbug. […]
  Done
File third_party/blink/renderer/core/dom/document.cc:

- Patch Set #24, Line 6615: // TODO(yoav): Only run this if we're not aiting on styles

- ditto; link to the crbug
  Done
File third_party/blink/renderer/core/loader/modulescript/module_tree_linker.cc:

- Patch Set #25, Line 468: mojom::blink::FetchMilestone::kMilestoneNone);

- So we set the milestone-based loading priority only to the top-level script, right? […]
  Done
File third_party/blink/renderer/core/script/html_parser_script_runner.cc:

- Patch Set #25, Line 461: // TODO(yoav): specify this. Also, schedule a task to only run this after
  ditto; link to the crbug

- Done

- Patch Set #25, Line 463: if (document_) {

- If we don't have this call, what would occur? […]
  Indeed. This makes sure that the calls are not racy compared to defer. I'll add a comment.
File third_party/blink/renderer/core/script/script_loader.cc:

- Patch Set #25, Line 827: // TODO(yoav): Specify this and move this to the right place based on the

- Could you include crbug. […]
  Added a comment
File third_party/blink/renderer/core/script/script_runner.h:
- Patch Set #25, Line 72: void ScheduleReadyMilestoneScripts(mojom::blink::FetchMilestone milestone);
  I feel the logic would be much clearer if we have two separate methods: […]
  That doesn't actually work, as all the call sites also need to schedule the scripts to run.
File third_party/blink/renderer/core/script/script_runner.cc:

- Patch Set #24, Line 69: case ScriptSchedulingType::kMilestone: {
  The block is here just to limit the scope of the `milestone` variable right?

- Indeed

To view, visit change 2698428. To unsubscribe, or for help writing mail filters, visit settings.

Dominic Farolino (Gerrit)

unread,

Oct 28, 2021, 5:50:34 PM10/28/21

to Yoav Weiss, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org, Kinuko Yasuda, Kouhei Ueno, Hiroshige Hayashizaki, Yutaka Hirano, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Nate Chapin, Stephen Chenney

Attention is currently required from: Kouhei Ueno, Yoav Weiss, Hiroshige Hayashizaki.

1 comment:

Patchset:
- Patch Set #27:
  What was the verdict with this? Is this just waiting for people to pick up some day or something?

To view, visit change 2698428. To unsubscribe, or for help writing mail filters, visit settings.

Yoav Weiss (Gerrit)

unread,

Oct 29, 2021, 12:59:13 AM10/29/21

to blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org, Xiaocheng Hu, Kouhei Ueno, Yutaka Hirano, Dominic Farolino

Attention is currently required from: Kouhei Ueno, Dominic Farolino, Hiroshige Hayashizaki, Yutaka Hirano.

Yoav Weiss has uploaded this change for review.

Prototype milestone attribute support for scripts

Proposal doc: https://docs.google.com/document/d/15k6sLw3hscfsD1BD51FJ_qWLIOVziS33a3Ld-kB-G3w/edit#

Change-Id: I0c2fe30e1b6cb99d5b4ed09e2aec7350faafdf28
Bug: 1217733
---

M third_party/blink/renderer/core/html/parser/preload_request.h
M third_party/blink/renderer/core/script/script_scheduling_type.h
M third_party/blink/renderer/core/loader/font_preload_manager.cc
M third_party/blink/renderer/core/loader/link_loader.cc
M third_party/blink/renderer/core/html/keywords.json5
M third_party/blink/renderer/core/loader/preload_helper.cc
A third_party/blink/web_tests/wpt_internal/milestone/resources/current.js
M third_party/blink/renderer/platform/loader/fetch/resource_request.cc
M third_party/blink/public/mojom/fetch/fetch_api_request.mojom
M third_party/blink/renderer/core/script/pending_script.h
M third_party/blink/renderer/platform/loader/fetch/script_fetch_options.cc
M third_party/blink/renderer/core/workers/worker_or_worklet_global_scope.cc
M third_party/blink/renderer/core/script/script_loader.cc
M third_party/blink/renderer/core/script/pending_script.cc
R third_party/blink/renderer/core/loader/loading_attributes.h
A third_party/blink/renderer/core/loader/loading_attributes.cc
M third_party/blink/renderer/core/html/html_script_element.h
M third_party/blink/renderer/core/loader/image_loader.cc
M third_party/blink/renderer/core/paint/paint_timing.cc
M third_party/blink/renderer/core/script/html_parser_script_runner.cc
M third_party/blink/renderer/core/html/link_style.cc
M third_party/blink/renderer/platform/loader/fetch/resource_request.h
M third_party/blink/renderer/core/html/parser/html_preload_scanner.cc
M third_party/blink/renderer/core/script/script_runner.h
A third_party/blink/web_tests/http/tests/inspector-protocol/resources/milestone-frame.html
M third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
A third_party/blink/web_tests/wpt_internal/milestone/resources/inject-style.js
M third_party/blink/renderer/core/script/script_element_base.h
M third_party/blink/renderer/core/script/mock_script_element_base.h
M third_party/blink/renderer/core/dom/document.h
M third_party/blink/renderer/core/svg/svg_script_element.h
M third_party/blink/web_tests/webexposed/element-instance-property-listing-expected.txt
M third_party/blink/renderer/core/html/html_script_element.idl
M third_party/blink/renderer/core/dom/document.cc
A third_party/blink/web_tests/wpt_internal/milestone/script-execution-order.html
M third_party/blink/renderer/core/loader/build.gni
M third_party/blink/renderer/core/script/script_loader.h
M third_party/blink/renderer/core/loader/modulescript/module_tree_linker.cc
M third_party/blink/renderer/core/script/dynamic_module_resolver.cc
M third_party/blink/renderer/platform/runtime_enabled_features.json5
M third_party/blink/renderer/core/html/parser/preload_request.cc
A third_party/blink/web_tests/wpt_internal/milestone/script-execution-order-with-no-paint.html
A third_party/blink/web_tests/wpt_internal/milestone/milestone-added-after-milestone.html
M third_party/blink/renderer/core/script/script_runner.cc
D third_party/blink/renderer/core/loader/importance_attribute.cc
A third_party/blink/web_tests/http/tests/inspector-protocol/timeline/milestone-priority-expected.txt
A third_party/blink/web_tests/wpt_internal/milestone/resources/inject-milestone-script.py
A third_party/blink/web_tests/http/tests/inspector-protocol/timeline/milestone-priority.js
M third_party/blink/renderer/platform/loader/fetch/script_fetch_options.h
M third_party/blink/web_tests/webexposed/global-interface-listing-expected.txt
M third_party/blink/renderer/core/html/html_attribute_names.json5
A third_party/blink/web_tests/wpt_internal/milestone/resources/empty.css
M third_party/blink/renderer/core/html/html_script_element.cc
53 files changed, 572 insertions(+), 44 deletions(-)

To view, visit change 2698428. To unsubscribe, or for help writing mail filters, visit settings.

Yoav Weiss (Gerrit)

unread,

Oct 29, 2021, 12:59:20 AM10/29/21

to blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org, Xiaocheng Hu, Kinuko Yasuda, Kouhei Ueno, Hiroshige Hayashizaki, Yutaka Hirano, Dominic Farolino, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Nate Chapin, Stephen Chenney

Attention is currently required from: Kouhei Ueno, Dominic Farolino, Hiroshige Hayashizaki, Yutaka Hirano.

6 comments:

Patchset:

- Patch Set #27:
  What was the verdict with this? Is this just waiting for people to pick up some day or something?

- I got distracted..
  In the meantime, xiaochengh@ started working on a `renderblocking` attribute, which has some overlap with the "before-first-render" milestone prototyped here.
File third_party/blink/renderer/core/html/parser/html_preload_scanner.cc:

- Patch Set #26, Line 831: mojom::blink::FetchMilestone milestone_;
  = kNone;

- Done
File third_party/blink/renderer/core/script/script_runner.h:

- Patch Set #26, Line 127: static constexpr mojom::blink::FetchMilestone const milestones_[] = {
  kMilestones

- Removed entirely
File third_party/blink/renderer/core/script/script_runner.h:

- Patch Set #24, Line 126: Vector<mojom::blink::FetchMilestone> milestones_vector_ = {

- My comment is more around having a separate list at all, and it possibly getting out of sync with th […]
  Dropped the array entirely and went with casting
File third_party/blink/renderer/core/script/script_runner.cc:
- Patch Set #24, Line 74: ++render_blocking_scripts_;
  I imagine other elements that will use this attribute will want to make a similar check... […]
  Done

- Patch Set #24, Line 263: for (auto milestone : milestones_vector_) {

- See my comment in script_runner.h. […]
  Done

Dominic Farolino (Gerrit)

unread,

Oct 29, 2021, 10:57:35 AM10/29/21

to Yoav Weiss, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org, Xiaocheng Hu, Kinuko Yasuda, Kouhei Ueno, Hiroshige Hayashizaki, Yutaka Hirano, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Nate Chapin, Stephen Chenney

Attention is currently required from: Kouhei Ueno, Yoav Weiss, Hiroshige Hayashizaki, Yutaka Hirano.

1 comment:

Patchset:

I got distracted.. […]

I see. I am a little concerned that we are creating an explosion of small loading-influencing attributes that will be super hard for developers to understand:
 - `milestone`
 - `importance`
 - `async`
 - `defer`
 - `renderblocking`
 - `loading=lazy` // not for script, but still

Should we try and talk about this at a higher-level to make sure we are producing primitives that are very easy to understand and consume?

To view, visit change 2698428. To unsubscribe, or for help writing mail filters, visit settings.

Dominic Farolino (Gerrit)

unread,

Aug 23, 2023, 3:41:47 PM8/23/23

to Yoav Weiss, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org, Xiaocheng Hu, Kinuko Yasuda, Kouhei Ueno, Hiroshige Hayashizaki, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Nate Chapin, Stephen Chenney

Attention is currently required from: Hiroshige Hayashizaki, Yoav Weiss.

1 comment:

Patchset:
- Patch Set #27:
  Removing myself from the attn set of this CL, since I noticed it in my Gerrit dashboard. Do you have any plans for this Yoav?

To view, visit change 2698428. To unsubscribe, or for help writing mail filters, visit settings.

Yoav Weiss (Gerrit)

unread,

Aug 24, 2023, 12:17:09 AM8/24/23

to blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org, Xiaocheng Hu, Kinuko Yasuda, Kouhei Ueno, Hiroshige Hayashizaki, Dominic Farolino, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Nate Chapin, Stephen Chenney

Attention is currently required from: Dominic Farolino, Hiroshige Hayashizaki.

1 comment:

Patchset:
- Patch Set #27:
  Removing myself from the attn set of this CL, since I noticed it in my Gerrit dashboard. […]
  No current plans to push this further..

To view, visit change 2698428. To unsubscribe, or for help writing mail filters, visit settings.

Yoav Weiss (Gerrit)

unread,

Aug 24, 2023, 12:17:15 AM8/24/23

to blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, darin...@chromium.org, dom+...@chromium.org, fmalit...@chromium.org, gavinp...@chromium.org, gavinp+p...@chromium.org, hiroshig...@chromium.org, ipc-securi...@chromium.org, jbroma...@chromium.org, kinuko...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, pdr+svgw...@chromium.org, prerenderi...@chromium.org, Xiaocheng Hu, Kinuko Yasuda, Kouhei Ueno, Hiroshige Hayashizaki, Dominic Farolino, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Nate Chapin, Stephen Chenney

Yoav Weiss abandoned this change.

Abandoned

No current plans

To view, visit change 2698428. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward

0 new messages

Search

Clear search

Close search

Google apps

Main menu