Move static RegistrableDomainFilterBuilder methods to an anonymous namespace

chrome/browser/browsing_data/registrable_domain_filter_builder.cc

Index: chrome/browser/browsing_data/registrable_domain_filter_builder.cc
diff --git a/chrome/browser/browsing_data/registrable_domain_filter_builder.cc b/chrome/browser/browsing_data/registrable_domain_filter_builder.cc
index 2fe4b83f356503b10cff17e06b7e5fd1bdfc3c91..37d747fa7ca3d3fd5b3b6f1b49de016dd3422295 100644
--- a/chrome/browser/browsing_data/registrable_domain_filter_builder.cc
+++ b/chrome/browser/browsing_data/registrable_domain_filter_builder.cc
@@ -35,6 +35,88 @@ bool IsSubdomainOfARegistrableDomain(const std::string& domain) {
 // 2. IsSubdomainOfARegistrableDomain(domain)      - e.g. www.google.com
 // 3. GetDomainAndRegistry(domain, _) == ""        - e.g. localhost, 127.0.0.1
 
+
+// True if the domain of |url| is in the whitelist, or isn't in the blacklist.
+// The whitelist or blacklist is represented as |registerable_domains|
+// and |mode|.
+bool MatchesURL(
+    const std::set<std::string>& registerable_domains,
+    BrowsingDataFilterBuilder::Mode mode,
+    const GURL& url) {
+  std::string url_registerable_domain =
+      GetDomainAndRegistry(url, INCLUDE_PRIVATE_REGISTRIES);
+  return (registerable_domains.find(
+              url_registerable_domain != "" ? url_registerable_domain
+                                            : url.host()) !=
+          registerable_domains.end()) ==
+         (mode == BrowsingDataFilterBuilder::WHITELIST);
+}
+
+// True if the pattern something in the whitelist, or doesn't match something
+// in the blacklist.
+// The whitelist or blacklist is represented as |patterns|,  and |mode|.
+bool MatchesWebsiteSettingsPattern(
+    const std::vector<ContentSettingsPattern>& domain_patterns,
+    BrowsingDataFilterBuilder::Mode mode,
+    const ContentSettingsPattern& pattern) {
+  for (const ContentSettingsPattern& domain : domain_patterns) {
+    DCHECK(domain.IsValid());
+    Relation relation = pattern.Compare(domain);
+    if (relation == Relation::IDENTITY || relation == Relation::PREDECESSOR)
+      return mode == BrowsingDataFilterBuilder::WHITELIST;
+  }
+  return mode != BrowsingDataFilterBuilder::WHITELIST;
+}
+
+// True if no domains can see the given cookie and we're a blacklist, or any
+// domains can see the cookie and we're a whitelist.
+// The whitelist or blacklist is represented as |domains_and_ips| and |mode|.
+bool MatchesCookieForRegisterableDomainsAndIPs(
+    const std::set<std::string>& domains_and_ips,
+    BrowsingDataFilterBuilder::Mode mode,
+    const net::CanonicalCookie& cookie) {
+  if (domains_and_ips.empty())
+    return mode == BrowsingDataFilterBuilder::BLACKLIST;
+  std::string cookie_domain = cookie.Domain();
+  if (cookie.IsDomainCookie())
+    cookie_domain = cookie_domain.substr(1);
+  std::string parsed_cookie_domain =
+      GetDomainAndRegistry(cookie_domain, INCLUDE_PRIVATE_REGISTRIES);
+  // This means we're an IP address or an internal hostname.
+  if (parsed_cookie_domain.empty())
+    parsed_cookie_domain = cookie_domain;
+  return (mode == BrowsingDataFilterBuilder::WHITELIST) ==
+      (domains_and_ips.find(parsed_cookie_domain) != domains_and_ips.end());
+}
+
+// True if none of the supplied domains matches this Channel ID's server ID
+// and we're a blacklist, or one of them does and we're a whitelist.
+// The whitelist or blacklist is represented as |domains_and_ips| and |mode|.
+bool MatchesChannelIDForRegisterableDomainsAndIPs(
+    const std::set<std::string>& domains_and_ips,
+    BrowsingDataFilterBuilder::Mode mode,
+    const std::string& channel_id_server_id) {
+  return ((mode == BrowsingDataFilterBuilder::WHITELIST) ==
+      (domains_and_ips.find(channel_id_server_id) != domains_and_ips.end()));
+}
+
+// True if none of the supplied domains matches this plugin's |site| and we're
+// a blacklist, or one of them does and we're a whitelist. The whitelist or
+// blacklist is represented by |domains_and_ips| and |mode|.
+bool MatchesPluginSiteForRegisterableDomainsAndIPs(
+    const std::set<std::string>& domains_and_ips,
+    BrowsingDataFilterBuilder::Mode mode,
+    const std::string& site) {
+  // If |site| is a third- or lower-level domain, find the corresponding eTLD+1.
+  std::string domain_or_ip =
+      GetDomainAndRegistry(site, INCLUDE_PRIVATE_REGISTRIES);
+  if (domain_or_ip.empty())
+    domain_or_ip = site;
+
+  return ((mode == BrowsingDataFilterBuilder::WHITELIST) ==
+      (domains_and_ips.find(domain_or_ip) != domains_and_ips.end()));
+}
+
 }  // namespace
 
 RegistrableDomainFilterBuilder::RegistrableDomainFilterBuilder(Mode mode)
