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

chrome/browser/browsing_data/browsing_data_remover_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/browsing_data/browsing_data_remover.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <set>
@@ skipping 10 matching lines @@
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "build/build_config.h"
 #include "chrome/browser/autofill/personal_data_manager_factory.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #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"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/password_manager/password_store_factory.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/autofill/core/browser/autofill_profile.h"
@@ skipping 215 matching lines @@
 
   StoragePartitionRemovalData storage_partition_removal_data_;
 
   DISALLOW_COPY_AND_ASSIGN(TestStoragePartition);
 };
 
 // Custom matcher to verify is-same-origin relationship to given reference
 // 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)
       : reference_(reference) {}
 
   virtual bool MatchAndExplain(const url::Origin& origin,
                                MatchResultListener* listener) const {
     return reference_.IsSameOriginWith(origin);
   }
 
   virtual void DescribeTo(::std::ostream* os) const {
     *os << "is same origin with " << reference_;
   }
 
   virtual void DescribeNegationTo(::std::ostream* os) const {
     *os << "is not same origin with " << reference_;
   }
 
  private:
   const url::Origin& reference_;
 };
 
 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 -------------------------------------------------------------------
 
 class RemoveCookieTester {
  public:
   RemoveCookieTester() {}
 
   // Returns true, if the given cookie exists in the cookie store.
   bool ContainsCookie() {
@@ skipping 1861 matching lines @@
 // monitor case again.
 TEST_F(BrowsingDataRemoverTest, DISABLED_DomainReliability_NoMonitor) {
   BlockUntilBrowsingDataRemoved(
       BrowsingDataRemover::EVERYTHING,
       BrowsingDataRemover::REMOVE_HISTORY |
       BrowsingDataRemover::REMOVE_COOKIES, false);
 }
 
 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);
 }
 
 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);
 }
 
 TEST_F(BrowsingDataRemoverTest, RemovePasswordStatistics) {
   RemovePasswordsTester tester(GetProfile());
 
   EXPECT_CALL(*tester.store(), RemoveStatisticsCreatedBetweenImpl(
                                    base::Time(), base::Time::Max()));
@@ skipping 14 matching lines @@
 TEST_F(BrowsingDataRemoverTest, RemovePasswordsByOrigin) {
   RemovePasswordsTester tester(GetProfile());
   const url::Origin expectedOrigin(kOrigin1);
 
   EXPECT_CALL(*tester.store(),
               RemoveLoginsByOriginAndTimeImpl(SameOrigin(expectedOrigin), _, _))
       .WillOnce(Return(password_manager::PasswordStoreChangeList()));
   BlockUntilOriginDataRemoved(BrowsingDataRemover::EVERYTHING,
                               BrowsingDataRemover::REMOVE_PASSWORDS, kOrigin1);
 }