Support restricting browsing data removal for downloads by origin.

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 9f644d60c8b78fd27afff1a4738fd1896c86b4b7..9d6c25ea6f08cb20ad63b3ea7597f7b3af1a63f1 100644
--- a/chrome/browser/browsing_data/browsing_data_remover.cc
+++ b/chrome/browser/browsing_data/browsing_data_remover.cc
@@ -288,14 +288,15 @@ void BrowsingDataRemover::RemoveImpl(int remove_mask,
                                   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;
 
@@ -443,7 +444,9 @@ void BrowsingDataRemover::RemoveImpl(int remove_mask,
     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());