[DO NOT COMMIT] Re-use the dafsa code for s-w-r histograms

[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

[Adam Rice, 2015-10-15 13:58:01]

ricea@chromium.org changed reviewers:
+ tyoshino@chromium.org

[Adam Rice, 2015-10-15 13:58:01]

tyoshino, if you have time could you review this for readability and general
correctness.

[tyoshino (SeeGerritForStatus), 2015-10-19 07:15:10]

https://codereview.chromium.org/1303973009/diff/120001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1303973009/diff/120001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:173: 
inserted accidentally?

https://codereview.chromium.org/1303973009/diff/120001/net/base/dafsa/lookup_...
File net/base/dafsa/lookup_string.cc (right):

https://codereview.chromium.org/1303973009/diff/120001/net/base/dafsa/lookup_...
net/base/dafsa/lookup_string.cc:89: int LookupString(const unsigned char* graph,
You've moved this out of the anonymous namespace. Now net::LookupString is
visible to everyone and sounds too generic. How about prefixing this with DAFSA,
etc.?

https://codereview.chromium.org/1303973009/diff/120001/net/net.gypi
File net/net.gypi (right):

https://codereview.chromium.org/1303973009/diff/120001/net/net.gypi#newcode26
net/net.gypi:26: 'base/completion_callback.h',
moved accidentally?

[Adam Rice, 2015-10-19 12:03:07]

https://codereview.chromium.org/1303973009/diff/120001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1303973009/diff/120001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:173: 
On 2015/10/19 07:15:10, tyoshino wrote:
> inserted accidentally?

Yes. Fixed.

https://codereview.chromium.org/1303973009/diff/120001/net/base/dafsa/lookup_...
File net/base/dafsa/lookup_string.cc (right):

https://codereview.chromium.org/1303973009/diff/120001/net/base/dafsa/lookup_...
net/base/dafsa/lookup_string.cc:89: int LookupString(const unsigned char* graph,
On 2015/10/19 07:15:10, tyoshino wrote:
> You've moved this out of the anonymous namespace. Now net::LookupString is
> visible to everyone and sounds too generic. How about prefixing this with
DAFSA,
> etc.?

Good point, thank you. I changed the name to LookupStringInDafsa(), which I
think is pretty self-explanatory.

https://codereview.chromium.org/1303973009/diff/120001/net/net.gypi
File net/net.gypi (right):

https://codereview.chromium.org/1303973009/diff/120001/net/net.gypi#newcode26
net/net.gypi:26: 'base/completion_callback.h',
On 2015/10/19 07:15:10, tyoshino wrote:
> moved accidentally?

Yes. I don't know why. Moved back.

[Bryan McQuade, 2015-10-19 12:58:15]

bmcquade@chromium.org changed reviewers:
+ bmcquade@chromium.org

[Bryan McQuade, 2015-10-19 12:58:16]

https://codereview.chromium.org/1303973009/diff/140001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1303973009/diff/140001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:97:
include_in_stale_while_revalidate_experiment_ = true;
just FYI (happened to find this CL): we don't want MetricsWebContentsObserver to
become a home for code for all the variants of things people want to track
related to page load metrics. That will quickly make the class unmanageable as
different teams add their logic. See page_load_histograms.cc for an example of
what we don't want to turn into. :)

Instead, if you want to log things related to page load metrics, we'd like to
add an observer interface to MetricsWebContentsObserver (maybe
PageLoadMetricsObserver) that provides you with the hooks you need to keep track
of and log metrics that your team needs.

Please reach out to csharrison@ to discuss what that API might look like. I
believe we should be able to prioritize that work to allow you to keep this
moving forward in a way that doesn't complicate the core
MetricsWebContentsObserver implementation.

[Adam Rice, 2015-10-19 13:34:44]

https://codereview.chromium.org/1303973009/diff/140001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1303973009/diff/140001/components/page_load_m...
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:
> just FYI (happened to find this CL): we don't want MetricsWebContentsObserver
to
> become a home for code for all the variants of things people want to track
> related to page load metrics. That will quickly make the class unmanageable as
> different teams add their logic. See page_load_histograms.cc for an example of
> what we don't want to turn into. :)
> 
> Instead, if you want to log things related to page load metrics, we'd like to
> add an observer interface to MetricsWebContentsObserver (maybe
> PageLoadMetricsObserver) that provides you with the hooks you need to keep
track
> of and log metrics that your team needs.
> 
> Please reach out to csharrison@ to discuss what that API might look like. I
> believe we should be able to prioritize that work to allow you to keep this
> moving forward in a way that doesn't complicate the core
> MetricsWebContentsObserver implementation.

SGTM.

[tyoshino (SeeGerritForStatus), 2015-10-30 06:29:59]

https://codereview.chromium.org/1303973009/diff/140001/net/base/dafsa/lookup_...
File net/base/dafsa/lookup_string.h (right):

https://codereview.chromium.org/1303973009/diff/140001/net/base/dafsa/lookup_...
net/base/dafsa/lookup_string.h:15: const int kPrivateRule = 4;
Now these constants are moved out of the anonymous namespace. They, especially
kNotFound, are given too generic name. Rename?

https://codereview.chromium.org/1303973009/diff/140001/net/base/swr_histogram...
File net/base/swr_histogram_domains/BUILD.gn (right):

https://codereview.chromium.org/1303973009/diff/140001/net/base/swr_histogram...
net/base/swr_histogram_domains/BUILD.gn:1: # Copyright (c) 2014 The Chromium
Authors. All rights reserved.
let's use 2015 as the contents are very different from the original.

[tyoshino (SeeGerritForStatus), 2015-10-30 07:48:02]

https://codereview.chromium.org/1303973009/diff/140001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1303973009/diff/140001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:146:
PAGE_LOAD_HISTOGRAM(
no need to align with the grouping above? bg or not bg

https://codereview.chromium.org/1303973009/diff/140001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1303973009/diff/140001/net/BUILD.gn#newcode108
net/BUILD.gn:108: "//net/base/swr_histogram_domains",
swr_histogram_domains won't be used so widely as registry_controlled_domains. Do
we have any option to have smaller set of rules depend on this?

[Adam Rice, 2015-11-02 13:53:54]

https://codereview.chromium.org/1303973009/diff/140001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1303973009/diff/140001/components/page_load_m...
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 to align with the grouping above? bg or not bg

When I tested manually, the BG or not BG distinction didn't work how I would
intuitively expect. For example, navigations across domains within a tab are
considered to start in the background.

So I decided it was best just to ignore the distinction and log everything.

https://codereview.chromium.org/1303973009/diff/140001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1303973009/diff/140001/net/BUILD.gn#newcode108
net/BUILD.gn:108: "//net/base/swr_histogram_domains",
On 2015/10/30 07:48:02, tyoshino wrote:
> swr_histogram_domains won't be used so widely as registry_controlled_domains.
Do
> we have any option to have smaller set of rules depend on this?

Okay, I've broken it out into a separate target so that it no longer gets
compiled into libnet.{so|a}.

https://codereview.chromium.org/1303973009/diff/140001/net/base/dafsa/lookup_...
File net/base/dafsa/lookup_string.h (right):

https://codereview.chromium.org/1303973009/diff/140001/net/base/dafsa/lookup_...
net/base/dafsa/lookup_string.h:15: const int kPrivateRule = 4;
On 2015/10/30 06:29:59, tyoshino wrote:
> Now these constants are moved out of the anonymous namespace. They, especially
> kNotFound, are given too generic name. Rename?

I added "Dafsa" to each of the names. It's a bit ugly but it does work to
disambiguate them.

https://codereview.chromium.org/1303973009/diff/140001/net/base/swr_histogram...
File net/base/swr_histogram_domains/BUILD.gn (right):

https://codereview.chromium.org/1303973009/diff/140001/net/base/swr_histogram...
net/base/swr_histogram_domains/BUILD.gn:1: # Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2015/10/30 06:29:59, tyoshino wrote:
> let's use 2015 as the contents are very different from the original.

Done.

[Adam Rice, 2015-11-02 14:06:26]

ricea@chromium.org changed reviewers:
+ csharrison@chromium.org

[Adam Rice, 2015-11-02 14:06:27]

+csharrison please review the use of the PageLoadMetricsObserver under
//chrome/browser/page_load_metrics/observers/

[Bryan McQuade, 2015-11-02 14:22:39]

https://codereview.chromium.org/1303973009/diff/140001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1303973009/diff/140001/components/page_load_m...
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 2015/10/30 07:48:02, tyoshino wrote:
> > no need to align with the grouping above? bg or not bg
> 
> When I tested manually, the BG or not BG distinction didn't work how I would
> intuitively expect. For example, navigations across domains within a tab are
> considered to start in the background.
> 
> So I decided it was best just to ignore the distinction and log everything.

That seems like a bug. Can you open a bug with your findings assigned to
csharrison and cc me?

[tyoshino (SeeGerritForStatus), 2015-11-04 07:11:28]

histograms.xml

What's the reason the suffix entry is listed between the affected-histogram
entries?

[tyoshino (SeeGerritForStatus), 2015-11-04 07:25:12]

https://codereview.chromium.org/1303973009/diff/180001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.h
(right):

https://codereview.chromium.org/1303973009/diff/180001/chrome/browser/page_lo...
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 a forward declaration in this file instead)?

https://codereview.chromium.org/1303973009/diff/180001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.h:25:
// True if the committed URL is one of the 100 domains of interest to the
unless you need to emphasis "100" for the readers of this file, we should not
include the detailed number in this file as it's more likely to become stale.

[tyoshino (SeeGerritForStatus), 2015-11-04 08:34:07]

https://codereview.chromium.org/1303973009/diff/180001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right):

https://codereview.chromium.org/1303973009/diff/180001/chrome/browser/page_lo...
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
File net/BUILD.gn (right):

https://codereview.chromium.org/1303973009/diff/180001/net/BUILD.gn#newcode1285
net/BUILD.gn:1285: "//base:base",
:base can be omitted? I like making deps specific but for consistency with the
other code in this file.

https://codereview.chromium.org/1303973009/diff/180001/net/BUILD.gn#newcode1286
net/BUILD.gn:1286: "//net/base/swr_histogram_domains",
can we make distinction between net/base/swr_histogram_domains and
net/swr_histogram_domains more clear on this BUILD file? rename either of them a
bit?

https://codereview.chromium.org/1303973009/diff/180001/net/base/swr_histogram...
File net/base/swr_histogram_domains/swr_histogram_domains.gperf (right):

https://codereview.chromium.org/1303973009/diff/180001/net/base/swr_histogram...
net/base/swr_histogram_domains/swr_histogram_domains.gperf:12: // This file is
converted to a DAFSA by make-dafsa.py.
make_dafsa

https://codereview.chromium.org/1303973009/diff/180001/net/base/swr_histogram...
File net/base/swr_histogram_domains/swr_histogram_domains.h (right):

https://codereview.chromium.org/1303973009/diff/180001/net/base/swr_histogram...
net/base/swr_histogram_domains/swr_histogram_domains.h:19: // ignored. |host| is
expected to have be canonicalised to lowercase (as
remove "have"?

[tyoshino (SeeGerritForStatus), 2015-11-04 08:34:31]

l-g-t-m once the comments above are addressed!

[Adam Rice, 2015-11-04 09:23:45]

On 2015/11/04 07:11:28, tyoshino wrote:
> histograms.xml
> 
> What's the reason the suffix entry is listed between the affected-histogram
> entries?

When I ran pretty_print.py it moved it there. I assumed that pretty_print.py
would
always move it, but I just moved it to the top of the <histogram_suffixes> tag
and
ran pretty_print.py again and it didn't move it again.

[Adam Rice, 2015-11-05 05:30:40]

https://codereview.chromium.org/1303973009/diff/180001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.h
(right):

https://codereview.chromium.org/1303973009/diff/180001/chrome/browser/page_lo...
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 be moved to .cc file (put a forward declaration in this file instead)?

Done.

https://codereview.chromium.org/1303973009/diff/180001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.h:25:
// True if the committed URL is one of the 100 domains of interest to the
On 2015/11/04 07:25:12, tyoshino wrote:
> unless you need to emphasis "100" for the readers of this file, we should not
> include the detailed number in this file as it's more likely to become stale.

Good point, thank you.

https://codereview.chromium.org/1303973009/diff/180001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right):

https://codereview.chromium.org/1303973009/diff/180001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:22: //
OnPageLoadMetricsGoingAway.
On 2015/11/04 08:34:07, tyoshino wrote:
> merge with the comments at L18-19?

The FromGWS metrics are permanent, but the StaleWhileRevalidate ones should be
removed after 1 or 2 releases. To make it easy to remove, I'd rather not change
the comment above.

https://codereview.chromium.org/1303973009/diff/180001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1303973009/diff/180001/net/BUILD.gn#newcode1285
net/BUILD.gn:1285: "//base:base",
On 2015/11/04 08:34:07, tyoshino wrote:
> :base can be omitted? I like making deps specific but for consistency with the
> other code in this file.

Done, thanks.

https://codereview.chromium.org/1303973009/diff/180001/net/BUILD.gn#newcode1286
net/BUILD.gn:1286: "//net/base/swr_histogram_domains",
On 2015/11/04 08:34:07, tyoshino wrote:
> can we make distinction between net/base/swr_histogram_domains and
> net/swr_histogram_domains more clear on this BUILD file? rename either of them
a
> bit?

I renamed //net:swr_histogram_domains to swr_domain_matcher. The structure of
the build targets is a bit unnatural for GN, in order to make it work the same
as the GYP build.

https://codereview.chromium.org/1303973009/diff/180001/net/base/swr_histogram...
File net/base/swr_histogram_domains/swr_histogram_domains.gperf (right):

https://codereview.chromium.org/1303973009/diff/180001/net/base/swr_histogram...
net/base/swr_histogram_domains/swr_histogram_domains.gperf:12: // This file is
converted to a DAFSA by make-dafsa.py.
On 2015/11/04 08:34:07, tyoshino wrote:
> make_dafsa

Done.

https://codereview.chromium.org/1303973009/diff/180001/net/base/swr_histogram...
File net/base/swr_histogram_domains/swr_histogram_domains.h (right):

https://codereview.chromium.org/1303973009/diff/180001/net/base/swr_histogram...
net/base/swr_histogram_domains/swr_histogram_domains.h:19: // ignored. |host| is
expected to have be canonicalised to lowercase (as
On 2015/11/04 08:34:07, tyoshino wrote:
> remove "have"?

Done.

[tyoshino (SeeGerritForStatus), 2015-11-05 09:00:02]

lgtm

https://codereview.chromium.org/1303973009/diff/180001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right):

https://codereview.chromium.org/1303973009/diff/180001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:22: //
OnPageLoadMetricsGoingAway.
On 2015/11/05 05:30:39, Adam Rice wrote:
> On 2015/11/04 08:34:07, tyoshino wrote:
> > merge with the comments at L18-19?
> 
> The FromGWS metrics are permanent, but the StaleWhileRevalidate ones should be
> removed after 1 or 2 releases. To make it easy to remove, I'd rather not
change
> the comment above.

Ah, got it!

[Adam Rice, 2015-11-05 09:24:09]

ricea@chromium.org changed reviewers:
+ eroman@chromium.org

[Adam Rice, 2015-11-05 09:24:10]

+eroman, could you review for //net OWNERS?

Please reassign as appropriate. I'd like to get this landed quite soon.

Although the addition of swr_histogram_domains is temporary, I think making it
possible to use the dafsa code for things other that registry_controlled_domains
is probably generally useful.

There are many ways the code could be structured and my choice was quite
arbitrary so please suggest whatever you think fits best in //net.

[Charlie Harrison, 2015-11-05 14:34:35]

On 2015/11/05 09:24:10, Adam Rice wrote:
> +eroman, could you review for //net OWNERS?
> 
> Please reassign as appropriate. I'd like to get this landed quite soon.
> 
> Although the addition of swr_histogram_domains is temporary, I think making it
> possible to use the dafsa code for things other that
registry_controlled_domains
> is probably generally useful.
> 
> There are many ways the code could be structured and my choice was quite
> arbitrary so please suggest whatever you think fits best in //net.

page_load_metrics lgtm, adding rdsmith to rubberstamp (neither Bryan nor I are
committers).

[Charlie Harrison, 2015-11-05 14:35:04]

csharrison@chromium.org changed reviewers:
+ rdsmith@chromium.org

[eroman, 2015-11-06 23:35:50]

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 filenames.

https://codereview.chromium.org/1303973009/diff/220001/net/BUILD.gn#newcode1289
net/BUILD.gn:1289: "//net/base/swr_histogram_domains",
This doesn't show up in codesearch for me, where is it? Also this naming seems
weird given that the files in this source set are also under
swr_histogram_domains

https://codereview.chromium.org/1303973009/diff/220001/net/base/dafsa/lookup_...
File net/base/dafsa/lookup_string.cc (right):

https://codereview.chromium.org/1303973009/diff/220001/net/base/dafsa/lookup_...
net/base/dafsa/lookup_string.cc:15: bool GetNextOffset(const unsigned char**
pos,
This shows up as a new file, is the expectation that I am to review this?

Presumably this is just a move.

Make sure you run something like --similarity=5  or whatever you need so that
this get recognized as a move and I can see any actual deltas.

https://codereview.chromium.org/1303973009/diff/220001/net/base/dafsa/lookup_...
File net/base/dafsa/lookup_string.h (right):

https://codereview.chromium.org/1303973009/diff/220001/net/base/dafsa/lookup_...
net/base/dafsa/lookup_string.h:19: // Lookup a domain key in a byte array
|graph| generated by make_dafsa.py.  The
As a standalone function this needs a lot more context in the documentation
explaining what it is.

It may have made sense as part of registry_controlled_domains, but here it needs
more background.

https://codereview.chromium.org/1303973009/diff/220001/net/base/dafsa/lookup_...
net/base/dafsa/lookup_string.h:22: int NET_EXPORT_PRIVATE
LookupStringInDafsa(const unsigned char* graph,
why NET_EXPORT_PRIVATE and not NET_EXPORT?

https://codereview.chromium.org/1303973009/diff/220001/net/base/swr_histogram...
File net/base/swr_histogram_domains/swr_histogram_domains.gperf (right):

https://codereview.chromium.org/1303973009/diff/220001/net/base/swr_histogram...
net/base/swr_histogram_domains/swr_histogram_domains.gperf:14: // *Two domains
appear in effective_tld_names.dat and so were manually removed.
What is the privacy story here?

Doesn't this give visibility into per-domain browsing history?

Don't you need to use something like
https://sites.google.com/a/chromium.org/dev/developers/design-documents/rappor
if you want to be doing this?

https://codereview.chromium.org/1303973009/diff/220001/net/net.gypi
File net/net.gypi (right):

https://codereview.chromium.org/1303973009/diff/220001/net/net.gypi#newcode22
net/net.gypi:22: 'base/dafsa/lookup_string.cc',
Several problems here:

  (1) Why is this in a subdirectory of base? This could just as easily be
dafsa_lookup_string.h, there is nothing else in that subdirectory (and also not
sure subdirectories of base are a pattern I want to encourage).

  (2) "dafsa" is not a particularly descriptive name. I would be surprised if
many chromium developers could tell you what that meant.

 (3) Unit test?

https://codereview.chromium.org/1303973009/diff/220001/net/net.gypi#newcode1301
net/net.gypi:1301:
'base/swr_histogram_domains/swr_histogram_domains_unittest.cc',
similar issue here:

  (1) Why in a subdirectory
  (2) swr_* is not a helpful name

[Adam Rice, 2015-11-09 11:01:21]

I have split up the CL so that we can focus on the issues separately.

The first part (making LookupStringInFixedSet public) is at
https://codereview.chromium.org/1412343008/.

https://codereview.chromium.org/1303973009/diff/220001/net/base/dafsa/lookup_...
File net/base/dafsa/lookup_string.cc (right):

https://codereview.chromium.org/1303973009/diff/220001/net/base/dafsa/lookup_...
net/base/dafsa/lookup_string.cc:15: bool GetNextOffset(const unsigned char**
pos,
On 2015/11/06 23:35:50, eroman wrote:
> This shows up as a new file, is the expectation that I am to review this?
> 
> Presumably this is just a move.
> 
> Make sure you run something like --similarity=5  or whatever you need so that
> this get recognized as a move and I can see any actual deltas.

No, this is not new code. I have added --similarity=5.

https://codereview.chromium.org/1303973009/diff/220001/net/base/dafsa/lookup_...
File net/base/dafsa/lookup_string.h (right):

https://codereview.chromium.org/1303973009/diff/220001/net/base/dafsa/lookup_...
net/base/dafsa/lookup_string.h:19: // Lookup a domain key in a byte array
|graph| generated by make_dafsa.py.  The
On 2015/11/06 23:35:50, eroman wrote:
> As a standalone function this needs a lot more context in the documentation
> explaining what it is.
> 
> It may have made sense as part of registry_controlled_domains, but here it
needs
> more background.

I have expanded on the comment to hopefully make it more useful.

https://codereview.chromium.org/1303973009/diff/220001/net/base/dafsa/lookup_...
net/base/dafsa/lookup_string.h:22: int NET_EXPORT_PRIVATE
LookupStringInDafsa(const unsigned char* graph,
On 2015/11/06 23:35:50, eroman wrote:
> why NET_EXPORT_PRIVATE and not NET_EXPORT?

I was just being conservative. Changed.

https://codereview.chromium.org/1303973009/diff/220001/net/base/swr_histogram...
File net/base/swr_histogram_domains/swr_histogram_domains.gperf (right):

https://codereview.chromium.org/1303973009/diff/220001/net/base/swr_histogram...
net/base/swr_histogram_domains/swr_histogram_domains.gperf:14: // *Two domains
appear in effective_tld_names.dat and so were manually removed.
On 2015/11/06 23:35:50, eroman wrote:
> What is the privacy story here?
> 
> Doesn't this give visibility into per-domain browsing history?
> 
> Don't you need to use something like
> https://sites.google.com/a/chromium.org/dev/developers/design-documents/rappor
> if you want to be doing this?

The privacy protections are:

1) It's a big list, so although we receive information that a user visited
page(s) under one or more of these domains, we don't know which. We could
improve the privacy protection by making the list bigger, but at a cost of
larger binary size. The current list takes ~800 bytes in compiled form.
2) It's a diverse list, since the domains are chosen based on technical aspects
of the site (specifically use of stale-while-revalidate for third-party
subresources) and popularity rather than content. This means that visiting a
site on this list doesn't reveal anything particular about a user.
3) These are popular sites, so many users will visit them, making the fact that
a user visited one or more of them very low in information.

Rappor permits us to get specific information in the aggregate without revealing
specific information about any individual. In this case we don't need specific
information at all--we are not interested in which domain was visited, only how
long it took.

I have cleared it with the Chrome Privacy Team.

https://codereview.chromium.org/1303973009/diff/220001/net/net.gypi
File net/net.gypi (right):

https://codereview.chromium.org/1303973009/diff/220001/net/net.gypi#newcode22
net/net.gypi:22: 'base/dafsa/lookup_string.cc',
On 2015/11/06 23:35:50, eroman wrote:
> Several problems here:
> 
>   (1) Why is this in a subdirectory of base? This could just as easily be
> dafsa_lookup_string.h, there is nothing else in that subdirectory (and also
not
> sure subdirectories of base are a pattern I want to encourage).
> 
>   (2) "dafsa" is not a particularly descriptive name. I would be surprised if
> many chromium developers could tell you what that meant.
> 
>  (3) Unit test?

Renamed to base/lookup_string_in_fixed_set.*

Also added unit tests.

https://codereview.chromium.org/1303973009/diff/220001/net/net.gypi#newcode1301
net/net.gypi:1301:
'base/swr_histogram_domains/swr_histogram_domains_unittest.cc',
On 2015/11/06 23:35:50, eroman wrote:
> similar issue here:
> 
>   (1) Why in a subdirectory

I was just copying the structure of registry_controlled_domains. It feels
somehow that .gperf files are "special" and deserve their own subdirectory.

I will remove the subdirectory.

>   (2) swr_* is not a helpful name

I will change it to stale_while_revalidate_experiment_domains.

[Adam Rice, 2015-11-11 05:34:59]

Description was changed from

==========
Re-use the dafsa code for s-w-r histograms

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
==========

to

==========
[DO NOT COMMIT] Re-use the dafsa code for s-w-r histograms

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.

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.

BUG=348877
TEST=net_unittests
==========

[Adam Rice, 2015-11-11 05:35:54]

The second part of the CL is now uploaded at
https://codereview.chromium.org/1433893002/

[Adam Rice, 2015-11-11 17:39:27]

Description was changed from

==========
[DO NOT COMMIT] Re-use the dafsa code for s-w-r histograms

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.

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.

BUG=348877
TEST=net_unittests
==========

to

==========
[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
==========