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

3 views
Skip to first unread message

Yoav Weiss (Gerrit)

unread,
Jun 1, 2021, 3:54:54 AMJun 1
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.

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

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I0c2fe30e1b6cb99d5b4ed09e2aec7350faafdf28
Gerrit-Change-Number: 2698428
Gerrit-PatchSet: 24
Gerrit-Owner: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
Gerrit-Reviewer: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Yutaka Hirano <yhi...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Fredrik Söderquist <f...@opera.com>
Gerrit-CC: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Yoav Weiss <yoav...@chromium.org>
Gerrit-Attention: Dominic Farolino <d...@chromium.org>
Gerrit-Attention: Yutaka Hirano <yhi...@chromium.org>
Gerrit-MessageType: newpatchset

Yutaka Hirano (Gerrit)

unread,
Jun 2, 2021, 2:29:21 AMJun 2
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.

View Change

5 comments:

  • Patchset:

  • 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:

  • File third_party/blink/renderer/core/script/script_scheduling_type.h:

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

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I0c2fe30e1b6cb99d5b4ed09e2aec7350faafdf28
Gerrit-Change-Number: 2698428
Gerrit-PatchSet: 24
Gerrit-Owner: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
Gerrit-Reviewer: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Yutaka Hirano <yhi...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Fredrik Söderquist <f...@opera.com>
Gerrit-CC: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Yoav Weiss <yoav...@chromium.org>
Gerrit-Attention: Dominic Farolino <d...@chromium.org>
Gerrit-Comment-Date: Wed, 02 Jun 2021 06:29:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Dominic Farolino (Gerrit)

unread,
Jun 3, 2021, 6:24:20 PMJun 3
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.

View Change

9 comments:

  • Patchset:

    • Patch Set #24:

      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:

  • File third_party/blink/renderer/core/html/keywords.json5:

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

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I0c2fe30e1b6cb99d5b4ed09e2aec7350faafdf28
Gerrit-Change-Number: 2698428
Gerrit-PatchSet: 24
Gerrit-Owner: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
Gerrit-Reviewer: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Yutaka Hirano <yhi...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Fredrik Söderquist <f...@opera.com>
Gerrit-CC: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Yoav Weiss <yoav...@chromium.org>
Gerrit-Attention: Yutaka Hirano <yhi...@chromium.org>
Gerrit-Comment-Date: Thu, 03 Jun 2021 22:24:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Yutaka Hirano <yhi...@chromium.org>
Gerrit-MessageType: comment

Kouhei Ueno (Gerrit)

unread,
Jun 6, 2021, 10:52:24 PMJun 6
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.

View Change

1 comment:

  • Patchset:

    • Patch Set #24:

      Highlight:

      • I love the idea of milestone attribute, and the attribute plumbing logic looks good.

      Lowlight:

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

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I0c2fe30e1b6cb99d5b4ed09e2aec7350faafdf28
Gerrit-Change-Number: 2698428
Gerrit-PatchSet: 24
Gerrit-Owner: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
Gerrit-Reviewer: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Yutaka Hirano <yhi...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Fredrik Söderquist <f...@opera.com>
Gerrit-CC: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Yoav Weiss <yoav...@chromium.org>
Gerrit-Attention: Yutaka Hirano <yhi...@chromium.org>
Gerrit-Comment-Date: Mon, 07 Jun 2021 02:52:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Kinuko Yasuda (Gerrit)

unread,
Jun 6, 2021, 11:28:33 PMJun 6
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.

View Change

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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I0c2fe30e1b6cb99d5b4ed09e2aec7350faafdf28
Gerrit-Change-Number: 2698428
Gerrit-PatchSet: 24
Gerrit-Owner: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
Gerrit-Reviewer: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Yutaka Hirano <yhi...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Fredrik Söderquist <f...@opera.com>
Gerrit-CC: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-CC: Kinuko Yasuda <kin...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Yoav Weiss <yoav...@chromium.org>
Gerrit-Attention: Yutaka Hirano <yhi...@chromium.org>
Gerrit-Comment-Date: Mon, 07 Jun 2021 03:28:23 +0000

Yoav Weiss (Gerrit)

unread,
Jun 7, 2021, 4:24:13 PMJun 7
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.

