Prerender: Retrigger UpdateSpeculationCandidates on BFCache restoration for prerender [chromium/src : main]
Taiyo Mizuhashi (Gerrit)
Attention is currently required from: Taiyo Mizuhashi.
Taiyo Mizuhashi uploaded patch set #10 to this change.
Prerender: Retrigger UpdateSpeculationCandidates on BFCache restoration for prerender
Before this CL, speculation rules preloading is not processed again on
the pages restored from BFCache. This CL introduces a feature to
retrigger speculation rules on BFCache restoration for prerender by retriggering `DocumentSpeculationRules::UpdateSpeculationCandidates()`.
For more information, please see the design doc on crbug.
Note that
- This project tries to target preload(both prefetch/
prerender). This CL focuses on prerender first.
- This CL focuses on verifying the default prerendering behavior first
(list rules, eagerness is kEager). This CL also expects to be worked with other prerendering cases and these tests will be added in the
following CL.
Bug: 1449163
Change-Id: I9b3390eee68fb075a472558d4c79d0df861bf49c
---
M content/browser/preloading/prerender/prerender_browsertest.cc
M third_party/blink/common/features.cc
M third_party/blink/public/common/features.h
M third_party/blink/renderer/core/exported/web_view_impl.cc
M third_party/blink/renderer/core/speculation_rules/document_speculation_rules.cc
M third_party/blink/renderer/core/speculation_rules/document_speculation_rules.h
6 files changed, 99 insertions(+), 14 deletions(-)
To view, visit change 4578345. To unsubscribe, or for help writing mail filters, visit settings.
Taiyo Mizuhashi (Gerrit)
Attention is currently required from: Hiroki Nakagawa, Huanpo Lin.
Taiyo Mizuhashi would like Hiroki Nakagawa and Huanpo Lin to review this change.
Prerender: Retrigger UpdateSpeculationCandidates on BFCache restoration for prerender
Before this CL, speculation rules preloading is not processed again on
the pages restored from BFCache. This CL introduces a feature to
retrigger speculation rules on BFCache restoration for prerender by retriggering `DocumentSpeculationRules::UpdateSpeculationCandidates()`.
For more information, please see the design doc on crbug.
Note that
- This project tries to target preload(both prefetch/
prerender). This CL focuses on prerender first.
- This CL focuses on verifying the default prerendering behavior first
(list rules, eagerness is kEager). This CL also expects to be worked with other prerendering cases and these tests will be added in the
following CL.
Bug: 1449163
Change-Id: I9b3390eee68fb075a472558d4c79d0df861bf49c
---
M content/browser/preloading/prerender/prerender_browsertest.cc
M third_party/blink/common/features.cc
M third_party/blink/public/common/features.h
M third_party/blink/renderer/core/exported/web_view_impl.cc
M third_party/blink/renderer/core/speculation_rules/document_speculation_rules.cc
M third_party/blink/renderer/core/speculation_rules/document_speculation_rules.h
6 files changed, 100 insertions(+), 14 deletions(-)
Taiyo Mizuhashi (Gerrit)
Attention is currently required from: Hiroki Nakagawa, Huanpo Lin.
1 comment:
Patchset:
+nhiroki@, robertlin@: Can you please review this CL first? Thanks!
To view, visit change 4578345. To unsubscribe, or for help writing mail filters, visit settings.
Hiroki Nakagawa (Gerrit)
Attention is currently required from: Huanpo Lin, Taiyo Mizuhashi.
Patch set 11:Code-Review +1
9 comments:
Patchset:
LGTM, thanks!
File content/browser/preloading/prerender/prerender_browsertest.cc:
Patch Set #11, Line 7614:
falseCan you keep this comment? `/*ignore_outstanding_network_request=*/false`
Patch Set #11, Line 7651:
// BFCache restoration.Now we could remove this TODO comment?
Patch Set #11, Line 7685:
// When kRetriggerPreloadingOnBFCacheRestoration is enabled:Instead of explaining in comments, how about representing this as code like this?
```
// In PrerenderBackForwardCacheRestorationBrowserTest
bool ShouldRetriggerOnRestoration() {
return GetParam();
}// In test
if (ShouldRetriggerOnRestoration()) {
...
} else {
...
}
```"retriggering." (adding a trailing period)
Patch Set #11, Line 7700:
// When kRetriggerPreloadingOnBFCacheRestoration is disabled:Ditto. This comment may not be necessary if the function name is clear.
Patch Set #11, Line 7709:
// BFCache restoration.Ditto.
Patch Set #11, Line 7745:
// When kRetriggerPreloadingOnBFCacheRestoration is enabled:Ditto.
File third_party/blink/renderer/core/speculation_rules/document_speculation_rules.cc:
Patch Set #11, Line 434:
// TODO(crbug.com/1449163): Add implementation for Prefetch.Can you briefly explain the current behavior on prefetch? Otherwise, code readers don't understand what we need to implement for prefetch. For example,
```
// This retriggers only speculation rules prerender, not speculation rules
// prefetch, as prefetch has a different lifecycle model from prerender.
// TODO(crbug.com/1449163): Add implementation for Prefetch.
```(Feel free to rewrite)
By the way, do we plan to implement something for prefetch here? If not and instead we're going to implement something in the browser side, then we may not have to put TODO comments here in the first place.
To view, visit change 4578345. To unsubscribe, or for help writing mail filters, visit settings.
Hiroki Nakagawa (Gerrit)
Attention is currently required from: Huanpo Lin, Taiyo Mizuhashi.
Taiyo Mizuhashi has uploaded this change for review.
Prerender: Retrigger UpdateSpeculationCandidates on BFCache restoration for prerender
Before this CL, speculation rules preloading is not processed again on
the pages restored from BFCache. This CL introduces a feature to
retrigger speculation rules on BFCache restoration for prerender by retriggering `DocumentSpeculationRules::UpdateSpeculationCandidates()`.
For more information, please see the design doc on crbug.
Note that
- This project tries to target preload(both prefetch/
prerender). This CL focuses on prerender first.
- This CL focuses on verifying the default prerendering behavior first
(list rules, eagerness is kEager). This CL also expects to be worked with other prerendering cases and these tests will be added in the
following CL.
Bug: 1449163
Change-Id: I9b3390eee68fb075a472558d4c79d0df861bf49c
---
M content/browser/preloading/prerender/prerender_browsertest.cc
M third_party/blink/common/features.cc
M third_party/blink/public/common/features.h
M third_party/blink/renderer/core/exported/web_view_impl.cc
M third_party/blink/renderer/core/speculation_rules/document_speculation_rules.cc
M third_party/blink/renderer/core/speculation_rules/document_speculation_rules.h
6 files changed, 100 insertions(+), 14 deletions(-)
To view, visit change 4578345. To unsubscribe, or for help writing mail filters, visit settings.
Huanpo Lin (Gerrit)
Attention is currently required from: Hiroki Nakagawa, Taiyo Mizuhashi.
Patch set 11:Code-Review +1
3 comments:
Patchset:
LGTM, thanks!
File content/browser/preloading/prerender/prerender_browsertest.cc:
Patch Set #11, Line 7685:
// When kRetriggerPreloadingOnBFCacheRestoration is enabled:Instead of explaining in comments, how about representing this as code like this? […]
+1
File third_party/blink/renderer/core/speculation_rules/document_speculation_rules.cc:
Patch Set #11, Line 434:
// TODO(crbug.com/1449163): Add implementation for Prefetch.
Can you briefly explain the current behavior on prefetch? Otherwise, code readers don't understand w […]
A bit curious about whether this TODO is desired or any better place to put this, because in the design doc says simply re-trigger of this function call won't be sufficient for prefetch.
To view, visit change 4578345. To unsubscribe, or for help writing mail filters, visit settings.
Taiyo Mizuhashi (Gerrit)
Attention is currently required from: Hiroki Nakagawa.
8 comments:
Patchset:
Thank you for the review!
File content/browser/preloading/prerender/prerender_browsertest.cc:
Can you keep this comment? `/*ignore_outstanding_network_request=*/false`
Done
Patch Set #11, Line 7651:
// BFCache restoration.Now we could remove this TODO comment?
Done
Patch Set #11, Line 7685:
// When kRetriggerPreloadingOnBFCacheRestoration is enabled:
+1
Done
Patch Set #11, Line 7700:
// When kRetriggerPreloadingOnBFCacheRestoration is disabled:Ditto. This comment may not be necessary if the function name is clear.
Done
Patch Set #11, Line 7709:
// BFCache restoration.Ditto.
Done
Patch Set #11, Line 7745:
// When kRetriggerPreloadingOnBFCacheRestoration is enabled:Ditto.
Done
File third_party/blink/renderer/core/speculation_rules/document_speculation_rules.cc:
Patch Set #11, Line 434:
// TODO(crbug.com/1449163): Add implementation for Prefetch.
A bit curious about whether this TODO is desired or any better place to put this, because in the des […]
My thought was it would be good to have a TODO comment indicating that this CL is not sufficient for prefetch, but yes I also believed this is not the best place for a comment, as you mentioned.
I now believe that the CL description is sufficient to have this information, so I'm going to remove it. What are your thoughts?
To view, visit change 4578345. To unsubscribe, or for help writing mail filters, visit settings.
Taiyo Mizuhashi (Gerrit)
Attention is currently required from: Hiroki Nakagawa.
1 comment:
File content/browser/preloading/prerender/prerender_browsertest.cc:
"retriggering. […]
Done
To view, visit change 4578345. To unsubscribe, or for help writing mail filters, visit settings.
Huanpo Lin (Gerrit)
Attention is currently required from: Hiroki Nakagawa, Taiyo Mizuhashi.
Patch set 12:Code-Review +1
1 comment:
File third_party/blink/renderer/core/speculation_rules/document_speculation_rules.cc:
Patch Set #11, Line 434:
// TODO(crbug.com/1449163): Add implementation for Prefetch.
My thought was it would be good to have a TODO comment indicating that this CL is not sufficient for […]
IMHO, it is OK to remove it here since the crbug says that preloading is the scope. It should be sufficient to mention it in the description.
To view, visit change 4578345. To unsubscribe, or for help writing mail filters, visit settings.
Taiyo Mizuhashi (Gerrit)
Attention is currently required from: Hiroki Nakagawa, Ken Okada.
Taiyo Mizuhashi would like Ken Okada to review this change.
Prerender: Retrigger UpdateSpeculationCandidates on BFCache restoration for prerender
Before this CL, speculation rules preloading is not processed again on
the pages restored from BFCache. This CL introduces a feature to
retrigger speculation rules on BFCache restoration for prerender by retriggering `DocumentSpeculationRules::UpdateSpeculationCandidates()`.
For more information, please see the design doc on crbug.
Note that
- This project tries to target preload(both prefetch/
prerender). This CL focuses on prerender first.
- This CL focuses on verifying the default prerendering behavior first
(list rules, eagerness is kEager). This CL also expects to be worked with other prerendering cases and these tests will be added in the
following CL.
Bug: 1449163
Change-Id: I9b3390eee68fb075a472558d4c79d0df861bf49c
---
M content/browser/preloading/prerender/prerender_browsertest.cc
M third_party/blink/common/features.cc
M third_party/blink/public/common/features.h
M third_party/blink/renderer/core/exported/web_view_impl.cc
M third_party/blink/renderer/core/speculation_rules/document_speculation_rules.cc
M third_party/blink/renderer/core/speculation_rules/document_speculation_rules.h
6 files changed, 93 insertions(+), 18 deletions(-)
Taiyo Mizuhashi (Gerrit)
Attention is currently required from: Hiroki Nakagawa, Ken Okada.
1 comment:
Patchset:
+kenoss@: Hello, can you please review this with DevTools perspective?
I confirm that DevTools doesn’t work on BFCache restoration at this CL, but the reason is a part of crbug.com/1448790 and I believe this CL maintains necessary CDP events to work with DevTools. I would like to confirm this is aligned with your thought.
To view, visit change 4578345. To unsubscribe, or for help writing mail filters, visit settings.
Taiyo Mizuhashi (Gerrit)
Attention is currently required from: Adithya Srinivasan, Hiroki Nakagawa, Ken Okada.
Taiyo Mizuhashi would like Adithya Srinivasan to review this change.
Prerender: Retrigger UpdateSpeculationCandidates on BFCache restoration for prerender
Before this CL, speculation rules preloading is not processed again on
the pages restored from BFCache. This CL introduces a feature to
retrigger speculation rules on BFCache restoration for prerender by retriggering `DocumentSpeculationRules::UpdateSpeculationCandidates()`.
For more information, please see the design doc on crbug.
Note that
- This project tries to target preload(both prefetch/
prerender). This CL focuses on prerender first.
- This CL focuses on verifying the default prerendering behavior first
(list rules, eagerness is kEager). This CL also expects to be worked with other prerendering cases and these tests will be added in the
following CL.
Bug: 1449163
Change-Id: I9b3390eee68fb075a472558d4c79d0df861bf49c
---
M content/browser/preloading/prerender/prerender_browsertest.cc
M third_party/blink/common/features.cc
M third_party/blink/public/common/features.h
M third_party/blink/renderer/core/exported/web_view_impl.cc
M third_party/blink/renderer/core/speculation_rules/document_speculation_rules.cc
M third_party/blink/renderer/core/speculation_rules/document_speculation_rules.h
6 files changed, 93 insertions(+), 18 deletions(-)
To view, visit change 4578345. To unsubscribe, or for help writing mail filters, visit settings.
Taiyo Mizuhashi (Gerrit)
Attention is currently required from: Adithya Srinivasan, Hiroki Nakagawa, Ken Okada.
1 comment:
Patchset:
+adithyas@: Hello, could you please review especially under
third_party/blink/renderer/core/* ? Thanks.
To view, visit change 4578345. To unsubscribe, or for help writing mail filters, visit settings.
Ken Okada (Gerrit)
Attention is currently required from: Adithya Srinivasan, Hiroki Nakagawa, Taiyo Mizuhashi.
Patch set 12:Code-Review +1
1 comment:
Patchset:
+kenoss@: Hello, can you please review this with DevTools perspective? […]
I tried and confirmed that DevTools received retriggered status updates. Thanks!
To view, visit change 4578345. To unsubscribe, or for help writing mail filters, visit settings.
Adithya Srinivasan (Gerrit)
Attention is currently required from: Hiroki Nakagawa, Taiyo Mizuhashi.
Patch set 12:Code-Review +1
2 comments:
Patchset:
speculation_rules changes LGTM! (with comment addressed)
File third_party/blink/renderer/core/speculation_rules/document_speculation_rules.h:
Patch Set #12, Line 69:
void RetriggerUpdateSpeculationCandidates();nit: We've usually named these methods based on the trigger that caused it and not by what the method itself does, so I would rename it to `DocumentRestoredFromBFCache()`
To view, visit change 4578345. To unsubscribe, or for help writing mail filters, visit settings.
Taiyo Mizuhashi (Gerrit)
Attention is currently required from: Taiyo Mizuhashi.
Taiyo Mizuhashi uploaded patch set #14 to this change.
Prerender: Retrigger UpdateSpeculationCandidates on BFCache restoration for prerender
Before this CL, speculation rules preloading is not processed again on
the pages restored from BFCache. This CL introduces a feature to
retrigger speculation rules on BFCache restoration for prerender by retriggering `DocumentSpeculationRules::UpdateSpeculationCandidates()`.
For more information, please see the design doc on crbug.
Note that
- This project tries to target preload(both prefetch/
prerender). This CL focuses on prerender first(Just calling
`UpdateSpeculationCandidates()` is insufficient for the current prefetch
implementation to retrigger).
- This CL focuses on verifying the default prerendering behavior first
(list rules, eagerness is kEager). This CL also expects to be worked with other prerendering cases and these tests will be added in the
following CL.
- DevTools doesn’t work on BFCache restoration at this CL, but the
reason is a part of crbug.com/1448790 and this CL will maintain
necessary CDP events to work with DevTools.
Bug: 1449163
Change-Id: I9b3390eee68fb075a472558d4c79d0df861bf49c
---
M content/browser/preloading/prerender/prerender_browsertest.cc
M third_party/blink/common/features.cc
M third_party/blink/public/common/features.h
M third_party/blink/renderer/core/exported/web_view_impl.cc
M third_party/blink/renderer/core/speculation_rules/document_speculation_rules.cc
M third_party/blink/renderer/core/speculation_rules/document_speculation_rules.h
6 files changed, 93 insertions(+), 18 deletions(-)
To view, visit change 4578345. To unsubscribe, or for help writing mail filters, visit settings.
Taiyo Mizuhashi (Gerrit)
Attention is currently required from: Taiyo Mizuhashi.
Taiyo Mizuhashi uploaded patch set #15 to this change.
Taiyo Mizuhashi (Gerrit)
4 comments:
Patchset:
I tried and confirmed that DevTools received retriggered status updates. […]
Thanks for checking!
Patchset:
Thank you for the review!
File third_party/blink/renderer/core/speculation_rules/document_speculation_rules.h:
Patch Set #12, Line 69:
void RetriggerUpdateSpeculationCandidates();
nit: We've usually named these methods based on the trigger that caused it and not by what the metho […]
DONE, thanks for pointing this out!
File third_party/blink/renderer/core/speculation_rules/document_speculation_rules.cc:
Patch Set #11, Line 434:
// TODO(crbug.com/1449163): Add implementation for Prefetch.
IMHO, it is OK to remove it here since the crbug says that preloading is the scope. […]
Thanks, I revised the CL description.
To view, visit change 4578345. To unsubscribe, or for help writing mail filters, visit settings.
Taiyo Mizuhashi (Gerrit)
Attention is currently required from: Taiyo Mizuhashi.
Patch set 16:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
12 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/speculation_rules/document_speculation_rules.cc
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/renderer/core/speculation_rules/document_speculation_rules.h
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/renderer/core/exported/web_view_impl.cc
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
Prerender: Retrigger UpdateSpeculationCandidates on BFCache restoration
for prerender
Before this CL, speculation rules preloading is not processed again on
the pages restored from BFCache. This CL introduces a feature to
retrigger speculation rules on BFCache restoration for prerender by
retriggering `DocumentSpeculationRules::UpdateSpeculationCandidates()`.
For more information, please see the design doc on crbug.
Note that
- This project tries to target preload(both prefetch/
prerender). This CL focuses on prerender first(Just calling
`UpdateSpeculationCandidates()` is insufficient for the current prefetch
implementation to retrigger).
- This CL focuses on verifying the default prerendering behavior first
(list rules, eagerness is kEager). This CL also expects to be worked
with other prerendering cases and these tests will be added in the
following CL.
- DevTools doesn’t work on BFCache restoration at this CL, but the
reason is a part of crbug.com/1448790 and this CL will maintain
necessary CDP events to work with DevTools.
Bug: 1449163
Change-Id: I9b3390eee68fb075a472558d4c79d0df861bf49c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4578345
Commit-Queue: Taiyo Mizuhashi <ta...@chromium.org>
Reviewed-by: Ken Okada <ken...@chromium.org>
Reviewed-by: Adithya Srinivasan <adit...@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhi...@chromium.org>
Reviewed-by: Huanpo Lin <robe...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1156094}
---
M content/browser/preloading/prerender/prerender_browsertest.cc
M third_party/blink/common/features.cc
M third_party/blink/public/common/features.h
M third_party/blink/renderer/core/exported/web_view_impl.cc
M third_party/blink/renderer/core/speculation_rules/document_speculation_rules.cc
M third_party/blink/renderer/core/speculation_rules/document_speculation_rules.h
6 files changed, 93 insertions(+), 18 deletions(-)