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

Issue 1303973009: [DO NOT COMMIT] Re-use the dafsa code for s-w-r histograms (Closed)

Created:
5 years, 3 months ago by Adam Rice
Modified:
5 years ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, Bryan McQuade, davidben
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[DO NOT COMMIT] Re-use the dafsa code for s-w-r histograms This CL has been split into https://codereview.chromium.org/1412343008/ and https://codereview.chromium.org/1433893002/. This version does not contain the latest changes. The dafsa code used by registry_controller_domains can be used as a generic string set. This is useful for matching domains for histograms for the stale-while-revalidate experiment. The LookupString() function which was file-local in //net/base/registry_controlled_domains has been moved to //net/base/dafsa so that it can used from other files. //net/base/swr_histogram_domains adds roughly 800 bytes to the binary size and so will be removed promptly once the experiment is over. BUG=348877 TEST=net_unittests

Patch Set 1 #

Patch Set 2 : Improved domain list. #

Patch Set 3 : Remove uk.com and br.com manually. #

Patch Set 4 : Add StaleWhileRevalidateExperiment histograms. #

Patch Set 5 : Code simplification, cleanup and rebase. #

Patch Set 6 : Make the s-w-r histograms log the right metrics. #

Patch Set 7 : Add histograms to histograms.xml #

Total comments: 6

Patch Set 8 : Apply changes by tyoshino. #

Total comments: 11

Patch Set 9 : Modify to use new PageLoadMetricsObserver interface + Rebase + fixes from tyoshino review. #

Patch Set 10 : Correct the build files. #

Total comments: 15

Patch Set 11 : Rename build rules. Put StaleWhileRevalidateMetricsObserver in chrome namespace. #

Patch Set 12 : Rebase to fix patch errors on try bots. #

Total comments: 14
Unified diffs Side-by-side diffs Delta from patch set Stats (+629 lines, -1415 lines) Patch
A + chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +17 lines, -12 lines 0 comments Download
A chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +56 lines, -0 lines 0 comments Download
M chrome/browser/page_load_metrics/page_load_metrics_initialize.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/ui/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M net/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +14 lines, -0 lines 2 comments Download
A net/base/dafsa/lookup_string.h View 1 2 3 4 5 6 7 8 1 chunk +29 lines, -0 lines 4 comments Download
A net/base/dafsa/lookup_string.cc View 1 2 3 4 5 6 7 8 1 chunk +152 lines, -0 lines 2 comments Download
M net/base/registry_controlled_domains/BUILD.gn View 1 chunk +1 line, -1 line 0 comments Download
M net/base/registry_controlled_domains/registry_controlled_domain.cc View 1 2 3 4 5 6 7 8 3 chunks +8 lines, -141 lines 0 comments Download
A net/base/swr_histogram_domains/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +18 lines, -0 lines 0 comments Download
A net/base/swr_histogram_domains/swr_histogram_domains.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +27 lines, -0 lines 0 comments Download
A net/base/swr_histogram_domains/swr_histogram_domains.cc View 1 2 3 4 5 6 7 1 chunk +79 lines, -0 lines 0 comments Download
A net/base/swr_histogram_domains/swr_histogram_domains.gperf View 1 2 3 4 5 6 7 8 9 10 1 chunk +117 lines, -0 lines 2 comments Download
A net/base/swr_histogram_domains/swr_histogram_domains_unittest.cc View 1 1 chunk +66 lines, -0 lines 0 comments Download
M net/net.gyp View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +21 lines, -2 lines 0 comments Download
M net/net.gypi View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +3 lines, -0 lines 4 comments Download
A + net/tools/dafsa/PRESUBMIT.py View 1 chunk +3 lines, -3 lines 0 comments Download
A + net/tools/dafsa/make_dafsa.py View 0 chunks +-1 lines, --1 lines 0 comments Download
A + net/tools/dafsa/make_dafsa_unittest.py View 0 chunks +-1 lines, --1 lines 0 comments Download
D net/tools/tld_cleanup/PRESUBMIT.py View 1 chunk +0 lines, -32 lines 0 comments Download
D net/tools/tld_cleanup/make_dafsa.py View 1 chunk +0 lines, -469 lines 0 comments Download
D net/tools/tld_cleanup/make_dafsa_unittest.py View 1 chunk +0 lines, -757 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +10 lines, -0 lines 0 comments Download

