Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(146)

Issue 1433893002: Add StaleWhileRevalidateExperiment histograms. (Closed)

Created:
5 years, 1 month ago by Adam Rice
Modified:
5 years ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, asvitkine+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@s-w-r-dafsa
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add StaleWhileRevalidateExperiment histograms. Add histograms PageLoad.Clients.StaleWhileRevalidateExperiment.Timing2.NavigationToLoadEventFired, PageLoad.Clients.StaleWhileRevalidateExperiment.Timing2.NavigationToFirstLayout, and PageLoad.Clients.StaleWhileRevalidateExperiment.Timing2.NavigationToFirstTextPaint. These behave the same as the base histograms, but they are only recorded for a subset of domains which have render-blocking resources served with the Cache-Control stale-while-revalidate directive. These histograms permit us to measure the benefit in load time from the implementation of the directive wihout noise from domains which don't use it. This CL was split from the original CL https://codereview.chromium.org/1303973009/ with changes to the filenames and target names in //net. BUG=348877 TEST=manual Committed: https://crrev.com/51d6bcfd10cb847da284a7aa1202f92ea37b74ca Cr-Commit-Position: refs/heads/master@{#361549}

Patch Set 1 #

Total comments: 6

Patch Set 2 : Changes from kinuko review. #

Total comments: 2

Patch Set 3 : Change suffix name in histograms.xml #

Patch Set 4 : Rebase. #

Patch Set 5 : Fix compile error. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+418 lines, -12 lines) Patch
A + chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.h View 1 2 chunks +13 lines, -12 lines 0 comments Download
A chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.cc View 1 2 3 4 1 chunk +55 lines, -0 lines 0 comments Download
M chrome/browser/page_load_metrics/page_load_metrics_initialize.cc View 1 2 3 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/ui/BUILD.gn View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M net/BUILD.gn View 1 2 3 2 chunks +29 lines, -0 lines 0 comments Download
A net/base/stale_while_revalidate_experiment_domains.h View 1 1 chunk +25 lines, -0 lines 0 comments Download
A net/base/stale_while_revalidate_experiment_domains.cc View 1 chunk +76 lines, -0 lines 0 comments Download
A net/base/stale_while_revalidate_experiment_domains.gperf View 1 chunk +117 lines, -0 lines 0 comments Download
A net/base/stale_while_revalidate_experiment_domains_unittest.cc View 1 chunk +63 lines, -0 lines 0 comments Download
M net/net.gyp View 3 chunks +19 lines, -0 lines 0 comments Download
M net/net.gypi View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 1 chunk +10 lines, -0 lines 0 comments Download

Messages

Total messages: 27 (10 generated)
Adam Rice
Apart from the filenames and build target names in //net, this is the same as ...
5 years, 1 month ago (2015-11-11 05:33:26 UTC) #3
Adam Rice
+eroman for //net changes.
5 years, 1 month ago (2015-11-13 00:05:13 UTC) #5
Charlie Harrison
Adding kinuko@ for ownership lgtm for page_load_metrics stuff, assuming no changes to previous CL.
5 years, 1 month ago (2015-11-13 01:28:25 UTC) #7
kinuko
lgtm for the part I could review https://codereview.chromium.org/1433893002/diff/1/chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.h File chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.h (right): https://codereview.chromium.org/1433893002/diff/1/chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.h#newcode13 chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.h:13: } Doesn't ...
5 years, 1 month ago (2015-11-17 08:18:48 UTC) #8
Adam Rice
https://codereview.chromium.org/1433893002/diff/1/chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.h File chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.h (right): https://codereview.chromium.org/1433893002/diff/1/chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.h#newcode13 chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.h:13: } On 2015/11/17 08:18:47, kinuko wrote: > Doesn't look ...
5 years, 1 month ago (2015-11-17 18:29:21 UTC) #9
Adam Rice
ping eroman. Can you handle this, or do you want to reassign?
5 years, 1 month ago (2015-11-18 06:34:26 UTC) #10
eroman
lgtm
5 years, 1 month ago (2015-11-19 06:13:18 UTC) #11
Adam Rice
+pkasting for chrome/browser/ui/BUILD.gn
5 years, 1 month ago (2015-11-19 14:37:52 UTC) #13
Peter Kasting
RS LGTM
5 years, 1 month ago (2015-11-19 20:06:43 UTC) #14
Adam Rice
+isherman for histograms.xml.
5 years, 1 month ago (2015-11-20 10:24:00 UTC) #16
Ilya Sherman
histograms.xml lgtm % comment: https://codereview.chromium.org/1433893002/diff/20001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1433893002/diff/20001/tools/metrics/histograms/histograms.xml#newcode77291 tools/metrics/histograms/histograms.xml:77291: + <suffix name="StaleWhileRevalidateExperiment" I think ...
5 years, 1 month ago (2015-11-21 01:22:18 UTC) #17
Adam Rice
https://codereview.chromium.org/1433893002/diff/20001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1433893002/diff/20001/tools/metrics/histograms/histograms.xml#newcode77291 tools/metrics/histograms/histograms.xml:77291: + <suffix name="StaleWhileRevalidateExperiment" On 2015/11/21 01:22:18, Ilya Sherman wrote: ...
5 years, 1 month ago (2015-11-22 14:07:04 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1433893002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1433893002/80001
5 years ago (2015-11-24 11:05:23 UTC) #21
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_rel_ng/builds/100547)
5 years ago (2015-11-24 13:40:09 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1433893002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1433893002/80001
5 years ago (2015-11-25 02:29:03 UTC) #25
commit-bot: I haz the power
Committed patchset #5 (id:80001)
5 years ago (2015-11-25 03:23:51 UTC) #26
commit-bot: I haz the power
5 years ago (2015-11-25 03:25:25 UTC) #27
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/51d6bcfd10cb847da284a7aa1202f92ea37b74ca
Cr-Commit-Position: refs/heads/master@{#361549}

Powered by Google App Engine
This is Rietveld 408576698