[PageLoadMetrics] Add LCP/FCP slicing by navigation type [chromium/src : main]
Ming-Ying Chung (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Ian Clelland
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 1 comment![]( "Open in Gerrit")
Votes added by Michal Mocny
| Commit-Queue | +0 |
1 comment
Related details
- Ian Clelland
- Ming-Ying Chung
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 added 1 comment![]( "Open in Gerrit")
I'm OOO this week, and iclelland is no longer working in this area. Added alternative reviewers for an initial stab.
(I know Annie took a quick look last Friday and wondered about handling reporting for background->foreground paints potentially reporting all of the background time as paint latency?)
Related details
- Annie Sullivan
- Johannes Henkel
- Ming-Ying Chung
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ming-Ying Chung (Gerrit)
Ming-Ying Chung added 1 comment![]( "Open in Gerrit")
PTAL
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 added 2 comments![]( "Open in Gerrit")
For now, please consider this a question about the design, and let me know what you think. It is quite possible that what you have is better than the strawman I'm suggesting, but if so, I'd like to learn why please. :-)
GetNavigationTypeSuffix(nav_scenario_)}),The way I understand this, this enum value `nav_scenario_` distinguishes different "types" of page loads.
The place where we figure out which type of page load something is is in the PageLoadTracker, and it doesn't change per page load, which is why in this changelist, it's getting passed into the observer when its constructed. And page_load_metrics_initialize.cc and page_load_metrics_embedder_base.cc wire that up.
I can see that this works.
However:
It's problematic that this observer and the page load tracker are going to be very tightly coupled, via these aforementioned mechanisms, and when some other observer needs this information, they'll also be very tightly coupled. Worst case in a way, if the enum gets very popular (this is not guaranteed), things would turn into a hairball.
What do you think about the following:
1) Add some accessor to PageLoadMetricsObserverDelegate to get the nav scenario enum value. GetNavScenario (or whatever is a descriptive name).
2) In page_load_tracker.{h,cc}, override and implement the accessor. If needed, you can add a field to PageLoadTracker to store it there. If the tracker must get it from the page_load_metrics_initialize.cc file, I guess that's OK, perhaps you can look into what's a reasonable way to accomplish that? (e.g., with some precedents?)
3) In this file, get the value with GetDelegate().GetNavScenario() (instead of nav_scenario_).
The advantage would be that there's no tight coupling and uma_page_load_metrics_observer.cc would not become special and require doing stuff to the embedder and/or initialization code.
What do you think?
Related details
- Annie Sullivan
- Ming-Ying Chung
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. |
Ming-Ying Chung (Gerrit)
Ming-Ying Chung added 3 comments![]( "Open in Gerrit")
PTAL
The way I understand this, this enum value `nav_scenario_` distinguishes different "types" of page loads.
The place where we figure out which type of page load something is is in the PageLoadTracker, and it doesn't change per page load, which is why in this changelist, it's getting passed into the observer when its constructed. And page_load_metrics_initialize.cc and page_load_metrics_embedder_base.cc wire that up.
I can see that this works.
However:
It's problematic that this observer and the page load tracker are going to be very tightly coupled, via these aforementioned mechanisms, and when some other observer needs this information, they'll also be very tightly coupled. Worst case in a way, if the enum gets very popular (this is not guaranteed), things would turn into a hairball.
What do you think about the following:
1) Add some accessor to PageLoadMetricsObserverDelegate to get the nav scenario enum value. GetNavScenario (or whatever is a descriptive name).
2) In page_load_tracker.{h,cc}, override and implement the accessor. If needed, you can add a field to PageLoadTracker to store it there. If the tracker must get it from the page_load_metrics_initialize.cc file, I guess that's OK, perhaps you can look into what's a reasonable way to accomplish that? (e.g., with some precedents?)
3) In this file, get the value with GetDelegate().GetNavScenario() (instead of nav_scenario_).
The advantage would be that there's no tight coupling and uma_page_load_metrics_observer.cc would not become special and require doing stuff to the embedder and/or initialization code.
What do you think?
Done. Introduced `GetNavigationScenario()` `components/page_load_metrics/browser/navigation_scenario.h`. Please let me know if this is the desired direction.
The way I understand this, this enum value `nav_scenario_` distinguishes different "types" of page loads.
The place where we figure out which type of page load something is is in the PageLoadTracker, and it doesn't change per page load, which is why in this changelist, it's getting passed into the observer when its constructed. And page_load_metrics_initialize.cc and page_load_metrics_embedder_base.cc wire that up.
I can see that this works.
However:
It's problematic that this observer and the page load tracker are going to be very tightly coupled, via these aforementioned mechanisms, and when some other observer needs this information, they'll also be very tightly coupled. Worst case in a way, if the enum gets very popular (this is not guaranteed), things would turn into a hairball.
What do you think about the following:
1) Add some accessor to PageLoadMetricsObserverDelegate to get the nav scenario enum value. GetNavScenario (or whatever is a descriptive name).
2) In page_load_tracker.{h,cc}, override and implement the accessor. If needed, you can add a field to PageLoadTracker to store it there. If the tracker must get it from the page_load_metrics_initialize.cc file, I guess that's OK, perhaps you can look into what's a reasonable way to accomplish that? (e.g., with some precedents?)
3) In this file, get the value with GetDelegate().GetNavScenario() (instead of nav_scenario_).
The advantage would be that there's no tight coupling and uma_page_load_metrics_observer.cc would not become special and require doing stuff to the embedder and/or initialization code.
What do you think?
Done. Introduced `GetNavigationScenario()` and made NavigationScenario into a standalone file.
Related details
- Annie Sullivan
- Ian Clelland
- Johannes Henkel
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ming-Ying Chung (Gerrit)
Ming-Ying Chung voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Annie Sullivan
- Ian Clelland
- Johannes Henkel
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |