Add an OriginFilterBuilder class for [white|black]listing origins.

chrome/browser/browsing_data/browsing_data_remover_unittest.cc

Index: chrome/browser/browsing_data/browsing_data_remover_unittest.cc
diff --git a/chrome/browser/browsing_data/browsing_data_remover_unittest.cc b/chrome/browser/browsing_data/browsing_data_remover_unittest.cc
index 6aa8694afc017c6a6d4d4fb8118bdd2c45c2f804..b3bfc5c4878668761ce1d53d30b73f6bdf8214ac 100644
--- a/chrome/browser/browsing_data/browsing_data_remover_unittest.cc
+++ b/chrome/browser/browsing_data/browsing_data_remover_unittest.cc
@@ -28,6 +28,7 @@
 #include "chrome/browser/browsing_data/browsing_data_helper.h"
 #include "chrome/browser/browsing_data/browsing_data_remover_factory.h"
 #include "chrome/browser/browsing_data/browsing_data_remover_test_util.h"
+#include "chrome/browser/browsing_data/origin_filter_builder.h"
 #include "chrome/browser/domain_reliability/service_factory.h"
 #include "chrome/browser/download/chrome_download_manager_delegate.h"
 #include "chrome/browser/favicon/favicon_service_factory.h"
@@ -263,6 +264,9 @@ class TestStoragePartition : public StoragePartition {
 // origin.
 // (We cannot use equality-based matching because operator== is not defined for
 // Origin, and we in fact want to rely on IsSameOrigin for matching purposes.)
+// TODO(msramek): This is only used for backends that take url::Origin instead
+// of an url filter predicate to match URLs. Remove this when we fully switch
+// to url filter predicates.
 class SameOriginMatcher : public MatcherInterface<const url::Origin&> {
  public:
   explicit SameOriginMatcher(const url::Origin& reference)
@@ -289,6 +293,55 @@ inline Matcher<const url::Origin&> SameOrigin(const url::Origin& reference) {
   return MakeMatcher(new SameOriginMatcher(reference));
 }
 
+// Custom matcher to test the equivalence of two URL filters. Since those are
+// blackbox predicates, we can only approximate the equivalence by testing
+// whether the filter give the same answer for several URLs. This is currently
+// good enough for our testing purposes, to distinguish whitelists
+// and blacklists, empty and non-empty filters and such.
+// TODO(msramek): BrowsingDataRemover and some of its backends support URL
+// filters, but its constructor currently only takes a single URL and constructs
+// its own url filter. If an url filter was directly passed to
+// BrowsingDataRemover (what should eventually be the case), we can use the same
+// instance in the test as well, and thus simply test base::Callback::Equals()
+// in this matcher.
+class ProbablySameFilterMatcher
+    : public MatcherInterface<const base::Callback<bool(const GURL&)>&> {
+ public:
+  explicit ProbablySameFilterMatcher(
+      const base::Callback<bool(const GURL&)>& filter)
+      : to_match_(filter) {
+  }
+
+  virtual bool MatchAndExplain(const base::Callback<bool(const GURL&)>& filter,
+                               MatchResultListener* listener) const {
+    const GURL urls_to_test_[] =
+        {kOrigin1, kOrigin2, kOrigin3, GURL("invalid spec")};
+    for (GURL url : urls_to_test_) {
+      if (filter.Run(url) != to_match_.Run(url)) {
+        *listener << "The filters differ on the URL " << url;
+        return false;
+      }
+    }
+    return true;
+  }
+
+  virtual void DescribeTo(::std::ostream* os) const {
+    *os << "is probably the same url filter as " << &to_match_;
+  }
+
+  virtual void DescribeNegationTo(::std::ostream* os) const {
+    *os << "is definitely NOT the same url filter as " << &to_match_;
+  }
+
+ private:
+  const base::Callback<bool(const GURL&)>& to_match_;
+};
+
+inline Matcher<const base::Callback<bool(const GURL&)>&> ProbablySameFilter(
+    const base::Callback<bool(const GURL&)>& filter) {
+  return MakeMatcher(new ProbablySameFilterMatcher(filter));
+}
+
 }  // namespace
 
 // Testers -------------------------------------------------------------------
@@ -2170,8 +2223,12 @@ TEST_F(BrowsingDataRemoverTest, DISABLED_DomainReliability_NoMonitor) {
 
 TEST_F(BrowsingDataRemoverTest, RemoveDownloadsByTimeOnly) {
   RemoveDownloadsTester tester(GetProfile());
+  OriginFilterBuilder builder(OriginFilterBuilder::BLACKLIST);
+  base::Callback<bool(const GURL&)> filter = builder.BuildSameOriginFilter();

[Timo Reimann, 2016/02/11 22:46:43]

It takes a bit of cognitive effort to understand that we're using an empty,
black-listed origin filter in order to match URLs unconditionally. We could be
more clear about our intention by passing a simple always-true filter instead,
just like the EmptyFilter defined in download_manager_impl.cc.

It might even be worth to move that into OriginFilterBuilder so that it can be
reused.

[msramek, 2016/02/12 14:24:50]

I was originally thinking about having a tri-state mode enum { EMPTY, WHITELIST,
BLACKLIST } to avoid this cognitive effort, but then decided against it, as it
would be even more confusing if you do:

OriginFilterBuilder builder(EMPTY);
builder.AddOrigin(...);  // <- the mode is "EMPTY", but the filter
                         // is in fact not empty

Should AddOrigin(...) fail, or should the mode automatically flip to BLACKLIST?
It was starting to feel a bit overengineered to me.

It is also awkward that when constructing the filter in BrowsingDataRemover, we
actually have to start with blacklist and then flip to whitelist when one origin
is added. The builder pattern is intended for incremental construction of
objects, which this case really isn't, and if it wasn't the primary usecase, I
would be against including the ChangeMode() method.

But at least for convenience in tests, I guess a static method to generate an
empty filter is useful.

[Timo Reimann, 2016/02/15 07:39:15]

If flipping from "no filter"  (i.e., an empty black list) to a non-empty white
list (or even black list, for that matter) is really the primary use case, how
about just requiring clients of the API to start over with a new
OriginFilterBuilder once the first origin comes in? It'd be just as easy
syntactically and allow to remove the ChangeMode method.

The one downside I see is that you cannot constify the OriginFilterBuilder
instance anymore. However, to me that's a more explicit way of saying that the
instance may change  than to mutate its internal state. 

[msramek, 2016/02/18 12:48:42]

The builder looks much better now, and you're able to build more than one
filter. In that world, it's quite reasonable to add a few origins, build a
whitelist, then add a few more, then build a blacklist for the same set, then
add a few more and build another whitelist etc.

It currently doesn't seem to me that SetMode() doesn't fit into the model, so I
kept it.

 
-  EXPECT_CALL(*tester.download_manager(), RemoveDownloadsBetween(_, _));
+  EXPECT_CALL(
+      *tester.download_manager(),
+      RemoveDownloadsByURLAndTime(ProbablySameFilter(filter), _, _));
 
   BlockUntilBrowsingDataRemoved(BrowsingDataRemover::EVERYTHING,
                                 BrowsingDataRemover::REMOVE_DOWNLOADS, false);
@@ -2179,10 +2236,13 @@ TEST_F(BrowsingDataRemoverTest, RemoveDownloadsByTimeOnly) {
 
 TEST_F(BrowsingDataRemoverTest, RemoveDownloadsByOrigin) {
   RemoveDownloadsTester tester(GetProfile());
-  const url::Origin expectedOrigin(kOrigin1);
+  OriginFilterBuilder builder(OriginFilterBuilder::WHITELIST);
+  builder.AddOrigin(url::Origin(kOrigin1));
+  base::Callback<bool(const GURL&)> filter = builder.BuildSameOriginFilter();
 
-  EXPECT_CALL(*tester.download_manager(),
-              RemoveDownloadsByOriginAndTime(SameOrigin(expectedOrigin), _, _));
+  EXPECT_CALL(
+      *tester.download_manager(),
+      RemoveDownloadsByURLAndTime(ProbablySameFilter(filter), _, _));
 
   BlockUntilOriginDataRemoved(BrowsingDataRemover::EVERYTHING,
                               BrowsingDataRemover::REMOVE_DOWNLOADS, kOrigin1);