@@ -53,17 +135,15 @@ void RegistrableDomainFilterBuilder::AddRegisterableDomain(
 
 base::Callback<bool(const GURL&)>
 RegistrableDomainFilterBuilder::BuildGeneralFilter() const {
-  std::set<std::string>* domains = new std::set<std::string>(domain_list_);
-  return base::Bind(&RegistrableDomainFilterBuilder::MatchesURL,
-                    base::Owned(domains), mode());
+  std::set<std::string> domains(domain_list_);
+  return base::Bind(MatchesURL, domains, mode());

[Bernhard Bauer, 2016/11/23 15:53:07]

This will copy the set. That could be fine if you're okay with it, but if you
want to avoid the copy, I think you'll need base::Passed(std::move(domains))
(the std::move() to move it into the callback, and base::Passed() to have the
callback move it when running).

[msramek, 2016/11/24 17:38:08]

The copy on the previous line (|domain_list_|->|domains|) should be a copy - the
Build*() methods should not destroy the Builder's internal state, as that would
be unexpected for the builder design pattern.

The copy of |domains| on this line can be a move, since |domains| is just a
local variable that's not used later.

We should not use base::Passed(), as the callback would let go of |domains| and
we could not call ::Run() twice, which we definitely do. But that's not a
problem - you can see that |domains| are bound to "const std::set<std::string>&"
in ::MatchesURL, so that's not an additional copy.

Since the first two operations are copy + move, I just got rid of the
intermediate step and pass |domain_list_| directly into base::Bind().

And since I am now confused why it's even called |domain_list_| when it's a set,
I renamed it to |domains_|.

[Bernhard Bauer, 2016/11/25 13:53:19]

Ok, that makes sense. Thanks!

Actually, now that you mention calling the callback repeatedly: should we go
ahead and use base::RepeatingCallback?

[msramek, 2016/11/25 17:03:43]

Well, I see three options:
1. Status quo. Don't be explicit, just rely on the fact that base::Callback and
base::RepeatingCallback are the same thing.
2. Change this to base::RepeatingCallback here, in BrowsingDataRemover, and all
BrowsingDataRemover's backends and their subclasses and modules and... could be
on the order of hundred callsites.
3. Change it to a RepeatingCallback just here.

I don't want to do #2, at least not now. According to
https://cs.chromium.org/chromium/src/base/bind.h?q=bind.h&sq=package:chromium...
it might be desirable at some point though.

From #1 and #3, #1 (status quo) seems better to me, because we're at least
consistently using the same type.

Although I can see an argument that if base::Callback ever changed in the future
not to be equal to base::RepeatingCallback, #3 would not compile, which might be
a good thing. So if you prefer, I can do that.

[Bernhard Bauer, 2016/11/25 17:18:31]

Yes, that's what I understand from
https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md.

I mean, you don't have to shave this particular yak in this CL, but all these
will have to be changed to RepeatingCallback eventually, so we might as well
start now.

[msramek, 2016/12/07 13:21:34]

Alright! I changed all base::Bind to base::BindRepeating, so that they create a
base::RepeatingCallback. However, the methods override the ones from
BrowsingDataFilterBuilder, so I can't change the signature without changing it
in at least 3 additional files as well, which I think doesn't belong to this CL.
I'll send a followup soon.

 }
 
 base::Callback<bool(const ContentSettingsPattern& pattern)>
 RegistrableDomainFilterBuilder
     ::BuildWebsiteSettingsPatternMatchesFilter() const {
-  std::vector<ContentSettingsPattern>* patterns_from_domains =
-      new std::vector<ContentSettingsPattern>();
-  patterns_from_domains->reserve(domain_list_.size());
+  std::vector<ContentSettingsPattern> patterns_from_domains;
+  patterns_from_domains.reserve(domain_list_.size());
 
   for (const std::string& domain : domain_list_) {
     std::unique_ptr<ContentSettingsPattern::BuilderInterface> builder(
@@ -75,46 +155,36 @@ RegistrableDomainFilterBuilder
     if (IsRegistrableDomain(domain))
       builder->WithDomainWildcard();
 
-    patterns_from_domains->push_back(builder->Build());
+    patterns_from_domains.push_back(builder->Build());
   }
 
-  for (const ContentSettingsPattern& domain : *patterns_from_domains) {
+  for (const ContentSettingsPattern& domain : patterns_from_domains) {
     DCHECK(domain.IsValid());
   }
 
-  return base::Bind(
-      &RegistrableDomainFilterBuilder::MatchesWebsiteSettingsPattern,
-      base::Owned(patterns_from_domains), mode());
+  return base::Bind(&MatchesWebsiteSettingsPattern,
+                    std::move(patterns_from_domains), mode());
 }
 
 base::Callback<bool(const net::CanonicalCookie& cookie)>
 RegistrableDomainFilterBuilder::BuildCookieFilter() const {
-  std::set<std::string>* domains_and_ips =
-      new std::set<std::string>(domain_list_);
-  return base::Bind(
-      &RegistrableDomainFilterBuilder
-          ::MatchesCookieForRegisterableDomainsAndIPs,
-      base::Owned(domains_and_ips), mode());
+  std::set<std::string> domains_and_ips(domain_list_);
+  return base::Bind(&MatchesCookieForRegisterableDomainsAndIPs,
+                    std::move(domains_and_ips), mode());
 }
 
 base::Callback<bool(const std::string& cookie)>
 RegistrableDomainFilterBuilder::BuildChannelIDFilter() const {
-  std::set<std::string>* domains_and_ips =
-      new std::set<std::string>(domain_list_);
-  return base::Bind(
-      &RegistrableDomainFilterBuilder
-          ::MatchesChannelIDForRegisterableDomainsAndIPs,
-      base::Owned(domains_and_ips), mode());
+  std::set<std::string> domains_and_ips(domain_list_);
+  return base::Bind(&MatchesChannelIDForRegisterableDomainsAndIPs,
+                    std::move(domains_and_ips), mode());
 }
 
 base::Callback<bool(const std::string& site)>
 RegistrableDomainFilterBuilder::BuildPluginFilter() const {
-  std::set<std::string>* domains_and_ips =
-      new std::set<std::string>(domain_list_);
-  return base::Bind(
-      &RegistrableDomainFilterBuilder
-          ::MatchesPluginSiteForRegisterableDomainsAndIPs,
-      base::Owned(domains_and_ips), mode());
+  std::set<std::string> domains_and_ips(domain_list_);
+  return base::Bind(&MatchesPluginSiteForRegisterableDomainsAndIPs,
+                    std::move(domains_and_ips), mode());
 }
 
 bool RegistrableDomainFilterBuilder::operator==(
@@ -125,76 +195,3 @@ bool RegistrableDomainFilterBuilder::operator==(
 bool RegistrableDomainFilterBuilder::IsEmpty() const {
   return domain_list_.empty();
 }
-
-// static
-bool RegistrableDomainFilterBuilder::MatchesURL(
-    std::set<std::string>* registerable_domains,
-    Mode mode,
-    const GURL& url) {
-  std::string url_registerable_domain =
-      GetDomainAndRegistry(url, INCLUDE_PRIVATE_REGISTRIES);
-  return (registerable_domains->find(
-              url_registerable_domain != "" ? url_registerable_domain
-                                            : url.host()) !=
-          registerable_domains->end()) ==
-         (mode == WHITELIST);
-}
-
-// static
-bool RegistrableDomainFilterBuilder::MatchesWebsiteSettingsPattern(
-    std::vector<ContentSettingsPattern>* domain_patterns,
-    Mode mode,
-    const ContentSettingsPattern& pattern) {
-  for (const ContentSettingsPattern& domain : *domain_patterns) {
-    DCHECK(domain.IsValid());
-    Relation relation = pattern.Compare(domain);
-    if (relation == Relation::IDENTITY || relation == Relation::PREDECESSOR)
-      return mode == WHITELIST;
-  }
-  return mode != WHITELIST;
-}
-
-// static
-bool RegistrableDomainFilterBuilder::MatchesCookieForRegisterableDomainsAndIPs(
-    std::set<std::string>* domains_and_ips,
-    Mode mode,
-    const net::CanonicalCookie& cookie) {
-  if (domains_and_ips->empty())
-    return mode == BLACKLIST;
-  std::string cookie_domain = cookie.Domain();
-  if (cookie.IsDomainCookie())
-    cookie_domain = cookie_domain.substr(1);
-  std::string parsed_cookie_domain =
-      GetDomainAndRegistry(cookie_domain, INCLUDE_PRIVATE_REGISTRIES);
-  // This means we're an IP address or an internal hostname.
-  if (parsed_cookie_domain.empty())
-    parsed_cookie_domain = cookie_domain;
-  return (mode == WHITELIST) == (domains_and_ips->find(parsed_cookie_domain) !=
-                                 domains_and_ips->end());
-}
-
-// static
-bool
-RegistrableDomainFilterBuilder::MatchesChannelIDForRegisterableDomainsAndIPs(
-    std::set<std::string>* domains_and_ips,
-    Mode mode,
-    const std::string& channel_id_server_id) {
-  return ((mode == WHITELIST) == (domains_and_ips->find(channel_id_server_id) !=
-                                  domains_and_ips->end()));
-}
-
-// static
-bool
-RegistrableDomainFilterBuilder::MatchesPluginSiteForRegisterableDomainsAndIPs(
-    std::set<std::string>* domains_and_ips,
-    Mode mode,
-    const std::string& site) {
-  // If |site| is a third- or lower-level domain, find the corresponding eTLD+1.
-  std::string domain_or_ip =
-      GetDomainAndRegistry(site, INCLUDE_PRIVATE_REGISTRIES);
-  if (domain_or_ip.empty())
-    domain_or_ip = site;
-
-  return ((mode == WHITELIST) == (domains_and_ips->find(domain_or_ip) !=
-                                  domains_and_ips->end()));
-}