Support DSEv1 Prefetch for PreloadServingMetrics.{Initiator}.{All/Srp} [chromium/src : main]
Huanpo Lin (Gerrit)
Huanpo Lin added 1 comment![]( "Open in Gerrit")
Related details
- Hiroki Nakagawa
- Ken Okada
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa added 1 comment![]( "Open in Gerrit")
if (is_served_by_legacy_search_prefetch_) {
base::UmaHistogramEnumeration(
base::StrCat(
{"PreloadServingMetrics.", *navigation_initiator_string_, ".All"}),
UsedInstantLoad::kPrefetch);
if (is_url_srp_) {
base::UmaHistogramEnumeration(
base::StrCat({"PreloadServingMetrics.", *navigation_initiator_string_,
".SRP"}),
UsedInstantLoad::kPrefetch);
}
return;
}I'd prefer to avoid scattering UMA recording code over several functions. Can we pass `is_served_by_legacy_search_prefetch_` to `RecordPreloadServingMetricsByNavigationInitiator()` below and record this case in the function as well?
Related details
- Huanpo Lin
- Ken Okada
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. |
Ken Okada (Gerrit)
Ken Okada added 4 comments![]( "Open in Gerrit")
Currently, DSEv1 Prefetch is classified as other in`Other` (quoted) would be better?
class SearchPrefetchNavigationThrottle : public content::NavigationThrottle {It's highly non trivial why we don't set `true` at the timing of serving and use navigation id, and set `true` latter by `SearchPrefetchNavigationThrottle`. It should be described as comments.
How about adding this?
```
// Marks `NavigationHandleUserData` when a navigation is served by search prefetch.
//
// While `//content` can access `content::NavigationHandle` from `FrameTreeNodeId`
// inside a `URLLoaderInterceptor` (via `FrameTreeNode`), `//chrome` cannot access
// it because `FrameTreeNode` is not exposed across the Content API boundary.
// As a result, `SearchPrefetchURLLoaderInterceptor` cannot directly mark
// `page_load_metrics::NavigationHandleUserData`. To bridge this gap:
//
// - `SearchPrefetchURLLoaderInterceptor` registers the navigation ID with
// `SearchPrefetchService` upon intercepting a request.
// - This throttle checks `SearchPrefetchService::IsServingNavigation()` in
// `WillProcessResponse()` to mark `NavigationHandleUserData`.
// - The tracked navigation ID is cleaned up in this throttle's destructor.
```
Please keep this blank line. The above CHECK is precondition.
if (is_served_by_legacy_search_prefetch_) {
base::UmaHistogramEnumeration(
base::StrCat(
{"PreloadServingMetrics.", *navigation_initiator_string_, ".All"}),
UsedInstantLoad::kPrefetch);
if (is_url_srp_) {
base::UmaHistogramEnumeration(
base::StrCat({"PreloadServingMetrics.", *navigation_initiator_string_,
".SRP"}),
UsedInstantLoad::kPrefetch);
}
return;
}I'd prefer to avoid scattering UMA recording code over several functions. Can we pass `is_served_by_legacy_search_prefetch_` to `RecordPreloadServingMetricsByNavigationInitiator()` below and record this case in the function as well?
+1 (But I understand why Robert wrote this...)
Related details
- Huanpo Lin
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. |
Ken Okada (Gerrit)
Ken Okada added 2 comments![]( "Open in Gerrit")
`is_served_by_legacy_search_prefetch_` in NavigationHandleUserData forThe CL description mentions is_served_by_legacy_search_prefetch_ in NavigationHandleUserData, but the actual field name in NavigationHandleUserData is is_served_by_search_prefetch_.
bool is_served_by_legacy_search_prefetch_ = false;- NavigationHandleUserData: is_served_by_search_prefetch_
- PreloadServingMetricsPLMO: is_served_by_legacy_search_prefetch_
Is the difference intended?
Huanpo Lin (Gerrit)
Huanpo Lin added 6 comments![]( "Open in Gerrit")
`Other` (quoted) would be better?
Done
`is_served_by_legacy_search_prefetch_` in NavigationHandleUserData forThe CL description mentions is_served_by_legacy_search_prefetch_ in NavigationHandleUserData, but the actual field name in NavigationHandleUserData is is_served_by_search_prefetch_.
Updated to be consistent. `is_served_by_legacy_search_prefetch_` is used, PTAL.
class SearchPrefetchNavigationThrottle : public content::NavigationThrottle {It's highly non trivial why we don't set `true` at the timing of serving and use navigation id, and set `true` latter by `SearchPrefetchNavigationThrottle`. It should be described as comments.
How about adding this?
```
// Marks `NavigationHandleUserData` when a navigation is served by search prefetch.
//
// While `//content` can access `content::NavigationHandle` from `FrameTreeNodeId`
// inside a `URLLoaderInterceptor` (via `FrameTreeNode`), `//chrome` cannot access
// it because `FrameTreeNode` is not exposed across the Content API boundary.
// As a result, `SearchPrefetchURLLoaderInterceptor` cannot directly mark
// `page_load_metrics::NavigationHandleUserData`. To bridge this gap:
//
// - `SearchPrefetchURLLoaderInterceptor` registers the navigation ID with
// `SearchPrefetchService` upon intercepting a request.
// - This throttle checks `SearchPrefetchService::IsServingNavigation()` in
// `WillProcessResponse()` to mark `NavigationHandleUserData`.
// - The tracked navigation ID is cleaned up in this throttle's destructor.
```
Updated, thanks.
- NavigationHandleUserData: is_served_by_search_prefetch_
- PreloadServingMetricsPLMO: is_served_by_legacy_search_prefetch_
Is the difference intended?
is_served_by_legacy_search_prefetch_ is the desired one, the first one wasn't updated by accident. Updated, PTAL.
Please keep this blank line. The above CHECK is precondition.
Done
if (is_served_by_legacy_search_prefetch_) {
base::UmaHistogramEnumeration(
base::StrCat(
{"PreloadServingMetrics.", *navigation_initiator_string_, ".All"}),
UsedInstantLoad::kPrefetch);
if (is_url_srp_) {
base::UmaHistogramEnumeration(
base::StrCat({"PreloadServingMetrics.", *navigation_initiator_string_,
".SRP"}),
UsedInstantLoad::kPrefetch);
}
return;
}Ken OkadaI'd prefer to avoid scattering UMA recording code over several functions. Can we pass `is_served_by_legacy_search_prefetch_` to `RecordPreloadServingMetricsByNavigationInitiator()` below and record this case in the function as well?
+1 (But I understand why Robert wrote this...)
- Hiroki Nakagawa
- Ken Okada
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ken Okada (Gerrit)
Ken Okada added 5 comments![]( "Open in Gerrit")
Could you add the same logic to RecordFirstContentfulPaint? Or, it becomes trivial if we rebase it to https://chromium-review.git.corp.google.com/c/chromium/src/+/8254911.
search_prefetch_service_ = service->GetWeakPtr();Since `MaybeCreateAndAdd()` already performs the lookup and null-check for `SearchPrefetchService`, we can pass the service (or its `base::WeakPtr`) directly into the constructor (or `CHECK` it inside the constructor) to avoid duplicating the nested lookups:
```cpp
// static
void SearchPrefetchNavigationThrottle::MaybeCreateAndAdd(
content::NavigationThrottleRegistry& registry) {
// ...
auto* profile =
Profile::FromBrowserContext(web_contents->GetBrowserContext());
if (!profile) {
return;
}
auto* service = SearchPrefetchServiceFactory::GetForProfile(profile);
if (!service) {
return;
}
registry.AddThrottle(
std::make_unique<SearchPrefetchNavigationThrottle>(registry, service->GetWeakPtr()));
}
```
bool is_served_by_legacy_search_prefetch = false) const;Please avoid a default value. I believe this argument shouldn't be omitted.
bool did_nav_use_bfcache,[nit] `is_served_by_legacy_search_prefetch` is a similar input like `did_nav_use_bfcache` and I prefer after it.
} else if (did_nav_use_bfcache) {I prefer BFCache first as this is a variant of prefetch. Perhaps, after prerender? (My understanding of the SearchPrefetch prerender case is handled by the below prerender logic.)
Related details
- Hiroki Nakagawa
- Huanpo Lin
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. |