Introduce PreloadServingMetrics.{ChromeInitiatorLocation}.{SrpAll} [chromium/src : main]
Huanpo Lin (Gerrit)
Huanpo Lin added 1 comment![]( "Open in Gerrit")
Related details
- Hiroki Nakagawa
- Ken Okada
- 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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
void PreloadServingMetricsPageLoadMetricsObserver::
OnRestoreFromBackForwardCache(
const page_load_metrics::mojom::PageLoadTiming& timing,
content::NavigationHandle* navigation_handle) {
is_bfcache_ = true;
RetrieveNavigationInitiatorLocationAndSrp(navigation_handle);
MaybeRecord();Currently, BFCache cases are handled in a wrong way. Maybe, the first navigation is not recorded correctly, and the BFCache activation is recorded twice on activation and page close, Complete?
Related details
- Hiroki Nakagawa
- Huanpo Lin
- Ken Okada
- 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. |
Huanpo Lin (Gerrit)
Huanpo Lin added 1 comment![]( "Open in Gerrit")
void PreloadServingMetricsPageLoadMetricsObserver::
OnRestoreFromBackForwardCache(
const page_load_metrics::mojom::PageLoadTiming& timing,
content::NavigationHandle* navigation_handle) {
is_bfcache_ = true;
RetrieveNavigationInitiatorLocationAndSrp(navigation_handle);
MaybeRecord();Currently, BFCache cases are handled in a wrong way. Maybe, the first navigation is not recorded correctly, and the BFCache activation is recorded twice on activation and page close, Complete?
I see, I've updated the logic, PTAL.
My understanding is that:
1. `PreloadServingMetricsPageLoadMetricsObserver::OnEnterBackForwardCache` needs to record the first navigation (non BFCache) before the navigation entering the cache.
2. After recording in `PreloadServingMetricsPageLoadMetricsObserver::OnEnterBackForwardCache`, the capsule is reset to avoid double recording if the page is never restored.
3. `PreloadServingMetricsPageLoadMetricsObserver::OnRestoreFromBackForwardCache` will set the correct information if the page is restored.
4. `OnComplete/FlushMetricsOnAppEnterBackground` will record the information if available.
Is my understanding correct?
Related details
- Hiroki Nakagawa
- Ken Okada
- 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
GURL url_a(embedded_test_server()->GetURL("a.com", "/title1.html"));nit: Can we use a.test and b.test?
IIRC, any host will be resolved into the same address, and it just works.
Related details
- Hiroki Nakagawa
- Huanpo Lin
- Ken Okada
- 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 PreloadServingMetricsPageLoadMetricsObserver::
OnRestoreFromBackForwardCache(
const page_load_metrics::mojom::PageLoadTiming& timing,
content::NavigationHandle* navigation_handle) {
is_bfcache_ = true;
RetrieveNavigationInitiatorLocationAndSrp(navigation_handle);
MaybeRecord();Huanpo LinCurrently, BFCache cases are handled in a wrong way. Maybe, the first navigation is not recorded correctly, and the BFCache activation is recorded twice on activation and page close, Complete?
I see, I've updated the logic, PTAL.
My understanding is that:
1. `PreloadServingMetricsPageLoadMetricsObserver::OnEnterBackForwardCache` needs to record the first navigation (non BFCache) before the navigation entering the cache.
2. After recording in `PreloadServingMetricsPageLoadMetricsObserver::OnEnterBackForwardCache`, the capsule is reset to avoid double recording if the page is never restored.
3. `PreloadServingMetricsPageLoadMetricsObserver::OnRestoreFromBackForwardCache` will set the correct information if the page is restored.
4. `OnComplete/FlushMetricsOnAppEnterBackground` will record the information if available.
Is my understanding correct?
Yes, this direction sounds correct to me.
Huanpo Lin (Gerrit)
Huanpo Lin added 1 comment![]( "Open in Gerrit")
include curranmax@ for tools/metrics/histograms/metadata/preloading/histograms.xml, PTAL
Related details
- Hiroki Nakagawa
- Ken Okada
- Max Curran
- 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. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa added 2 comments![]( "Open in Gerrit")
kNoPreload = 3,This enum order seems very random. Can we sort them in the order of the "preloading pipeline" stages? (In my mental model, bfcache comes after prefetch and prerender)
```
enum class PreloadServingMetricsType {
kNoPreload = 0,
kPrefetch = 1,
kPrerender = 2,
kBFCache = 3,
kMaxValue = kBFCache,
};
```
I know we won't be able to strictly maintain this order as new types are appended, but it makes sense to organize it logically for now.
}Instead of nesting if-statements for kPrefetch and kNoPreload, can we flatten them for simplicity like this?
```
auto type = PreloadServingMetricsType::kNoPreload;
if (prerender_initial_preload_serving_metrics) {
type = PreloadServingMetricsType::kPrerender;
} else if (is_bfcache) {
type = PreloadServingMetricsType::kBFCache;
} else if (const auto* prefetch_match = GetMeaningfulPrefetchMatchMetrics();
prefetch_match && prefetch_match->IsActualMatch()) {
type = PreloadServingMetricsType::kPrefetch;
}
```
Also, checking BFCache, Prerender, and Prefetch in this order may be more natural? (BFCache restoration happens after prefetch/prerender)
```
auto type = PreloadServingMetricsType::kNoPreload;
if (is_bfcache) {
type = PreloadServingMetricsType::kBFCache;
} else if (prerender_initial_preload_serving_metrics) {
type = PreloadServingMetricsType::kPrerender;
} else if (const auto* prefetch_match = GetMeaningfulPrefetchMatchMetrics();
prefetch_match && prefetch_match->IsActualMatch()) {
type = PreloadServingMetricsType::kPrefetch;
}
```
Related details
- Huanpo Lin
Huanpo Lin (Gerrit)
Huanpo Lin added 2 comments![]( "Open in Gerrit")
GURL url_a(embedded_test_server()->GetURL("a.com", "/title1.html"));nit: Can we use a.test and b.test?
IIRC, any host will be resolved into the same address, and it just works.
void PreloadServingMetricsPageLoadMetricsObserver::
OnRestoreFromBackForwardCache(
const page_load_metrics::mojom::PageLoadTiming& timing,
content::NavigationHandle* navigation_handle) {
is_bfcache_ = true;
RetrieveNavigationInitiatorLocationAndSrp(navigation_handle);
MaybeRecord();Huanpo LinCurrently, BFCache cases are handled in a wrong way. Maybe, the first navigation is not recorded correctly, and the BFCache activation is recorded twice on activation and page close, Complete?
Takashi ToyoshimaI see, I've updated the logic, PTAL.
My understanding is that:
1. `PreloadServingMetricsPageLoadMetricsObserver::OnEnterBackForwardCache` needs to record the first navigation (non BFCache) before the navigation entering the cache.
2. After recording in `PreloadServingMetricsPageLoadMetricsObserver::OnEnterBackForwardCache`, the capsule is reset to avoid double recording if the page is never restored.
3. `PreloadServingMetricsPageLoadMetricsObserver::OnRestoreFromBackForwardCache` will set the correct information if the page is restored.
4. `OnComplete/FlushMetricsOnAppEnterBackground` will record the information if available.
Is my understanding correct?
Yes, this direction sounds correct to me.
- Huanpo Lin
- Ken Okada
- Max Curran
- Rakina Zata Amni
- Takashi Toyoshima
Rakina Zata Amni (Gerrit)
Rakina Zata Amni voted Code-Review+1![]( "Open in Gerrit")
- Huanpo Lin
- Ken Okada
- Max Curran
- Takashi Toyoshima
Ken Okada (Gerrit)
Ken Okada added 23 comments![]( "Open in Gerrit")
IN_PROC_BROWSER_TEST_F(BackForwardCachePageLoadMetricsObserverBrowserTest,Could you add a short description for every test?
EXPECT_TRUE(ui_test_utils::NavigateToURL(browser(), url_b));Use `about:blank` for flush.
bool is_bfcache_ = false;`has_restored_from_bfcache_` is better as "This (PLMO) is BFCache" is not true.
bool is_srp_ = false;`is_url_srp_` is better as "This (PLMO) is SRP" is not true, but "The page's url is SRP."
std::string navigation_type_string_ = "Other";Could you use `std::optional<std::string>` and explicitly set `"Other"` in retrieve?
std::string navigation_type_string_ = "Other";Could you add TODO for a bug for refactoring phase for me? https://docs.google.com/document/d/1d9k-YDEdT35LDVN3BkILyDVqZKlv-b6F0yV_OZRVIQk/edit?resourcekey=0-Jr0Dysk9Cabb0vZlG-ESXg&tab=t.0#heading=h.dygbqkif9aw5
PreloadServingMetricsPageLoadMetricsObserver::OnEnterBackForwardCache(Prefer placing it and OnRestoreFromBackForwardCache after FlushMetricsOnAppEnterBackground.
preload_serving_metrics_capsule_ =Could you add a comment e.g. "Take a `PreloadServingMetrics` of the `NavigationHandle` representing the navigation that used BFCache. Note that the `NavigationHandle` differs from the one created this PLMO, and the `PreloadServingMetrics` for it has been reset."
// `OnFirstContentfulPaintInPage()` is called after `OnCommit()` (or1. Could you check `PLMO::OnFirstContentfulPaintInPage` is called for BFCache?
2. Existence of `preload_serving_metrics_capsule_` is important invariant and I'd like to preserve.
So, if 1 is yes, add `if (is_bf_cache_) { return; }`. If 1 is false, revert it and add a comment for BFCache.
---
If you set PSMC on OnRestoreFromBackForwardCache, the `CHECK` looks to pass.
preload_serving_metrics_capsule_->RecordPreloadServingMetricsByInitiator(Could you make it clear what concept we use? `Initiator`/`InitiatorLocation`/`NavigationType`? (If you don't have strong opinion, I vote `NavigationInitiator`.) Also, please update the doc for the concept.
cc: nhi...@chromium.org for naming
preload_serving_metrics_capsule_->RecordPreloadServingMetricsByInitiator(This and taking PSMC for BFCache sound weird for BFCache, but correcting it needs more refactoring. Could you add a TODO comment?
bool is_bfcache) const;The order should be `is_bfcache`, `navigation_initiator_string` and then `is_srp`, as `is_bfcache` is like the existence of `PSM`.
const std::string& navigation_type_string,Same to the comment on components/page_load_metrics/browser/observers/preload_serving_metrics_page_load_metrics_observer.cc L151.
kNoPreload = 3,1. It's not PSMType. How about `UsedInstantLoad`?
2. I'd like to use `kNoInstantLoad = 0,` as it should be called the default. (And it should be MECE, but "No preloads && Used BFCache" can be satisfied.)
3. `kPrefetch < kPrerender` as it's the order in preload pipeline.
```
enum class UsedInstantLoad {
kNoInstantLoad = 0,
kPrefetch = 1,
kPrerender = 2,
kBFCache = 3,
kMaxValue = kBFCache,
};
```
cc: nhi...@chromium.org for naming
} else if (is_bfcache) {`is_bfcache` should be checked first.
"PreloadServingMetrics." + navigation_type_string + ".All", type);Prefer `base::StrCat`, as it is better in performance [[cs](https://source.chromium.org/chromium/chromium/src/+/main:base/strings/strcat_internal.h;l=26;drc=60df9044934a2841875904f21e9673afe970da1e)].
log->RecordPreloadServingMetricsByInitiator("Other", false, false);Add `/*arg_name=*/`s.
} // namespace contentCould you add a test for a case that `PreloadServingMetrics.Other.SRP` is recorded?
<variant name="Other" summary="Initiated from Other"/>other
<variants name="SrpAll">How about `NavigationDestinationSuffix`?
<variant name="All" summary="all navigations"/>All
<variant name="SRP" summary="Google Search Result Pages"/>Navigation to Google Search Result Page
Recorded when navigation finishes. Records the preload mechanism used forthe instant load technology?
Related details
- Huanpo Lin
- Max Curran
- Takashi Toyoshima
Max Curran (Gerrit)
Max Curran voted and added 1 comment![]( "Open in Gerrit")
Votes added by Max Curran
| Code-Review | +1 |
1 comment
- Huanpo Lin
- Takashi Toyoshima
Code-Coverage
Code-Owners
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa added 1 comment![]( "Open in Gerrit")
kNoPreload = 3,1. It's not PSMType. How about `UsedInstantLoad`?
2. I'd like to use `kNoInstantLoad = 0,` as it should be called the default. (And it should be MECE, but "No preloads && Used BFCache" can be satisfied.)
3. `kPrefetch < kPrerender` as it's the order in preload pipeline.```
enum class UsedInstantLoad {
kNoInstantLoad = 0,
kPrefetch = 1,
kPrerender = 2,
kBFCache = 3,
kMaxValue = kBFCache,
};
```cc: nhi...@chromium.org for naming
1. It's not PSMType. How about `UsedInstantLoad`?
+1 to renaming.
I think I see your point: generally speaking, "Preload" doesn't include BFCache. Personally, I'm not a big fan of using "Instant", since that's just the consequence of the load rather than the loading type itself. However, I don't come up with alternatives, and `UsedInstantLoad` looks appropriate in this situation.
Huanpo Lin (Gerrit)
Huanpo Lin voted and added 25 comments![]( "Open in Gerrit")
Votes added by Huanpo Lin
| Commit-Queue | +1 |
25 comments
IN_PROC_BROWSER_TEST_F(BackForwardCachePageLoadMetricsObserverBrowserTest,Could you add a short description for every test?
Done
EXPECT_TRUE(ui_test_utils::NavigateToURL(browser(), url_b));Use `about:blank` for flush.
Done
`has_restored_from_bfcache_` is better as "This (PLMO) is BFCache" is not true.
Done
`is_url_srp_` is better as "This (PLMO) is SRP" is not true, but "The page's url is SRP."
Done
Could you use `std::optional<std::string>` and explicitly set `"Other"` in retrieve?
Done
Could you add TODO for a bug for refactoring phase for me? https://docs.google.com/document/d/1d9k-YDEdT35LDVN3BkILyDVqZKlv-b6F0yV_OZRVIQk/edit?resourcekey=0-Jr0Dysk9Cabb0vZlG-ESXg&tab=t.0#heading=h.dygbqkif9aw5
Done
PreloadServingMetricsPageLoadMetricsObserver::OnEnterBackForwardCache(Prefer placing it and OnRestoreFromBackForwardCache after FlushMetricsOnAppEnterBackground.
Done
Could you add a comment e.g. "Take a `PreloadServingMetrics` of the `NavigationHandle` representing the navigation that used BFCache. Note that the `NavigationHandle` differs from the one created this PLMO, and the `PreloadServingMetrics` for it has been reset."
Done
// `OnFirstContentfulPaintInPage()` is called after `OnCommit()` (or1. Could you check `PLMO::OnFirstContentfulPaintInPage` is called for BFCache?
2. Existence of `preload_serving_metrics_capsule_` is important invariant and I'd like to preserve.
So, if 1 is yes, add `if (is_bf_cache_) { return; }`. If 1 is false, revert it and add a comment for BFCache.
---
If you set PSMC on OnRestoreFromBackForwardCache, the `CHECK` looks to pass.
For (1), I believe it is yes. There are some other tests which are not explicitly testing BFCache were failing in the previous patch so I changed this and added the comments.
I've reverted back to use CHECK and added `has_entered_bfcache_`.
The `is_bf_cache_(has_restored_from_bfcache_)` is called on restored so it won't work for this part. `has_entered_bfcache_` is introduced instead to prevent this, PTAL whether this pattern is more desired.
preload_serving_metrics_capsule_->RecordPreloadServingMetricsByInitiator(This and taking PSMC for BFCache sound weird for BFCache, but correcting it needs more refactoring. Could you add a TODO comment?
Done
preload_serving_metrics_capsule_->RecordPreloadServingMetricsByInitiator(Could you make it clear what concept we use? `Initiator`/`InitiatorLocation`/`NavigationType`? (If you don't have strong opinion, I vote `NavigationInitiator`.) Also, please update the doc for the concept.
cc: nhi...@chromium.org for naming
Done
bool is_bfcache) const;The order should be `is_bfcache`, `navigation_initiator_string` and then `is_srp`, as `is_bfcache` is like the existence of `PSM`.
Sorry, I don't quite get this part why the order should be like this? The order currently is also the order of member declaration in the class. Could you rephrase it?
Same to the comment on components/page_load_metrics/browser/observers/preload_serving_metrics_page_load_metrics_observer.cc L151.
Done
kNoPreload = 3,This enum order seems very random. Can we sort them in the order of the "preloading pipeline" stages? (In my mental model, bfcache comes after prefetch and prerender)
```
enum class PreloadServingMetricsType {
kNoPreload = 0,
kPrefetch = 1,
kPrerender = 2,
kBFCache = 3,
kMaxValue = kBFCache,
};
```
Huanpo LinI know we won't be able to strictly maintain this order as new types are appended, but it makes sense to organize it logically for now.
Done
Hiroki Nakagawa1. It's not PSMType. How about `UsedInstantLoad`?
2. I'd like to use `kNoInstantLoad = 0,` as it should be called the default. (And it should be MECE, but "No preloads && Used BFCache" can be satisfied.)
3. `kPrefetch < kPrerender` as it's the order in preload pipeline.```
enum class UsedInstantLoad {
kNoInstantLoad = 0,
kPrefetch = 1,
kPrerender = 2,
kBFCache = 3,
kMaxValue = kBFCache,
};
```cc: nhi...@chromium.org for naming
1. It's not PSMType. How about `UsedInstantLoad`?
+1 to renaming.
I think I see your point: generally speaking, "Preload" doesn't include BFCache. Personally, I'm not a big fan of using "Instant", since that's just the consequence of the load rather than the loading type itself. However, I don't come up with alternatives, and `UsedInstantLoad` looks appropriate in this situation.
Done
`is_bfcache` should be checked first.
Done
Instead of nesting if-statements for kPrefetch and kNoPreload, can we flatten them for simplicity like this?
```
auto type = PreloadServingMetricsType::kNoPreload;
if (prerender_initial_preload_serving_metrics) {
type = PreloadServingMetricsType::kPrerender;
} else if (is_bfcache) {
type = PreloadServingMetricsType::kBFCache;
} else if (const auto* prefetch_match = GetMeaningfulPrefetchMatchMetrics();
prefetch_match && prefetch_match->IsActualMatch()) {
type = PreloadServingMetricsType::kPrefetch;
}
```Also, checking BFCache, Prerender, and Prefetch in this order may be more natural? (BFCache restoration happens after prefetch/prerender)
```
auto type = PreloadServingMetricsType::kNoPreload;
if (is_bfcache) {
type = PreloadServingMetricsType::kBFCache;
} else if (prerender_initial_preload_serving_metrics) {
type = PreloadServingMetricsType::kPrerender;
} else if (const auto* prefetch_match = GetMeaningfulPrefetchMatchMetrics();
prefetch_match && prefetch_match->IsActualMatch()) {
type = PreloadServingMetricsType::kPrefetch;
}
```
Done
"PreloadServingMetrics." + navigation_type_string + ".All", type);Prefer `base::StrCat`, as it is better in performance [[cs](https://source.chromium.org/chromium/chromium/src/+/main:base/strings/strcat_internal.h;l=26;drc=60df9044934a2841875904f21e9673afe970da1e)].
Done
log->RecordPreloadServingMetricsByInitiator("Other", false, false);Huanpo LinAdd `/*arg_name=*/`s.
Done
Could you add a test for a case that `PreloadServingMetrics.Other.SRP` is recorded?
Done
<variant name="Other" summary="Initiated from Other"/>Huanpo Linother
Done
<variants name="SrpAll">Huanpo LinHow about `NavigationDestinationSuffix`?
Done
<variant name="All" summary="all navigations"/>Huanpo LinAll
Done
<variant name="SRP" summary="Google Search Result Pages"/>Navigation to Google Search Result Page
Done
Recorded when navigation finishes. Records the preload mechanism used forHuanpo Linthe instant load technology?
Done
Related details
- Hiroki Nakagawa
- Ken Okada
- 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. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa added 2 comments![]( "Open in Gerrit")
GURL url_a(embedded_test_server()->GetURL("a.com", "/title1.html"));Huanpo Linnit: Can we use a.test and b.test?
IIRC, any host will be resolved into the same address, and it just works.
Yes, `a.test` and `b.test` will still work.
But as the other tests in this file use `a.com` and `b.com`, should it also follow the same pattern for consistency?
I think it's nice to use domains reserved for tests in general, but you don't need to change the existing usage in this CL.
kNoPreload = 3,Hiroki Nakagawa1. It's not PSMType. How about `UsedInstantLoad`?
2. I'd like to use `kNoInstantLoad = 0,` as it should be called the default. (And it should be MECE, but "No preloads && Used BFCache" can be satisfied.)
3. `kPrefetch < kPrerender` as it's the order in preload pipeline.```
enum class UsedInstantLoad {
kNoInstantLoad = 0,
kPrefetch = 1,
kPrerender = 2,
kBFCache = 3,
kMaxValue = kBFCache,
};
```cc: nhi...@chromium.org for naming
Huanpo Lin1. It's not PSMType. How about `UsedInstantLoad`?
+1 to renaming.
I think I see your point: generally speaking, "Preload" doesn't include BFCache. Personally, I'm not a big fan of using "Instant", since that's just the consequence of the load rather than the loading type itself. However, I don't come up with alternatives, and `UsedInstantLoad` looks appropriate in this situation.
Done
s/kNoPreload/kNoInstantLoad/
Also, please update the order.
Related details
- Huanpo Lin
- Ken Okada
- Takashi Toyoshima
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa voted and added 5 comments![]( "Open in Gerrit")
Votes added by Hiroki Nakagawa
| Code-Review | +1 |
5 comments
LGTM after all the comments are addressed.
page_load_metrics::PageLoadMetricsObserver::ObservePolicy
PreloadServingMetricsPageLoadMetricsObserver::OnEnterBackForwardCache(
const page_load_metrics::mojom::PageLoadTiming& timing) {
MaybeRecord();
has_entered_bfcache_ = true;
preload_serving_metrics_capsule_.reset();
return CONTINUE_OBSERVING;
}`MaybeRecord()` was not called on this event before this CL. Is this a fix for an existing bug?
base::test::ScopedFeatureList feature_list;
feature_list.InitWithFeaturesAndParameters(
{
{
features::kPrerender2FallbackPrefetchSpecRules,
{},
},
},
{});Why is this needed for BFCache test?
}Do we have a test case where both prefetch/prerender and BFCache are involved like this?
- prefetch page-A
- prerender page-A
- activate page-A --> kPrerender is recorded
- navigate away to page-B --> kNoPreload is recorded
- navigate back to page-A --> kBFCache is recorded
If not, can we add it?
base::test::ScopedFeatureList feature_list;
feature_list.InitWithFeaturesAndParameters(
{
{
features::kPrerender2FallbackPrefetchSpecRules,
{},
},
},
{});Ditto.
Ken Okada (Gerrit)
Ken Okada added 10 comments![]( "Open in Gerrit")
bool has_entered_bfcache_ = false;I feel that `has_entered_bfcache_` is redundant and can be removed. It is used in OnFirstContentfulPaintInPage, but I believe that it's not called during the period from enter to restore.
std::optional<std::string> navigation_type_string_;`navigation_initiator_string_`?
DLOG(ERROR) << "PreloadServingMetricsPageLoadMetricsObserver::"Remaining printf for debug?
// `OnFirstContentfulPaintInPage()` is called after `OnCommit()` (orAdd newline above.
// `OnFirstContentfulPaintInPage()` is called after `OnCommit()` (or1. Could you check `PLMO::OnFirstContentfulPaintInPage` is called for BFCache?
2. Existence of `preload_serving_metrics_capsule_` is important invariant and I'd like to preserve.
So, if 1 is yes, add `if (is_bf_cache_) { return; }`. If 1 is false, revert it and add a comment for BFCache.
---
If you set PSMC on OnRestoreFromBackForwardCache, the `CHECK` looks to pass.
For (1), I believe it is yes. There are some other tests which are not explicitly testing BFCache were failing in the previous patch so I changed this and added the comments.
I've reverted back to use CHECK and added `has_entered_bfcache_`.
The `is_bf_cache_(has_restored_from_bfcache_)` is called on restored so it won't work for this part. `has_entered_bfcache_` is introduced instead to prevent this, PTAL whether this pattern is more desired.
If you set PSMC on OnRestoreFromBackForwardCache, the CHECK looks to pass.
How about this point? This early return prevents to record FCP for navs used BFCache. (Behavioral change if we record. Currently not recorded.)
// Note that
// `preload_serving_metrics_capsule_` can be null if the page entered
// BackForwardCache before FCP occurred (which resets the capsule) or if FCP
// timing is delivered out of order.
Is this true? Restore sets PSMC.
page_load_metrics::PageLoadMetricsObserver::ObservePolicy
PreloadServingMetricsPageLoadMetricsObserver::OnEnterBackForwardCache(
const page_load_metrics::mojom::PageLoadTiming& timing) {
MaybeRecord();
has_entered_bfcache_ = true;
preload_serving_metrics_capsule_.reset();
return CONTINUE_OBSERVING;
}`MaybeRecord()` was not called on this event before this CL. Is this a fix for an existing bug?
It's OK to record this case, but it's nice to file a bug first. (Splitting a CL is better, but optional.) We should note it as a behavioral change in the commit message at least.
has_entered_bfcache_ = false;(It should be `= true;`, i.e. nop and we should remove it, as the fact "entered" is not changed.)
const std::string& navigation_type_string,`navigation_initiator_string`
bool has_restored_from_bfcache) const {`is_nav_used_bfcache` as `has_restored_from_bfcache` is a predicate for PLMO, but the arg is for navigation and `NavigationHandle`.
<int value="3" label="BFCache"/>[not blocking] as we can postpone this discussion and revise UMAs.
My personal preference is adding "InitiatedBy" and "Used" or "With" prefix for navigation initiator and used instant load for metrics. (Enum variants would be OK without prefix.) Because metrics names contain multiple infos, like navigation initiator, used instant load, navigation destination, e.g. `PreloadServingMetrics.PageLoad.Clients.PaintTiming.NavigationToFirstContentfulPaint.{NavigationInitiator}.{UsedInstantLoad}.{NavigationDestination}`. For example, I prefer
- PreloadServingMetrics.PageLoad.Clients.PaintTiming.NavigationToFirstContentfulPaint.InitiatedByOmniboxDefaultSearchEngine.WithPrerender.All (or UsedPrerender)
- PreloadServingMetrics.PageLoad.Clients.PaintTiming.NavigationToFirstContentfulPaint.InitiatedByOther.WithNoInstantLoad.SRP (or NoInstantLoad?)
instead of
- PreloadServingMetrics.PageLoad.Clients.PaintTiming.NavigationToFirstContentfulPaint.OmniboxDefaultSearchEngine.Prerender.All
- PreloadServingMetrics.PageLoad.Clients.PaintTiming.NavigationToFirstContentfulPaint.Other.NoInstantLoad.SRP
as the former are self-descriptive.
Related details
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")
bool is_bfcache) const;The order should be `is_bfcache`, `navigation_initiator_string` and then `is_srp`, as `is_bfcache` is like the existence of `PSM`.
Sorry, I don't quite get this part why the order should be like this? The order currently is also the order of member declaration in the class. Could you rephrase it?
The implementation is
```
void PreloadServingMetrics::RecordPreloadServingMetricsByNavigationInitiator(
const std::string& navigation_type_string,
bool is_url_srp,
bool has_restored_from_bfcache) const {
UsedInstantLoad type;
if (has_restored_from_bfcache) {
type = UsedInstantLoad::kBFCache;
} else if (prerender_initial_preload_serving_metrics) {
type = UsedInstantLoad::kPrerender;
} else if (const auto* prefetch_match = GetMeaningfulPrefetchMatchMetrics();
prefetch_match && prefetch_match->IsActualMatch()) {
type = UsedInstantLoad::kPrefetch;
} else {
type = UsedInstantLoad::kNoInstantLoad;
}
```
If we write the receiver as `self` [like in Rust](https://doc.rust-lang.org/std/keyword.self.html),
```
void PreloadServingMetrics::RecordPreloadServingMetricsByNavigationInitiator(
self, // Used to determine `UsedInstantLoad`
const std::string& navigation_type_string,
bool is_url_srp,
bool has_restored_from_bfcache // Used to determine `UsedInstantLoad`
) const {
```
So, I think it's better to collocate:
```
void PreloadServingMetrics::RecordPreloadServingMetricsByNavigationInitiator(
self, // Used to determine `UsedInstantLoad`
bool has_restored_from_bfcache // Used to determine `UsedInstantLoad`
const std::string& navigation_type_string,
bool is_url_srp,
) const {
```
UsedInstantLoad type;`type` is vague word, especially if we are handling multiple "type" concepts like `UsedInstantLoad` and `NavigationInitiator`. `used_instant_load` would be better.
Related details
- Huanpo Lin
Ken Okada (Gerrit)
Ken Okada added 2 comments![]( "Open in Gerrit")
// - A is restored from BackForwardCache.`Navigation B is started and used A, which is restored from BackForwardCache.`
TEST(PreloadServingMetricsTest,Please keep tests minimum and check only one case per test. I couldn't understand what is the core part of this test.
Huanpo Lin (Gerrit)
Huanpo Lin added 6 comments![]( "Open in Gerrit")
GURL url_a(embedded_test_server()->GetURL("a.com", "/title1.html"));Huanpo Linnit: Can we use a.test and b.test?
IIRC, any host will be resolved into the same address, and it just works.
Hiroki NakagawaYes, `a.test` and `b.test` will still work.
But as the other tests in this file use `a.com` and `b.com`, should it also follow the same pattern for consistency?
I think it's nice to use domains reserved for tests in general, but you don't need to change the existing usage in this CL.
Got it, updated.
DLOG(ERROR) << "PreloadServingMetricsPageLoadMetricsObserver::"Huanpo LinRemaining printf for debug?
Done
page_load_metrics::PageLoadMetricsObserver::ObservePolicy
PreloadServingMetricsPageLoadMetricsObserver::OnEnterBackForwardCache(
const page_load_metrics::mojom::PageLoadTiming& timing) {
MaybeRecord();
has_entered_bfcache_ = true;
preload_serving_metrics_capsule_.reset();
return CONTINUE_OBSERVING;
}`MaybeRecord()` was not called on this event before this CL. Is this a fix for an existing bug?
Before this CL, `PreloadServingMetricsPageLoadMetricsObserver::OnEnterBackForwardCache` uses the default behavior, calling `OnComplete`, which should be only called once for every PLMO IIUC, and STOP_OBSERVING, and MaybeRecord() is called in `OnComplete`.
But in this CL, `CONTINUE_OBSERVING` is used and `OnComplete` shouldn't be used as it is not necessarily the end of the PLMO.
https://chromium-review.git.corp.google.com/c/chromium/src/+/8129579/comment/99f53a3a_b3226559/ explains why it looks like the current patch.
kNoPreload = 3,Hiroki Nakagawa1. It's not PSMType. How about `UsedInstantLoad`?
2. I'd like to use `kNoInstantLoad = 0,` as it should be called the default. (And it should be MECE, but "No preloads && Used BFCache" can be satisfied.)
3. `kPrefetch < kPrerender` as it's the order in preload pipeline.```
enum class UsedInstantLoad {
kNoInstantLoad = 0,
kPrefetch = 1,
kPrerender = 2,
kBFCache = 3,
kMaxValue = kBFCache,
};
```cc: nhi...@chromium.org for naming
Huanpo Lin1. It's not PSMType. How about `UsedInstantLoad`?
+1 to renaming.
I think I see your point: generally speaking, "Preload" doesn't include BFCache. Personally, I'm not a big fan of using "Instant", since that's just the consequence of the load rather than the loading type itself. However, I don't come up with alternatives, and `UsedInstantLoad` looks appropriate in this situation.
Hiroki NakagawaDone
s/kNoPreload/kNoInstantLoad/
Also, please update the order.
Thanks, updated.
base::test::ScopedFeatureList feature_list;
feature_list.InitWithFeaturesAndParameters(
{
{
features::kPrerender2FallbackPrefetchSpecRules,
{},
},
},
{});Why is this needed for BFCache test?
It was copied from other cases, but it is actually not needed.
Removed, thanks.
base::test::ScopedFeatureList feature_list;
feature_list.InitWithFeaturesAndParameters(
{
{
features::kPrerender2FallbackPrefetchSpecRules,
{},
},
},
{});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. |