[soft navs] Add HistoryNavigation.SoftNavigationCount [chromium/src : main]
Johannes Henkel (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Annie Sullivan
- Michal Mocny
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 3 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
3 comments
class SoftNavigationBackForwardCacheTest : public MetricIntegrationTest {This is fine, but I think we discussed in a previous patch that if you extend `SoftNavigationTest` then you can share some base members and some shared SetUp, instead of using free functions and passing state around.
I'll leave resolved because this is just style.
soft_navigation_count_++;This is totally fine and matches existing implementation-- but some quick thoughts on the long term design:
- It seems to me like there some existing duplication amongst pageload observer types, but not everything is duplicated...
- We already have a common base class `PageLoadMetricsObserver`, (though it seems not to have much in the way of implementation defaults), and it has a default Delegate, which is typically `PageLoadTracker`, I think
- We also already have a common `OnComplete(timing)` which specific PLM observers override, and which acts as the gateway between fetching common metrics from the Delegate, and Recording specific metrics to UKM which differ per PLM observer type.
So the pattern is, I think:
- Common metrics are in PageLoadTracker
- Specific reporting is totally customized
- Timings for recording have defaults, but can be overridden
---
I think you are planning to add Soft-nav specific metrics to `PageLoadTracker`? But I wonder if the page-global soft-nav metrics (soft-nav-count, *-before-first-soft-nav, etc) should all go there as well?
You may already have presented that design, I'm just trying to catch up!
https://bit.ly/soft-navigation1. bit.ly links are basically an ad-serving system now, so we should update these.
2. The doc links to an old soft-navs design doc, I would link to either the WICG repo (https://github.com/WICG/soft-navigations) or the spec (https://wicg.github.io/soft-navigations/) -- Note: they both link to each other already.
Could you also update the existing xml docs/links?
Related details
- Annie Sullivan
- Johannes Henkel
- Robert Kaplow
- Scott Haseley
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. |
Johannes Henkel (Gerrit)
Johannes Henkel added 3 comments![]( "Open in Gerrit")
soft_navigation_count_++;This is totally fine and matches existing implementation-- but some quick thoughts on the long term design:
- It seems to me like there some existing duplication amongst pageload observer types, but not everything is duplicated...
- We already have a common base class `PageLoadMetricsObserver`, (though it seems not to have much in the way of implementation defaults), and it has a default Delegate, which is typically `PageLoadTracker`, I think
- We also already have a common `OnComplete(timing)` which specific PLM observers override, and which acts as the gateway between fetching common metrics from the Delegate, and Recording specific metrics to UKM which differ per PLM observer type.
So the pattern is, I think:
- Common metrics are in PageLoadTracker
- Specific reporting is totally customized
- Timings for recording have defaults, but can be overridden
---
I think you are planning to add Soft-nav specific metrics to `PageLoadTracker`? But I wonder if the page-global soft-nav metrics (soft-nav-count, *-before-first-soft-nav, etc) should all go there as well?
You may already have presented that design, I'm just trying to catch up!
Yeah, thank you! So for now, what I'm trying to get done is the counts, and I figured what I have here is the simplest way to accomplish it.
For the other metrics, my current thinking / rough plan is that I'll try to carve out a class from the ukm_page_load_metrics_observer.cc (the logic about always logging the "previous" soft nav event), and then see how to use that in the prerender and bfcache observers.
I've also been reading the page load tracker and the metrics dispatcher, and will see how / if adding to these is needed. Mostly I'm worried about the mixing of delayed dispatch (e.g. timings that are waiting for other frames) with more synchronous dispatch like the OnSoftNavigation hook that I'm using here. But for the count, I think it's correct-ish.
1. bit.ly links are basically an ad-serving system now, so we should update these.
2. The doc links to an old soft-navs design doc, I would link to either the WICG repo (https://github.com/WICG/soft-navigations) or the spec (https://wicg.github.io/soft-navigations/) -- Note: they both link to each other already.Could you also update the existing xml docs/links?
- Annie Sullivan
- Robert Kaplow
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Annie Sullivan
- Robert Kaplow
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Kaplow (Gerrit)
Robert Kaplow voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Annie Sullivan
- Johannes Henkel
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Henkel (Gerrit)
Johannes Henkel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Johannes Henkel
| Commit-Queue | +2 |
Thanks a lot!
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[soft navs] Add HistoryNavigation.SoftNavigationCount
This metric works just like Pageload.SoftNavigationcount and
PrerenderPageLoad.SoftNavigationCount.
Privacy doc:
https://docs.google.com/document/d/1ZhUkF63xINZDVQ3e5Bg370TpOKGf6_-_AuznrzB2tEk/edit?resourcekey=0-dtxS11-tTye7_GjEzMHuyw
- M chrome/browser/page_load_metrics/integration_tests/soft_navigation_metrics_browsertest.cc
- M components/page_load_metrics/browser/observers/back_forward_cache_page_load_metrics_observer.cc
- M components/page_load_metrics/browser/observers/back_forward_cache_page_load_metrics_observer.h
- M tools/metrics/ukm/ukm.xml
Code-Review: +1 by Michal Mocny, +1 by Robert Kaplow, +1 by Scott Haseley
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |