Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2245213002:
Add PageLoad.* metrics for Offline Previews (Closed)
|
Created:
4 years, 4 months ago by RyanSturm Modified:
4 years, 4 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, csharrison+watch_chromium.org, loading-reviews+metrics_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd PageLoad.* metrics for Offline Previews
This adds UMA for Offline Page Previews to determine how fast various
load events occur when a user is shown an offline page as part of
previews.
BUG=631197
Committed: https://crrev.com/956546229b20322ce2941f059eae0e3d6fc9949b
Cr-Commit-Position: refs/heads/master@{#413821}
Patch Set 1 #Patch Set 2 : Self review #
Total comments: 20
Patch Set 3 : tbansal comments #Patch Set 4 : rebase #Patch Set 5 : typo #
Total comments: 6
Patch Set 6 : csharrison comments #
Total comments: 8
Patch Set 7 : csharrison comments #Messages
Total messages: 52 (31 generated)
The CQ bit was checked by ryansturm@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by ryansturm@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.
ryansturm@chromium.org changed reviewers: + tbansal@chromium.org
tbansal: PTAL at * before I send the CL out more broadly.
lgtm % nits. https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:123: // Whether the page is an offline preview. may be change to is_offline_preview_ (similar to variable name used at other places in this CL). https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:123: // Whether the page is an offline preview. It might be useful to define what is an "offline preview" for future editors of this file. The line that you added to histograms.xml looks good (or something similar): Offline page previews are shown when a user's effective connection type is prohibitively slow. https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.cc:57: if (is_offline_preview_ && WasStartedInForegroundOptionalEventInForeground( Would it be more readable if the boolean condition was reversed? if(!a || !b) return; // do something. https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.cc:69: if (is_offline_preview_ && WasStartedInForegroundOptionalEventInForeground( This condition is same as everywhere else. Can it be factored out to something like: bool ShouldRecordTimingsMetrics() const {} https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.h (right): https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.h:38: // Observer responsible for recording core page load metrics releveant to typo in "releveant" https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer_unittest.cc:20: const char kDefaultTestUrl[] = "https://google.com"; s/kDefaultTestUrl/kDefaultTestUrl1/ https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer_unittest.cc:30: // Whether a |web_contents| is an offline pages preview. Overridden in No need for comments here. Comments from the base class should suffice. https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer_unittest.cc:124: page_load_metrics::PageLoadTiming timing_; Why does this need to be protected (and not private)? https://codereview.chromium.org/2245213002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2245213002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:101076: + when a users effective connection type is prohibitvely slow."/> typo in "prohibitvely". https://codereview.chromium.org/2245213002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:101076: + when a users effective connection type is prohibitvely slow."/> s/users/user's/
The CQ bit was checked by ryansturm@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by ryansturm@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...
ryansturm@chromium.org changed reviewers: + csharrison@chromium.org, dimich@chromium.org, holte@chromium.org
csharrison: PTAL @ chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.* dimich: PTAL @ offline_page_tab_helper.* holte: PTAL @ histograms.xml https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:123: // Whether the page is an offline preview. On 2016/08/18 22:04:53, tbansal1 wrote: > may be change to is_offline_preview_ (similar to variable name used at other > places in this CL). Done. https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:123: // Whether the page is an offline preview. On 2016/08/18 22:04:52, tbansal1 wrote: > It might be useful to define what is an "offline preview" for future editors of > this file. The line that you added to histograms.xml looks good (or something > similar): Offline page previews are shown when a user's effective connection > type is prohibitively slow. Done. https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.cc:57: if (is_offline_preview_ && WasStartedInForegroundOptionalEventInForeground( On 2016/08/18 22:04:53, tbansal1 wrote: > Would it be more readable if the boolean condition was reversed? > if(!a || !b) return; > > // do something. Done. https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.cc:69: if (is_offline_preview_ && WasStartedInForegroundOptionalEventInForeground( On 2016/08/18 22:04:53, tbansal1 wrote: > This condition is same as everywhere else. Can it be factored out to something > like: > bool ShouldRecordTimingsMetrics() const {} It doesn't really shorten much. It would need to take three arguments (or 2 if it was class method instead of non-class). So it's shortening a (bool && method(a,b)) to (method(a,b,bool)). https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.h (right): https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.h:38: // Observer responsible for recording core page load metrics releveant to On 2016/08/18 22:04:53, tbansal1 wrote: > typo in "releveant" Done. https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer_unittest.cc:20: const char kDefaultTestUrl[] = "https://google.com"; On 2016/08/18 22:04:53, tbansal1 wrote: > s/kDefaultTestUrl/kDefaultTestUrl1/ Done. https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer_unittest.cc:30: // Whether a |web_contents| is an offline pages preview. Overridden in On 2016/08/18 22:04:53, tbansal1 wrote: > No need for comments here. Comments from the base class should suffice. Done. https://codereview.chromium.org/2245213002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer_unittest.cc:124: page_load_metrics::PageLoadTiming timing_; On 2016/08/18 22:04:53, tbansal1 wrote: > Why does this need to be protected (and not private)? Done. https://codereview.chromium.org/2245213002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2245213002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:101076: + when a users effective connection type is prohibitvely slow."/> On 2016/08/18 22:04:53, tbansal1 wrote: > s/users/user's/ Done. https://codereview.chromium.org/2245213002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:101076: + when a users effective connection type is prohibitvely slow."/> On 2016/08/18 22:04:53, tbansal1 wrote: > typo in "prohibitvely". Done.
histograms lgtm
histograms lgtm
Looks good, just a few questions. https://codereview.chromium.org/2245213002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2245213002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.cc:25: const char kHistogramOfflinePreviewsPrefix[] = Performing string appends at runtime for these metrics seems like the wrong approach to me. Could you just prepend "Preview" to the metric names? https://codereview.chromium.org/2245213002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.h (right): https://codereview.chromium.org/2245213002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.h:27: extern const char kHistogramDOMContentLoadedEventFiredSuffix[]; Are these metrics all needed? Do these need to directly compare with DRP metrics? If you could, it would be great to trim this a bit (i.e. first contentful paint, first image paint and first text paint are all very similar). https://codereview.chromium.org/2245213002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2245213002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer_unittest.cc:74: EXPECT_TRUE(!expected); Can you EXPECT_FALSE?
The CQ bit was checked by ryansturm@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...
csharrison: PTAL, thanks for the review. https://codereview.chromium.org/2245213002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2245213002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.cc:25: const char kHistogramOfflinePreviewsPrefix[] = On 2016/08/19 20:00:22, Charlie Harrison wrote: > Performing string appends at runtime for these metrics seems like the wrong > approach to me. Could you just prepend "Preview" to the metric names? Done. In the future, we'll add other previews, so we might move back to the prefix/suffix, but that makes little sense in the context of this CL. https://codereview.chromium.org/2245213002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.h (right): https://codereview.chromium.org/2245213002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.h:27: extern const char kHistogramDOMContentLoadedEventFiredSuffix[]; On 2016/08/19 20:00:22, Charlie Harrison wrote: > Are these metrics all needed? Do these need to directly compare with DRP > metrics? If you could, it would be great to trim this a bit (i.e. first > contentful paint, first image paint and first text paint are all very similar). Done. https://codereview.chromium.org/2245213002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2245213002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer_unittest.cc:74: EXPECT_TRUE(!expected); On 2016/08/19 20:00:23, Charlie Harrison wrote: > Can you EXPECT_FALSE? Done.
Thanks! Couple more things. https://codereview.chromium.org/2245213002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.h (right): https://codereview.chromium.org/2245213002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.h:26: extern const char kHistogramOfflinePreviewsPrefix[]; No longer needed. https://codereview.chromium.org/2245213002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.h:27: extern const char kHistogramDOMContentLoadedEventFired[]; These should have Preview somewhere in the name. https://codereview.chromium.org/2245213002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2245213002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer_unittest.cc:69: void ExpectEqualOrUnset(const base::Optional<base::TimeDelta>& expected, Looks like this method is unused. Let's get rid of it. https://codereview.chromium.org/2245213002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer_unittest.cc:128: } // namespace previews nit: "} // namespace previews"
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
csharrison: PTAL https://codereview.chromium.org/2245213002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.h (right): https://codereview.chromium.org/2245213002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.h:26: extern const char kHistogramOfflinePreviewsPrefix[]; On 2016/08/19 20:34:14, Charlie Harrison wrote: > No longer needed. Done. https://codereview.chromium.org/2245213002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.h:27: extern const char kHistogramDOMContentLoadedEventFired[]; On 2016/08/19 20:34:14, Charlie Harrison wrote: > These should have Preview somewhere in the name. Done. https://codereview.chromium.org/2245213002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2245213002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer_unittest.cc:69: void ExpectEqualOrUnset(const base::Optional<base::TimeDelta>& expected, On 2016/08/19 20:34:14, Charlie Harrison wrote: > Looks like this method is unused. Let's get rid of it. Done. https://codereview.chromium.org/2245213002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer_unittest.cc:128: } // namespace previews On 2016/08/19 20:34:14, Charlie Harrison wrote: > nit: "} // namespace previews" Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the fast turnaround. LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm for offline_page_tab_helper. It'd be nice (not neccessarily now) to generalize this to capture all offline loads, not only the ones that were caused by NQE - I suspect all of them will have similar load timing characteristics, no matter how triggered.
On 2016/08/20 00:14:44, Dmitry Titov wrote: > lgtm for offline_page_tab_helper. > > It'd be nice (not neccessarily now) to generalize this to capture all offline > loads, not only the ones that were caused by NQE - I suspect all of them will > have similar load timing characteristics, no matter how triggered. I'll open a bug for it. I suspect that would be interesting data to have for comparisons against previews. Thanks for the review!
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2245213002/#ps120001 (title: "csharrison comments")
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by ryansturm@chromium.org
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by ryansturm@chromium.org
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by ryansturm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add PageLoad.* metrics for Offline Previews This adds UMA for Offline Page Previews to determine how fast various load events occur when a user is shown an offline page as part of previews. BUG=631197 ========== to ========== Add PageLoad.* metrics for Offline Previews This adds UMA for Offline Page Previews to determine how fast various load events occur when a user is shown an offline page as part of previews. BUG=631197 Committed: https://crrev.com/956546229b20322ce2941f059eae0e3d6fc9949b Cr-Commit-Position: refs/heads/master@{#413821} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/956546229b20322ce2941f059eae0e3d6fc9949b Cr-Commit-Position: refs/heads/master@{#413821} |