View Change

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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I0c2fe30e1b6cb99d5b4ed09e2aec7350faafdf28
Gerrit-Change-Number: 2698428
Gerrit-PatchSet: 24
Gerrit-Owner: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
Gerrit-Reviewer: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Yutaka Hirano <yhi...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Fredrik Söderquist <f...@opera.com>
Gerrit-CC: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-CC: Kinuko Yasuda <kin...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Yutaka Hirano <yhi...@chromium.org>
Gerrit-Comment-Date: Mon, 07 Jun 2021 20:24:01 +0000

Dominic Farolino (Gerrit)

unread,
Jun 7, 2021, 6:17:47 PMJun 7
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.

View Change

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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I0c2fe30e1b6cb99d5b4ed09e2aec7350faafdf28
Gerrit-Change-Number: 2698428
Gerrit-PatchSet: 24
Gerrit-Owner: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
Gerrit-Reviewer: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Yutaka Hirano <yhi...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Fredrik Söderquist <f...@opera.com>
Gerrit-CC: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-CC: Kinuko Yasuda <kin...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Yoav Weiss <yoav...@chromium.org>
Gerrit-Attention: Yutaka Hirano <yhi...@chromium.org>
Gerrit-Comment-Date: Mon, 07 Jun 2021 22:17:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Yoav Weiss <yoav...@chromium.org>
Gerrit-MessageType: comment

Kouhei Ueno (Gerrit)

unread,
Jun 8, 2021, 12:36:44 AMJun 8
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.

View Change

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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I0c2fe30e1b6cb99d5b4ed09e2aec7350faafdf28
Gerrit-Change-Number: 2698428
Gerrit-PatchSet: 24
Gerrit-Owner: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
Gerrit-Reviewer: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Yutaka Hirano <yhi...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Fredrik Söderquist <f...@opera.com>
Gerrit-CC: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-CC: Kinuko Yasuda <kin...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Yoav Weiss <yoav...@chromium.org>
Gerrit-Attention: Dominic Farolino <d...@chromium.org>
Gerrit-Attention: Yutaka Hirano <yhi...@chromium.org>
Gerrit-Comment-Date: Tue, 08 Jun 2021 04:36:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Yoav Weiss <yoav...@chromium.org>
Comment-In-Reply-To: Dominic Farolino <d...@chromium.org>
Gerrit-MessageType: comment

Yoav Weiss (Gerrit)

unread,
Jun 8, 2021, 3:18:26 PMJun 8
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.

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

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I0c2fe30e1b6cb99d5b4ed09e2aec7350faafdf28
Gerrit-Change-Number: 2698428
Gerrit-PatchSet: 25
Gerrit-Owner: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
Gerrit-Reviewer: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Yutaka Hirano <yhi...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Fredrik Söderquist <f...@opera.com>
Gerrit-CC: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-CC: Kinuko Yasuda <kin...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Yoav Weiss <yoav...@chromium.org>
Gerrit-Attention: Dominic Farolino <d...@chromium.org>
Gerrit-Attention: Yutaka Hirano <yhi...@chromium.org>
Gerrit-MessageType: newpatchset

Hiroshige Hayashizaki (Gerrit)

unread,
Jun 8, 2021, 5:35:59 PMJun 8
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.

View Change

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:

  • File third_party/blink/renderer/core/dom/document.cc:

    • 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:

  • 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:

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

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I0c2fe30e1b6cb99d5b4ed09e2aec7350faafdf28
Gerrit-Change-Number: 2698428
Gerrit-PatchSet: 25
Gerrit-Owner: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
Gerrit-Reviewer: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Yutaka Hirano <yhi...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Fredrik Söderquist <f...@opera.com>
Gerrit-CC: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-CC: Kinuko Yasuda <kin...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Yoav Weiss <yoav...@chromium.org>
Gerrit-Attention: Dominic Farolino <d...@chromium.org>
Gerrit-Attention: Yutaka Hirano <yhi...@chromium.org>
Gerrit-Comment-Date: Tue, 08 Jun 2021 21:35:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-MessageType: comment

Yoav Weiss (Gerrit)

unread,
Jun 8, 2021, 11:55:55 PMJun 8
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.

View Change

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:

    • typo: "waiting"

      Fixed!

  • File third_party/blink/renderer/core/dom/increment_load_event_delay_count.cc:

  • File third_party/blink/renderer/core/html/keywords.json5:

    • Fixed!

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

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

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I0c2fe30e1b6cb99d5b4ed09e2aec7350faafdf28
Gerrit-Change-Number: 2698428
Gerrit-PatchSet: 25
Gerrit-Owner: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
Gerrit-Reviewer: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Yutaka Hirano <yhi...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Fredrik Söderquist <f...@opera.com>
Gerrit-CC: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-CC: Kinuko Yasuda <kin...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Dominic Farolino <d...@chromium.org>
Gerrit-Attention: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-Attention: Yutaka Hirano <yhi...@chromium.org>
Gerrit-Comment-Date: Wed, 09 Jun 2021 03:55:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Dominic Farolino <d...@chromium.org>
Comment-In-Reply-To: Hiroshige Hayashizaki <hiro...@chromium.org>

