Track non-bfcached traverse navigation's AFT [chromium/src : main]
Lingqi Chi (Gerrit)
Lingqi Chi added 1 comment![]( "Open in Gerrit")
Related details
- Hiroki Nakagawa
- Keita Suzuki
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keita Suzuki (Gerrit)
Keita Suzuki voted and added 1 comment![]( "Open in Gerrit")
Votes added by Keita Suzuki
| Code-Review | +1 |
1 comment
- Hiroki Nakagawa
- Lingqi Chi
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 2 comments![]( "Open in Gerrit")
Sorry for the late review.
is_traverse_navigation_ = navigation_handle->IsHistory();According to the header comment, `IsHistory()` returns true even for session restore navigation. I wonder if we may want to distinguish them as well, because they may be slower than regular history navigations (for example, process allocation for tab restore, session restore on browser restart).
Related details
- Lingqi Chi
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. |
Minoru Chikamune (Gerrit)
Minoru Chikamune voted and added 1 comment![]( "Open in Gerrit")
Votes added by Minoru Chikamune
| Code-Review | +1 |
1 comment
is_traverse_navigation_ = navigation_handle->IsHistory();According to the header comment, `IsHistory()` returns true even for session restore navigation. I wonder if we may want to distinguish them as well, because they may be slower than regular history navigations (for example, process allocation for tab restore, session restore on browser restart).
Related details
- Lingqi Chi
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Hiroki Nakagawa
| Code-Review | +1 |
1 comment
is_traverse_navigation_ = navigation_handle->IsHistory();Minoru ChikamuneAccording to the header comment, `IsHistory()` returns true even for session restore navigation. I wonder if we may want to distinguish them as well, because they may be slower than regular history navigations (for example, process allocation for tab restore, session restore on browser restart).
+1
navigation_handle->GetRestoreType() == kNotRestored
See also: https://crrev.com/c/2551302
On second thought, the "original" metric is sliced by the "traverse" in https://developer.mozilla.org/en-US/docs/Web/API/NavigationActivation/navigationType IIUC and it contains the restore cases. If we want to compare them, the current version is correct.
It's fine with me to land this as is. If needed, we can make another slice to distinguish restore from others later.
Lingqi Chi (Gerrit)
Lingqi Chi added 2 comments![]( "Open in Gerrit")
Thank you!!
I also changed the code to use kHistogramGWSAFTEndWithPreNavigationLatency per Shunya-san's suggestion.
is_traverse_navigation_ = navigation_handle->IsHistory();Minoru ChikamuneAccording to the header comment, `IsHistory()` returns true even for session restore navigation. I wonder if we may want to distinguish them as well, because they may be slower than regular history navigations (for example, process allocation for tab restore, session restore on browser restart).
Hiroki Nakagawa+1
navigation_handle->GetRestoreType() == kNotRestored
See also: https://crrev.com/c/2551302
On second thought, the "original" metric is sliced by the "traverse" in https://developer.mozilla.org/en-US/docs/Web/API/NavigationActivation/navigationType IIUC and it contains the restore cases. If we want to compare them, the current version is correct.
It's fine with me to land this as is. If needed, we can make another slice to distinguish restore from others later.
Thank you! I think it makes a lot of sense to me. If restore navigation is slow and non-restore are not, maybe we need to optimize for the restore navigation instead of looking at the bf navigation case.
Added a breakdown metric for it.
Related details
- Hiroki Nakagawa
- Minoru Chikamune
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. |
Minoru Chikamune (Gerrit)
Minoru Chikamune voted and added 3 comments![]( "Open in Gerrit")
Votes added by Minoru Chikamune
| Code-Review | +1 |
3 comments
LGTM % nits
but only track traverse navigations.How about:
but only tracks {TraverseRestoreType}.
but only track traverse navigations.How about:
but only tracks {TraverseRestoreType}.
Related details
- Hiroki Nakagawa
- Lingqi Chi
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. |
Shunya Shishido (Gerrit)
Shunya Shishido added 1 comment![]( "Open in Gerrit")
Is it possible to write a test, maybe in chrome/browser/page_load_metrics/observers/gws_page_load_metrics_observer_browsertest.cc, or chrome/browser/page_load_metrics/observers/gws_page_load_metrics_observer_unittest.cc?
Related details
- Hiroki Nakagawa
- Lingqi Chi
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. |
Minoru Chikamune (Gerrit)
Minoru Chikamune voted and added 2 comments![]( "Open in Gerrit")
Votes added by Minoru Chikamune
| Code-Review | +1 |
2 comments
LGTM % histogram description (summary)
TEST_REQUIRES_NO_CACHING);Q: What does happen when the page is restored from bfcache? In my understanding, the PLMO doesn't check OnRestoreFromBackForwardCache nor OnEnterBackForwardCache. Does this mean that the bfcache cases are also recorded?
Related details
- Hiroki Nakagawa
- Keita Suzuki
- Lingqi Chi
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. |
Minoru Chikamune (Gerrit)
Minoru Chikamune added 1 comment![]( "Open in Gerrit")
TEST_REQUIRES_NO_CACHING);Q: What does happen when the page is restored from bfcache? In my understanding, the PLMO doesn't check OnRestoreFromBackForwardCache nor OnEnterBackForwardCache. Does this mean that the bfcache cases are also recorded?
Never mind. I checked it by myself. When bfcache was enabled, the back navigation did not trigger GWSPageLoadMetricsObserver::OnStart nor GWSPageLoadMetricsObserver::LogMetricsOnComplete.
[Trace log]
https://ui.perfetto.dev/#!/?s=8977656e1956f609fe29a6572008877a4e7e7a3f
Shunya Shishido (Gerrit)
Shunya Shishido added 1 comment![]( "Open in Gerrit")
Looks good but could you start the dry-run again?
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa voted and added 3 comments![]( "Open in Gerrit")
Votes added by Hiroki Nakagawa
| Code-Review | +1 |
3 comments
Thanks! LGTM
is_traverse_navigation_ = navigation_handle->IsHistory();Minoru ChikamuneAccording to the header comment, `IsHistory()` returns true even for session restore navigation. I wonder if we may want to distinguish them as well, because they may be slower than regular history navigations (for example, process allocation for tab restore, session restore on browser restart).
Hiroki Nakagawa+1
navigation_handle->GetRestoreType() == kNotRestored
See also: https://crrev.com/c/2551302
Lingqi ChiOn second thought, the "original" metric is sliced by the "traverse" in https://developer.mozilla.org/en-US/docs/Web/API/NavigationActivation/navigationType IIUC and it contains the restore cases. If we want to compare them, the current version is correct.
It's fine with me to land this as is. If needed, we can make another slice to distinguish restore from others later.
Thank you! I think it makes a lot of sense to me. If restore navigation is slow and non-restore are not, maybe we need to optimize for the restore navigation instead of looking at the bf navigation case.
Added a breakdown metric for it.
Acknowledged
const auto aft_end_with_pre_navi_latency =Can we make the name consistent, that is, `pre_navi` to `prenavigation`?
Related details
- Keita Suzuki
- Lingqi Chi
Lingqi Chi (Gerrit)
Lingqi Chi added 4 comments![]( "Open in Gerrit")
Is it possible to write a test, maybe in chrome/browser/page_load_metrics/observers/gws_page_load_metrics_observer_browsertest.cc, or chrome/browser/page_load_metrics/observers/gws_page_load_metrics_observer_unittest.cc?
I added browser tests for it! PTAL 😊
TEST_REQUIRES_NO_CACHING);Q: What does happen when the page is restored from bfcache? In my understanding, the PLMO doesn't check OnRestoreFromBackForwardCache nor OnEnterBackForwardCache. Does this mean that the bfcache cases are also recorded?
Never mind. I checked it by myself. When bfcache was enabled, the back navigation did not trigger GWSPageLoadMetricsObserver::OnStart nor GWSPageLoadMetricsObserver::LogMetricsOnComplete.
[Trace log]
https://ui.perfetto.dev/#!/?s=8977656e1956f609fe29a6572008877a4e7e7a3f
Right. I explained the reason that why I do not track BFCache navigation for now.
How about:
but only tracks {TraverseRestoreType}.
Done
How about:
but only tracks {TraverseRestoreType}.
- Keita Suzuki
- Minoru Chikamune
- Shunya Shishido
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. |
Lingqi Chi (Gerrit)
Lingqi Chi added 3 comments![]( "Open in Gerrit")
thank you! (had some difficulty when using new workspace 😂
TEST_REQUIRES_NO_CACHING);Minoru ChikamuneQ: What does happen when the page is restored from bfcache? In my understanding, the PLMO doesn't check OnRestoreFromBackForwardCache nor OnEnterBackForwardCache. Does this mean that the bfcache cases are also recorded?
Lingqi ChiNever mind. I checked it by myself. When bfcache was enabled, the back navigation did not trigger GWSPageLoadMetricsObserver::OnStart nor GWSPageLoadMetricsObserver::LogMetricsOnComplete.
[Trace log]
https://ui.perfetto.dev/#!/?s=8977656e1956f609fe29a6572008877a4e7e7a3f
Right. I explained the reason that why I do not track BFCache navigation for now.
Done
Can we make the name consistent, that is, `pre_navi` to `prenavigation`?
Done
Lingqi Chi (Gerrit)
Lingqi Chi added 1 comment![]( "Open in Gerrit")
- Ian Clelland
- Keita Suzuki
- Minoru Chikamune
- Shunya Shishido
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. |
Shunya Shishido (Gerrit)
Shunya Shishido voted and added 1 comment![]( "Open in Gerrit")
Votes added by Shunya Shishido
| Code-Review | +1 |
Lingqi ChiIs it possible to write a test, maybe in chrome/browser/page_load_metrics/observers/gws_page_load_metrics_observer_browsertest.cc, or chrome/browser/page_load_metrics/observers/gws_page_load_metrics_observer_unittest.cc?
I added browser tests for it! PTAL 😊
- Ian Clelland
- Keita Suzuki
- Lingqi Chi
- Minoru Chikamune
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |