[DeferUnusePreload] Preload requests via PostTask if likely unused [chromium/src : main]

0 views
Skip to first unread message

Shunya Shishido (Gerrit)

unread,
Apr 15, 2024, 7:39:19 PMApr 15
to Hiroshige Hayashizaki, Yoshisato Yanagisawa, Kouhei Ueno, Minoru Chikamune, Alex N. Jose, Chromium LUCI CQ, chromium...@chromium.org, Nate Chapin, Yoav Weiss (@Shopify), blink-rev...@chromium.org, blink-...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org
Attention needed from Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune and Yoshisato Yanagisawa

New activity on the change

Open in Gerrit

Related details

Attention is currently required from:
  • Hiroshige Hayashizaki
  • Kouhei Ueno
  • Minoru Chikamune
  • Yoshisato Yanagisawa
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Ief7451fab7602be2cd4aa9660a4568e418eff19e
Gerrit-Change-Number: 5447206
Gerrit-PatchSet: 3
Gerrit-Owner: Shunya Shishido <sisid...@chromium.org>
Gerrit-Reviewer: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
Gerrit-Reviewer: Minoru Chikamune <chik...@chromium.org>
Gerrit-Reviewer: Shunya Shishido <sisid...@chromium.org>
Gerrit-Reviewer: Yoshisato Yanagisawa <yyana...@chromium.org>
Gerrit-CC: Alex N. Jose <ale...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Yoav Weiss (@Shopify) <yoav...@chromium.org>
Gerrit-Attention: Kouhei Ueno <kou...@chromium.org>
Gerrit-Attention: Yoshisato Yanagisawa <yyana...@chromium.org>
Gerrit-Attention: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-Attention: Minoru Chikamune <chik...@chromium.org>
Gerrit-Comment-Date: Mon, 15 Apr 2024 23:39:05 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Yoshisato Yanagisawa (Gerrit)

unread,
Apr 15, 2024, 9:53:37 PMApr 15
to Shunya Shishido, Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune, Alex N. Jose, Chromium LUCI CQ, chromium...@chromium.org, Nate Chapin, Yoav Weiss (@Shopify), blink-rev...@chromium.org, blink-...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org
Attention needed from Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune and Shunya Shishido

Yoshisato Yanagisawa voted and added 2 comments

Votes added by Yoshisato Yanagisawa

Code-Review+1

2 comments

Patchset-level comments
File-level comment, Patchset 3 (Latest):
Yoshisato Yanagisawa . resolved

lgtm

File third_party/blink/renderer/core/html/parser/html_preload_scanner.h
Line 185, Patchset 3 (Latest): Vector<KURL> potentially_unused_preloads_;
Yoshisato Yanagisawa . unresolved

How is the number of unused preloads? Will it be less than 100?

Open in Gerrit

Related details

Attention is currently required from:
  • Hiroshige Hayashizaki
  • Kouhei Ueno
  • Minoru Chikamune
  • Shunya Shishido
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Ief7451fab7602be2cd4aa9660a4568e418eff19e
Gerrit-Change-Number: 5447206
Gerrit-PatchSet: 3
Gerrit-Owner: Shunya Shishido <sisid...@chromium.org>
Gerrit-Reviewer: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
Gerrit-Reviewer: Minoru Chikamune <chik...@chromium.org>
Gerrit-Reviewer: Shunya Shishido <sisid...@chromium.org>
Gerrit-Reviewer: Yoshisato Yanagisawa <yyana...@chromium.org>
Gerrit-CC: Alex N. Jose <ale...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Yoav Weiss (@Shopify) <yoav...@chromium.org>
Gerrit-Attention: Kouhei Ueno <kou...@chromium.org>
Gerrit-Attention: Shunya Shishido <sisid...@chromium.org>
Gerrit-Attention: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-Attention: Minoru Chikamune <chik...@chromium.org>
Gerrit-Comment-Date: Tue, 16 Apr 2024 01:53:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Minoru Chikamune (Gerrit)

unread,
Apr 15, 2024, 10:00:27 PMApr 15
to Shunya Shishido, Yoshisato Yanagisawa, Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune, Alex N. Jose, Chromium LUCI CQ, chromium...@chromium.org, Nate Chapin, Yoav Weiss (@Shopify), blink-rev...@chromium.org, blink-...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org
Attention needed from Hiroshige Hayashizaki, Kouhei Ueno and Shunya Shishido

Minoru Chikamune added 1 comment

File third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
Line 1359, Patchset 3 (Latest): // If the preload request which is labelled as potentially unused, do not
// start the loading immediately, put the task instead.
freezable_task_runner_->PostTask(
FROM_HERE,
WTF::BindOnce(&ResourceFetcher::StartLoadAndFinishIfFailed,
WrapWeakPersistent(this), WrapPersistent(resource),
std::move(params.MutableResourceRequest().MutableBody()),
load_blocking_policy, params.GetRenderBlockingBehavior(),
params.Url()));
Minoru Chikamune . unresolved

In my understanding, `ResourceFetcher::RequestResource()` is called from the following functions via `GetImageLoader().UpdateFromElement()`. I guess these direct fetch requests can be delayed. And perhaps this might not be your intention.

  • HTMLImageElement::ParseAttribute()
  • HTMLImageElement::ForceReload()
  • HTMLImageElement::SelectSourceURL()
Open in Gerrit

Related details

Attention is currently required from:
  • Hiroshige Hayashizaki
  • Kouhei Ueno
  • Shunya Shishido
Gerrit-Comment-Date: Tue, 16 Apr 2024 02:00:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Shunya Shishido (Gerrit)

unread,
Apr 16, 2024, 2:12:31 AMApr 16
to Yoshisato Yanagisawa, Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune, Alex N. Jose, Chromium LUCI CQ, chromium...@chromium.org, Nate Chapin, Yoav Weiss (@Shopify), blink-rev...@chromium.org, blink-...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org
Attention needed from Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune and Yoshisato Yanagisawa