Messages

Total messages: 29 (7 generated)
Adam Rice
tyoshino, if you have time could you review this for readability and general correctness.
5 years, 2 months ago (2015-10-15 13:58:01 UTC) #2
tyoshino (SeeGerritForStatus)
https://codereview.chromium.org/1303973009/diff/120001/components/page_load_metrics/browser/metrics_web_contents_observer.cc File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1303973009/diff/120001/components/page_load_metrics/browser/metrics_web_contents_observer.cc#newcode173 components/page_load_metrics/browser/metrics_web_contents_observer.cc:173: inserted accidentally? https://codereview.chromium.org/1303973009/diff/120001/net/base/dafsa/lookup_string.cc File net/base/dafsa/lookup_string.cc (right): https://codereview.chromium.org/1303973009/diff/120001/net/base/dafsa/lookup_string.cc#newcode89 net/base/dafsa/lookup_string.cc:89: int ...
5 years, 2 months ago (2015-10-19 07:15:10 UTC) #3
Adam Rice
https://codereview.chromium.org/1303973009/diff/120001/components/page_load_metrics/browser/metrics_web_contents_observer.cc File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1303973009/diff/120001/components/page_load_metrics/browser/metrics_web_contents_observer.cc#newcode173 components/page_load_metrics/browser/metrics_web_contents_observer.cc:173: On 2015/10/19 07:15:10, tyoshino wrote: > inserted accidentally? Yes. ...
5 years, 2 months ago (2015-10-19 12:03:07 UTC) #4
Bryan McQuade
https://codereview.chromium.org/1303973009/diff/140001/components/page_load_metrics/browser/metrics_web_contents_observer.cc File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1303973009/diff/140001/components/page_load_metrics/browser/metrics_web_contents_observer.cc#newcode97 components/page_load_metrics/browser/metrics_web_contents_observer.cc:97: include_in_stale_while_revalidate_experiment_ = true; just FYI (happened to find this ...
5 years, 2 months ago (2015-10-19 12:58:16 UTC) #6
Adam Rice
https://codereview.chromium.org/1303973009/diff/140001/components/page_load_metrics/browser/metrics_web_contents_observer.cc File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1303973009/diff/140001/components/page_load_metrics/browser/metrics_web_contents_observer.cc#newcode97 components/page_load_metrics/browser/metrics_web_contents_observer.cc:97: include_in_stale_while_revalidate_experiment_ = true; On 2015/10/19 12:58:16, Bryan McQuade wrote: ...
5 years, 2 months ago (2015-10-19 13:34:44 UTC) #7
tyoshino (SeeGerritForStatus)
https://codereview.chromium.org/1303973009/diff/140001/net/base/dafsa/lookup_string.h File net/base/dafsa/lookup_string.h (right): https://codereview.chromium.org/1303973009/diff/140001/net/base/dafsa/lookup_string.h#newcode15 net/base/dafsa/lookup_string.h:15: const int kPrivateRule = 4; Now these constants are ...
5 years, 1 month ago (2015-10-30 06:29:59 UTC) #8
tyoshino (SeeGerritForStatus)
https://codereview.chromium.org/1303973009/diff/140001/components/page_load_metrics/browser/metrics_web_contents_observer.cc File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1303973009/diff/140001/components/page_load_metrics/browser/metrics_web_contents_observer.cc#newcode146 components/page_load_metrics/browser/metrics_web_contents_observer.cc:146: PAGE_LOAD_HISTOGRAM( no need to align with the grouping above? ...
5 years, 1 month ago (2015-10-30 07:48:02 UTC) #9
Adam Rice
https://codereview.chromium.org/1303973009/diff/140001/components/page_load_metrics/browser/metrics_web_contents_observer.cc File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1303973009/diff/140001/components/page_load_metrics/browser/metrics_web_contents_observer.cc#newcode146 components/page_load_metrics/browser/metrics_web_contents_observer.cc:146: PAGE_LOAD_HISTOGRAM( On 2015/10/30 07:48:02, tyoshino wrote: > no need ...
5 years, 1 month ago (2015-11-02 13:53:54 UTC) #10
Adam Rice
+csharrison please review the use of the PageLoadMetricsObserver under //chrome/browser/page_load_metrics/observers/
5 years, 1 month ago (2015-11-02 14:06:27 UTC) #12
Bryan McQuade
https://codereview.chromium.org/1303973009/diff/140001/components/page_load_metrics/browser/metrics_web_contents_observer.cc File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1303973009/diff/140001/components/page_load_metrics/browser/metrics_web_contents_observer.cc#newcode146 components/page_load_metrics/browser/metrics_web_contents_observer.cc:146: PAGE_LOAD_HISTOGRAM( On 2015/11/02 13:53:54, Adam Rice wrote: > On ...
5 years, 1 month ago (2015-11-02 14:22:39 UTC) #13
tyoshino (SeeGerritForStatus)
histograms.xml What's the reason the suffix entry is listed between the affected-histogram entries?
5 years, 1 month ago (2015-11-04 07:11:28 UTC) #14
tyoshino (SeeGerritForStatus)
https://codereview.chromium.org/1303973009/diff/180001/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/1303973009/diff/180001/chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.h#newcode10 chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.h:10: #include "content/public/browser/navigation_handle.h" can be moved to .cc file (put ...
5 years, 1 month ago (2015-11-04 07:25:12 UTC) #15
tyoshino (SeeGerritForStatus)
https://codereview.chromium.org/1303973009/diff/180001/chrome/browser/page_load_metrics/page_load_metrics_initialize.cc File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right): https://codereview.chromium.org/1303973009/diff/180001/chrome/browser/page_load_metrics/page_load_metrics_initialize.cc#newcode22 chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:22: // OnPageLoadMetricsGoingAway. merge with the comments at L18-19? https://codereview.chromium.org/1303973009/diff/180001/net/BUILD.gn ...
5 years, 1 month ago (2015-11-04 08:34:07 UTC) #16
tyoshino (SeeGerritForStatus)
l-g-t-m once the comments above are addressed!
5 years, 1 month ago (2015-11-04 08:34:31 UTC) #17
Adam Rice
On 2015/11/04 07:11:28, tyoshino wrote: > histograms.xml > > What's the reason the suffix entry ...
5 years, 1 month ago (2015-11-04 09:23:45 UTC) #18
Adam Rice
https://codereview.chromium.org/1303973009/diff/180001/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/1303973009/diff/180001/chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.h#newcode10 chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.h:10: #include "content/public/browser/navigation_handle.h" On 2015/11/04 07:25:12, tyoshino wrote: > can ...
5 years, 1 month ago (2015-11-05 05:30:40 UTC) #19
tyoshino (SeeGerritForStatus)
lgtm https://codereview.chromium.org/1303973009/diff/180001/chrome/browser/page_load_metrics/page_load_metrics_initialize.cc File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right): https://codereview.chromium.org/1303973009/diff/180001/chrome/browser/page_load_metrics/page_load_metrics_initialize.cc#newcode22 chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:22: // OnPageLoadMetricsGoingAway. On 2015/11/05 05:30:39, Adam Rice wrote: ...
5 years, 1 month ago (2015-11-05 09:00:02 UTC) #20
Adam Rice
+eroman, could you review for //net OWNERS? Please reassign as appropriate. I'd like to get ...
5 years, 1 month ago (2015-11-05 09:24:10 UTC) #22
Charlie Harrison
On 2015/11/05 09:24:10, Adam Rice wrote: > +eroman, could you review for //net OWNERS? > ...
5 years, 1 month ago (2015-11-05 14:34:35 UTC) #23
eroman
https://codereview.chromium.org/1303973009/diff/220001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/1303973009/diff/220001/net/BUILD.gn#newcode1281 net/BUILD.gn:1281: source_set("swr_domain_matcher") { this name isn't really congruent with the ...
5 years, 1 month ago (2015-11-06 23:35:50 UTC) #25
Adam Rice
I have split up the CL so that we can focus on the issues separately. ...
5 years, 1 month ago (2015-11-09 11:01:21 UTC) #26
Adam Rice
5 years, 1 month ago (2015-11-11 05:35:54 UTC) #28
The second part of the CL is now uploaded at
https://codereview.chromium.org/1433893002/

Powered by Google App Engine
This is Rietveld 408576698