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

Issue 1479493002: Reland of "Add StaleWhileRevalidateExperiment histograms." (Closed)

Created:
5 years 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@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Reland of "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 originally landed as https://codereview.chromium.org/1433893002/ but reverted due to accidentally introducing a static initialiser. This CL is identical to the previous one except that it no longer aliases base::StringPiece::npos to work around the static initialiser issue http://crbug.com/561283. TBR=eroman BUG=348877 TEST=manual Committed: https://crrev.com/7afa52378c589e9810345b369669bdcc60261b68 Cr-Commit-Position: refs/heads/master@{#362491}

Patch Set 1 : Original CL. #

Patch Set 2 : With static initialisation fix. #

Total comments: 3

Patch Set 3 : Don't alias base::StringPiece::npos at all. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+416 lines, -12 lines) Patch
A + chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.h View 2 chunks +13 lines, -12 lines 0 comments Download
A chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.cc View 1 chunk +55 lines, -0 lines 0 comments Download
M chrome/browser/page_load_metrics/page_load_metrics_initialize.cc View 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/ui/BUILD.gn View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 chunk +1 line, -0 lines 0 comments Download
M net/BUILD.gn View 2 chunks +29 lines, -0 lines 0 comments Download
A net/base/stale_while_revalidate_experiment_domains.h View 1 chunk +25 lines, -0 lines 0 comments Download
A net/base/stale_while_revalidate_experiment_domains.cc View 1 2 1 chunk +74 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 chunk +1 line, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 chunk +10 lines, -0 lines 0 comments Download

Messages

Total messages: 34 (15 generated)
Adam Rice
Identical to http://codereview.chromium.org/1433893002/ except for a minor change to //net/base/stale_while_revalidate_experiment_domains.cc to avoid introducing a static ...
5 years ago (2015-11-25 09:21:17 UTC) #2
kinuko
On 2015/11/25 09:21:17, Adam Rice wrote: > Identical to http://codereview.chromium.org/1433893002/ except for a minor > ...
5 years ago (2015-11-25 09:28:16 UTC) #3
Adam Rice
On 2015/11/25 09:28:16, kinuko wrote: > Is it possible to re-upload the original patch first ...
5 years ago (2015-11-25 10:14:45 UTC) #5
Peter Kasting
You don't need my re-sign-off since my previous LGTM stands, but: https://codereview.chromium.org/1479493002/diff/40001/net/base/stale_while_revalidate_experiment_domains.cc File net/base/stale_while_revalidate_experiment_domains.cc (right): ...
5 years ago (2015-11-25 11:37:28 UTC) #6
kinuko (google)
(lgtm++, there seem no changes for the files I could review) https://codereview.chromium.org/1479493002/diff/40001/net/base/stale_while_revalidate_experiment_domains.cc File net/base/stale_while_revalidate_experiment_domains.cc (right): ...
5 years ago (2015-11-25 12:37:30 UTC) #8
Adam Rice
https://codereview.chromium.org/1479493002/diff/40001/net/base/stale_while_revalidate_experiment_domains.cc File net/base/stale_while_revalidate_experiment_domains.cc (right): https://codereview.chromium.org/1479493002/diff/40001/net/base/stale_while_revalidate_experiment_domains.cc#newcode41 net/base/stale_while_revalidate_experiment_domains.cc:41: const size_t npos = base::StringPiece::npos; On 2015/11/25 11:37:28, Peter ...
5 years ago (2015-11-25 12:42:24 UTC) #9
kinuko
(lgtm from right address)
5 years ago (2015-11-25 13:52:25 UTC) #10
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1479493002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1479493002/60001
5 years ago (2015-11-25 15:40:11 UTC) #14
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years ago (2015-11-25 17:55:16 UTC) #16
Ilya Sherman
Histograms LGTM. In the future, feel free to TBR owners for files that haven't changed ...
5 years ago (2015-11-25 21:44:15 UTC) #17
Adam Rice
I got lgtm from eroman via hangouts, so submitting TBR to make the CQ happy.
5 years ago (2015-12-01 18:27:29 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1479493002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1479493002/60001
5 years ago (2015-12-01 18:28:35 UTC) #22
eroman
changes since reland LGTM
5 years ago (2015-12-01 18:28:38 UTC) #23
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/148228)
5 years ago (2015-12-01 18:40:41 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1479493002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1479493002/60001
5 years ago (2015-12-01 19:56:50 UTC) #27
commit-bot: I haz the power
Committed patchset #3 (id:60001)
5 years ago (2015-12-01 20:55:36 UTC) #29
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/7afa52378c589e9810345b369669bdcc60261b68 Cr-Commit-Position: refs/heads/master@{#362491}
5 years ago (2015-12-01 20:56:22 UTC) #31
Nico
The build file changes in this file are pretty wrong. Reviewers, please read https://code.google.com/p/chromium/issues/detail?id=569114#c1
5 years ago (2015-12-11 19:45:33 UTC) #33
Adam Rice
5 years ago (2015-12-16 21:32:40 UTC) #34
Message was sent while issue was closed.
On 2015/12/11 19:45:33, Nico wrote:
> The build file changes in this file are pretty wrong. Reviewers, please read
> https://code.google.com/p/chromium/issues/detail?id=569114#c1

The compile warnings are fixed in
https://codereview.chromium.org/1518353002/. I explained some of the
reasoning behind the (admittedly not great) build changes in the bug
that Nico linked above.

Powered by Google App Engine
This is Rietveld 408576698