Shunya Shishido added 2 comments

File third_party/blink/renderer/core/html/parser/html_preload_scanner.h
Line 185, Patchset 3: Vector<KURL> potentially_unused_preloads_;
Yoshisato Yanagisawa . unresolved

How is the number of unused preloads? Will it be less than 100?

Shunya Shishido

The number of records from the database is limited up to 10.
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/predictors/loading_predictor_config.cc;l=63;drc=c3a7009be69ddc4492e8bb4e6dcb12bb0b1d3b71;bpv=1;bpt=1

Actually unused preloads are about 1.7 resources per page from UMA. I believe this is enough and using vector makes sense.

File third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
Line 1359, Patchset 3: // If the preload request which is labelled as potentially unused, do not

// start the loading immediately, put the task instead.
freezable_task_runner_->PostTask(
FROM_HERE,
WTF::BindOnce(&ResourceFetcher::StartLoadAndFinishIfFailed,
WrapWeakPersistent(this), WrapPersistent(resource),
std::move(params.MutableResourceRequest().MutableBody()),
load_blocking_policy, params.GetRenderBlockingBehavior(),
params.Url()));
Minoru Chikamune . resolved

In my understanding, `ResourceFetcher::RequestResource()` is called from the following functions via `GetImageLoader().UpdateFromElement()`. I guess these direct fetch requests can be delayed. And perhaps this might not be your intention.

  • HTMLImageElement::ParseAttribute()
  • HTMLImageElement::ForceReload()
  • HTMLImageElement::SelectSourceURL()
Shunya Shishido

As we discussed offline, `params.IsPotentiallyUnusedPreload()` in L1357 is true only when `SetIsPotentiallyUnusedPreload()` is called. `IsPotentiallyUnusedPreload()` return true when the flag is passed from the preload scanner, so this doesn't introduce effects to other callers.

Also, I removed `IsPotentiallyUnusedPreload()` check from `ResourceNeedsLoad()` in the latest patchset.

Open in Gerrit

Related details

Attention is currently required from:
  • Hiroshige Hayashizaki
  • Kouhei Ueno
  • Minoru Chikamune
  • Yoshisato Yanagisawa
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Ief7451fab7602be2cd4aa9660a4568e418eff19e
Gerrit-Change-Number: 5447206
Gerrit-PatchSet: 4
Gerrit-Owner: Shunya Shishido <sisid...@chromium.org>
Gerrit-Reviewer: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
Gerrit-Reviewer: Minoru Chikamune <chik...@chromium.org>
Gerrit-Reviewer: Shunya Shishido <sisid...@chromium.org>
Gerrit-Reviewer: Yoshisato Yanagisawa <yyana...@chromium.org>
Gerrit-CC: Alex N. Jose <ale...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Yoav Weiss (@Shopify) <yoav...@chromium.org>
Gerrit-Attention: Kouhei Ueno <kou...@chromium.org>
Gerrit-Attention: Yoshisato Yanagisawa <yyana...@chromium.org>
Gerrit-Attention: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-Attention: Minoru Chikamune <chik...@chromium.org>
Gerrit-Comment-Date: Tue, 16 Apr 2024 06:12:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Yoshisato Yanagisawa <yyana...@chromium.org>
Comment-In-Reply-To: Minoru Chikamune <chik...@chromium.org>
satisfied_requirement
unsatisfied_requirement
open
diffy

Hiroshige Hayashizaki (Gerrit)

unread,
Apr 16, 2024, 2:50:12 AMApr 16
to Shunya Shishido, Yoshisato Yanagisawa, Kouhei Ueno, Minoru Chikamune, Alex N. Jose, Chromium LUCI CQ, chromium...@chromium.org, Nate Chapin, Yoav Weiss (@Shopify), blink-rev...@chromium.org, blink-...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org
Attention needed from Kouhei Ueno, Minoru Chikamune, Shunya Shishido and Yoshisato Yanagisawa

Hiroshige Hayashizaki added 4 comments

File third_party/blink/renderer/core/html/parser/html_preload_scanner.h
Line 185, Patchset 3: Vector<KURL> potentially_unused_preloads_;
Yoshisato Yanagisawa . unresolved

How is the number of unused preloads? Will it be less than 100?

Shunya Shishido

The number of records from the database is limited up to 10.
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/predictors/loading_predictor_config.cc;l=63;drc=c3a7009be69ddc4492e8bb4e6dcb12bb0b1d3b71;bpv=1;bpt=1

Actually unused preloads are about 1.7 resources per page from UMA. I believe this is enough and using vector makes sense.

Hiroshige Hayashizaki

Probably it's helpful to write this information in the source code comment.

File third_party/blink/renderer/core/html/parser/html_preload_scanner.cc
Line 1163, Patchset 4 (Latest): locators, skip_preload_scan, potentially_unused_preloads);
Hiroshige Hayashizaki . unresolved

nit: std::move()

File third_party/blink/renderer/platform/loader/fetch/resource.h
Line 559, Patchset 4 (Latest): bool is_potentially_unused_preload_ = false;
Hiroshige Hayashizaki . unresolved

This flag is set but not used in this CL.
Do you have a plan to use this flag in subsequent CLs?

I was writing the comment below but then I noticed this flag is not used, and my comment depends on how this flag is used.

---

Currently `is_potentially_unused_preload_` means "The Resource have preloading request(s) with potentially_unused_preload LCPP flag", but maybe it should be "The Resource has only preloading request(s) with potentially_unused_preload LCPP flag and no other requests"?

(i.e. currently is_potentially_unused_preload_ remains true if the Resource is created by preloading with potentially_unused_preload LCPP flag, even if the Resource is actually claimed by subsequent non-preloading request or we have an explicit <link rel=preload>)

