Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2927283002:
Add PageLoad metrics for multi-tab loading cases (Closed)
|
Created:
3 years, 6 months ago by Kunihiko Sakamoto Modified:
3 years, 6 months ago CC:
chromium-reviews, csharrison+watch_chromium.org, loading-reviews+metrics_chromium.org, speed-metrics-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd PageLoad metrics for multi-tab loading cases
This adds PageLoad histograms targeted for page loads that had other
loading tabs when the navigation started. This would help to understand
the impact of request throttling.
BUG=729951
Review-Url: https://codereview.chromium.org/2927283002
Cr-Commit-Position: refs/heads/master@{#481094}
Committed: https://chromium.googlesource.com/chromium/src/+/da5f02d5361c2b74bec668ee8e6adcfb42b23a21
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add ForegroundTo{FCP,FMP} #
Total comments: 14
Patch Set 3 : comments addressed #Messages
Total messages: 62 (44 generated)
The CQ bit was checked by ksakamoto@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by ksakamoto@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Description was changed from ========== WIP ========== to ========== Add PageLoad metrics for multi-tab loading cases This adds PageLoad histograms targeted for page loads that had other loading tabs when the navigation started. This would help to understand the impact of request throttling. BUG=729951 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:20001) has been deleted
ksakamoto@chromium.org changed reviewers: + bmcquade@chromium.org, toyoshim@chromium.org
The CQ bit was checked by ksakamoto@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
non-owner lgtm with a suggestion for a followup. https://codereview.chromium.org/2927283002/diff/40001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2927283002/diff/40001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.cc:40: Can we also have targetted ForegroundToX variants here? This class will focus on background tab loading practically, and that should be the case we want to be careful about ForegroundToX metrics. It's fine to add them in the next followup CL.
Bryan, would you take a look? https://codereview.chromium.org/2927283002/diff/40001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2927283002/diff/40001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.cc:40: On 2017/06/13 06:45:17, Takashi Toyoshima wrote: > Can we also have targetted ForegroundToX variants here? > This class will focus on background tab loading practically, and that should be > the case we want to be careful about ForegroundToX metrics. > > It's fine to add them in the next followup CL. Good idea, added in this CL.
still lgtm. thanks!
bmcquade@: ping?
kinuko@chromium.org changed reviewers: + csharrison@chromium.org
+csharrison too https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.cc:151: if (web_contents && web_contents->IsLoading() && I think this is basically using the same signal as the one session restore / tab loader uses, which means that more resource loading could happen after that. This could be ok for the first cut but I'm afraid it would not enable us to get the stats like for https://bugs.chromium.org/p/chromium/issues/detail?id=644958#c20
https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.cc:151: if (web_contents && web_contents->IsLoading() && On 2017/06/15 06:10:26, kinuko wrote: > I think this is basically using the same signal as the one session restore / tab > loader uses, which means that more resource loading could happen after that. > This could be ok for the first cut but I'm afraid it would not enable us to get > the stats like for > https://bugs.chromium.org/p/chromium/issues/detail?id=644958#c20 Yeah, this will not catch the case where a tab is loading contents after the load event. I think having some false-negative is OK for our current purpose (A/B testing of the request throttler).
kinuko@chromium.org changed reviewers: + kinuko@chromium.org
A few comments, generally looks good though https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.cc:127: bool MultiTabLoadingPageLoadMetricsObserver::IsAnyTabLoading( Do these methods have any tests associated with them? Do you mind adding some? https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.cc:135: web_contents != navigation_handle->GetWebContents()) { nit: Retrieve the web contents before the double loop, since it involves multiple (virtual) method calls. Same below. https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.cc:151: if (web_contents && web_contents->IsLoading() && Can |web_contents| be null here? I see other consumers with this pattern not checking. https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.h (right): https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.h:34: MultiTabLoadingPageLoadMetricsObserver() {} nit: Move empty constructor to cc file. https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.h:42: nit: Usually we leave no whitespace between the overriden method declarations. https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer_unittest.cc:72: base::WrapUnique(new TestMultiTabLoadingPageLoadMetricsObserver( nit: use base::MakeUnique to avoid bare new.
The CQ bit was checked by ksakamoto@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ksakamoto@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ksakamoto@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ksakamoto@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:120001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:80001) has been deleted
PTAL https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.cc:127: bool MultiTabLoadingPageLoadMetricsObserver::IsAnyTabLoading( On 2017/06/15 14:01:44, on gerrit - ping aggressively wrote: > Do these methods have any tests associated with them? Do you mind adding some? Added browser tests. https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.cc:135: web_contents != navigation_handle->GetWebContents()) { On 2017/06/15 14:01:44, on gerrit - ping aggressively wrote: > nit: Retrieve the web contents before the double loop, since it involves > multiple (virtual) method calls. Same below. Done. https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.cc:151: if (web_contents && web_contents->IsLoading() && On 2017/06/15 14:01:44, on gerrit - ping aggressively wrote: > Can |web_contents| be null here? I see other consumers with this pattern not > checking. Removed. (For Android TabModel, it seems null-check is needed so I'll keep it.) https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.h (right): https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.h:34: MultiTabLoadingPageLoadMetricsObserver() {} On 2017/06/15 14:01:44, on gerrit - ping aggressively wrote: > nit: Move empty constructor to cc file. Done. https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer.h:42: On 2017/06/15 14:01:44, on gerrit - ping aggressively wrote: > nit: Usually we leave no whitespace between the overriden method declarations. Done. https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2927283002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/multi_tab_loading_page_load_metrics_observer_unittest.cc:72: base::WrapUnique(new TestMultiTabLoadingPageLoadMetricsObserver( On 2017/06/15 14:01:44, on gerrit - ping aggressively wrote: > nit: use base::MakeUnique to avoid bare new. Done.
LGTM thanks for adding the browser tests.
LGTM, thanks!
The CQ bit was checked by ksakamoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from toyoshim@chromium.org Link to the patchset: https://codereview.chromium.org/2927283002/#ps140001 (title: "comments addressed")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ksakamoto@chromium.org changed reviewers: + isherman@chromium.org
oops, +isherman for histograms.xml
Looks great, lgtm/2 fwiw
Metrics LGTM
The CQ bit was checked by ksakamoto@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1498007858894780,
"parent_rev": "56e130ab164dbd2949c0355a273d97d0c9d0d4a3", "commit_rev":
"da5f02d5361c2b74bec668ee8e6adcfb42b23a21"}
Message was sent while issue was closed.
Description was changed from ========== Add PageLoad metrics for multi-tab loading cases This adds PageLoad histograms targeted for page loads that had other loading tabs when the navigation started. This would help to understand the impact of request throttling. BUG=729951 ========== to ========== Add PageLoad metrics for multi-tab loading cases This adds PageLoad histograms targeted for page loads that had other loading tabs when the navigation started. This would help to understand the impact of request throttling. BUG=729951 Review-Url: https://codereview.chromium.org/2927283002 Cr-Commit-Position: refs/heads/master@{#481094} Committed: https://chromium.googlesource.com/chromium/src/+/da5f02d5361c2b74bec668ee8e6a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:140001) as https://chromium.googlesource.com/chromium/src/+/da5f02d5361c2b74bec668ee8e6a... |