Kinuko Yasuda (Gerrit)

unread,
Jun 9, 2021, 5:32:50 AMJun 9
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.

View Change

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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I0c2fe30e1b6cb99d5b4ed09e2aec7350faafdf28
Gerrit-Change-Number: 2698428
Gerrit-PatchSet: 26
Gerrit-Owner: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
Gerrit-Reviewer: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Yutaka Hirano <yhi...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Fredrik Söderquist <f...@opera.com>
Gerrit-CC: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-CC: Kinuko Yasuda <kin...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Kouhei Ueno <kou...@chromium.org>
Gerrit-Attention: Yoav Weiss <yoav...@chromium.org>
Gerrit-Attention: Dominic Farolino <d...@chromium.org>
Gerrit-Attention: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-Attention: Yutaka Hirano <yhi...@chromium.org>
Gerrit-Comment-Date: Wed, 09 Jun 2021 09:32:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kouhei Ueno <kou...@chromium.org>

Dominic Farolino (Gerrit)

unread,
Jun 11, 2021, 8:57:10 PMJun 11
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.

View Change

1 comment:

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

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

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I0c2fe30e1b6cb99d5b4ed09e2aec7350faafdf28
Gerrit-Change-Number: 2698428
Gerrit-PatchSet: 26
Gerrit-Owner: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
Gerrit-Reviewer: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Yutaka Hirano <yhi...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Fredrik Söderquist <f...@opera.com>
Gerrit-CC: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-CC: Kinuko Yasuda <kin...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Kouhei Ueno <kou...@chromium.org>
Gerrit-Attention: Yoav Weiss <yoav...@chromium.org>
Gerrit-Attention: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-Attention: Yutaka Hirano <yhi...@chromium.org>
Gerrit-Comment-Date: Sat, 12 Jun 2021 00:57:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Yoav Weiss <yoav...@chromium.org>
Comment-In-Reply-To: Dominic Farolino <d...@chromium.org>

Yutaka Hirano (Gerrit)

unread,
Jun 14, 2021, 1:45:12 AMJun 14
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.

View Change

2 comments:

  • File third_party/blink/renderer/core/html/parser/html_preload_scanner.cc:

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

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

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I0c2fe30e1b6cb99d5b4ed09e2aec7350faafdf28
Gerrit-Change-Number: 2698428
Gerrit-PatchSet: 26
Gerrit-Owner: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
Gerrit-Reviewer: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Yutaka Hirano <yhi...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Fredrik Söderquist <f...@opera.com>
Gerrit-CC: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-CC: Kinuko Yasuda <kin...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Kouhei Ueno <kou...@chromium.org>
Gerrit-Attention: Yoav Weiss <yoav...@chromium.org>
Gerrit-Attention: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-Comment-Date: Mon, 14 Jun 2021 05:44:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Yoav Weiss (Gerrit)

unread,
Jun 29, 2021, 3:48:48 PMJun 29
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.

View Change

8 comments:

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

    • Could you add some comments and a link to crbug. […]

      Done

  • File third_party/blink/renderer/core/dom/document.cc:

    • ditto; link to the crbug

      Done

  • File third_party/blink/renderer/core/loader/modulescript/module_tree_linker.cc:

    • 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

    • 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:

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

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I0c2fe30e1b6cb99d5b4ed09e2aec7350faafdf28
Gerrit-Change-Number: 2698428
Gerrit-PatchSet: 27
Gerrit-Owner: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
Gerrit-Reviewer: Yoav Weiss <yoav...@chromium.org>
Gerrit-Reviewer: Yutaka Hirano <yhi...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Fredrik Söderquist <f...@opera.com>
Gerrit-CC: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-CC: Kinuko Yasuda <kin...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Kouhei Ueno <kou...@chromium.org>
Gerrit-Attention: Dominic Farolino <d...@chromium.org>
Gerrit-Attention: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-Comment-Date: Tue, 29 Jun 2021 19:48:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Dominic Farolino <d...@chromium.org>
Reply all
Reply to author
Forward
0 new messages