File third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
Line 1353, Patchset 4 (Latest): if (ResourceNeedsLoad(resource, policy, should_defer)) {
Hiroshige Hayashizaki . unresolved

How about reverting ShouldDeferResource() changes and:

```
if (ResourceNeedsLoad(resource, policy, should_defer)) {
if (params.IsPotentiallyUnusedPreload()) {
...PostTask...
} else {
StartLoadAndFinishIfFailed();
}
}
```
  • Current code starts (async) start load even if ResourceNeedsLoad() returns false for non-IsPotentiallyUnusedPreload reasons (e.g. MHTML, already loaded resources, etc.).
  • The cases where ShouldDeferResource() == true are different from what we do for IsPotentiallyUnusedPreload().
Open in Gerrit

Related details

Attention is currently required from:
  • Kouhei Ueno
  • Minoru Chikamune
  • Shunya Shishido
  • Yoshisato Yanagisawa
Gerrit-Attention: Shunya Shishido <sisid...@chromium.org>
Gerrit-Attention: Minoru Chikamune <chik...@chromium.org>
Gerrit-Comment-Date: Tue, 16 Apr 2024 06:49:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Yoshisato Yanagisawa <yyana...@chromium.org>
Comment-In-Reply-To: Shunya Shishido <sisid...@chromium.org>
satisfied_requirement
unsatisfied_requirement
open
diffy

Minoru Chikamune (Gerrit)

unread,
Apr 16, 2024, 3:02:39 AMApr 16
to Shunya Shishido, Yoshisato Yanagisawa, Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune, Alex N. Jose, Chromium LUCI CQ, chromium...@chromium.org, Nate Chapin, Yoav Weiss (@Shopify), blink-rev...@chromium.org, blink-...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org
Attention needed from Hiroshige Hayashizaki, Kouhei Ueno, Shunya Shishido and Yoshisato Yanagisawa

Minoru Chikamune added 1 comment

File third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
Line 1353, Patchset 4 (Latest): if (ResourceNeedsLoad(resource, policy, should_defer)) {
Hiroshige Hayashizaki . unresolved

How about reverting ShouldDeferResource() changes and:

```
if (ResourceNeedsLoad(resource, policy, should_defer)) {
if (params.IsPotentiallyUnusedPreload()) {
...PostTask...
} else {
StartLoadAndFinishIfFailed();
}
}
```
  • Current code starts (async) start load even if ResourceNeedsLoad() returns false for non-IsPotentiallyUnusedPreload reasons (e.g. MHTML, already loaded resources, etc.).
  • The cases where ShouldDeferResource() == true are different from what we do for IsPotentiallyUnusedPreload().
Minoru Chikamune

Maybe you meant "reverting ResourceNeedsLoad()"?

Open in Gerrit

Related details

Attention is currently required from:
  • Hiroshige Hayashizaki
  • Kouhei Ueno
  • Shunya Shishido
  • Yoshisato Yanagisawa
Gerrit-Attention: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-Comment-Date: Tue, 16 Apr 2024 07:02:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Hiroshige Hayashizaki <hiro...@chromium.org>
satisfied_requirement
unsatisfied_requirement
open
diffy

Shunya Shishido (Gerrit)

unread,
Apr 16, 2024, 3:12:42 AMApr 16
to Yoshisato Yanagisawa, Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune, Alex N. Jose, Chromium LUCI CQ, chromium...@chromium.org, Nate Chapin, Yoav Weiss (@Shopify), blink-rev...@chromium.org, blink-...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org
Attention needed from Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune and Yoshisato Yanagisawa

Shunya Shishido added 2 comments

File third_party/blink/renderer/platform/loader/fetch/resource.h
Line 559, Patchset 4 (Latest): bool is_potentially_unused_preload_ = false;
Hiroshige Hayashizaki . unresolved

This flag is set but not used in this CL.
Do you have a plan to use this flag in subsequent CLs?

I was writing the comment below but then I noticed this flag is not used, and my comment depends on how this flag is used.

---

Currently `is_potentially_unused_preload_` means "The Resource have preloading request(s) with potentially_unused_preload LCPP flag", but maybe it should be "The Resource has only preloading request(s) with potentially_unused_preload LCPP flag and no other requests"?

(i.e. currently is_potentially_unused_preload_ remains true if the Resource is created by preloading with potentially_unused_preload LCPP flag, even if the Resource is actually claimed by subsequent non-preloading request or we have an explicit <link rel=preload>)

Shunya Shishido

Until the patchset 3 this flag is used, forgot to remove it in the patchset 4.
However, I slightly feel that this flag is still needed.

Let me digest your comment and update the CL again.

File third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
Line 1353, Patchset 4 (Latest): if (ResourceNeedsLoad(resource, policy, should_defer)) {
Hiroshige Hayashizaki . unresolved

How about reverting ShouldDeferResource() changes and:

```
if (ResourceNeedsLoad(resource, policy, should_defer)) {
if (params.IsPotentiallyUnusedPreload()) {
...PostTask...
} else {
StartLoadAndFinishIfFailed();
}
}
```
  • Current code starts (async) start load even if ResourceNeedsLoad() returns false for non-IsPotentiallyUnusedPreload reasons (e.g. MHTML, already loaded resources, etc.).
  • The cases where ShouldDeferResource() == true are different from what we do for IsPotentiallyUnusedPreload().
Minoru Chikamune

Maybe you meant "reverting ResourceNeedsLoad()"?

Shunya Shishido

I believe hiroshige@ meant reverting ShouldDeferResource().
Let me rewrite this.

Open in Gerrit

Related details

Attention is currently required from:
  • Hiroshige Hayashizaki
  • Kouhei Ueno
  • Minoru Chikamune
  • Yoshisato Yanagisawa
Gerrit-Attention: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-Attention: Minoru Chikamune <chik...@chromium.org>
Gerrit-Comment-Date: Tue, 16 Apr 2024 07:12:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Hiroshige Hayashizaki <hiro...@chromium.org>
Comment-In-Reply-To: Minoru Chikamune <chik...@chromium.org>
satisfied_requirement
unsatisfied_requirement
open
diffy

Shunya Shishido (Gerrit)

unread,
Apr 16, 2024, 9:13:26 AMApr 16
to Tricium, Yoshisato Yanagisawa, Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune, Alex N. Jose, Chromium LUCI CQ, chromium...@chromium.org, Nate Chapin, Yoav Weiss (@Shopify), blink-rev...@chromium.org, blink-...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org
Attention needed from Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune and Yoshisato Yanagisawa

Shunya Shishido added 4 comments

File third_party/blink/renderer/core/html/parser/html_preload_scanner.h
Line 185, Patchset 3: Vector<KURL> potentially_unused_preloads_;
Yoshisato Yanagisawa . resolved

How is the number of unused preloads? Will it be less than 100?

Shunya Shishido

The number of records from the database is limited up to 10.
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/predictors/loading_predictor_config.cc;l=63;drc=c3a7009be69ddc4492e8bb4e6dcb12bb0b1d3b71;bpv=1;bpt=1

Actually unused preloads are about 1.7 resources per page from UMA. I believe this is enough and using vector makes sense.

Hiroshige Hayashizaki

Probably it's helpful to write this information in the source code comment.

Shunya Shishido

Done

File third_party/blink/renderer/core/html/parser/html_preload_scanner.cc
Line 1163, Patchset 4: locators, skip_preload_scan, potentially_unused_preloads);
Hiroshige Hayashizaki . resolved

nit: std::move()

Shunya Shishido

Done

File third_party/blink/renderer/platform/loader/fetch/resource.h
Line 559, Patchset 4: bool is_potentially_unused_preload_ = false;
Hiroshige Hayashizaki . unresolved

This flag is set but not used in this CL.
Do you have a plan to use this flag in subsequent CLs?

I was writing the comment below but then I noticed this flag is not used, and my comment depends on how this flag is used.

---

Currently `is_potentially_unused_preload_` means "The Resource have preloading request(s) with potentially_unused_preload LCPP flag", but maybe it should be "The Resource has only preloading request(s) with potentially_unused_preload LCPP flag and no other requests"?

(i.e. currently is_potentially_unused_preload_ remains true if the Resource is created by preloading with potentially_unused_preload LCPP flag, even if the Resource is actually claimed by subsequent non-preloading request or we have an explicit <link rel=preload>)

Shunya Shishido

Until the patchset 3 this flag is used, forgot to remove it in the patchset 4.
However, I slightly feel that this flag is still needed.

Let me digest your comment and update the CL again.

Shunya Shishido

Let me keep `is_potentially_unused_preload_`.

even if the Resource is actually claimed by subsequent non-preloading request or we have an explicit <link rel=preload>)

Updated to get `is_potentially_unused_preload_` false in `Resource::MatchPreload()`, which is called if the subsequent request is non-preloading. Regarding an explicit <link rel=preload>, that's what we'd like to make some delay, so WAI. Added more tests to make the behavior explicit.

File third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
Line 1353, Patchset 4: if (ResourceNeedsLoad(resource, policy, should_defer)) {
Hiroshige Hayashizaki . resolved

How about reverting ShouldDeferResource() changes and:

```
if (ResourceNeedsLoad(resource, policy, should_defer)) {
if (params.IsPotentiallyUnusedPreload()) {
...PostTask...
} else {
StartLoadAndFinishIfFailed();
}
}
```
  • Current code starts (async) start load even if ResourceNeedsLoad() returns false for non-IsPotentiallyUnusedPreload reasons (e.g. MHTML, already loaded resources, etc.).
  • The cases where ShouldDeferResource() == true are different from what we do for IsPotentiallyUnusedPreload().
Minoru Chikamune

Maybe you meant "reverting ResourceNeedsLoad()"?

Shunya Shishido

I believe hiroshige@ meant reverting ShouldDeferResource().
Let me rewrite this.

Shunya Shishido

Done

Open in Gerrit

Related details

Attention is currently required from:
  • Hiroshige Hayashizaki
  • Kouhei Ueno
  • Minoru Chikamune
  • Yoshisato Yanagisawa
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Ief7451fab7602be2cd4aa9660a4568e418eff19e
Gerrit-Change-Number: 5447206
Gerrit-PatchSet: 6
Gerrit-Owner: Shunya Shishido <sisid...@chromium.org>
Gerrit-Reviewer: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
Gerrit-Reviewer: Minoru Chikamune <chik...@chromium.org>
Gerrit-Reviewer: Shunya Shishido <sisid...@chromium.org>
Gerrit-Reviewer: Yoshisato Yanagisawa <yyana...@chromium.org>
Gerrit-CC: Alex N. Jose <ale...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Yoav Weiss (@Shopify) <yoav...@chromium.org>
Gerrit-Attention: Kouhei Ueno <kou...@chromium.org>
Gerrit-Attention: Yoshisato Yanagisawa <yyana...@chromium.org>
Gerrit-Attention: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-Attention: Minoru Chikamune <chik...@chromium.org>
Gerrit-Comment-Date: Tue, 16 Apr 2024 13:13:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Yoshisato Yanagisawa <yyana...@chromium.org>
Comment-In-Reply-To: Shunya Shishido <sisid...@chromium.org>
satisfied_requirement
unsatisfied_requirement
open
diffy

Yoshisato Yanagisawa (Gerrit)

unread,
Apr 16, 2024, 7:55:06 PMApr 16
to Shunya Shishido, Tricium, Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune, Alex N. Jose, Chromium LUCI CQ, chromium...@chromium.org, Nate Chapin, Yoav Weiss (@Shopify), blink-rev...@chromium.org, blink-...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org
Attention needed from Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune and Shunya Shishido

Yoshisato Yanagisawa added 2 comments

Patchset-level comments
File-level comment, Patchset 4:
Yoshisato Yanagisawa . resolved

still lgtm

File third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
Line 1361, Patchset 6 (Latest): } else if (!resource->IsPotentiallyUnusedPreload()) {
Yoshisato Yanagisawa . unresolved

I feel this part is not trivial. Questions I have had are:
1. why you need to check both params.IsPotentiallyUnusedPreload() and resource->IsPotentiallyUnusedPreload() here and there. What the difference between them? Moreover, why you need two inputs?
2. why only resource->IsPotentiallyUnusedPreload() result is respected?
3. why is it fine not to load `!resource->IsPotentiallyUnusedPreload()` case?

Open in Gerrit

Related details

Attention is currently required from:
  • Hiroshige Hayashizaki
  • Kouhei Ueno
  • Minoru Chikamune
  • Shunya Shishido
Gerrit-Attention: Shunya Shishido <sisid...@chromium.org>
Gerrit-Attention: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-Attention: Minoru Chikamune <chik...@chromium.org>
Gerrit-Comment-Date: Tue, 16 Apr 2024 23:54:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Shunya Shishido (Gerrit)

unread,
Apr 18, 2024, 4:49:41 AMApr 18
to Tricium, Yoshisato Yanagisawa, Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune, Alex N. Jose, Chromium LUCI CQ, chromium...@chromium.org, Nate Chapin, Yoav Weiss (@Shopify), blink-rev...@chromium.org, blink-...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org
Attention needed from Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune and Yoshisato Yanagisawa

Shunya Shishido added 4 comments

File third_party/blink/renderer/platform/loader/fetch/resource.h
Line 559, Patchset 4: bool is_potentially_unused_preload_ = false;
Hiroshige Hayashizaki . resolved

This flag is set but not used in this CL.
Do you have a plan to use this flag in subsequent CLs?

I was writing the comment below but then I noticed this flag is not used, and my comment depends on how this flag is used.

---

Currently `is_potentially_unused_preload_` means "The Resource have preloading request(s) with potentially_unused_preload LCPP flag", but maybe it should be "The Resource has only preloading request(s) with potentially_unused_preload LCPP flag and no other requests"?

(i.e. currently is_potentially_unused_preload_ remains true if the Resource is created by preloading with potentially_unused_preload LCPP flag, even if the Resource is actually claimed by subsequent non-preloading request or we have an explicit <link rel=preload>)

Shunya Shishido

Until the patchset 3 this flag is used, forgot to remove it in the patchset 4.
However, I slightly feel that this flag is still needed.

Let me digest your comment and update the CL again.

Shunya Shishido

Let me keep `is_potentially_unused_preload_`.

even if the Resource is actually claimed by subsequent non-preloading request or we have an explicit <link rel=preload>)

Updated to get `is_potentially_unused_preload_` false in `Resource::MatchPreload()`, which is called if the subsequent request is non-preloading. Regarding an explicit <link rel=preload>, that's what we'd like to make some delay, so WAI. Added more tests to make the behavior explicit.

Shunya Shishido

As we discussed, we don't need `is_potentially_unused_preload_` in Resource anymore, and use the potentially unused preload list from the LCPP through FetchContext.

File third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
Line 1231, Patchset 9 (Latest): ShouldDeferResource(resource_type, params, is_potentially_unused_preload);
Shunya Shishido . resolved

I ended up to pass the `is_potentially_unused_preload` info outside of `ShouldDeferResource()`, because this flag is needed to schedule the loading task later in this method.

Line 1361, Patchset 6: } else if (!resource->IsPotentiallyUnusedPreload()) {
Yoshisato Yanagisawa . resolved

I feel this part is not trivial. Questions I have had are:
1. why you need to check both params.IsPotentiallyUnusedPreload() and resource->IsPotentiallyUnusedPreload() here and there. What the difference between them? Moreover, why you need two inputs?
2. why only resource->IsPotentiallyUnusedPreload() result is respected?
3. why is it fine not to load `!resource->IsPotentiallyUnusedPreload()` case?

Shunya Shishido

I think comment became stale in the latest change, but let me try to reply:

`params` is the fetch parameter, which is the params for the current resource fetch. On the other hand, `resource` may come from the previous resource fetch, for example preload, memory cache etc. I used to use them to distinguish whether the current resource fetch is for the preload request or subsequent request which use previous preloaded resource.

`resource->IsPotentiallyUnusedPreload()` used to indicate "the resource is already created by the preload effort, but the actual load is scheduled, but not started yet". In that case the subsequent request doesn't start the fetch.

In the latest patch, subsequent requests start loading, and scheduled loading won't be dispatched (it's blocked in L2455).

