Add removal filter support for Cookies, Storage, and Content Settings.

chrome/browser/browsing_data/browsing_data_remover.cc

Index: chrome/browser/browsing_data/browsing_data_remover.cc
diff --git a/chrome/browser/browsing_data/browsing_data_remover.cc b/chrome/browser/browsing_data/browsing_data_remover.cc
index 1fab3eaf2f266882fc8a34409f5144a20b76c0e0..b2e2b5685f303a737f0a2dd3e795d5814d5fa84f 100644
--- a/chrome/browser/browsing_data/browsing_data_remover.cc
+++ b/chrome/browser/browsing_data/browsing_data_remover.cc
@@ -158,10 +158,20 @@ base::Callback<void(T)> IgnoreArgument(const base::Closure& callback) {
 // Helper to create callback for BrowsingDataRemover::DoesOriginMatchMask.
 bool DoesOriginMatchMask(

[msramek, 2016/02/29 17:46:07]

nit: DoesOriginMatchMaskAndUrls?

[dmurph, 2016/03/01 00:09:59]

Done.

     int origin_type_mask,
+    const base::Callback<bool(const GURL&)>& predicate,

[msramek, 2016/02/29 17:46:07]

nit: Can you please generally refer to this as |url_filter|, like in my previous
CL, for consistency? Unless I'm missing some nuance, filter and predicate are
synonymous in this context. I would also emphasize that it's a filter for URLs,
since 'predicate' or 'filter' in itself is very vague.

[dmurph, 2016/03/01 00:10:00]

Predicate implies that if you return true, then the action happens. Filter does
not, and confused me a lot. Thus I'd rather use predicate. Let me know if you
don't like that, it just means I would want to put comments every that say "true
means remove".

[msramek, 2016/03/01 17:24:09]

I don't particularly mind the word 'predicate', but the inconsistency. The class
is named OriginFilterBuilder, so what it produces should be called a filter.
Though I admit that filter could be interpreted as "true means it survives the
deletion".

We can keep 'predicate' in standalone methods like this that are not strongly
related to OriginFilterBuilder, but I would still call it at least
|url_predicate|. This reads to me as "int number;" or "std::vector<T> vector;".

[michaeln, 2016/03/09 20:25:51]

naming nit: How about origin_matcher_fn? Also if this truly is about origin
rather than url matching, should we consider using the url::Origin datatype
here?

[msramek, 2016/03/10 20:10:08]

Origin matching is also not exact, since sometimes it's origin+subdomains.

The idea is to use a URL->boolean predicate, so that each backend can simply ask
"should I delete this URL?" and doesn't have to know about url::Origin.

The OriginFilterBuilder then supplies special kinds of URL->boolean predicates
that just happen to have an equivalence relation over URLs of the same origin,
or origin+subdomains. Origin->boolean is still just a special case of
URL->boolean.

I understand that in storage partition, it's going to be messy, since items are
stored per origin there, so it doesn't make sense to delete google.com/a and not
google.com/b. But for example in history or download manager, we could easily
delete URLs matching an arbitrary predicate.

     const GURL& origin,
     storage::SpecialStoragePolicy* special_storage_policy) {
-  return BrowsingDataHelper::DoesOriginMatchMask(
-      origin, origin_type_mask, special_storage_policy);
+  return predicate.Run(origin) &&
+         BrowsingDataHelper::DoesOriginMatchMask(origin, origin_type_mask,
+                                                 special_storage_policy);
+}
+
+bool PrimaryPatternGURLPredicate(

[Timo Reimann, 2016/02/29 22:38:15]

I think the function name could be improved. Prefixing it by some descriptive
verb would help.

[dmurph, 2016/03/01 00:09:59]

Done.

+    const base::Callback<bool(const GURL&)> predicate,
+    const ContentSettingsPattern& primary_pattern,
+    const ContentSettingsPattern& secondary_pattern) {

[Timo Reimann, 2016/02/29 22:38:15]

|secondary_pattern| doesn't seem to be used in this function?

[dmurph, 2016/03/01 00:09:59]

Nope. I need to convert from the two patterns to the predicate. See my comment
below. I'll implement a new callback for this.

+  GURL url(primary_pattern.ToString());
+  return !url.is_valid() || predicate.Run(url);

[Mike West, 2016/02/29 09:27:38]

When can an invalid URL appear here? Why would an invalid URL be treated as
matching?

[msramek, 2016/02/29 17:46:07]

Heh, this is quite tricky - if I have a content setting pattern such as
"https://*", and I remove the origin "https://google.com", should we remove the
pattern, because it matches the origin, or not, since it has nothing to do with
the origin specifically?

Fortunately, we're dealing here with APP_BANNER and SITE_ENGAGEMENT, which are
website settings, and not content settings, so users cannot manually input
complex primary patterns, they don't use secondary patterns, and the default
setting [*,*] doesn't exist.

SITE_ENGAGEMENT is always stored per origin, APP_BANNER per domain (plus
subdomains, i.e. *://[*.]domain:*). So for SITE_ENGAGEMENT we can do exact
origin matching, for APP_BANNER we should probably delete the entry when any
origin with that domain as host is deleted.

In both cases, ContentSettingsPattern::Matches is probably what you want to use.

Furthermore, please document that this method only makes sense with the
mentioned constraints (the primary pattern is always origin or host, and the
secondary pattern is wildcard).

[dmurph, 2016/03/01 00:10:00]

Unfortunately, I can't use |Matches|, as I'm not given a GURL. Instead, I need
to pass the GURL to the predicate. Please advise on how to correctly do this. 
Maybe the Filter class should support bool(ContentSettingsPattern) callback
generation? I did this, tell me what you think.

 }
 
 void ClearHostnameResolutionCacheOnIOThread(IOThread* io_thread) {
@@ -326,12 +336,21 @@ void BrowsingDataRemover::SetRemoving(bool is_removing) {
 void BrowsingDataRemover::Remove(const TimeRange& time_range,
                                  int remove_mask,
                                  int origin_type_mask) {
-  RemoveImpl(time_range, remove_mask, GURL(), origin_type_mask);
+  OriginFilterBuilder builder(OriginFilterBuilder::BLACKLIST);
+  RemoveImpl(time_range, remove_mask, builder, origin_type_mask);
+}
+
+void BrowsingDataRemover::RemoveWithFilter(const TimeRange& time_range,
+                      int remove_mask,
+                      int origin_type_mask,
+                      const OriginFilterBuilder& origin_filter) {

[msramek, 2016/02/29 17:46:07]

This is actually a filter builder, not a filter. I would perhaps also argue that
we should pass the filter itself, but that depends on where do we want to make
the decision between exact origin vs subdomains, and I'm afraid we still haven't
fleshed that out.

[dmurph, 2016/03/01 00:10:00]

I understand. For now I was just passing the builder, as we might need to
generate different callbacks. (See comment above, where we might want to have
the builder give us another callback that could accept ContentSettingsPatterns).

[msramek, 2016/03/01 17:24:09]

Acknowledged. Yes, after adding ContentSettingPattern, we'll have to pass the
builder.

+  RemoveImpl(time_range, remove_mask, origin_filter,
+                                  origin_type_mask);
 }
 
 void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
                                      int remove_mask,
-                                     const GURL& remove_url,
+                                     const OriginFilterBuilder& origin_filter,
                                      int origin_type_mask) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ -345,18 +364,8 @@ void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
   remove_mask_ = remove_mask;
   origin_type_mask_ = origin_type_mask;
 
-  // TODO(msramek): Replace |remove_origin| with |filter| in all backends.
-  const url::Origin remove_origin(remove_url);
-  OriginFilterBuilder builder(OriginFilterBuilder::BLACKLIST);
-  if (!remove_url.is_empty()) {
-    // Make sure that only URLs representing origins, with no extra components,
-    // are passed to this class.
-    DCHECK_EQ(remove_url, remove_url.GetOrigin());
-    builder.SetMode(OriginFilterBuilder::WHITELIST);
-    builder.AddOrigin(url::Origin(remove_origin));
-  }
   base::Callback<bool(const GURL& url)> same_origin_filter =
-      builder.BuildSameOriginFilter();
+      origin_filter.BuildSameOriginFilter();
 
   PrefService* prefs = profile_->GetPrefs();
   bool may_delete_history = prefs->GetBoolean(
@@ -392,20 +401,11 @@ void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
         HistoryServiceFactory::GetForProfile(
             profile_, ServiceAccessType::EXPLICIT_ACCESS);
     if (history_service) {
-      // Selective history deletion is currently done through HistoryUI ->
-      // HistoryBackend -> HistoryService, and that is for individual URLs,
-      // not origins. The code below is currently unused, as the only callsite
-      // supplying |remove_url| is the unittest.
-      // TODO(msramek): Make it possible to delete history per origin, not just
-      // per URL, and use that functionality here.
-      std::set<GURL> restrict_urls;
-      if (!remove_url.is_empty())
-        restrict_urls.insert(remove_url);
+      // TODO(dmurph): Support all backends with filter (crbug.com/589586).
       content::RecordAction(UserMetricsAction("ClearBrowsingData_History"));
       waiting_for_clear_history_ = true;
-
       history_service->ExpireLocalAndRemoteHistoryBetween(
-          WebHistoryServiceFactory::GetForProfile(profile_), restrict_urls,
+          WebHistoryServiceFactory::GetForProfile(profile_), std::set<GURL>(),

[Mike West, 2016/02/29 09:27:38]

If you're going to remove this mechanism in favor of the origin filter, I'd
prefer not to leave vestiges of it throughout the codebase. What's the plan for
replacing/updating this API?

[dmurph, 2016/03/01 00:10:00]

See the bug. This API is NOT in use anywhere except for tests for this class.
This initial patch adds support for all of the backends that I need to unblock
the "Important Sites" CBD feature, and then I want to follow up when I'm
unblocked w/ the rest of the backends. Is this alright?

           delete_begin_, delete_end_,
           base::Bind(&BrowsingDataRemover::OnHistoryDeletionDone,
                      weak_ptr_factory_.GetWeakPtr()),
@@ -415,7 +415,8 @@ void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
       // The extension activity contains details of which websites extensions
       // were active on. It therefore indirectly stores details of websites a
       // user has visited so best clean from here as well.
-      extensions::ActivityLog::GetInstance(profile_)->RemoveURLs(restrict_urls);
+      extensions::ActivityLog::GetInstance(profile_)->RemoveURLs(
+          std::set<GURL>());

[Mike West, 2016/02/29 09:27:38]

Ditto.

[dmurph, 2016/03/01 00:09:59]

See above.

 #endif
     }
 
@@ -428,6 +429,7 @@ void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
 
     // The power consumption history by origin contains details of websites
     // that were visited.
+    // TODO(dmurph): Support all backends with filter (crbug.com/589586).
     power::OriginPowerMap* origin_power_map =
         power::OriginPowerMapFactory::GetForBrowserContext(profile_);
     if (origin_power_map)
@@ -437,6 +439,7 @@ void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
     // reveals some history: we have no mechanism to track when these items were
     // created, so we'll clear them all. Better safe than sorry.
     if (g_browser_process->io_thread()) {
+      // TODO(dmurph): Support all backends with filter (crbug.com/589586).
       waiting_for_clear_hostname_resolution_cache_ = true;
       BrowserThread::PostTaskAndReply(
           BrowserThread::IO, FROM_HERE,
@@ -446,6 +449,7 @@ void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
                      weak_ptr_factory_.GetWeakPtr()));
     }
     if (profile_->GetNetworkPredictor()) {
+      // TODO(dmurph): Support all backends with filter (crbug.com/589586).
       waiting_for_clear_network_predictor_ = true;
       BrowserThread::PostTaskAndReply(
           BrowserThread::IO, FROM_HERE,
@@ -458,6 +462,7 @@ void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
     // As part of history deletion we also delete the auto-generated keywords.
     TemplateURLService* keywords_model =
         TemplateURLServiceFactory::GetForProfile(profile_);
+
     if (keywords_model && !keywords_model->loaded()) {
       template_url_sub_ = keywords_model->RegisterOnLoadedCallback(
           base::Bind(&BrowsingDataRemover::OnKeywordsLoaded,
@@ -466,7 +471,7 @@ void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
       waiting_for_clear_keyword_data_ = true;
     } else if (keywords_model) {
       keywords_model->RemoveAutoGeneratedForOriginBetween(
-          remove_url, delete_begin_, delete_end_);
+          GURL(), delete_begin_, delete_end_);

[Mike West, 2016/02/29 09:27:38]

Ditto.

[dmurph, 2016/03/01 00:09:59]

See above.

     }
 
     // The PrerenderManager keeps history of prerendered pages, so clear that.
@@ -476,6 +481,7 @@ void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
     prerender::PrerenderManager* prerender_manager =
         prerender::PrerenderManagerFactory::GetForProfile(profile_);
     if (prerender_manager) {
+      // TODO(dmurph): Support all backends with filter (crbug.com/589586).
       prerender_manager->ClearData(
           prerender::PrerenderManager::CLEAR_PRERENDER_CONTENTS |
           prerender::PrerenderManager::CLEAR_PRERENDER_HISTORY);
@@ -483,7 +489,7 @@ void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
 
     // If the caller is removing history for all hosts, then clear ancillary
     // historical information.
-    if (remove_url.is_empty()) {
+    if (origin_filter.IsEmptyBlacklist()) {

[msramek, 2016/02/29 17:46:07]

optional: Remove or RemoveWithFilter could remember whether or not a filter was
passed, then we can remove this method. It's a pity that we can't directly test
the base::Callback itself for emptiness.

       // We also delete the list of recently closed tabs. Since these expire,
       // they can't be more than a day old, so we can simply clear them all.
       sessions::TabRestoreService* tab_service =
@@ -505,6 +511,7 @@ void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
     // The saved Autofill profiles and credit cards can include the origin from
     // which these profiles and credit cards were learned.  These are a form of
     // history, so clear them as well.
+    // TODO(dmurph): Support all backends with filter (crbug.com/589586).
     scoped_refptr<autofill::AutofillWebDataService> web_data_service =
         WebDataServiceFactory::GetAutofillWebDataForProfile(
             profile_, ServiceAccessType::EXPLICIT_ACCESS);
@@ -603,6 +610,7 @@ void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
     // doesn't make sense to apply the time period of deleting in the last X
     // hours/days to the safebrowsing cookies since they aren't the result of
     // any user action.
+    // TODO(dmurph): Support all backends with filter (crbug.com/589586).
     if (delete_begin_ == base::Time()) {
       safe_browsing::SafeBrowsingService* sb_service =
           g_browser_process->safe_browsing_service();
@@ -610,6 +618,7 @@ void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
         scoped_refptr<net::URLRequestContextGetter> sb_context =
             sb_service->url_request_context();
         ++waiting_for_clear_cookies_count_;
+        // TODONOW

[Mike West, 2016/02/29 09:27:38]

?

[dmurph, 2016/03/01 00:10:00]

Haha I need to impl this still.

         BrowserThread::PostTask(
             BrowserThread::IO, FROM_HERE,
             base::Bind(&ClearCookiesOnIOThread, delete_begin_, delete_end_,
@@ -698,14 +707,19 @@ void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
 
   if (remove_mask & REMOVE_SITE_USAGE_DATA) {
     HostContentSettingsMapFactory::GetForProfile(profile_)
-        ->ClearSettingsForOneType(CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT);
+        ->ClearSettingsForOneTypeWithPredicate(
+            CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT,
+            base::Bind(&PrimaryPatternGURLPredicate, same_origin_filter));
   }
 
   if (remove_mask & REMOVE_SITE_USAGE_DATA || remove_mask & REMOVE_HISTORY) {
     HostContentSettingsMapFactory::GetForProfile(profile_)
-        ->ClearSettingsForOneType(CONTENT_SETTINGS_TYPE_APP_BANNER);
+        ->ClearSettingsForOneTypeWithPredicate(
+            CONTENT_SETTINGS_TYPE_APP_BANNER,
+            base::Bind(&PrimaryPatternGURLPredicate, same_origin_filter));
   }
 
+  // TODO(dmurph): Support all backends with filter (crbug.com/589586).
   if (remove_mask & REMOVE_PASSWORDS) {
     content::RecordAction(UserMetricsAction("ClearBrowsingData_Passwords"));
     password_manager::PasswordStore* password_store =
@@ -717,13 +731,8 @@ void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
       auto on_cleared_passwords =
           base::Bind(&BrowsingDataRemover::OnClearedPasswords,
                      weak_ptr_factory_.GetWeakPtr());
-      if (remove_url.is_empty()) {
-        password_store->RemoveLoginsCreatedBetween(delete_begin_, delete_end_,
-                                                   on_cleared_passwords);
-      } else {
-        password_store->RemoveLoginsByOriginAndTime(

[Mike West, 2016/02/29 09:27:38]

Ditto.

[dmurph, 2016/03/01 00:09:59]

See above comment.

-            remove_origin, delete_begin_, delete_end_, on_cleared_passwords);
-      }
+      password_store->RemoveLoginsCreatedBetween(delete_begin_, delete_end_,
+                                                 on_cleared_passwords);
     }
   }
 
@@ -741,6 +750,7 @@ void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
     }
   }
 
+  // TODO(dmurph): Support all backends with filter (crbug.com/589586).
   if (remove_mask & REMOVE_FORM_DATA) {
     content::RecordAction(UserMetricsAction("ClearBrowsingData_Autofill"));
     scoped_refptr<autofill::AutofillWebDataService> web_data_service =
@@ -845,9 +855,9 @@ void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
     }
 
     storage_partition->ClearData(
-        storage_partition_remove_mask, quota_storage_remove_mask, remove_url,
-        base::Bind(&DoesOriginMatchMask, origin_type_mask_), delete_begin_,
-        delete_end_,
+        storage_partition_remove_mask, quota_storage_remove_mask, GURL(),

[Mike West, 2016/02/29 09:27:38]

It looks like you're passing in the filter below, which means you don't need
this |remove_url| argument anymore. Why not remove it from the API entirely?

[dmurph, 2016/03/01 00:09:59]

I can do that, it hits a couple other areas in the code, so I created a bug for
the followup CL:
https://bugs.chromium.org/p/chromium/issues/detail?id=590891

I believe we're the last client of the 3rd argument, so this shouldn't be too
bad, just removing that argument for a bunch of callsites.

+        base::Bind(&DoesOriginMatchMask, origin_type_mask_, same_origin_filter),
+        delete_begin_, delete_end_,
         base::Bind(&BrowsingDataRemover::OnClearedStoragePartitionData,
                    weak_ptr_factory_.GetWeakPtr()));
   }
@@ -927,8 +937,10 @@ void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
       offline_pages::IsOfflinePagesEnabled()) {
     waiting_for_clear_offline_page_data_ = true;
     offline_pages::OfflinePageModelFactory::GetForBrowserContext(profile_)
-        ->ClearAll(base::Bind(&BrowsingDataRemover::OnClearedOfflinePageData,
-                              weak_ptr_factory_.GetWeakPtr()));
+        ->ClearWithURLPredicate(
+            same_origin_filter,
+            base::Bind(&BrowsingDataRemover::OnClearedOfflinePageData,
+                       weak_ptr_factory_.GetWeakPtr()));
   }
 #endif