Support restricting browsing data removal for downloads by origin.

chrome/browser/browsing_data/browsing_data_remover.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 <map>
 #include <set>
 #include <string>
 
@@ skipping 233 matching lines @@
 void BrowsingDataRemover::set_removing(bool is_removing) {
   DCHECK(is_removing_ != is_removing);
   is_removing_ = is_removing;
 }
 
 void BrowsingDataRemover::Remove(int remove_mask, int origin_type_mask) {
   RemoveImpl(remove_mask, GURL(), origin_type_mask);
 }
 
 void BrowsingDataRemover::RemoveImpl(int remove_mask,
                                      const GURL& origin,

[Mike West, 2015/07/22 20:16:03]

Please add a TODO to replace this with `url::Origin`.

                                      int origin_type_mask) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   set_removing(true);
   remove_mask_ = remove_mask;
   remove_origin_ = origin;
   origin_type_mask_ = origin_type_mask;
 
   PrefService* prefs = profile_->GetPrefs();
   bool may_delete_history = prefs->GetBoolean(
       prefs::kAllowDeletingBrowserHistory);
@@ skipping 16 matching lines @@
         UserMetricsAction("ClearBrowsingData_MaskContainsExtension"));
   }
   // If this fires, we added a new BrowsingDataHelper::OriginTypeMask without
   // updating the user metrics above.
   static_assert(
       BrowsingDataHelper::ALL == (BrowsingDataHelper::UNPROTECTED_WEB |
                                   BrowsingDataHelper::PROTECTED_WEB |
                                   BrowsingDataHelper::EXTENSION),
       "OriginTypeMask has been updated without updating user metrics");
 
+  std::set<GURL> restrict_urls;

[Mike West, 2015/07/22 20:16:03]

1. Why do we need a set here if we're adding a single URL?
2. Please use `url::Origin` for new code.
3. I know you're just moving this from it's current location, so you're not
responsible for the name, but it's bad. :) How about `origins_to_clear` or
something similar?

[Timo Reimann, 2015/07/22 23:22:15]

According to git history, the set was added back in 2010 and passed to the
history service unmodified
(https://chromium.googlesource.com/chromium/src/+/21f4d251210a4408da8a3279510e...).
My impression is that a set was chosen just to satisfy ExpireHistoryBetween()'s
contract easily.
One reason I can think of that would justify sticking to a set (or a container
in general) is that we may want to pass in multiple origins at one point. In
fact, the issue I plan to work on after #113621 (issue #78093) will require
passing in multiple origin filters in terms of match patterns, which seems to be
related to supporting multiple origins. WDYT?
Could you please show me where that is defined? I was only able to find a TODO
of yours saying "Land 'url::Origin'" here:
https://code.google.com/p/chromium/codesearch#chromium/src/url/scheme_host_po...
.
Glad you mention it, I actually wanted to raise the subject as well. I find it a
bit hard to properly name a container/variable whose non-empty state indicates
"remove just these" while the empty one should mean "remove all" (as opposed to
"remove none"). Cannot really come up with something better though, so let's go
with `origins_to_clear`.

Will keep your other comments touching set and url::Origin changes on hold until
we have resolved the two so that I can make any changes in one go.

[Mike West, 2015/07/23 09:56:49]

If there's a reasonable use case for multiple origins, great, keep them. In this
file, however, we're passing in a single origin, and that's pretty much it. Is
there a use case _here_ for a set of origins?

It's not clear why you'll need match patterns for 78093, but I suppose we can
talk about that when we get there. :)
I just landed it yesterday, so... :) It looks like CodeSearch hasn't caught up
yet; it's in `url/origin.h`, right next to `scheme_host_port.h`.

[Timo Reimann, 2015/07/23 23:11:07]

As of now, only for history::HistoryService::ExpireLocalAndRemoteHistoryBetween
which expects a set. Otherwise, there's no need. I tried to model things after
the history service but realize now that we can keep it simple.

I moved the create-and-populate-set logic back to where it was before and pass a
single origin into DownloadManager::RemoveDownloadsBetween.
Sure! For the sake of completeness, here's the API proposal document I put
together a while ago:
https://docs.google.com/document/d/17hWuwDVG7OOuEfLfA59CChbLZrbfeJ_yr_PY03XoL...
. Feel free to have a look.
Found it. Updated.

I also did a bit of renaming in BrowsingDataRemover to make sure that variable
names containing `origin` and `url` are actually Origin and GURL types,
respectively. Moreover, I removed the BrowsingDataRemover::remove_origin_ member
variable since it is never referenced outside of the
BrowsingDataRemover::RemoveImpl method, where it is redefined from the stack
immediately.

+  if (!remove_origin_.is_empty())
+    restrict_urls.insert(remove_origin_);
+
   if ((remove_mask & REMOVE_HISTORY) && may_delete_history) {
     history::HistoryService* history_service =
         HistoryServiceFactory::GetForProfile(
             profile_, ServiceAccessType::EXPLICIT_ACCESS);
     if (history_service) {
-      std::set<GURL> restrict_urls;
-      if (!remove_origin_.is_empty())
-        restrict_urls.insert(remove_origin_);
       content::RecordAction(UserMetricsAction("ClearBrowsingData_History"));
       waiting_for_clear_history_ = true;
 
       history_service->ExpireLocalAndRemoteHistoryBetween(
           WebHistoryServiceFactory::GetForProfile(profile_),
           restrict_urls, delete_begin_, delete_end_,
           base::Bind(&BrowsingDataRemover::OnHistoryDeletionDone,
                      base::Unretained(this)),
           &history_task_tracker_);
 
@@ skipping 127 matching lines @@
     // The SSL Host State that tracks SSL interstitial "proceed" decisions may
     // include origins that the user has visited, so it must be cleared.
     if (profile_->GetSSLHostStateDelegate())
       profile_->GetSSLHostStateDelegate()->Clear();
   }
 
   if ((remove_mask & REMOVE_DOWNLOADS) && may_delete_history) {
     content::RecordAction(UserMetricsAction("ClearBrowsingData_Downloads"));
     content::DownloadManager* download_manager =
         BrowserContext::GetDownloadManager(profile_);
-    download_manager->RemoveDownloadsBetween(delete_begin_, delete_end_);
+    download_manager->RemoveDownloadsBetween(restrict_urls,
+                                             delete_begin_,
+                                             delete_end_);
     DownloadPrefs* download_prefs = DownloadPrefs::FromDownloadManager(
         download_manager);
     download_prefs->SetSaveFilePath(download_prefs->DownloadPath());
   }
 
   uint32 storage_partition_remove_mask = 0;
 
   // We ignore the REMOVE_COOKIES request if UNPROTECTED_WEB is not set,
   // so that callers who request REMOVE_SITE_DATA with PROTECTED_WEB
   // don't accidentally remove the cookies that are associated with the
@@ skipping 648 matching lines @@
   waiting_for_clear_domain_reliability_monitor_ = false;
   NotifyAndDeleteIfDone();
 }
 
 // static
 BrowsingDataRemover::CallbackSubscription
     BrowsingDataRemover::RegisterOnBrowsingDataRemovedCallback(
         const BrowsingDataRemover::Callback& callback) {
   return GetOnBrowsingDataRemovedCallbacks()->Add(callback);
 }