File third_party/blink/renderer/platform/loader/fetch/resource_fetcher_test.cc
Line 1752, Patchset 9 (Latest):TEST_F(ResourceFetcherTest, IsPotentiallyUnusedPreload) {
Shunya Shishido . resolved

I added unit tests instead of web_tests, because handling PostTask is much easier.

Open in Gerrit

Related details

Attention is currently required from:
  • Hiroshige Hayashizaki
  • Kouhei Ueno
  • Minoru Chikamune
  • Yoshisato Yanagisawa
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Ief7451fab7602be2cd4aa9660a4568e418eff19e
Gerrit-Change-Number: 5447206
Gerrit-PatchSet: 9
Gerrit-Owner: Shunya Shishido <sisid...@chromium.org>
Gerrit-Reviewer: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
Gerrit-Reviewer: Minoru Chikamune <chik...@chromium.org>
Gerrit-Reviewer: Shunya Shishido <sisid...@chromium.org>
Gerrit-Reviewer: Yoshisato Yanagisawa <yyana...@chromium.org>
Gerrit-CC: Alex N. Jose <ale...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Yoav Weiss (@Shopify) <yoav...@chromium.org>
Gerrit-Attention: Kouhei Ueno <kou...@chromium.org>
Gerrit-Attention: Yoshisato Yanagisawa <yyana...@chromium.org>
Gerrit-Attention: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-Attention: Minoru Chikamune <chik...@chromium.org>
Gerrit-Comment-Date: Thu, 18 Apr 2024 08:49:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Shunya Shishido (Gerrit)

unread,
Apr 18, 2024, 5:01:19 AMApr 18
to Tricium, Yoshisato Yanagisawa, Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune, Alex N. Jose, Chromium LUCI CQ, chromium...@chromium.org, Nate Chapin, Yoav Weiss (@Shopify), blink-rev...@chromium.org, blink-...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org
Attention needed from Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune and Yoshisato Yanagisawa

Shunya Shishido voted and added 1 comment

Votes added by Shunya Shishido

Commit-Queue+1

1 comment

File third_party/blink/renderer/core/lcp_critical_path_predictor/lcp_critical_path_predictor.cc
Line 39, Patchset 10 (Latest): !preconnected_origins_.empty() || !unused_preloads_.empty();
Shunya Shishido . resolved

This is not directly related to this CL, but maybe I should add it.

Open in Gerrit

Related details

Attention is currently required from:
  • Hiroshige Hayashizaki
  • Kouhei Ueno
  • Minoru Chikamune
  • Yoshisato Yanagisawa
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Ief7451fab7602be2cd4aa9660a4568e418eff19e
Gerrit-Change-Number: 5447206
Gerrit-PatchSet: 10
Gerrit-Owner: Shunya Shishido <sisid...@chromium.org>
Gerrit-Reviewer: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
Gerrit-Reviewer: Minoru Chikamune <chik...@chromium.org>
Gerrit-Reviewer: Shunya Shishido <sisid...@chromium.org>
Gerrit-Reviewer: Yoshisato Yanagisawa <yyana...@chromium.org>
Gerrit-CC: Alex N. Jose <ale...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Yoav Weiss (@Shopify) <yoav...@chromium.org>
Gerrit-Attention: Kouhei Ueno <kou...@chromium.org>
Gerrit-Attention: Yoshisato Yanagisawa <yyana...@chromium.org>
Gerrit-Attention: Hiroshige Hayashizaki <hiro...@chromium.org>
Gerrit-Attention: Minoru Chikamune <chik...@chromium.org>
Gerrit-Comment-Date: Thu, 18 Apr 2024 09:01:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Minoru Chikamune (Gerrit)

unread,
Apr 18, 2024, 5:57:51 AMApr 18
to Shunya Shishido, Tricium, Yoshisato Yanagisawa, Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune, Alex N. Jose, Chromium LUCI CQ, chromium...@chromium.org, Nate Chapin, Yoav Weiss (@Shopify), blink-rev...@chromium.org, blink-...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org
Attention needed from Hiroshige Hayashizaki, Kouhei Ueno, Shunya Shishido and Yoshisato Yanagisawa

Minoru Chikamune added 1 comment

File third_party/blink/renderer/core/lcp_critical_path_predictor/lcp_critical_path_predictor.cc
Line 39, Patchset 10 (Latest): !preconnected_origins_.empty() || !unused_preloads_.empty();
Shunya Shishido . unresolved

This is not directly related to this CL, but maybe I should add it.

Minoru Chikamune

This is not directly related to this CL, but maybe I should add it.

I think you shouldn't update this. This is only used by the following place to record `Blink.LCPP.PotentiallyLCPResourcePriorityBoosts2`. Your experiment doesn't boost fetch priority.

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc;l=2836;drc=9e110c64fee9c391021dc81944912f906e6ba5d5

Open in Gerrit

Related details

Attention is currently required from:
  • Hiroshige Hayashizaki
  • Kouhei Ueno
  • Shunya Shishido
  • Yoshisato Yanagisawa
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ief7451fab7602be2cd4aa9660a4568e418eff19e
    Gerrit-Change-Number: 5447206
    Gerrit-PatchSet: 10
    Gerrit-Owner: Shunya Shishido <sisid...@chromium.org>
    Gerrit-Reviewer: Hiroshige Hayashizaki <hiro...@chromium.org>
    Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
    Gerrit-Reviewer: Minoru Chikamune <chik...@chromium.org>
    Gerrit-Reviewer: Shunya Shishido <sisid...@chromium.org>
    Gerrit-Reviewer: Yoshisato Yanagisawa <yyana...@chromium.org>
    Gerrit-CC: Alex N. Jose <ale...@chromium.org>
    Gerrit-CC: Nate Chapin <jap...@chromium.org>
    Gerrit-CC: Yoav Weiss (@Shopify) <yoav...@chromium.org>
    Gerrit-Attention: Kouhei Ueno <kou...@chromium.org>
    Gerrit-Attention: Yoshisato Yanagisawa <yyana...@chromium.org>
    Gerrit-Attention: Shunya Shishido <sisid...@chromium.org>
    Gerrit-Attention: Hiroshige Hayashizaki <hiro...@chromium.org>
    Gerrit-Comment-Date: Thu, 18 Apr 2024 09:57:40 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Shunya Shishido <sisid...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Shunya Shishido (Gerrit)

    unread,
    Apr 18, 2024, 6:49:05 AMApr 18
    to Tricium, Yoshisato Yanagisawa, Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune, Alex N. Jose, Chromium LUCI CQ, chromium...@chromium.org, Nate Chapin, Yoav Weiss (@Shopify), blink-rev...@chromium.org, blink-...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org
    Attention needed from Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune and Yoshisato Yanagisawa

    Shunya Shishido added 1 comment

    File third_party/blink/renderer/core/lcp_critical_path_predictor/lcp_critical_path_predictor.cc
    Line 39, Patchset 10: !preconnected_origins_.empty() || !unused_preloads_.empty();
    Shunya Shishido . resolved

    This is not directly related to this CL, but maybe I should add it.

    Minoru Chikamune

    This is not directly related to this CL, but maybe I should add it.

    I think you shouldn't update this. This is only used by the following place to record `Blink.LCPP.PotentiallyLCPResourcePriorityBoosts2`. Your experiment doesn't boost fetch priority.

    https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc;l=2836;drc=9e110c64fee9c391021dc81944912f906e6ba5d5

    Shunya Shishido

    OK removed. But that sounds the method name is confusing then...

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Hiroshige Hayashizaki
    • Kouhei Ueno
    • Minoru Chikamune
    • Yoshisato Yanagisawa
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ief7451fab7602be2cd4aa9660a4568e418eff19e
    Gerrit-Change-Number: 5447206
    Gerrit-PatchSet: 11
    Gerrit-Owner: Shunya Shishido <sisid...@chromium.org>
    Gerrit-Reviewer: Hiroshige Hayashizaki <hiro...@chromium.org>
    Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
    Gerrit-Reviewer: Minoru Chikamune <chik...@chromium.org>
    Gerrit-Reviewer: Shunya Shishido <sisid...@chromium.org>
    Gerrit-Reviewer: Yoshisato Yanagisawa <yyana...@chromium.org>
    Gerrit-CC: Alex N. Jose <ale...@chromium.org>
    Gerrit-CC: Nate Chapin <jap...@chromium.org>
    Gerrit-CC: Yoav Weiss (@Shopify) <yoav...@chromium.org>
    Gerrit-Attention: Kouhei Ueno <kou...@chromium.org>
    Gerrit-Attention: Yoshisato Yanagisawa <yyana...@chromium.org>
    Gerrit-Attention: Hiroshige Hayashizaki <hiro...@chromium.org>
    Gerrit-Attention: Minoru Chikamune <chik...@chromium.org>
    Gerrit-Comment-Date: Thu, 18 Apr 2024 10:48:53 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Shunya Shishido <sisid...@chromium.org>
    Comment-In-Reply-To: Minoru Chikamune <chik...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Kouhei Ueno (Gerrit)

    unread,
    Apr 18, 2024, 11:22:44 AMApr 18
    to Shunya Shishido, Tricium, Yoshisato Yanagisawa, Hiroshige Hayashizaki, Minoru Chikamune, Alex N. Jose, Chromium LUCI CQ, chromium...@chromium.org, Nate Chapin, Yoav Weiss (@Shopify), blink-rev...@chromium.org, blink-...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org
    Attention needed from Hiroshige Hayashizaki, Minoru Chikamune, Shunya Shishido and Yoshisato Yanagisawa

    Kouhei Ueno added 1 comment

    File third_party/blink/renderer/platform/loader/fetch/fetch_context.h
    Line 199, Patchset 11 (Latest): // predcitor, which is attached to the frame. This returns nullptr for Shared
    Kouhei Ueno . unresolved

    empty Vector

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Hiroshige Hayashizaki
    • Minoru Chikamune
    • Shunya Shishido
    • Yoshisato Yanagisawa
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: Ief7451fab7602be2cd4aa9660a4568e418eff19e
      Gerrit-Change-Number: 5447206
      Gerrit-PatchSet: 11
      Gerrit-Owner: Shunya Shishido <sisid...@chromium.org>
      Gerrit-Reviewer: Hiroshige Hayashizaki <hiro...@chromium.org>
      Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
      Gerrit-Reviewer: Minoru Chikamune <chik...@chromium.org>
      Gerrit-Reviewer: Shunya Shishido <sisid...@chromium.org>
      Gerrit-Reviewer: Yoshisato Yanagisawa <yyana...@chromium.org>
      Gerrit-CC: Alex N. Jose <ale...@chromium.org>
      Gerrit-CC: Nate Chapin <jap...@chromium.org>
      Gerrit-CC: Yoav Weiss (@Shopify) <yoav...@chromium.org>
      Gerrit-Attention: Yoshisato Yanagisawa <yyana...@chromium.org>
      Gerrit-Attention: Shunya Shishido <sisid...@chromium.org>
      Gerrit-Attention: Hiroshige Hayashizaki <hiro...@chromium.org>
      Gerrit-Attention: Minoru Chikamune <chik...@chromium.org>
      Gerrit-Comment-Date: Thu, 18 Apr 2024 15:22:31 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Shunya Shishido (Gerrit)

      unread,
      Apr 18, 2024, 8:33:14 PMApr 18
      to Tricium, Yoshisato Yanagisawa, Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune, Alex N. Jose, Chromium LUCI CQ, chromium...@chromium.org, Nate Chapin, Yoav Weiss (@Shopify), blink-rev...@chromium.org, blink-...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org
      Attention needed from Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune and Yoshisato Yanagisawa

      Shunya Shishido added 1 comment

      File third_party/blink/renderer/platform/loader/fetch/fetch_context.h
      Line 199, Patchset 11: // predcitor, which is attached to the frame. This returns nullptr for Shared
      Kouhei Ueno . resolved

      empty Vector

      Shunya Shishido

      Done

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Hiroshige Hayashizaki
      • Kouhei Ueno
      • Minoru Chikamune
      • Yoshisato Yanagisawa
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Review
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: Ief7451fab7602be2cd4aa9660a4568e418eff19e
      Gerrit-Change-Number: 5447206
      Gerrit-PatchSet: 12
      Gerrit-Owner: Shunya Shishido <sisid...@chromium.org>
      Gerrit-Reviewer: Hiroshige Hayashizaki <hiro...@chromium.org>
      Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
      Gerrit-Reviewer: Minoru Chikamune <chik...@chromium.org>
      Gerrit-Reviewer: Shunya Shishido <sisid...@chromium.org>
      Gerrit-Reviewer: Yoshisato Yanagisawa <yyana...@chromium.org>
      Gerrit-CC: Alex N. Jose <ale...@chromium.org>
      Gerrit-CC: Nate Chapin <jap...@chromium.org>
      Gerrit-CC: Yoav Weiss (@Shopify) <yoav...@chromium.org>
      Gerrit-Attention: Kouhei Ueno <kou...@chromium.org>
      Gerrit-Attention: Yoshisato Yanagisawa <yyana...@chromium.org>
      Gerrit-Attention: Hiroshige Hayashizaki <hiro...@chromium.org>
      Gerrit-Attention: Minoru Chikamune <chik...@chromium.org>
      Gerrit-Comment-Date: Fri, 19 Apr 2024 00:33:03 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      Comment-In-Reply-To: Kouhei Ueno <kou...@chromium.org>
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Minoru Chikamune (Gerrit)

      unread,
      Apr 18, 2024, 9:43:48 PMApr 18
      to Shunya Shishido, Tricium, Yoshisato Yanagisawa, Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune, Alex N. Jose, Chromium LUCI CQ, chromium...@chromium.org, Nate Chapin, Yoav Weiss (@Shopify), blink-rev...@chromium.org, blink-...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org
      Attention needed from Hiroshige Hayashizaki, Kouhei Ueno, Shunya Shishido and Yoshisato Yanagisawa

      Minoru Chikamune added 1 comment

      Patchset-level comments
      File-level comment, Patchset 12 (Latest):
      Minoru Chikamune . unresolved

      Perhaps kLCPPDeferUnusedPreload flag is not checked in the read path? If so, please add it to disable this feature in Control group.

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Hiroshige Hayashizaki
      • Kouhei Ueno
      • Shunya Shishido
      • Yoshisato Yanagisawa
      Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement is not satisfiedCode-Review
        • requirement is not satisfiedNo-Unresolved-Comments
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: Ief7451fab7602be2cd4aa9660a4568e418eff19e
        Gerrit-Change-Number: 5447206
        Gerrit-PatchSet: 12
        Gerrit-Owner: Shunya Shishido <sisid...@chromium.org>
        Gerrit-Reviewer: Hiroshige Hayashizaki <hiro...@chromium.org>
        Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
        Gerrit-Reviewer: Minoru Chikamune <chik...@chromium.org>
        Gerrit-Reviewer: Shunya Shishido <sisid...@chromium.org>
        Gerrit-Reviewer: Yoshisato Yanagisawa <yyana...@chromium.org>
        Gerrit-CC: Alex N. Jose <ale...@chromium.org>
        Gerrit-CC: Nate Chapin <jap...@chromium.org>
        Gerrit-CC: Yoav Weiss (@Shopify) <yoav...@chromium.org>
        Gerrit-Attention: Kouhei Ueno <kou...@chromium.org>
        Gerrit-Attention: Yoshisato Yanagisawa <yyana...@chromium.org>
        Gerrit-Attention: Shunya Shishido <sisid...@chromium.org>
        Gerrit-Attention: Hiroshige Hayashizaki <hiro...@chromium.org>
        Gerrit-Comment-Date: Fri, 19 Apr 2024 01:43:37 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: No
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Shunya Shishido (Gerrit)

        unread,
        Apr 18, 2024, 9:49:20 PMApr 18
        to Tricium, Yoshisato Yanagisawa, Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune, Alex N. Jose, Chromium LUCI CQ, chromium...@chromium.org, Nate Chapin, Yoav Weiss (@Shopify), blink-rev...@chromium.org, blink-...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org
        Attention needed from Hiroshige Hayashizaki, Kouhei Ueno, Minoru Chikamune and Yoshisato Yanagisawa

        Shunya Shishido added 1 comment

        Patchset-level comments
        Minoru Chikamune . resolved

        Perhaps kLCPPDeferUnusedPreload flag is not checked in the read path? If so, please add it to disable this feature in Control group.

        Attention is currently required from:
        • Hiroshige Hayashizaki
        • Kouhei Ueno
        • Minoru Chikamune
        • Yoshisato Yanagisawa
        Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement is not satisfiedCode-Review
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: Ief7451fab7602be2cd4aa9660a4568e418eff19e
        Gerrit-Change-Number: 5447206
        Gerrit-PatchSet: 12
        Gerrit-Owner: Shunya Shishido <sisid...@chromium.org>
        Gerrit-Reviewer: Hiroshige Hayashizaki <hiro...@chromium.org>
        Gerrit-Reviewer: Kouhei Ueno <kou...@chromium.org>
        Gerrit-Reviewer: Minoru Chikamune <chik...@chromium.org>
        Gerrit-Reviewer: Shunya Shishido <sisid...@chromium.org>
        Gerrit-Reviewer: Yoshisato Yanagisawa <yyana...@chromium.org>
        Gerrit-CC: Alex N. Jose <ale...@chromium.org>
        Gerrit-CC: Nate Chapin <jap...@chromium.org>
        Gerrit-CC: Yoav Weiss (@Shopify) <yoav...@chromium.org>
        Gerrit-Attention: Kouhei Ueno <kou...@chromium.org>
        Gerrit-Attention: Yoshisato Yanagisawa <yyana...@chromium.org>
        Gerrit-Attention: Hiroshige Hayashizaki <hiro...@chromium.org>
        Gerrit-Attention: Minoru Chikamune <chik...@chromium.org>
        Gerrit-Comment-Date: Fri, 19 Apr 2024 01:49:09 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: No
        Comment-In-Reply-To: Minoru Chikamune <chik...@chromium.org>
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy