Add dry run implementation for Navigation Fast Fetch [chromium/src : main]
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 1 comment![]( "Open in Gerrit")
rakina@: PTAL overall.
toyoshim@: PTAL histograms.
Related details
- Rakina Zata Amni
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 3 comments![]( "Open in Gerrit")
NavigationFastFetch::CalculateEligibilityReason(NavigationRequest& request) {[Not blocking this CL] It's helpful if we add comments why we require these conditions.
I also feel it's helpful to roughly categorize the checks by their reasons (not sure if we can clearly categorize though), e.g.
- Because a `NavigationThrottle` checks the condition.
- Because the navigation is less likely to issue a network request. (kHistory, SW)
- Because Prefetch code checks the condition.
- Because we intentionally scope this feature. (e.g. requiring HTTPS as a general custom for new features? or just due to higher complexity/ambiguity like DevTools?)
request.frame_tree_node()->current_frame_host()->GetSiteInstance());Can we also consider a request ineligible if `storage_partition` isn't the `BrowserContext`'s default `StoragePartition`?
So, this check should be split into two tests:
- StoragePartition check: the navigation target's StoragePartition should be the default StoragePartition.
- SW check: The default StoragePartition's SWContext doesn't have SW for the URL.
This is also to avoid the confusion e.g. "which StorageParition should/did we test?". Sticking more into the default StoragePartition will avoid such confusion.
// TODO(crbug.com/529425553): Check if there is on-going prefetch.As chatted offline, probably we shouldn't check this and just rely on Prefetch layer to merge/dedup if such ongoing prefetch exists.
Related details
- Kenichi Ishibashi
- Rakina Zata Amni
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 3 comments![]( "Open in Gerrit")
NavigationFastFetch::CalculateEligibilityReason(NavigationRequest& request) {[Not blocking this CL] It's helpful if we add comments why we require these conditions.
I also feel it's helpful to roughly categorize the checks by their reasons (not sure if we can clearly categorize though), e.g.
- Because a `NavigationThrottle` checks the condition.
- Because the navigation is less likely to issue a network request. (kHistory, SW)
- Because Prefetch code checks the condition.
- Because we intentionally scope this feature. (e.g. requiring HTTPS as a general custom for new features? or just due to higher complexity/ambiguity like DevTools?)
The histogram has "experimental" in its name, indicating that these conditions and priorities can change in the future. I would like to prioritize flexibility over fixing them in detail through comments.
request.frame_tree_node()->current_frame_host()->GetSiteInstance());Can we also consider a request ineligible if `storage_partition` isn't the `BrowserContext`'s default `StoragePartition`?
So, this check should be split into two tests:
- StoragePartition check: the navigation target's StoragePartition should be the default StoragePartition.
- SW check: The default StoragePartition's SWContext doesn't have SW for the URL.
This is also to avoid the confusion e.g. "which StorageParition should/did we test?". Sticking more into the default StoragePartition will avoid such confusion.
Would you elaborate on why we limit to the default StoragePartition? I can imagine a scenario where the user has multiple profiles (private, work).
// TODO(crbug.com/529425553): Check if there is on-going prefetch.As chatted offline, probably we shouldn't check this and just rely on Prefetch layer to merge/dedup if such ongoing prefetch exists.
+1 for now. I put this TODO since this eligibility check may be used for actual navigation fast fetching logic in the future.
Related details
- Hiroshige Hayashizaki
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 26 comments![]( "Open in Gerrit")
// Computes the eligibility reason based on the NavigationRequest.
static EligibilityReason CalculateEligibilityReason(
NavigationRequest& request);
// Records final lifecycle outcome UMA and cleans up fast fetch state.
void RecordOutcome(NavigationRequest& request, NavigationOutcome outcome);They don't access any member. So, consider to be a function in anonymous namespace in the cc file.
// Returns true if the navigation is eligible for Fast Fetch.protected + friend, or private + ForTesting wrapper.
// Returns the detailed reason for eligibility or ineligibility.ForTesting, or keep this protected and make tests friend class.
// Returns the time when the eligibility check was performed.not used?
static std::unique_ptr<NavigationFastFetch> Create(If we prepare a Crete that contains the ctor and some required operations to be enforced, we usually hide the ctor in protected or private scope.
// Final outcome of the navigation for eligible requests.ditto
// numeric values should never be reused.Insert an empty line
class CONTENT_EXPORT NavigationFastFetch {NavitationFastFetch{Evaluator,Experiment,Manager} or something?
#include "base/timer/elapsed_timer.h"#include "content/browser/renderer_host/render_frame_host_impl.h"
#include "net/base/net_errors.h"not used?
if (!request.IsInOutermostMainFrame()) {Just in case, prerender passes this check and will be counted in your metrics. I'm not sure if this is intended.
}Should we explicitly exclude Same-Document navigations from Fast Fetch eligibility?
Since this feature aims to parallelize the network fetch for a new document resource, Same-Document navigations (which by definition do not fetch a new document) seem inherently ineligible. It might be better to add a specific EligibilityReason such as kSameDocument and return it here to keep the metrics focused on navigations that actually involve a document load.
void NavigationFastFetch::RecordOutcome(NavigationRequest& request,not used?
// Copyright 2026 The Chromium AuthorsCan we add following scenarios?
- Restarted navigations: Verifying if metrics are recorded twice.
- Prerender Activation: Verifying that they are (or are not) eligible as intended.
- Negative Opportunity Time: Verifying behavior when fetch_start is before eligibility_check_time.
#include "net/dns/mock_host_resolver.h"not used?
#include "content/browser/storage_partition_impl.h"maybe impl part is not used and this include is unnecessary?
#include "content/public/test/mock_devtools_agent_host.h"maybe Mock is not used?
content/public/browser/devtools_agent_host.h is the right include?
#include "content/public/test/test_browser_context.h"optionally unnecessary as RenderViewHostImplTestHarness gives required things.
#include "net/http/http_request_headers.h"used?
#include "third_party/blink/public/mojom/loader/referrer.mojom.h"same
NavigationFastFetch* fast_fetch() const { return fast_fetch_.get(); }While this is under experiment, it's good to name this as ForTesting as no production code relies on this now?
void NavigationRequest::BeginNavigationImpl() {I noticed a potential double-recording of UMA metrics when a navigation restarts.
In certain scenarios, a navigation might start as Same-Document but then be forced to restart as a Cross-Document navigation via ResetForCrossDocumentRestart(). In this case, BeginNavigationImpl() is called again for the same NavigationRequest.
Under the current implementation, NavigationFastFetch::Create() would be called twice, logging the EligibilityReason twice and skewing the data. Since we are primarily interested in the final, correct state of the navigation (especially for those performing a network fetch), we should ensure that any existing fast_fetch_ instance is either reset or its metrics discarded when a restart occurs.
fast_fetch_ = NavigationFastFetch::Create(*this);We prefer implementing any kind of new features behind a base::Feature flag?
units="ms" expires_after="2026-10-01">us
Related details
- Hiroshige Hayashizaki
- Kenichi Ishibashi
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
void NavigationRequest::BeginNavigationImpl() {I noticed a potential double-recording of UMA metrics when a navigation restarts.
In certain scenarios, a navigation might start as Same-Document but then be forced to restart as a Cross-Document navigation via ResetForCrossDocumentRestart(). In this case, BeginNavigationImpl() is called again for the same NavigationRequest.
Under the current implementation, NavigationFastFetch::Create() would be called twice, logging the EligibilityReason twice and skewing the data. Since we are primarily interested in the final, correct state of the navigation (especially for those performing a network fetch), we should ensure that any existing fast_fetch_ instance is either reset or its metrics discarded when a restart occurs.
cc: @rak...@chromium.org
Can you double check if my understanding is correct?
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 4 comments![]( "Open in Gerrit")
kHasServiceWorker = 8,What if the page is already prerendered/prefetched/have http cahce entries?
request.GetWebContents()->GetBrowserContext()->GetStoragePartition(Instead of using the SiteInstance for the current RFH, would it be more accurate to check `request.GetBrowserContext()->GetStoragePartitionForUrl(request.GetURL(), ...)`)?
void NavigationRequest::BeginNavigationImpl() {Takashi ToyoshimaI noticed a potential double-recording of UMA metrics when a navigation restarts.
In certain scenarios, a navigation might start as Same-Document but then be forced to restart as a Cross-Document navigation via ResetForCrossDocumentRestart(). In this case, BeginNavigationImpl() is called again for the same NavigationRequest.
Under the current implementation, NavigationFastFetch::Create() would be called twice, logging the EligibilityReason twice and skewing the data. Since we are primarily interested in the final, correct state of the navigation (especially for those performing a network fetch), we should ensure that any existing fast_fetch_ instance is either reset or its metrics discarded when a restart occurs.
cc: @rak...@chromium.org
Can you double check if my understanding is correct?
Yeah cross-document restarts can happen. It looks like we will log the eligibility twice, but the outcome won't be recorded if same-document navs are excluded. Maybe it's fine, you would just see more same-document navs (I expect this restart to be rare)
fast_fetch_->OnRequestFailed(*this, status, skip_throttles);Some failed navigations don't call OnRequestFailedInternal. e.g. the NavigationRequest gets deleted directly (OnNavigationClientDisconnected, new NavigationRequest takes over, etc). Maybe you should trigger this at destruction, or maybe even use WebContentsObserver DidStart/Finish/etc?
Related details
- Hiroshige Hayashizaki
- Kenichi Ishibashi
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 30 comments![]( "Open in Gerrit")
// Computes the eligibility reason based on the NavigationRequest.
static EligibilityReason CalculateEligibilityReason(
NavigationRequest& request);
// Records final lifecycle outcome UMA and cleans up fast fetch state.
void RecordOutcome(NavigationRequest& request, NavigationOutcome outcome);They don't access any member. So, consider to be a function in anonymous namespace in the cc file.
Done
// Returns true if the navigation is eligible for Fast Fetch.protected + friend, or private + ForTesting wrapper.
Removed.
// Returns the detailed reason for eligibility or ineligibility.ForTesting, or keep this protected and make tests friend class.
Done
// Returns the time when the eligibility check was performed.Kenichi Ishibashinot used?
Removed.
static std::unique_ptr<NavigationFastFetch> Create(If we prepare a Crete that contains the ctor and some required operations to be enforced, we usually hide the ctor in protected or private scope.
Done
// Final outcome of the navigation for eligible requests.Kenichi Ishibashiditto
Done
What if the page is already prerendered/prefetched/have http cahce entries?
Covered prerendered/prefetched. I think http cache case should be fine, as it is considered network fetch (with cache hit).
kHasSignedExchange = 7,Kenichi Ishibashinot used
Added check in CalculateEligibilityReason().
// numeric values should never be reused.Kenichi IshibashiInsert an empty line
Done
class CONTENT_EXPORT NavigationFastFetch {Kenichi IshibashiNavitationFastFetch{Evaluator,Experiment,Manager} or something?
Renamed: NavitationFastFetchManager.
#include "base/timer/elapsed_timer.h"Kenichi Ishibashi#include "content/browser/renderer_host/render_frame_host_impl.h"
Done
#include "net/base/net_errors.h"Kenichi Ishibashinot used?
Removed.
Just in case, prerender passes this check and will be counted in your metrics. I'm not sure if this is intended.
Added check.
Should we explicitly exclude Same-Document navigations from Fast Fetch eligibility?
Since this feature aims to parallelize the network fetch for a new document resource, Same-Document navigations (which by definition do not fetch a new document) seem inherently ineligible. It might be better to add a specific EligibilityReason such as kSameDocument and return it here to keep the metrics focused on navigations that actually involve a document load.
Good point. Excluded same document navigations.
request.GetWebContents()->GetBrowserContext()->GetStoragePartition(Instead of using the SiteInstance for the current RFH, would it be more accurate to check `request.GetBrowserContext()->GetStoragePartitionForUrl(request.GetURL(), ...)`)?
IIUC GetStoragePartitionForUrl() is deprecated.
void NavigationFastFetch::RecordOutcome(NavigationRequest& request,Kenichi Ishibashinot used?
Done
Can we add following scenarios?
- Restarted navigations: Verifying if metrics are recorded twice.
- Prerender Activation: Verifying that they are (or are not) eligible as intended.
- Negative Opportunity Time: Verifying behavior when fetch_start is before eligibility_check_time.
Added first two tests in browser_tests. The last one was added as unittest.
#include "net/dns/mock_host_resolver.h"Kenichi Ishibashinot used?
Removed.
#include "base/logging.h"Kenichi Ishibashinot used?
Removed.
maybe impl part is not used and this include is unnecessary?
Removed.
#include "content/public/test/mock_devtools_agent_host.h"maybe Mock is not used?
content/public/browser/devtools_agent_host.h is the right include?
Done
#include "content/public/test/test_browser_context.h"optionally unnecessary as RenderViewHostImplTestHarness gives required things.
Removed.
#include "net/http/http_request_headers.h"Kenichi Ishibashiused?
Removed. I think it was a leftover when I tried to write unittests for service worker check (moved to browsertests).
#include "third_party/blink/public/mojom/loader/referrer.mojom.h"Kenichi Ishibashisame
Removed.
NavigationFastFetch* fast_fetch() const { return fast_fetch_.get(); }While this is under experiment, it's good to name this as ForTesting as no production code relies on this now?
Done
void NavigationRequest::BeginNavigationImpl() {Takashi ToyoshimaI noticed a potential double-recording of UMA metrics when a navigation restarts.
In certain scenarios, a navigation might start as Same-Document but then be forced to restart as a Cross-Document navigation via ResetForCrossDocumentRestart(). In this case, BeginNavigationImpl() is called again for the same NavigationRequest.
Under the current implementation, NavigationFastFetch::Create() would be called twice, logging the EligibilityReason twice and skewing the data. Since we are primarily interested in the final, correct state of the navigation (especially for those performing a network fetch), we should ensure that any existing fast_fetch_ instance is either reset or its metrics discarded when a restart occurs.
Rakina Zata Amnicc: @rak...@chromium.org
Can you double check if my understanding is correct?
Yeah cross-document restarts can happen. It looks like we will log the eligibility twice, but the outcome won't be recorded if same-document navs are excluded. Maybe it's fine, you would just see more same-document navs (I expect this restart to be rare)
Moved eligibility reason histogram recording in the destructor of FastFetch instance and add a method to suppress histogram recording.
fast_fetch_ = NavigationFastFetch::Create(*this);We prefer implementing any kind of new features behind a base::Feature flag?
The initial phase is just collecting histograms. Added a flag and marked it as enabled by default so that we can record histograms. The flag will act like a kill switch.
I'll add feature params (default disabled) when we add functional changes.
fast_fetch_->OnRequestFailed(*this, status, skip_throttles);Some failed navigations don't call OnRequestFailedInternal. e.g. the NavigationRequest gets deleted directly (OnNavigationClientDisconnected, new NavigationRequest takes over, etc). Maybe you should trigger this at destruction, or maybe even use WebContentsObserver DidStart/Finish/etc?
I see, thank you for the info. Since I want to record failed information (esp skip_throttles), let me add a TODO comment to consider covering these cases.
units="ms" expires_after="2026-10-01">Kenichi Ishibashius
Done
Related details
- Hiroshige Hayashizaki
- Rakina Zata Amni
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted and added 6 comments![]( "Open in Gerrit")
Votes added by Takashi Toyoshima
| Code-Review | +1 |
6 comments
// Final outcome of the navigation for eligible requests.nit: boilerplate and empty line.
https://chromium.googlesource.com/chromium/src/tools/+/refs/heads/main/metrics/histograms/README.md#usage
// numeric values should never be reused.nit: insert an empty line before the LINT.
#define kOpportunityBaseName "Navigation.Experimental.FastFetch.OpportunityTime"Using a macro and `std::string_view` constants here is a bit unconventional for Chromium. Since histogram names should be literals for best performance and ease of grepping, it's usually better to just use the full string literals directly in the `UmaHistogram` calls or use `static constexpr char[]` constants.
Suggestion:
```cpp
static constexpr char kOpportunityTimeLoaderStart[] =
\"Navigation.Experimental.FastFetch.OpportunityTime.LoaderStart\";
static constexpr char kOpportunityTimeFetchStart[] =
\"Navigation.Experimental.FastFetch.OpportunityTime.FetchStart\";
```
Or just inline them.
// staticShould this be a class method?
If I don't miss something, this seems independent from the instance, and depends only on `request`. If so, this could be a function in an anonymous namespace,
void NavigationRequest::BeginNavigationImpl() {Takashi ToyoshimaI noticed a potential double-recording of UMA metrics when a navigation restarts.
In certain scenarios, a navigation might start as Same-Document but then be forced to restart as a Cross-Document navigation via ResetForCrossDocumentRestart(). In this case, BeginNavigationImpl() is called again for the same NavigationRequest.
Under the current implementation, NavigationFastFetch::Create() would be called twice, logging the EligibilityReason twice and skewing the data. Since we are primarily interested in the final, correct state of the navigation (especially for those performing a network fetch), we should ensure that any existing fast_fetch_ instance is either reset or its metrics discarded when a restart occurs.
Rakina Zata Amnicc: @rak...@chromium.org
Can you double check if my understanding is correct?
Kenichi IshibashiYeah cross-document restarts can happen. It looks like we will log the eligibility twice, but the outcome won't be recorded if same-document navs are excluded. Maybe it's fine, you would just see more same-document navs (I expect this restart to be rare)
Moved eligibility reason histogram recording in the destructor of FastFetch instance and add a method to suppress histogram recording.
Acknowledged
navigations.Comment that some of clients may not have high resolution timer that can send metrics with microseconds precision.
Related details
- Hiroshige Hayashizaki
- Kenichi Ishibashi
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 6 comments![]( "Open in Gerrit")
request.GetWebContents()->GetBrowserContext()->GetStoragePartition(Kenichi IshibashiInstead of using the SiteInstance for the current RFH, would it be more accurate to check `request.GetBrowserContext()->GetStoragePartitionForUrl(request.GetURL(), ...)`)?
IIUC GetStoragePartitionForUrl() is deprecated.
oh right, nevermind then. I guess you can use `GetStoragePartitionWithCurrentSiteInfo()` here which would be doing the same not-so-accurate thing here but at least you don't need to do it manually? Also I think you mentioned about skipping non-default storage partition cases, where is that checked? Maybe you can update the eligibility check at the end / at commit / failure when you would already have that information?
const EligibilityReason eligibility_reason_;Can you please add comments?
kCommitted = 0,Does it matter if the network request gets redirected? Can that affect the eligibility checks above?
StoragePartition* storage_partition =Please fix this WARNING reported by autoreview issue finding: Consider using `request.GetStoragePartitionWithCurrentSiteInfo()` (or `request.GetStoragePartition()`, if available) instead of manually reaching into the `WebContents` and the `current_frame_host`. It encapsulates this logic nicely.
fast_fetch_->OnRequestFailed(*this, status, skip_throttles);Kenichi IshibashiSome failed navigations don't call OnRequestFailedInternal. e.g. the NavigationRequest gets deleted directly (OnNavigationClientDisconnected, new NavigationRequest takes over, etc). Maybe you should trigger this at destruction, or maybe even use WebContentsObserver DidStart/Finish/etc?
I see, thank you for the info. Since I want to record failed information (esp skip_throttles), let me add a TODO comment to consider covering these cases.
I think you can cover the other cases not covered here at destruction time if you noticed you haven't logged commit/failure? I guess it depends on what you will use the data for. How would these failure / cancellation results help you evaluate fast fetch opportunity?
CONTENT_EXPORT BASE_DECLARE_FEATURE(kNavigationFastFetchDryRun);Please move this to `content/common/features.h` since it's only used within `//content`
Related details
- Hiroshige Hayashizaki
- Kenichi Ishibashi
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 12 comments![]( "Open in Gerrit")
request.GetWebContents()->GetBrowserContext()->GetStoragePartition(Kenichi IshibashiInstead of using the SiteInstance for the current RFH, would it be more accurate to check `request.GetBrowserContext()->GetStoragePartitionForUrl(request.GetURL(), ...)`)?
Rakina Zata AmniIIUC GetStoragePartitionForUrl() is deprecated.
oh right, nevermind then. I guess you can use `GetStoragePartitionWithCurrentSiteInfo()` here which would be doing the same not-so-accurate thing here but at least you don't need to do it manually? Also I think you mentioned about skipping non-default storage partition cases, where is that checked? Maybe you can update the eligibility check at the end / at commit / failure when you would already have that information?
From my code reading, we can't rely on GetStoragePartitionWithCurrentSiteInfo () at this point because NavigationRequest initializes `site_info_` via `explicit SiteInfo(BrowserContext* browser_context)`, which creates an empty site info.
NavigationRequest updates `site_info_` later in NavigationRequest::StartNavigation using GetSiteInfoForCommonParamsURL().
Also I think you mentioned about skipping non-default storage partition cases...
hiroshige@ suggested it, but I think we should not skip non-default storage partitions since a user could have multiple profiles (work, private etc).
Maybe you can update the eligibility check at the end / at commit / failure when you would already have that information?
The key question here is whether we can determine if a navigation is fast fetch eligible right when the NavigationRequest is created. The goal of this CL is to add a histogram to answer that, so pushing the eligibility check to a later stage would not help.
Can you please add comments?
Done
Does it matter if the network request gets redirected? Can that affect the eligibility checks above?
Yes, it matters. I omitted it since this CL doesn't handle it. Subsequent CLs will add it. Added TODO for now.
// Final outcome of the navigation for eligible requests.nit: boilerplate and empty line.
https://chromium.googlesource.com/chromium/src/tools/+/refs/heads/main/metrics/histograms/README.md#usage
Done
nit: insert an empty line before the LINT.
Done
#define kOpportunityBaseName "Navigation.Experimental.FastFetch.OpportunityTime"Using a macro and `std::string_view` constants here is a bit unconventional for Chromium. Since histogram names should be literals for best performance and ease of grepping, it's usually better to just use the full string literals directly in the `UmaHistogram` calls or use `static constexpr char[]` constants.
Suggestion:
```cpp
static constexpr char kOpportunityTimeLoaderStart[] =
\"Navigation.Experimental.FastFetch.OpportunityTime.LoaderStart\";
static constexpr char kOpportunityTimeFetchStart[] =
\"Navigation.Experimental.FastFetch.OpportunityTime.FetchStart\";
```
Or just inline them.
I thought it conflicts with another suggestion.
https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#Don_t-Use-the-Same-Histogram-Logging-Call-in-Multiple-Places
But I agree that grep-ability is better. Done.
Should this be a class method?
If I don't miss something, this seems independent from the instance, and depends only on `request`. If so, this could be a function in an anonymous namespace,
Done
Please fix this WARNING reported by autoreview issue finding: Consider using `request.GetStoragePartitionWithCurrentSiteInfo()` (or `request.GetStoragePartition()`, if available) instead of manually reaching into the `WebContents` and the `current_frame_host`. It encapsulates this logic nicely.
Please see other comment.
fast_fetch_->OnRequestFailed(*this, status, skip_throttles);Kenichi IshibashiSome failed navigations don't call OnRequestFailedInternal. e.g. the NavigationRequest gets deleted directly (OnNavigationClientDisconnected, new NavigationRequest takes over, etc). Maybe you should trigger this at destruction, or maybe even use WebContentsObserver DidStart/Finish/etc?
Rakina Zata AmniI see, thank you for the info. Since I want to record failed information (esp skip_throttles), let me add a TODO comment to consider covering these cases.
I think you can cover the other cases not covered here at destruction time if you noticed you haven't logged commit/failure? I guess it depends on what you will use the data for. How would these failure / cancellation results help you evaluate fast fetch opportunity?
Added a logic to record outcome when cancelled in the destructor of NavigationFastFetchManager.
CONTENT_EXPORT BASE_DECLARE_FEATURE(kNavigationFastFetchDryRun);Please move this to `content/common/features.h` since it's only used within `//content`
Done
Comment that some of clients may not have high resolution timer that can send metrics with microseconds precision.
- Hiroshige Hayashizaki
- Rakina Zata Amni
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted and added 1 comment![]( "Open in Gerrit")
Votes added by Takashi Toyoshima
| Code-Review | +1 |
1 comment
- Hiroshige Hayashizaki
- Kenichi Ishibashi
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 2 comments![]( "Open in Gerrit")
Thanks, mostly LGTM but just one more question about the StoragePartition stuff
request.GetWebContents()->GetBrowserContext()->GetStoragePartition(Kenichi IshibashiInstead of using the SiteInstance for the current RFH, would it be more accurate to check `request.GetBrowserContext()->GetStoragePartitionForUrl(request.GetURL(), ...)`)?
Rakina Zata AmniIIUC GetStoragePartitionForUrl() is deprecated.
Kenichi Ishibashioh right, nevermind then. I guess you can use `GetStoragePartitionWithCurrentSiteInfo()` here which would be doing the same not-so-accurate thing here but at least you don't need to do it manually? Also I think you mentioned about skipping non-default storage partition cases, where is that checked? Maybe you can update the eligibility check at the end / at commit / failure when you would already have that information?
From my code reading, we can't rely on GetStoragePartitionWithCurrentSiteInfo () at this point because NavigationRequest initializes `site_info_` via `explicit SiteInfo(BrowserContext* browser_context)`, which creates an empty site info.
NavigationRequest updates `site_info_` later in NavigationRequest::StartNavigation using GetSiteInfoForCommonParamsURL().
Also I think you mentioned about skipping non-default storage partition cases...
hiroshige@ suggested it, but I think we should not skip non-default storage partitions since a user could have multiple profiles (work, private etc).
Maybe you can update the eligibility check at the end / at commit / failure when you would already have that information?
The key question here is whether we can determine if a navigation is fast fetch eligible right when the NavigationRequest is created. The goal of this CL is to add a histogram to answer that, so pushing the eligibility check to a later stage would not help.
oh, right, it looks like `GetStoragePartitionWithCurrentSiteInfo()` would always go to the default storage partition in this case. Meanwhile yours would always result in the current RFH's storage partition. Neither are guaranteed to be the same as the target URL's storage partition.
How much does it matter if you do the early network request with the wrong storage partition? (Will you be sending it with the wrong cookies?)
If it's potentially problematic, I wonder if we can use a combination of these to determine the target URL's storage partition more accurately, which I think doesn't require SiteInfo?
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/site_info.cc;l=337;drc=04a9f36f1551524fb2494bf2429de20d2c1f337e
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_request.cc;l=4650;drc=04a9f36f1551524fb2494bf2429de20d2c1f337e
(If it's acceptable to send with a potentially inaccurate storage partition, maybe the current one is fine)
hiroshige@ suggested it, but I think we should not skip non-default storage partitions since a user could have multiple profiles (work, private etc).
ah ok, maybe this part of the doc should be updated https://docs.google.com/document/d/1tmqth2B6GmKgfBQzLvvTYB0duvj6CaLKPMuJ8s9GTto/edit?resourcekey=0-OXUvn4V3kpi0wQcNl26JZw&disco=AAAB9NHl1jk
Related details
- Hiroshige Hayashizaki
- Kenichi Ishibashi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 3 comments![]( "Open in Gerrit")
NavigationFastFetch::CalculateEligibilityReason(NavigationRequest& request) {Kenichi Ishibashi[Not blocking this CL] It's helpful if we add comments why we require these conditions.
I also feel it's helpful to roughly categorize the checks by their reasons (not sure if we can clearly categorize though), e.g.
- Because a `NavigationThrottle` checks the condition.
- Because the navigation is less likely to issue a network request. (kHistory, SW)
- Because Prefetch code checks the condition.
- Because we intentionally scope this feature. (e.g. requiring HTTPS as a general custom for new features? or just due to higher complexity/ambiguity like DevTools?)
The histogram has "experimental" in its name, indicating that these conditions and priorities can change in the future. I would like to prioritize flexibility over fixing them in detail through comments.
sg.
request.frame_tree_node()->current_frame_host()->GetSiteInstance());Kenichi IshibashiCan we also consider a request ineligible if `storage_partition` isn't the `BrowserContext`'s default `StoragePartition`?
So, this check should be split into two tests:
- StoragePartition check: the navigation target's StoragePartition should be the default StoragePartition.
- SW check: The default StoragePartition's SWContext doesn't have SW for the URL.
This is also to avoid the confusion e.g. "which StorageParition should/did we test?". Sticking more into the default StoragePartition will avoid such confusion.
Would you elaborate on why we limit to the default StoragePartition? I can imagine a scenario where the user has multiple profiles (private, work).
Because the current Prefetch implementation is bound to the default StoragePartition.
We can design the non-default-StoragePartition support for Nav Fast Fetch feature separately, but I think it's better to start with sticking to the default StorageParition for Nav Fast Fetch as well (so that we can experiment with Prefetch-backed prototype), and consider non-default-StorageParition support separately if the metrics here suggests it's quite useful.
// TODO(crbug.com/529425553): Check if there is on-going prefetch.Kenichi IshibashiAs chatted offline, probably we shouldn't check this and just rely on Prefetch layer to merge/dedup if such ongoing prefetch exists.
+1 for now. I put this TODO since this eligibility check may be used for actual navigation fast fetching logic in the future.
Just to check: IIUC "actual navigation fast fetching logic in the future" is the longer-term implementation after Prefetch-backed experimental implementation. Am I understanding correctly?
Related details
- Kenichi Ishibashi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 2 comments![]( "Open in Gerrit")
bool NavigationRequest::HasPrefetchedSignedExchange() const {I perfer making this NavFastFetch-only by adding `CHECK(base::FeatureList::IsEnabled(features::kNavigationFastFetchDryRun))`, renaming to e.g. `HasPrefetchedSignedExchangeForNavigationFastFetch()`, as I suspect perhaps this isn't the source of truth.
Not sure though as I'm not an owner of signed exchange -- I WOULD be much more confident if we'd add similar code for Navigational Prefetch Cache.
// Controls the histogram recordings of the Navigation Fast Fetch feature.This is inserted in the middle of `kNavigationThrottleRunner2`, so should be moved before Line 555.
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 4 comments![]( "Open in Gerrit")
Thank you all for reviews and feedback.
request.GetWebContents()->GetBrowserContext()->GetStoragePartition(Kenichi IshibashiInstead of using the SiteInstance for the current RFH, would it be more accurate to check `request.GetBrowserContext()->GetStoragePartitionForUrl(request.GetURL(), ...)`)?
Rakina Zata AmniIIUC GetStoragePartitionForUrl() is deprecated.
Kenichi Ishibashioh right, nevermind then. I guess you can use `GetStoragePartitionWithCurrentSiteInfo()` here which would be doing the same not-so-accurate thing here but at least you don't need to do it manually? Also I think you mentioned about skipping non-default storage partition cases, where is that checked? Maybe you can update the eligibility check at the end / at commit / failure when you would already have that information?
Rakina Zata AmniFrom my code reading, we can't rely on GetStoragePartitionWithCurrentSiteInfo () at this point because NavigationRequest initializes `site_info_` via `explicit SiteInfo(BrowserContext* browser_context)`, which creates an empty site info.
NavigationRequest updates `site_info_` later in NavigationRequest::StartNavigation using GetSiteInfoForCommonParamsURL().
Also I think you mentioned about skipping non-default storage partition cases...
hiroshige@ suggested it, but I think we should not skip non-default storage partitions since a user could have multiple profiles (work, private etc).
Maybe you can update the eligibility check at the end / at commit / failure when you would already have that information?
The key question here is whether we can determine if a navigation is fast fetch eligible right when the NavigationRequest is created. The goal of this CL is to add a histogram to answer that, so pushing the eligibility check to a later stage would not help.
oh, right, it looks like `GetStoragePartitionWithCurrentSiteInfo()` would always go to the default storage partition in this case. Meanwhile yours would always result in the current RFH's storage partition. Neither are guaranteed to be the same as the target URL's storage partition.
How much does it matter if you do the early network request with the wrong storage partition? (Will you be sending it with the wrong cookies?)
If it's potentially problematic, I wonder if we can use a combination of these to determine the target URL's storage partition more accurately, which I think doesn't require SiteInfo?
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/site_info.cc;l=337;drc=04a9f36f1551524fb2494bf2429de20d2c1f337e
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_request.cc;l=4650;drc=04a9f36f1551524fb2494bf2429de20d2c1f337e(If it's acceptable to send with a potentially inaccurate storage partition, maybe the current one is fine)
hiroshige@ suggested it, but I think we should not skip non-default storage partitions since a user could have multiple profiles (work, private etc).
ah ok, maybe this part of the doc should be updated https://docs.google.com/document/d/1tmqth2B6GmKgfBQzLvvTYB0duvj6CaLKPMuJ8s9GTto/edit?resourcekey=0-OXUvn4V3kpi0wQcNl26JZw&disco=AAAB9NHl1jk
`GetStoragePartitionConfigForUrl()` seems also discouraged to use [1].
(I may be wrong, but to my knowledge) Since we plan to trigger only on the outermost top-level frame navigation, I don't think it will use the wrong StoragePartition? AI analysis said that cross-storage-partition navigation only happens:
- <webview> tag -> normal navigation
- Isolated Web Apps
At least, currently we only records histograms so there is no real concerns for now (yes, we should be careful when we add behavior changes).
Updated the 1-pager to align with this CL. Thank you for pointing this out.
request.frame_tree_node()->current_frame_host()->GetSiteInstance());Kenichi IshibashiCan we also consider a request ineligible if `storage_partition` isn't the `BrowserContext`'s default `StoragePartition`?
So, this check should be split into two tests:
- StoragePartition check: the navigation target's StoragePartition should be the default StoragePartition.
- SW check: The default StoragePartition's SWContext doesn't have SW for the URL.
This is also to avoid the confusion e.g. "which StorageParition should/did we test?". Sticking more into the default StoragePartition will avoid such confusion.
Hiroshige HayashizakiWould you elaborate on why we limit to the default StoragePartition? I can imagine a scenario where the user has multiple profiles (private, work).
Because the current Prefetch implementation is bound to the default StoragePartition.
We can design the non-default-StoragePartition support for Nav Fast Fetch feature separately, but I think it's better to start with sticking to the default StorageParition for Nav Fast Fetch as well (so that we can experiment with Prefetch-backed prototype), and consider non-default-StorageParition support separately if the metrics here suggests it's quite useful.
I see, my stance is: "We can design the non-default-StoragePartition support for Nav Fast Fetch feature separately". Unless there is a clear advantage to limiting it to only the default storage partition, I'd like to proceed in this direction. What do you think?
// TODO(crbug.com/529425553): Check if there is on-going prefetch.Kenichi IshibashiAs chatted offline, probably we shouldn't check this and just rely on Prefetch layer to merge/dedup if such ongoing prefetch exists.
Hiroshige Hayashizaki+1 for now. I put this TODO since this eligibility check may be used for actual navigation fast fetching logic in the future.
Just to check: IIUC "actual navigation fast fetching logic in the future" is the longer-term implementation after Prefetch-backed experimental implementation. Am I understanding correctly?
Yes, that's the plan (though we may revisit the approach based on omnibox on-enter prefetch results).
Related details
- Hiroshige Hayashizaki
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 1 comment![]( "Open in Gerrit")
GetStoragePartitionConfigForUrl() seems also discouraged to use [1].
Yeah it is discouraged if you have the SiteInfo, but currently we don't (interestingly it refers to the deprecated function as an alternative too). I think that's what the SiteInfo will use to if it doesn't hit this case earlier when creating its UrlInfoInit: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_request.cc;l=4650;drc=04a9f36f1551524fb2494bf2429de20d2c1f337e
So I think it's ok to follow those two handling to be more accurate, something like
```
SiteInstanceImpl* current_instance =
frame_tree_node_->current_frame_host()->GetSiteInstance();
if (current_instance->IsFixedStoragePartition()) {
storage_partition_config = (
current_instance->GetSecurityPrincipal().GetStoragePartitionConfig());
} else {
storage_partition_config =
GetStoragePartitionConfigForUrl(browser_context, site_url);
}
// get storage partition from the config
```
We can navigate to / from IWA on top level frames that changes StoragePartition, and I think it might matter for Chrome Apps / WebUI too. Consulting AI says you can use the wrong cookies and miss cache entries and maybe other privacy concerns. We are just collecting data right now so those are not a concern yet, but we can get more accurate data with the above handling I think.
Related details
- Hiroshige Hayashizaki
- Kenichi Ishibashi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |