Add support for split PSL list distinctions.

Add support for split Public Suffix List distinctions.

This adds support for the private additions to the Public Suffix List.

* Since net::RegistryControlledDomainService only contained static methods, this
  CL changes these methods to be contained within the namespace
  net::registry_controlled_domains and removes the class entirely.
* All methods defined as part of net::registry_controlled_domains now
  have a mandatory argument to specify whether the private registries
  should be included.
* Since the old implementation did not take into account the private
  registries, this sets all old callers to use EXCLUDE_PRIVATE as the
  net::registry_controlled_domains::PrivateRegistryFilter argument.
* Changes the parameter for including unknown registries or not to be an enum
  instead of a boolean, using a similar naming scheme as for the private
  registries: net::registry_controlled_domains::UnknownRegistryFilter.
* This also updates the effective-TLD data file to:
  45cfff9c781f 2013-04-23 11:51 +0100
  It includes changes from a number of Mozilla bugs, listed on
  https://hg.mozilla.org/mozilla-central/log/45cfff9c781f/netwerk/dns/effective_tld_names.dat
  between 290afd57d2a8 (2012-07-04 16:08 +0100) and
  45cfff9c781f (2013-04-23 11:51 +0100).

BUG=37436, 96086
R=brettw@chromium.org, erikwright@chromium.org, pam@chromium.org, rsleevi@chromium.org, sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199771

[nyquist, 2013-04-10 01:28:22]

pam: PTAL

Most interesting parts are: 
net/base/registry_controlled_domains/
net/tools/tld_cleanup/

[nyquist, 2013-04-10 01:33:46]

And FYI, this review is missing one file, namely the output from gperf. I
therefore have to run tryjobs through git try instead of git cl try.

The reason is that it is too large for Rietveld to handle:
Not uploading the patch for
net/base/registry_controlled_domains/effective_tld_names.cc because the file is
too large.

The file was generated with:
gperf -a -L C++ -C -c -o -t -k '*' -NFindDomain -D -m 5
effective_tld_names.gperf

[Pam (message me for reviews), 2013-04-10 17:18:17]

https://codereview.chromium.org/13979002/diff/53001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/13979002/diff/53001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:271: false,
It'd be great to change this second parameter to a more self-evident enum too,
while you're updating all the callers.

https://codereview.chromium.org/13979002/diff/53001/net/base/registry_control...
File net/base/registry_controlled_domains/registry_controlled_domain.cc (right):

https://codereview.chromium.org/13979002/diff/53001/net/base/registry_control...
net/base/registry_controlled_domains/registry_controlled_domain.cc:216: // in
its entirety.
Please reword the added comment to be more clear. Something like

Furthermore, if the apparent match is a private registry and we're not including
those, it can't be an actual match.

https://codereview.chromium.org/13979002/diff/53001/net/base/registry_control...
net/base/registry_controlled_domains/registry_controlled_domain.cc:217: if (rule
&& !(rule->is_private && filter == EXCLUDE_PRIVATE) &&
This new condition is a lot of negatives stacked up. It would be more readable
rephrased as

(filter == INCLUDE_PRIVATE || !rule->is_private)

https://codereview.chromium.org/13979002/diff/53001/net/base/registry_control...
net/base/registry_controlled_domains/registry_controlled_domain.cc:218:
base::strncasecmp(domain_str, rule->name, domain_length) == 0) {
Please re-format this to break the lines after each top-level && (i.e., not the
one inside the !()).

https://codereview.chromium.org/13979002/diff/53001/net/base/registry_control...
File net/base/registry_controlled_domains/registry_controlled_domain.h (right):

https://codereview.chromium.org/13979002/diff/53001/net/base/registry_control...
net/base/registry_controlled_domains/registry_controlled_domain.h:141: // the
host is assumed to be the registry.
Please add a sentence describing the |filter| parameter. Actually, please add it
up near the enum, so it can cover all these methods.

https://codereview.chromium.org/13979002/diff/53001/net/base/registry_control...
File net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc
(right):

https://codereview.chromium.org/13979002/diff/53001/net/base/registry_control...
net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc:38:
size_t GetRegistryLengthFromURLIncludePrivate(const std::string& url,
I suggest 
...IncludingPrivate
to be more grammatical.

https://codereview.chromium.org/13979002/diff/53001/net/base/registry_control...
net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc:259:
// Private registries.
Please add a comment listing the private domains in the data file.

https://codereview.chromium.org/13979002/diff/53001/net/tools/tld_cleanup/tld...
File net/tools/tld_cleanup/tld_cleanup.cc (right):

https://codereview.chromium.org/13979002/diff/53001/net/tools/tld_cleanup/tld...
net/tools/tld_cleanup/tld_cleanup.cc:25: //    and "// ===END PRIVATE
DOMAINS===" with is_private.
I'd just say

Marks ... as private.

since what it's really appending is a boolean, not "is_private".

https://codereview.chromium.org/13979002/diff/53001/net/tools/tld_cleanup/tld...
net/tools/tld_cleanup/tld_cleanup.cc:205: if (line_start + 1 < data.size() &&
This can still be "else if".

[nyquist, 2013-04-12 00:41:30]

Addressed all comments. PTAL.

https://codereview.chromium.org/13979002/diff/53001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/13979002/diff/53001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:271: false,
On 2013/04/10 17:18:17, Pam wrote:
> It'd be great to change this second parameter to a more self-evident enum too,
> while you're updating all the callers.

Done. Changed to EXCLUDE_PRIVATE_REGISTRIES. I am totally open to suggestions
though.

https://codereview.chromium.org/13979002/diff/53001/net/base/registry_control...
File net/base/registry_controlled_domains/registry_controlled_domain.cc (right):

https://codereview.chromium.org/13979002/diff/53001/net/base/registry_control...
net/base/registry_controlled_domains/registry_controlled_domain.cc:216: // in
its entirety.
On 2013/04/10 17:18:17, Pam wrote:
> Please reword the added comment to be more clear. Something like
> 
> Furthermore, if the apparent match is a private registry and we're not
including
> those, it can't be an actual match.

Done.

https://codereview.chromium.org/13979002/diff/53001/net/base/registry_control...
net/base/registry_controlled_domains/registry_controlled_domain.cc:217: if (rule
&& !(rule->is_private && filter == EXCLUDE_PRIVATE) &&
On 2013/04/10 17:18:17, Pam wrote:
> This new condition is a lot of negatives stacked up. It would be more readable
> rephrased as
> 
> (filter == INCLUDE_PRIVATE || !rule->is_private)

Done.

https://codereview.chromium.org/13979002/diff/53001/net/base/registry_control...
net/base/registry_controlled_domains/registry_controlled_domain.cc:218:
base::strncasecmp(domain_str, rule->name, domain_length) == 0) {
On 2013/04/10 17:18:17, Pam wrote:
> Please re-format this to break the lines after each top-level && (i.e., not
the
> one inside the !()).

Done.

https://codereview.chromium.org/13979002/diff/53001/net/base/registry_control...
File net/base/registry_controlled_domains/registry_controlled_domain.h (right):

https://codereview.chromium.org/13979002/diff/53001/net/base/registry_control...
net/base/registry_controlled_domains/registry_controlled_domain.h:141: // the
host is assumed to be the registry.
On 2013/04/10 17:18:17, Pam wrote:
> Please add a sentence describing the |filter| parameter. Actually, please add
it
> up near the enum, so it can cover all these methods.

Done.

https://codereview.chromium.org/13979002/diff/53001/net/base/registry_control...
File net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc
(right):

https://codereview.chromium.org/13979002/diff/53001/net/base/registry_control...
net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc:38:
size_t GetRegistryLengthFromURLIncludePrivate(const std::string& url,
On 2013/04/10 17:18:17, Pam wrote:
> I suggest 
> ...IncludingPrivate
> to be more grammatical.

Done.

https://codereview.chromium.org/13979002/diff/53001/net/base/registry_control...
net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc:259:
// Private registries.
On 2013/04/10 17:18:17, Pam wrote:
> Please add a comment listing the private domains in the data file.

Done.

https://codereview.chromium.org/13979002/diff/53001/net/tools/tld_cleanup/tld...
File net/tools/tld_cleanup/tld_cleanup.cc (right):

https://codereview.chromium.org/13979002/diff/53001/net/tools/tld_cleanup/tld...
net/tools/tld_cleanup/tld_cleanup.cc:25: //    and "// ===END PRIVATE
DOMAINS===" with is_private.
On 2013/04/10 17:18:17, Pam wrote:
> I'd just say
> 
> Marks ... as private.
> 
> since what it's really appending is a boolean, not "is_private".

Done.

https://codereview.chromium.org/13979002/diff/53001/net/tools/tld_cleanup/tld...
net/tools/tld_cleanup/tld_cleanup.cc:205: if (line_start + 1 < data.size() &&
On 2013/04/10 17:18:17, Pam wrote:
> This can still be "else if".

Changed to use else if and updated line_end in both private domain branches.

[Pam (message me for reviews), 2013-04-12 08:54:02]

https://codereview.chromium.org/13979002/diff/53001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/13979002/diff/53001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:271: false,
On 2013/04/12 00:41:30, nyquist wrote:
> On 2013/04/10 17:18:17, Pam wrote:
> > It'd be great to change this second parameter to a more self-evident enum
too,
> > while you're updating all the callers.
> 
> Done. Changed to EXCLUDE_PRIVATE_REGISTRIES. I am totally open to suggestions
> though.

Sorry, maybe I wasn't clear. I meant to change that second bool parameter to an
enum too, e.g {INCLUDE|EXCLUDE}_UNKNOWN_REGISTRIES.

https://codereview.chromium.org/13979002/diff/122001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://codereview.chromium.org/13979002/diff/122001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_quick_provider.cc:272: == 0) {
Please indent 4 more spaces.

Or, consider a namespace alias; see later comment on
registry_controlled_domain.h.

https://codereview.chromium.org/13979002/diff/122001/chrome/browser/history/h...
File chrome/browser/history/history_backend.cc (right):

https://codereview.chromium.org/13979002/diff/122001/chrome/browser/history/h...
chrome/browser/history/history_backend.cc:504: 0) &&
Please indent to 4 spaces inside the line above.

https://codereview.chromium.org/13979002/diff/122001/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain.h (right):

https://codereview.chromium.org/13979002/diff/122001/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.h:136: //
private additions in your lookup.
Fantastic explanatory comment, thanks.

https://codereview.chromium.org/13979002/diff/122001/net/tools/tld_cleanup/tl...
File net/tools/tld_cleanup/tld_cleanup.cc (right):

https://codereview.chromium.org/13979002/diff/122001/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup.cc:65: "// Copyright 2013 The Chromium
Authors. All rights reserved.\n"
Since we don't update copyright years in edited files, it seems like this should
remain 2012. (But still remove the "(c)".)

https://codereview.chromium.org/13979002/diff/122001/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup.cc:259: rule.is_private = false;
You need to keep track of whether the extra rule came from the private section
and set the flag accordingly. Also please add a test for that case.

[Pam (message me for reviews), 2013-04-12 09:45:14]

https://codereview.chromium.org/13979002/diff/53001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/13979002/diff/53001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:271: false,
On 2013/04/12 08:54:03, Pam wrote:
> On 2013/04/12 00:41:30, nyquist wrote:
> > On 2013/04/10 17:18:17, Pam wrote:
> > > It'd be great to change this second parameter to a more self-evident enum
> too,
> > > while you're updating all the callers.
> > 
> > Done. Changed to EXCLUDE_PRIVATE_REGISTRIES. I am totally open to
suggestions
> > though.
> 
> Sorry, maybe I wasn't clear. I meant to change that second bool parameter to
an
> enum too, e.g {INCLUDE|EXCLUDE}_UNKNOWN_REGISTRIES.

Just to clarify, I do like the change to add _REGISTRIES to this enum. I'm
asking to add another enum for the other parameter too.

https://codereview.chromium.org/13979002/diff/122001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://codereview.chromium.org/13979002/diff/122001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_quick_provider.cc:272: == 0) {
On 2013/04/12 08:54:03, Pam wrote:
> Please indent 4 more spaces.
> 
> Or, consider a namespace alias; see later comment on
> registry_controlled_domain.h.

...which I forgot to add. Added now.

https://codereview.chromium.org/13979002/diff/122001/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain.h (right):

https://codereview.chromium.org/13979002/diff/122001/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.h:126: 
WDYT about putting something like

class RegistryControlledDomainService;
typedef RegistryControlledDomainService RCDS;

here, as a shorter alias for all the callers to use? The style guide allows
namespace aliases within included headers for that purpose, and this is
analogous.

[nyquist, 2013-04-17 02:28:10]

PTAL. Still missing a test for the tld_cleanup target though, but the rest of
the comments should have been addressed.

https://codereview.chromium.org/13979002/diff/53001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/13979002/diff/53001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:271: false,
On 2013/04/12 09:45:14, Pam wrote:
> On 2013/04/12 08:54:03, Pam wrote:
> > On 2013/04/12 00:41:30, nyquist wrote:
> > > On 2013/04/10 17:18:17, Pam wrote:
> > > > It'd be great to change this second parameter to a more self-evident
enum
> > too,
> > > > while you're updating all the callers.
> > > 
> > > Done. Changed to EXCLUDE_PRIVATE_REGISTRIES. I am totally open to
> suggestions
> > > though.
> > 
> > Sorry, maybe I wasn't clear. I meant to change that second bool parameter to
> an
> > enum too, e.g {INCLUDE|EXCLUDE}_UNKNOWN_REGISTRIES.
> 
> Just to clarify, I do like the change to add _REGISTRIES to this enum. I'm
> asking to add another enum for the other parameter too.
> 

I see! Thanks for the clarification! Updated the code to include that now.

https://codereview.chromium.org/13979002/diff/122001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://codereview.chromium.org/13979002/diff/122001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_quick_provider.cc:272: == 0) {
On 2013/04/12 09:45:14, Pam wrote:
> On 2013/04/12 08:54:03, Pam wrote:
> > Please indent 4 more spaces.
> > 
> > Or, consider a namespace alias; see later comment on
> > registry_controlled_domain.h.
> 
> ...which I forgot to add. Added now.

Done.

https://codereview.chromium.org/13979002/diff/122001/chrome/browser/history/h...
File chrome/browser/history/history_backend.cc (right):

https://codereview.chromium.org/13979002/diff/122001/chrome/browser/history/h...
chrome/browser/history/history_backend.cc:504: 0) &&
On 2013/04/12 08:54:03, Pam wrote:
> Please indent to 4 spaces inside the line above.

Done. Extracted variable for clarity.

https://codereview.chromium.org/13979002/diff/122001/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain.h (right):

https://codereview.chromium.org/13979002/diff/122001/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.h:126: 
On 2013/04/12 09:45:14, Pam wrote:
> WDYT about putting something like
> 
> class RegistryControlledDomainService;
> typedef RegistryControlledDomainService RCDS;
> 
> here, as a shorter alias for all the callers to use? The style guide allows
> namespace aliases within included headers for that purpose, and this is
> analogous.

Done.

https://codereview.chromium.org/13979002/diff/122001/net/tools/tld_cleanup/tl...
File net/tools/tld_cleanup/tld_cleanup.cc (right):

https://codereview.chromium.org/13979002/diff/122001/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup.cc:65: "// Copyright 2013 The Chromium
Authors. All rights reserved.\n"
On 2013/04/12 08:54:03, Pam wrote:
> Since we don't update copyright years in edited files, it seems like this
should
> remain 2012. (But still remove the "(c)".)

Done.

https://codereview.chromium.org/13979002/diff/122001/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup.cc:259: rule.is_private = false;
On 2013/04/12 08:54:03, Pam wrote:
> You need to keep track of whether the extra rule came from the private section
> and set the flag accordingly. Also please add a test for that case.

Done, but missing test case.

[Pam (message me for reviews), 2013-04-18 19:54:42]

Also waiting for the new test.

https://codereview.chromium.org/13979002/diff/55012/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/13979002/diff/55012/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:269:
net::RegistryControlledDomainService::GetRegistryLength(
In principle, all these method calls could use net::RCDS too. But I think that's
going a bit too far. I only mention it in case you want to try to persuade me
otherwise. :)

https://codereview.chromium.org/13979002/diff/55012/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/alternate_nav_url_fetcher.cc (right):

https://codereview.chromium.org/13979002/diff/55012/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/alternate_nav_url_fetcher.cc:146: }
Wow, what an ugly tristate operation that used to be. Not that it's beautiful
now, but there's only so much one can do... Thanks for the cleanup.

https://codereview.chromium.org/13979002/diff/55012/net/base/registry_control...
File net/base/registry_controlled_domains/registry_controlled_domain.h (right):

https://codereview.chromium.org/13979002/diff/55012/net/base/registry_control...
net/base/registry_controlled_domains/registry_controlled_domain.h:149: //
Whenever there is no matching rule in the effective-TLD data (or in the
Thanks for this cleanup too.

Please merge all these sentences into one paragraph.

https://codereview.chromium.org/13979002/diff/55012/net/base/registry_control...
net/base/registry_controlled_domains/registry_controlled_domain.h:205: // the
default data, if the resource failed to load), returns 0 if filter is
Here and later in this comment, using |unknown_filter| (with the bars to mark it
as an argument) instead of just "filter" would make it clearer exactly what
you're talking about.

https://codereview.chromium.org/13979002/diff/55012/net/tools/tld_cleanup/tld...
File net/tools/tld_cleanup/tld_cleanup.cc (right):

https://codereview.chromium.org/13979002/diff/55012/net/tools/tld_cleanup/tld...
net/tools/tld_cleanup/tld_cleanup.cc:62: typedef std::set<ExtraRule> RuleSet;
Seems like you could also keep the extra rules in a RuleMap.

https://codereview.chromium.org/13979002/diff/55012/net/tools/tld_cleanup/tld...
net/tools/tld_cleanup/tld_cleanup.cc:238: // parsing code to handle this case.
This comment can be expanded, because this check will also catch the case where
the same domain is listed in both the public and the private sections.

https://codereview.chromium.org/13979002/diff/55012/net/tools/tld_cleanup/tld...
net/tools/tld_cleanup/tld_cleanup.cc:251: extra_rules.insert(extra_rule);
Also worth checking here, to make sure we don't trample an existing public rule
with a later private one.

[nyquist, 2013-04-20 19:13:30]

pam: PTAL

I have addressed all the comments, and I have now also split out the parsing
parts of the tld_cleanup to be able to unit test it, like you asked for.
I also moved around some gyp stuff. We apparently had a duplicate old target for
tld_cleanup.

https://codereview.chromium.org/13979002/diff/55012/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/13979002/diff/55012/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:269:
net::RegistryControlledDomainService::GetRegistryLength(
On 2013/04/18 19:54:42, Pam wrote:
> In principle, all these method calls could use net::RCDS too. But I think
that's
> going a bit too far. I only mention it in case you want to try to persuade me
> otherwise. :)

net::RCDS does not give a lot of context, so I figured by keeping the function
call using the full name would clarify what RCDS is a shortcut to for new
readers...

https://codereview.chromium.org/13979002/diff/55012/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/alternate_nav_url_fetcher.cc (right):

https://codereview.chromium.org/13979002/diff/55012/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/alternate_nav_url_fetcher.cc:146: }
On 2013/04/18 19:54:42, Pam wrote:
> Wow, what an ugly tristate operation that used to be. Not that it's beautiful
> now, but there's only so much one can do... Thanks for the cleanup.

Yeah, it wasn't very pretty. Also had to have the RCDS call within the if-block
to not call it more often than before.

https://codereview.chromium.org/13979002/diff/55012/net/base/registry_control...
File net/base/registry_controlled_domains/registry_controlled_domain.h (right):

https://codereview.chromium.org/13979002/diff/55012/net/base/registry_control...
net/base/registry_controlled_domains/registry_controlled_domain.h:149: //
Whenever there is no matching rule in the effective-TLD data (or in the
On 2013/04/18 19:54:42, Pam wrote:
> Thanks for this cleanup too.
> 
> Please merge all these sentences into one paragraph.

Done.

https://codereview.chromium.org/13979002/diff/55012/net/base/registry_control...
net/base/registry_controlled_domains/registry_controlled_domain.h:205: // the
default data, if the resource failed to load), returns 0 if filter is
On 2013/04/18 19:54:42, Pam wrote:
> Here and later in this comment, using |unknown_filter| (with the bars to mark
it
> as an argument) instead of just "filter" would make it clearer exactly what
> you're talking about.

Done.

https://codereview.chromium.org/13979002/diff/55012/net/tools/tld_cleanup/tld...
File net/tools/tld_cleanup/tld_cleanup.cc (right):

https://codereview.chromium.org/13979002/diff/55012/net/tools/tld_cleanup/tld...
net/tools/tld_cleanup/tld_cleanup.cc:62: typedef std::set<ExtraRule> RuleSet;
On 2013/04/18 19:54:42, Pam wrote:
> Seems like you could also keep the extra rules in a RuleMap.

Done.

https://codereview.chromium.org/13979002/diff/55012/net/tools/tld_cleanup/tld...
net/tools/tld_cleanup/tld_cleanup.cc:238: // parsing code to handle this case.
On 2013/04/18 19:54:42, Pam wrote:
> This comment can be expanded, because this check will also catch the case
where
> the same domain is listed in both the public and the private sections.

Done.

https://codereview.chromium.org/13979002/diff/55012/net/tools/tld_cleanup/tld...
net/tools/tld_cleanup/tld_cleanup.cc:251: extra_rules.insert(extra_rule);
On 2013/04/18 19:54:42, Pam wrote:
> Also worth checking here, to make sure we don't trample an existing public
rule
> with a later private one.

Done.

[Pam (message me for reviews), 2013-04-22 09:42:12]

Hi Tommy -- I know you're often up late, but I don't always read my email every
minute. So if you get to this today, please IM me directly and I'll take another
look.

https://codereview.chromium.org/13979002/diff/232005/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain.h (right):

https://codereview.chromium.org/13979002/diff/232005/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.h:153: //
registry length will be the length of the last subcomponent (ie. 3 for
Nit: this "ie." (id est = "that is") should be "e.g." (exempli gratia = "for
example").

https://codereview.chromium.org/13979002/diff/232005/net/tools/tld_cleanup/tl...
File net/tools/tld_cleanup/tld_cleanup_util.cc (right):

https://codereview.chromium.org/13979002/diff/232005/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup_util.cc:205: // that the real TLD is listed as
public.
This seems to conflict with the comment above about making sure the same domain
isn't listed as both public and private. I suppose this comment is only
referring to _some_ *.au domain also being listed in the private section, not
exactly .au or com.au?

https://codereview.chromium.org/13979002/diff/232005/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup_util.cc:237: NormalizeResult result =
NormalizeFileToRuleMap(in_filename, rules);
Style guide says non-const pointers rather than references for output
parameters.

https://codereview.chromium.org/13979002/diff/232005/net/tools/tld_cleanup/tl...
File net/tools/tld_cleanup/tld_cleanup_util.h (right):

https://codereview.chromium.org/13979002/diff/232005/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup_util.h:11: #include "base/files/file_path.h"
You can just forward-declare FilePath in this header, since it's only being used
as a const reference.

https://codereview.chromium.org/13979002/diff/232005/net/tools/tld_cleanup/tl...
File net/tools/tld_cleanup/tld_cleanup_util_unittest.cc (right):

https://codereview.chromium.org/13979002/diff/232005/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup_util_unittest.cc:14: TEST(TldCleanupUtilTest,
TwoRealTldsSuccessfullyRead) {
Yay tests!

https://codereview.chromium.org/13979002/diff/232005/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup_util_unittest.cc:20:
.Append(FILE_PATH_LITERAL("example1.dat"));
Instead of a bunch of tiny test files, it would be more extensible and more
readable to provide a way to inject string data, along with a helper function
here that takes "public" and "private" arguments and wraps them with the
expected comments.

[nyquist, 2013-04-23 21:58:26]

pam: Addressed all comments. PTAL.

https://codereview.chromium.org/13979002/diff/232005/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain.h (right):

https://codereview.chromium.org/13979002/diff/232005/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.h:153: //
registry length will be the length of the last subcomponent (ie. 3 for
On 2013/04/22 09:42:12, Pam wrote:
> Nit: this "ie." (id est = "that is") should be "e.g." (exempli gratia = "for
> example").

Done.

https://codereview.chromium.org/13979002/diff/232005/net/tools/tld_cleanup/tl...
File net/tools/tld_cleanup/tld_cleanup_util.cc (right):

https://codereview.chromium.org/13979002/diff/232005/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup_util.cc:205: // that the real TLD is listed as
public.
On 2013/04/22 09:42:12, Pam wrote:
> This seems to conflict with the comment above about making sure the same
domain
> isn't listed as both public and private. I suppose this comment is only
> referring to _some_ *.au domain also being listed in the private section, not
> exactly .au or com.au?

Done. Rephrased to clarify that.

https://codereview.chromium.org/13979002/diff/232005/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup_util.cc:237: NormalizeResult result =
NormalizeFileToRuleMap(in_filename, rules);
On 2013/04/22 09:42:12, Pam wrote:
> Style guide says non-const pointers rather than references for output
> parameters.

Done.

https://codereview.chromium.org/13979002/diff/232005/net/tools/tld_cleanup/tl...
File net/tools/tld_cleanup/tld_cleanup_util.h (right):

https://codereview.chromium.org/13979002/diff/232005/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup_util.h:11: #include "base/files/file_path.h"
On 2013/04/22 09:42:12, Pam wrote:
> You can just forward-declare FilePath in this header, since it's only being
used
> as a const reference.

Done.

https://codereview.chromium.org/13979002/diff/232005/net/tools/tld_cleanup/tl...
File net/tools/tld_cleanup/tld_cleanup_util_unittest.cc (right):

https://codereview.chromium.org/13979002/diff/232005/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup_util_unittest.cc:14: TEST(TldCleanupUtilTest,
TwoRealTldsSuccessfullyRead) {
On 2013/04/22 09:42:12, Pam wrote:
> Yay tests!

\o/

https://codereview.chromium.org/13979002/diff/232005/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup_util_unittest.cc:20:
.Append(FILE_PATH_LITERAL("example1.dat"));
On 2013/04/22 09:42:12, Pam wrote:
> Instead of a bunch of tiny test files, it would be more extensible and more
> readable to provide a way to inject string data, along with a helper function
> here that takes "public" and "private" arguments and wraps them with the
> expected comments.

Done.

[Pam (message me for reviews), 2013-04-24 11:59:43]

LGTM with the changes (two required, one optional) below. Thanks for this work!

- Pam

https://codereview.chromium.org/13979002/diff/245001/net/tools/tld_cleanup/tl...
File net/tools/tld_cleanup/tld_cleanup_util.h (right):

https://codereview.chromium.org/13979002/diff/245001/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup_util.h:40: // Loads the file described by
|in_filename|, and converts it to the internal
Comment needs updating

https://codereview.chromium.org/13979002/diff/245001/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup_util.h:44: RuleMap& rules);
Another output that should be a * instead of a &

https://codereview.chromium.org/13979002/diff/245001/net/tools/tld_cleanup/tl...
File net/tools/tld_cleanup/tld_cleanup_util_unittest.cc (right):

https://codereview.chromium.org/13979002/diff/245001/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup_util_unittest.cc:16: output += icann_domains;
Might be simpler/clearer to concatenate, rather than a series of +=.

output = "// blah\n" +
  icann_domains +
  "// end blah\n" + [etc.]

I don't feel strongly about it, though.

[nyquist, 2013-04-25 18:13:22]

Addressed all comments.

https://codereview.chromium.org/13979002/diff/245001/net/tools/tld_cleanup/tl...
File net/tools/tld_cleanup/tld_cleanup_util.h (right):

https://codereview.chromium.org/13979002/diff/245001/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup_util.h:40: // Loads the file described by
|in_filename|, and converts it to the internal
On 2013/04/24 11:59:43, Pam wrote:
> Comment needs updating

Done.

https://codereview.chromium.org/13979002/diff/245001/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup_util.h:44: RuleMap& rules);
On 2013/04/24 11:59:43, Pam wrote:
> Another output that should be a * instead of a &

Done.

https://codereview.chromium.org/13979002/diff/245001/net/tools/tld_cleanup/tl...
File net/tools/tld_cleanup/tld_cleanup_util_unittest.cc (right):

https://codereview.chromium.org/13979002/diff/245001/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup_util_unittest.cc:16: output += icann_domains;
On 2013/04/24 11:59:43, Pam wrote:
> Might be simpler/clearer to concatenate, rather than a series of +=.
> 
> output = "// blah\n" +
>   icann_domains +
>   "// end blah\n" + [etc.]
> 
> I don't feel strongly about it, though.

Done.

[nyquist, 2013-04-26 19:11:18]

brettw: For content/
erikwright: For chrome_frame/
sky: For chrome/ and ui/
rsleevi: For net/ (except net/tools/tld_cleanup/ if
https://codereview.chromium.org/14283006/ lands first)

[erikwright (departed), 2013-04-26 19:22:32]

LGTM for net/cookies and chrome_frame

[erikwright (departed), 2013-04-26 19:23:27]

On 2013/04/26 19:22:32, erikwright wrote:
> LGTM for net/cookies and chrome_frame

content/browser/net also LGTM.

[Ryan Sleevi, 2013-04-26 19:39:59]

https://codereview.chromium.org/13979002/diff/291026/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain.cc (right):

https://codereview.chromium.org/13979002/diff/291026/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:73: const
GURL& gurl, PrivateRegistryFilter filter) {
According to clang-format & chromium style, this should go on a newline.

https://codereview.chromium.org/13979002/diff/291026/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:84: const
std::string& host, PrivateRegistryFilter filter) {
same

https://codereview.chromium.org/13979002/diff/291026/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:94: const
GURL& gurl1, const GURL& gurl2, PrivateRegistryFilter filter) {
rinse repeat one param per line throughout.

https://codereview.chromium.org/13979002/diff/291026/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain.h (right):

https://codereview.chromium.org/13979002/diff/291026/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.h:128: typedef
RegistryControlledDomainService RCDS;
So I realize pam@ made the suggestion that this is likely permissible in the
style guide, because it's "similar" to a namespace alias (ala
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...
), but that style guide includes an important note:

"Note that an alias in a .h file is visible to everyone #including that file, so
public headers (those available outside a project) and headers transitively
#included by them, should avoid defining aliases, as part of the general goal of
keeping public APIs as small as possible."

This file is a "public" header of the net/ API, and so using an alias here
really is not desirable.

In the past, I had suggested to pkasting@ that we could eliminate this being a
class entirely, given that it's comprised of only static methods (it used by a
Singleton with affinity). Peter wasn't a fan of it then, because he was
considering refactoring/cleanup and would prefer an instance/singleton, but that
was ~1.5 years ago.

I would almost see this be a namespace with a set of functions -
net::public_suffix_list - that callers could then alias in their .cc files if
necessary as "namespace psl = net::public_suffix_list" - or perhaps
net::tld_registry. I'm partial to PSL myself, but can see the desire to have
tlds as well.

I realize that's a much bigger change here, in that it requires refactoring all
the callers, but because you're changing the public signatures of these methods,
you're already having to do that, so this might be the right time to switch.

WDYT?

https://codereview.chromium.org/13979002/diff/291026/net/tools/tld_cleanup/tl...
File net/tools/tld_cleanup/tld_cleanup_util.cc (right):

https://codereview.chromium.org/13979002/diff/291026/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup_util.cc:25: bool WriteRules(const RuleMap&
rules, base::FilePath* outfile) {
why did you change this to a pointer, instead of a ConstRef, given that it's
still an input-only parameter?

[sky, 2013-04-26 20:02:27]

LGTM

https://codereview.chromium.org/13979002/diff/291026/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/13979002/diff/291026/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_input.cc:280: size_t tld_length =
const

https://codereview.chromium.org/13979002/diff/291026/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://codereview.chromium.org/13979002/diff/291026/chrome/browser/autocompl...
chrome/browser/autocomplete/history_quick_provider.cc:268: size_t
registry_length =
const

https://codereview.chromium.org/13979002/diff/291026/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/13979002/diff/291026/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider.cc:800: size_t registry_length
=
const

[Pam (message me for reviews), 2013-04-29 09:25:55]

https://codereview.chromium.org/13979002/diff/291026/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/13979002/diff/291026/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_input.cc:280: size_t tld_length =
On 2013/04/26 20:02:27, sky wrote:
> const

Interesting. It's clear what that does, but it's not customary to use 'const'
like this (that is to say, I don't think I've ever seen it before, other than
just above). What are the advantages, and why would one choose to do it (add
'const') or not?

https://codereview.chromium.org/13979002/diff/291026/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain.h (right):

https://codereview.chromium.org/13979002/diff/291026/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.h:128: typedef
RegistryControlledDomainService RCDS;
On 2013/04/26 19:39:59, Ryan Sleevi wrote:
> So I realize pam@ made the suggestion that this is likely permissible in the
> style guide, because it's "similar" to a namespace alias (ala
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...
> ), but that style guide includes an important note:
> 
> "Note that an alias in a .h file is visible to everyone #including that file,
so
> public headers (those available outside a project) and headers transitively
> #included by them, should avoid defining aliases, as part of the general goal
of
> keeping public APIs as small as possible."
> 
> This file is a "public" header of the net/ API, and so using an alias here
> really is not desirable.
> 
> In the past, I had suggested to pkasting@ that we could eliminate this being a
> class entirely, given that it's comprised of only static methods (it used by a
> Singleton with affinity). Peter wasn't a fan of it then, because he was
> considering refactoring/cleanup and would prefer an instance/singleton, but
that
> was ~1.5 years ago.
> 
> I would almost see this be a namespace with a set of functions -
> net::public_suffix_list - that callers could then alias in their .cc files if
> necessary as "namespace psl = net::public_suffix_list" - or perhaps
> net::tld_registry. I'm partial to PSL myself, but can see the desire to have
> tlds as well.

Any intrinsic considerations aside, I wouldn't change the namespace to PSL with
"RegistryControlledDomain" and its ilk used everywhere else.

> I realize that's a much bigger change here, in that it requires refactoring
all
> the callers, but because you're changing the public signatures of these
methods,
> you're already having to do that, so this might be the right time to switch.
> 
> WDYT?

https://codereview.chromium.org/13979002/diff/291026/net/tools/tld_cleanup/tl...
File net/tools/tld_cleanup/tld_cleanup_util.cc (right):

https://codereview.chromium.org/13979002/diff/291026/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup_util.cc:25: bool WriteRules(const RuleMap&
rules, base::FilePath* outfile) {
On 2013/04/26 19:39:59, Ryan Sleevi wrote:
> why did you change this to a pointer, instead of a ConstRef, given that it's
> still an input-only parameter?

Probably my fault, for not reading closely enough. Sorry about that.

[sky, 2013-04-29 16:08:30]

https://codereview.chromium.org/13979002/diff/291026/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/13979002/diff/291026/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_input.cc:280: size_t tld_length =
On 2013/04/29 09:25:56, Pam wrote:
> On 2013/04/26 20:02:27, sky wrote:
> > const
> 
> Interesting. It's clear what that does, but it's not customary to use 'const'
> like this (that is to say, I don't think I've ever seen it before, other than
> just above). What are the advantages, and why would one choose to do it (add
> 'const') or not?

http://www.corp.google.com/eng/doc/cppguide.xml#Use_of_const

const makers it clear the value isn't going to change for the life of the
function. I personally find this very helpful when looking at random function as
a way to know a particular value doesn't change. In this particular case it
isn't that useful since the scope of tld_length is very narrow. But just as with
scoped_ptr things change over time.

The use of const inside of functions varies drastically based on the author of
the file. Peter strongly favors const. You  can see its use in other similar
places in the code here (267/268...) which is why I only pointed this out in
files that tend to use const like this.

[Ryan Sleevi, 2013-05-06 21:23:12]

Thanks again for taking this on, Tommy.

net/ LGTM, with a few style nits and a request for one (small) shuffling change
either pre-commit or as a followup.

https://codereview.chromium.org/13979002/diff/414003/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain.cc (right):

https://codereview.chromium.org/13979002/diff/414003/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:66:
FindDomainPtr g_find_domain_function = Perfect_Hash::FindDomain;
nit: newline here before the }

https://codereview.chromium.org/13979002/diff/414003/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:70:
PrivateRegistryFilter filter);
style nit: indentation is wrong
style nit: Before you commit, rather than going the forward decl route, just
move these to be static & within the unnamed namespace. Keeping them as you have
is good - because it makes it easier to diff.

Or if you want to do this in a follow-up CL, that works - just trying to keep
them together.

https://codereview.chromium.org/13979002/diff/414003/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:74:
PrivateRegistryFilter filter);
style nit: indentation.

https://codereview.chromium.org/13979002/diff/414003/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:77:
std::string GetDomainAndRegistry(
Please remove the occurrences of "// static" in this file, as they're no longer
applicable.

https://codereview.chromium.org/13979002/diff/414003/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:156:
g_find_domain_function = function ? function : Perfect_Hash::FindDomain;
nit: This "conflicts" with line 66, in that if you ever changed line 66, you
could get in a weird state here.

const FindDomainPtr kDefaultFindDomainFunction = Perfect_Hash::FindDomain;
FindDomainPtr g_find_domain_function = kDefaultFindDOmainFunction;

...
g_find_domain_function = (function ? function : kDefaultFindDomainFunction);

https://codereview.chromium.org/13979002/diff/414003/net/base/static_cookie_p...
File net/base/static_cookie_policy.cc (right):

https://codereview.chromium.org/13979002/diff/414003/net/base/static_cookie_p...
net/base/static_cookie_policy.cc:28: OK : ERR_ACCESS_DENIED;
style nit: +4 spaces

https://codereview.chromium.org/13979002/diff/414003/net/base/static_cookie_p...
net/base/static_cookie_policy.cc:51: OK : ERR_ACCESS_DENIED;
style nit: +4 spaces

[nyquist, 2013-05-06 22:30:56]

brettw: PTAL at content/
pam/rsleevi: PTAL at your parts

Addressed all known comments, except "move forward declared methods to anonymous
namespace just before commit" in
net/base/registry_controlled_domains/registry_controlled_domain.cc

https://codereview.chromium.org/13979002/diff/291026/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://codereview.chromium.org/13979002/diff/291026/chrome/browser/autocompl...
chrome/browser/autocomplete/history_quick_provider.cc:268: size_t
registry_length =
On 2013/04/26 20:02:27, sky wrote:
> const

Done.

https://codereview.chromium.org/13979002/diff/291026/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/13979002/diff/291026/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider.cc:800: size_t registry_length
=
On 2013/04/26 20:02:27, sky wrote:
> const

Done.

https://codereview.chromium.org/13979002/diff/291026/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain.cc (right):

https://codereview.chromium.org/13979002/diff/291026/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:73: const
GURL& gurl, PrivateRegistryFilter filter) {
On 2013/04/26 19:39:59, Ryan Sleevi wrote:
> According to clang-format & chromium style, this should go on a newline.

Done.

https://codereview.chromium.org/13979002/diff/291026/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:84: const
std::string& host, PrivateRegistryFilter filter) {
On 2013/04/26 19:39:59, Ryan Sleevi wrote:
> same

Done.

https://codereview.chromium.org/13979002/diff/291026/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:94: const
GURL& gurl1, const GURL& gurl2, PrivateRegistryFilter filter) {
On 2013/04/26 19:39:59, Ryan Sleevi wrote:
> rinse repeat one param per line throughout.

Done.

https://codereview.chromium.org/13979002/diff/291026/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain.h (right):

https://codereview.chromium.org/13979002/diff/291026/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.h:128: typedef
RegistryControlledDomainService RCDS;
Changed to namespace, but used the same name as the folder:
registry_controlled_domains.

Also, no aliasing is currently in use.

On 2013/04/29 09:25:56, Pam wrote:
> On 2013/04/26 19:39:59, Ryan Sleevi wrote:
> > So I realize pam@ made the suggestion that this is likely permissible in the
> > style guide, because it's "similar" to a namespace alias (ala
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...
> > ), but that style guide includes an important note:
> > 
> > "Note that an alias in a .h file is visible to everyone #including that
file,
> so
> > public headers (those available outside a project) and headers transitively
> > #included by them, should avoid defining aliases, as part of the general
goal
> of
> > keeping public APIs as small as possible."
> > 
> > This file is a "public" header of the net/ API, and so using an alias here
> > really is not desirable.
> > 
> > In the past, I had suggested to pkasting@ that we could eliminate this being
a
> > class entirely, given that it's comprised of only static methods (it used by
a
> > Singleton with affinity). Peter wasn't a fan of it then, because he was
> > considering refactoring/cleanup and would prefer an instance/singleton, but
> that
> > was ~1.5 years ago.
> > 
> > I would almost see this be a namespace with a set of functions -
> > net::public_suffix_list - that callers could then alias in their .cc files
if
> > necessary as "namespace psl = net::public_suffix_list" - or perhaps
> > net::tld_registry. I'm partial to PSL myself, but can see the desire to have
> > tlds as well.
> 
> Any intrinsic considerations aside, I wouldn't change the namespace to PSL
with
> "RegistryControlledDomain" and its ilk used everywhere else.
> 
> > I realize that's a much bigger change here, in that it requires refactoring
> all
> > the callers, but because you're changing the public signatures of these
> methods,
> > you're already having to do that, so this might be the right time to switch.
> > 
> > WDYT?
>

https://codereview.chromium.org/13979002/diff/291026/net/tools/tld_cleanup/tl...
File net/tools/tld_cleanup/tld_cleanup_util.cc (right):

https://codereview.chromium.org/13979002/diff/291026/net/tools/tld_cleanup/tl...
net/tools/tld_cleanup/tld_cleanup_util.cc:25: bool WriteRules(const RuleMap&
rules, base::FilePath* outfile) {
On 2013/04/29 09:25:56, Pam wrote:
> On 2013/04/26 19:39:59, Ryan Sleevi wrote:
> > why did you change this to a pointer, instead of a ConstRef, given that it's
> > still an input-only parameter?
> 
> Probably my fault, for not reading closely enough. Sorry about that.

Done.

https://codereview.chromium.org/13979002/diff/414003/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain.cc (right):

https://codereview.chromium.org/13979002/diff/414003/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:66:
FindDomainPtr g_find_domain_function = Perfect_Hash::FindDomain;
On 2013/05/06 21:23:13, Ryan Sleevi wrote:
> nit: newline here before the }

Done.

https://codereview.chromium.org/13979002/diff/414003/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:70:
PrivateRegistryFilter filter);
On 2013/05/06 21:23:13, Ryan Sleevi wrote:
> style nit: indentation is wrong
> style nit: Before you commit, rather than going the forward decl route, just
> move these to be static & within the unnamed namespace. Keeping them as you
have
> is good - because it makes it easier to diff.
> 
> Or if you want to do this in a follow-up CL, that works - just trying to keep
> them together.

Done with indentation.
Will move methods up before committing.

https://codereview.chromium.org/13979002/diff/414003/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:74:
PrivateRegistryFilter filter);
On 2013/05/06 21:23:13, Ryan Sleevi wrote:
> style nit: indentation.

Done.

https://codereview.chromium.org/13979002/diff/414003/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:77:
std::string GetDomainAndRegistry(
On 2013/05/06 21:23:13, Ryan Sleevi wrote:
> Please remove the occurrences of "// static" in this file, as they're no
longer
> applicable.

Done.

https://codereview.chromium.org/13979002/diff/414003/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:156:
g_find_domain_function = function ? function : Perfect_Hash::FindDomain;
On 2013/05/06 21:23:13, Ryan Sleevi wrote:
> nit: This "conflicts" with line 66, in that if you ever changed line 66, you
> could get in a weird state here.
> 
> const FindDomainPtr kDefaultFindDomainFunction = Perfect_Hash::FindDomain;
> FindDomainPtr g_find_domain_function = kDefaultFindDOmainFunction;
> 
> ...
> g_find_domain_function = (function ? function : kDefaultFindDomainFunction);

Done.

https://codereview.chromium.org/13979002/diff/414003/net/base/static_cookie_p...
File net/base/static_cookie_policy.cc (right):

https://codereview.chromium.org/13979002/diff/414003/net/base/static_cookie_p...
net/base/static_cookie_policy.cc:28: OK : ERR_ACCESS_DENIED;
On 2013/05/06 21:23:13, Ryan Sleevi wrote:
> style nit: +4 spaces

Done.

https://codereview.chromium.org/13979002/diff/414003/net/base/static_cookie_p...
net/base/static_cookie_policy.cc:51: OK : ERR_ACCESS_DENIED;
On 2013/05/06 21:23:13, Ryan Sleevi wrote:
> style nit: +4 spaces

Done.

[brettw, 2013-05-07 21:08:28]

content lgtm

https://codereview.chromium.org/13979002/diff/433001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/13979002/diff/433001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:1756:
request->first_party_for_cookies(), request->url(),
I thought the previous indenting (4 more spaces) made more sense here.

https://codereview.chromium.org/13979002/diff/433001/net/cookies/cookie_util.cc
File net/cookies/cookie_util.cc (right):

https://codereview.chromium.org/13979002/diff/433001/net/cookies/cookie_util....
net/cookies/cookie_util.cc:27: if (scheme == "http" || scheme == "https")
Nit: need {} here since content is multi-line.

[nyquist, 2013-05-07 21:53:57]

addressed nits from brettw

https://codereview.chromium.org/13979002/diff/433001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/13979002/diff/433001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:1756:
request->first_party_for_cookies(), request->url(),
On 2013/05/07 21:08:28, brettw wrote:
> I thought the previous indenting (4 more spaces) made more sense here.

Done.

https://codereview.chromium.org/13979002/diff/433001/net/cookies/cookie_util.cc
File net/cookies/cookie_util.cc (right):

https://codereview.chromium.org/13979002/diff/433001/net/cookies/cookie_util....
net/cookies/cookie_util.cc:27: if (scheme == "http" || scheme == "https")
On 2013/05/07 21:08:28, brettw wrote:
> Nit: need {} here since content is multi-line.

Done.

[Pam (message me for reviews), 2013-05-08 13:23:09]

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/autocompl...
chrome/browser/autocomplete/history_quick_provider.cc:268: size_t
registry_length =
Still no 'const'

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/content_s...
File chrome/browser/content_settings/local_shared_objects_container.cc (right):

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/content_s...
chrome/browser/content_settings/local_shared_objects_container.cc:90: if
(net::registry_controlled_domains::SameDomainOrHost(
This pattern shows up a lot in this file. Consider pulling it out into a helper
function in an anonymous namespace?

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/google/go...
File chrome/browser/google/google_util.cc (right):

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/google/go...
chrome/browser/google/google_util.cc:182:
net::registry_controlled_domains::GetRegistryLength(
This line can move up

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/intranet_...
File chrome/browser/intranet_redirect_detector.cc (right):

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/intranet_...
chrome/browser/intranet_redirect_detector.cc:144: origin : GURL();
This is hard to read. Split the SameDomainOrHost result into a local variable?

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/ui_manager.cc (right):

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/safe_brow...
chrome/browser/safe_browsing/ui_manager.cc:282:
net::registry_controlled_domains::GetDomainAndRegistry(
This is really hard to read. Consider splitting the RCDS call into a local
variable, or at least indent this piece 4 more, as a tiny step toward sanity.

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/ui/toolba...
File chrome/browser/ui/toolbar/back_forward_menu_model.cc (right):

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/back_forward_menu_model.cc:374:
controller.GetEntryAtIndex(i)->GetURL(),
I'd indent these 4 more

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/back_forward_menu_model.cc:385:
controller.GetEntryAtIndex(i)->GetURL(),
Ditto

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/ui/web_co...
File chrome/browser/ui/web_contents_modal_dialog_manager.cc (right):

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/ui/web_co...
chrome/browser/ui/web_contents_modal_dialog_manager.cc:137:
details.previous_url, details.entry->GetURL(),
Also here 4 more

https://codereview.chromium.org/13979002/diff/438014/chrome_frame/ready_mode/...
File chrome_frame/ready_mode/ready_mode.cc (right):

https://codereview.chromium.org/13979002/diff/438014/chrome_frame/ready_mode/...
chrome_frame/ready_mode/ready_mode.cc:163: GURL(url),
Indent 2 more, please

https://codereview.chromium.org/13979002/diff/438014/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain.cc (right):

https://codereview.chromium.org/13979002/diff/438014/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:71:
std::string GetDomainAndRegistryImpl(const std::string& host,
These two functions will move up into the anonymous namespace, right?

https://codereview.chromium.org/13979002/diff/438014/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:78:
std::string GetDomainAndRegistry(
Does this now fit all on one line? (Also check all the other functions below.)

https://codereview.chromium.org/13979002/diff/438014/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain.h (right):

https://codereview.chromium.org/13979002/diff/438014/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.h:126: namespace
registry_controlled_domains {
The diff is useless here, but I'm assuming all that changed since last review is
moving from a class to a namespace.

https://codereview.chromium.org/13979002/diff/438014/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc
(right):

https://codereview.chromium.org/13979002/diff/438014/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc:32:
return GetRegistryLength(
nit: Move the first param up and align the rest with it; i.e.,

return GetRegistryLength(GURL(url),
                         unknown_filter,
                         EXCLUDE_PRIVATE_REGISTRIES);

https://codereview.chromium.org/13979002/diff/438014/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc:41:
return GetRegistryLength(
Ditto

https://codereview.chromium.org/13979002/diff/438014/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc:135:
EXPECT_EQ(2U, GetRegistryLengthFromURL(
These should be formatted with the argument moved up and the next line aligned
too, if they'll fit.

[Pam (message me for reviews), 2013-05-08 13:28:38]

Oh, and please add bug 37436 to the BUG line too.

Thanks,
- Pam

[nyquist, 2013-05-08 22:16:59]

pam: PTAL. Addressed your comments in #33, and moved the last two methods into
an anonymous namespace in #34.

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/autocompl...
chrome/browser/autocomplete/history_quick_provider.cc:268: size_t
registry_length =
On 2013/05/08 13:23:09, Pam wrote:
> Still no 'const'

Done.

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/content_s...
File chrome/browser/content_settings/local_shared_objects_container.cc (right):

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/content_s...
chrome/browser/content_settings/local_shared_objects_container.cc:90: if
(net::registry_controlled_domains::SameDomainOrHost(
On 2013/05/08 13:23:09, Pam wrote:
> This pattern shows up a lot in this file. Consider pulling it out into a
helper
> function in an anonymous namespace?

Done.

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/google/go...
File chrome/browser/google/google_util.cc (right):

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/google/go...
chrome/browser/google/google_util.cc:182:
net::registry_controlled_domains::GetRegistryLength(
On 2013/05/08 13:23:09, Pam wrote:
> This line can move up

Done.

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/intranet_...
File chrome/browser/intranet_redirect_detector.cc (right):

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/intranet_...
chrome/browser/intranet_redirect_detector.cc:144: origin : GURL();
On 2013/05/08 13:23:09, Pam wrote:
> This is hard to read. Split the SameDomainOrHost result into a local variable?

Done.

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/ui_manager.cc (right):

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/safe_brow...
chrome/browser/safe_browsing/ui_manager.cc:282:
net::registry_controlled_domains::GetDomainAndRegistry(
On 2013/05/08 13:23:09, Pam wrote:
> This is really hard to read. Consider splitting the RCDS call into a local
> variable, or at least indent this piece 4 more, as a tiny step toward sanity.

Done. Added an inner return instead. I am assuming that the optimized compiled
code will be the same.

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/ui/toolba...
File chrome/browser/ui/toolbar/back_forward_menu_model.cc (right):

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/back_forward_menu_model.cc:374:
controller.GetEntryAtIndex(i)->GetURL(),
On 2013/05/08 13:23:09, Pam wrote:
> I'd indent these 4 more

Done.

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/back_forward_menu_model.cc:385:
controller.GetEntryAtIndex(i)->GetURL(),
On 2013/05/08 13:23:09, Pam wrote:
> Ditto

Done.

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/ui/web_co...
File chrome/browser/ui/web_contents_modal_dialog_manager.cc (right):

https://codereview.chromium.org/13979002/diff/438014/chrome/browser/ui/web_co...
chrome/browser/ui/web_contents_modal_dialog_manager.cc:137:
details.previous_url, details.entry->GetURL(),
On 2013/05/08 13:23:09, Pam wrote:
> Also here 4 more

Done.

https://codereview.chromium.org/13979002/diff/438014/chrome_frame/ready_mode/...
File chrome_frame/ready_mode/ready_mode.cc (right):

https://codereview.chromium.org/13979002/diff/438014/chrome_frame/ready_mode/...
chrome_frame/ready_mode/ready_mode.cc:163: GURL(url),
On 2013/05/08 13:23:09, Pam wrote:
> Indent 2 more, please

Done.

https://codereview.chromium.org/13979002/diff/438014/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain.cc (right):

https://codereview.chromium.org/13979002/diff/438014/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:71:
std::string GetDomainAndRegistryImpl(const std::string& host,
On 2013/05/08 13:23:09, Pam wrote:
> These two functions will move up into the anonymous namespace, right?

Yes, I will do that in a separate patch after this.

https://codereview.chromium.org/13979002/diff/438014/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:78:
std::string GetDomainAndRegistry(
On 2013/05/08 13:23:09, Pam wrote:
> Does this now fit all on one line? (Also check all the other functions below.)

No, it does not. 83 characters :-/
Checked all the other functions too.

https://codereview.chromium.org/13979002/diff/438014/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain.h (right):

https://codereview.chromium.org/13979002/diff/438014/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.h:126: namespace
registry_controlled_domains {
On 2013/05/08 13:23:09, Pam wrote:
> The diff is useless here, but I'm assuming all that changed since last review
is
> moving from a class to a namespace.

Yes, sadly it does not seem like Rietveld has whitespace ignoration support...

https://codereview.chromium.org/13979002/diff/438014/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc
(right):

https://codereview.chromium.org/13979002/diff/438014/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc:32:
return GetRegistryLength(
On 2013/05/08 13:23:09, Pam wrote:
> nit: Move the first param up and align the rest with it; i.e.,
> 
> return GetRegistryLength(GURL(url),
>                          unknown_filter,
>                          EXCLUDE_PRIVATE_REGISTRIES);

Done.

https://codereview.chromium.org/13979002/diff/438014/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc:41:
return GetRegistryLength(
On 2013/05/08 13:23:09, Pam wrote:
> Ditto

Done.

https://codereview.chromium.org/13979002/diff/438014/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc:135:
EXPECT_EQ(2U, GetRegistryLengthFromURL(
On 2013/05/08 13:23:09, Pam wrote:
> These should be formatted with the argument moved up and the next line aligned
> too, if they'll fit.

Done.

[Pam (message me for reviews), 2013-05-10 09:33:24]

LGTM, with formatting nits described below fixed.

Thanks for all your work getting this done.

- Pam

https://codereview.chromium.org/13979002/diff/483001/chrome/browser/content_s...
File chrome/browser/content_settings/local_shared_objects_container.cc (right):

https://codereview.chromium.org/13979002/diff/483001/chrome/browser/content_s...
chrome/browser/content_settings/local_shared_objects_container.cc:21: namespace
{
nit: blank line after namespace {

https://codereview.chromium.org/13979002/diff/483001/chrome/browser/content_s...
chrome/browser/content_settings/local_shared_objects_container.cc:30: }
nit: blank line before } closing a namespace

https://codereview.chromium.org/13979002/diff/483001/chrome/browser/content_s...
chrome/browser/content_settings/local_shared_objects_container.cc:101: if
(SameDomainOrHost(origin, domain_url))
Consider renaming this something like SamePublicDomainOrHost, just to make the
EXCLUDE_PRIVATE_REGISTRIES (and the fact that this isn't calling the RCDS
method) explicit. It's up to you whether to make this change.

https://codereview.chromium.org/13979002/diff/483001/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc
(right):

https://codereview.chromium.org/13979002/diff/483001/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc:174:
EXPECT_EQ(0U, GetRegistryLengthFromURL(
These can be moved up to align after the ( too, like in the block above, and
also all but a couple of the other lines that are changing in this patch.

https://codereview.chromium.org/13979002/diff/493001/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain.cc (right):

https://codereview.chromium.org/13979002/diff/493001/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:153:
std::string GetDomainAndRegistryImpl(
Again a useless diff (and most of these lines don't even have whitespace changes
-- what are you thinking, Rietveld?!), but I'm assuming it's just moving methods
into the anon namespace, with no other changes.

[Pam (message me for reviews), 2013-05-10 19:21:21]

Still LGTM. One comment correction.

https://codereview.chromium.org/13979002/diff/520004/chrome/browser/content_s...
File chrome/browser/content_settings/local_shared_objects_container.cc (right):

https://codereview.chromium.org/13979002/diff/520004/chrome/browser/content_s...
chrome/browser/content_settings/local_shared_objects_container.cc:23: // Helper
wrapper for net::registry_controlled_domains::SamePublicDomainOrHost
This one should stay "SameDomainOrHost"

[nyquist, 2013-05-10 20:43:36]

sorry. forgot to publish these comments earlier.

https://codereview.chromium.org/13979002/diff/483001/chrome/browser/content_s...
File chrome/browser/content_settings/local_shared_objects_container.cc (right):

https://codereview.chromium.org/13979002/diff/483001/chrome/browser/content_s...
chrome/browser/content_settings/local_shared_objects_container.cc:21: namespace
{
On 2013/05/10 09:33:24, Pam wrote:
> nit: blank line after namespace {

Done.

https://codereview.chromium.org/13979002/diff/483001/chrome/browser/content_s...
chrome/browser/content_settings/local_shared_objects_container.cc:30: }
On 2013/05/10 09:33:24, Pam wrote:
> nit: blank line before } closing a namespace

Done.

https://codereview.chromium.org/13979002/diff/483001/chrome/browser/content_s...
chrome/browser/content_settings/local_shared_objects_container.cc:101: if
(SameDomainOrHost(origin, domain_url))
On 2013/05/10 09:33:24, Pam wrote:
> Consider renaming this something like SamePublicDomainOrHost, just to make the
> EXCLUDE_PRIVATE_REGISTRIES (and the fact that this isn't calling the RCDS
> method) explicit. It's up to you whether to make this change.

Done.

https://codereview.chromium.org/13979002/diff/483001/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc
(right):

https://codereview.chromium.org/13979002/diff/483001/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc:174:
EXPECT_EQ(0U, GetRegistryLengthFromURL(
On 2013/05/10 09:33:24, Pam wrote:
> These can be moved up to align after the ( too, like in the block above, and
> also all but a couple of the other lines that are changing in this patch.

Done.

https://codereview.chromium.org/13979002/diff/493001/net/base/registry_contro...
File net/base/registry_controlled_domains/registry_controlled_domain.cc (right):

https://codereview.chromium.org/13979002/diff/493001/net/base/registry_contro...
net/base/registry_controlled_domains/registry_controlled_domain.cc:153:
std::string GetDomainAndRegistryImpl(
On 2013/05/10 09:33:24, Pam wrote:
> Again a useless diff (and most of these lines don't even have whitespace
changes
> -- what are you thinking, Rietveld?!), but I'm assuming it's just moving
methods
> into the anon namespace, with no other changes.

Yes.

I moved the methods up to the anon namespace, reordered them (because of their
internal dependencies), and removed the forward declaration of them outside of
the namespace.

[nyquist, 2013-05-10 20:53:19]

All done at this point.
Will run try-jobs and if all is good, will commit it manually, since this review
is missing one gigantic auto-generated file
(net/base/registry_controlled_domains/effective_tld_names.cc).

https://codereview.chromium.org/13979002/diff/520004/chrome/browser/content_s...
File chrome/browser/content_settings/local_shared_objects_container.cc (right):

https://codereview.chromium.org/13979002/diff/520004/chrome/browser/content_s...
chrome/browser/content_settings/local_shared_objects_container.cc:23: // Helper
wrapper for net::registry_controlled_domains::SamePublicDomainOrHost
On 2013/05/10 19:21:21, Pam wrote:
> This one should stay "SameDomainOrHost"

Done.

[nyquist, 2013-05-13 18:13:10]

Committed patchset #39 manually as r199771 (presubmit successful).