Introduce PLMO for preloading serving type for each initiator [chromium/src : main]
Huanpo Lin (Gerrit)
Huanpo Lin added 1 comment![]( "Open in Gerrit")
Related details
- Hiroki Nakagawa
- Ken Okada
- Takashi Toyoshima
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 3 comments![]( "Open in Gerrit")
Only high-level comments for now.
};Cloud you migrate the new PLMO into existing components/page_load_metrics/browser/observers/preload_serving_metrics_page_load_metrics_observer.*? OK to move it and recording logic (partially) to //chrome.
The existing one already has suffix logic. This CL is a natuarl enhancement of it. https://source.chromium.org/chromium/chromium/src/+/main:content/browser/preloading/preload_serving_metrics.cc;l=444-451;drc=ed4f6f5e1fca8223e6041e7e2c3fc4a9deff2c64
base::UmaHistogramEnumeration(histogram_name_all, serving_type);Related to the above comment.
It looks enough to record FCP if we need ratio of triggers. It's natural enhancement of existing `PreloadServingMetrics.PageLoad.Clients.PaintTiming.NavigationToFirstContentfulPaint.<suffix>` https://docs.google.com/document/d/1urfbqZd8Y6iCBenTWEpJFkLO4rm7GlUvnOjFRjQ-cII/edit?disco=AAAB9C04S2w
Related details
- Hiroki Nakagawa
- Huanpo Lin
- 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. |
Ken Okada (Gerrit)
Ken Okada added 1 comment![]( "Open in Gerrit")
Only high-level comments for now.
Let me know if you want me to handle it. I can write a CL.
Ken Okada (Gerrit)
Ken Okada added 1 comment![]( "Open in Gerrit")
base::UmaHistogramEnumeration(histogram_name_all, serving_type);Related to the above comment.
It looks enough to record FCP if we need ratio of triggers. It's natural enhancement of existing `PreloadServingMetrics.PageLoad.Clients.PaintTiming.NavigationToFirstContentfulPaint.<suffix>` https://docs.google.com/document/d/1urfbqZd8Y6iCBenTWEpJFkLO4rm7GlUvnOjFRjQ-cII/edit?disco=AAAB9C04S2w
Chatted with nhiroki. Keeping this metrics as is is suitable for our purpose.
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 4 comments![]( "Open in Gerrit")
Can you add a short description for the class?
#include "components/page_load_metrics/google/browser/google_url_util.h"Sorry if I asked this before but why you are adding this to //Chrome, rather than //components/page_load_metrics/google/browser/?
This google_url_util.h disallows placing this in //components/page_load_metrics/browser/, but the google/browser is the directory for observers who want to depend on it. If we have other dependency to //chrome, the current place will be the best.
If you have a plan to add dependency later, can you clarify it as a TODO comment or something so that we can ensure here is a right place for this observer?
// See PreloadServingType in
// tools/metrics/histograms/metadata/navigation/enums.xmlReplace with an empty line? This is directly expressed by the LINT today. So, just following the example will be the best practice unless something is special?
https://chromium.googlesource.com/chromium/src/tools/+/refs/heads/main/metrics/histograms/README.md#usage
content::NavigationHandle* navigation_handle) {For BFCache restores and Prerender activations, the `navigation_handle` provided to these methods is a new handle that may not contain the original `InitiatorLocation` metadata.
To ensure we correctly attribute these preloading hits to the original source (e.g., a bookmark click that was later BFCached), you should:
1. Add a member variable `std::optional<InitiatorLocation> initiator_location_` to the observer.
2. In `OnCommit`, always capture and store the location from the `navigation_handle`.
3. In `RecordMetrics`, use the stored `initiator_location_` if available, falling back to the handle's data (or `kOther`) only if necessary.
This is my understanding but did you ensure these scenarios in the browser tests?
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. |
Huanpo Lin (Gerrit)
Huanpo Lin voted and added 6 comments![]( "Open in Gerrit")
Votes added by Huanpo Lin
| Commit-Queue | +1 |
Ken OkadaOnly high-level comments for now.
Let me know if you want me to handle it. I can write a CL.
Allow me to mark as closed as another discussion thread is initiated somewhere else.
Can you add a short description for the class?
Done
#include "components/page_load_metrics/google/browser/google_url_util.h"Sorry if I asked this before but why you are adding this to //Chrome, rather than //components/page_load_metrics/google/browser/?
This google_url_util.h disallows placing this in //components/page_load_metrics/browser/, but the google/browser is the directory for observers who want to depend on it. If we have other dependency to //chrome, the current place will be the best.
If you have a plan to add dependency later, can you clarify it as a TODO comment or something so that we can ensure here is a right place for this observer?
The reason is the same as `chrome/browser/page_load_metrics/observers/navigation_initiator_page_load_metrics_observer.cc`. I've added some comments in this file, PTAL. Thanks.
// See PreloadServingType in
// tools/metrics/histograms/metadata/navigation/enums.xmlReplace with an empty line? This is directly expressed by the LINT today. So, just following the example will be the best practice unless something is special?
https://chromium.googlesource.com/chromium/src/tools/+/refs/heads/main/metrics/histograms/README.md#usage
Done
Cloud you migrate the new PLMO into existing components/page_load_metrics/browser/observers/preload_serving_metrics_page_load_metrics_observer.*? OK to move it and recording logic (partially) to //chrome.
The existing one already has suffix logic. This CL is a natuarl enhancement of it. https://source.chromium.org/chromium/chromium/src/+/main:content/browser/preloading/preload_serving_metrics.cc;l=444-451;drc=ed4f6f5e1fca8223e6041e7e2c3fc4a9deff2c64
As part of the plan migrating initiator enum back to //chrome, I think putting PLMO in //chrome is still necessary?
content::NavigationHandle* navigation_handle) {For BFCache restores and Prerender activations, the `navigation_handle` provided to these methods is a new handle that may not contain the original `InitiatorLocation` metadata.
To ensure we correctly attribute these preloading hits to the original source (e.g., a bookmark click that was later BFCached), you should:
1. Add a member variable `std::optional<InitiatorLocation> initiator_location_` to the observer.
2. In `OnCommit`, always capture and store the location from the `navigation_handle`.
3. In `RecordMetrics`, use the stored `initiator_location_` if available, falling back to the handle's data (or `kOther`) only if necessary.This is my understanding but did you ensure these scenarios in the browser tests?
I see, thanks. I've added `std::optional<InitiatorLocation> initiator_location_`.
About test coverage, there are omnibox related tests verifying prerender cases.
But for BFCache, if I understand correctly, it has only history navigation usage so I have added one kOther case only.
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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
content::NavigationHandle* navigation_handle) {Huanpo LinFor BFCache restores and Prerender activations, the `navigation_handle` provided to these methods is a new handle that may not contain the original `InitiatorLocation` metadata.
To ensure we correctly attribute these preloading hits to the original source (e.g., a bookmark click that was later BFCached), you should:
1. Add a member variable `std::optional<InitiatorLocation> initiator_location_` to the observer.
2. In `OnCommit`, always capture and store the location from the `navigation_handle`.
3. In `RecordMetrics`, use the stored `initiator_location_` if available, falling back to the handle's data (or `kOther`) only if necessary.This is my understanding but did you ensure these scenarios in the browser tests?
I see, thanks. I've added `std::optional<InitiatorLocation> initiator_location_`.
About test coverage, there are omnibox related tests verifying prerender cases.
But for BFCache, if I understand correctly, it has only history navigation usage so I have added one kOther case only.
I think the prerender activation has the same problem that the navigation handle is different among the first navigation and the second activation. So, it's curious if no tests failed...
Related details
- Hiroki Nakagawa
- 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. |