Support restricting browsing data removal for downloads by origin.

content/browser/download/download_manager_impl.h

Index: content/browser/download/download_manager_impl.h
diff --git a/content/browser/download/download_manager_impl.h b/content/browser/download/download_manager_impl.h
index 9c9d12deec392ed29fd7bfc6de1bf0d674214022..281db7d1929f11646ac045a31e44fc583911b70d 100644
--- a/content/browser/download/download_manager_impl.h
+++ b/content/browser/download/download_manager_impl.h
@@ -66,7 +66,8 @@ class CONTENT_EXPORT DownloadManagerImpl : public DownloadManager,
       scoped_ptr<DownloadCreateInfo> info,
       scoped_ptr<ByteStreamReader> stream,
       const DownloadUrlParameters::OnStartedCallback& on_started) override;
-  int RemoveDownloadsBetween(base::Time remove_begin,
+  int RemoveDownloadsBetween(const std::set<GURL>& restricted_urls,

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

Rename here as well (and I still don't understand why this is a set).

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

Renamed and replaced the set by Origin.

+                             base::Time remove_begin,
                              base::Time remove_end) override;
   int RemoveDownloads(base::Time remove_begin) override;
   int RemoveAllDownloads() override;
@@ -142,6 +143,11 @@ class CONTENT_EXPORT DownloadManagerImpl : public DownloadManager,
   // observer.
   void OnFileExistenceChecked(uint32 download_id, bool result);
 
+  // Determines if the given URL is among the set of restricted URLs. This is
+  // required for origin-specific removal of downloads.
+  bool IsRestrictedUrl(const GURL& url,

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

1. You can move this to an static function in an anonymous namespace in the .cc
file. No need to expose it on the header.

2. If you change from 'restricted', you could rename this to something like
'IsRemovableURL' and simply return `true` if the set is empty rather than
DCHECKing.

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

Done.
Had it like this at one point. Not really sure anymore why I changed it. Think I
found it to be more consistent with the other checks in
DownloadManagerImpl::RemoveDownloadsBetween, but design-wise I actually prefer
hiding the logic behind the function as well.
Will update as soon as we're clear on the set-vs-no-set matter.

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

Renamed and reimplemented the method. I use the unique (empty) origin to
represent the "clear all origins" state, and otherwise check for same origin. I
considered using pointers as well to define origin absence, but the unique state
approach seemed more appropriate to me. LMK what you think.

[Mike West, 2015/07/24 06:56:39]

This worries me a bit; we eventually want to build some sort of "Forget This
Site" button somewhere into the UI. If a user clicked that button on a sandboxed
site, I'd like to make sure we don't delete _all her data ever_. For the moment,
this is fine, but please add a comment to the `.h` file explaining the behavior,
with a `// TODO(mkwst): Reconsider this behavior if/when we build a "forget this
site" feature.` comment.

+                       const std::set<GURL>& restricted_urls);
+
   // Overridden from DownloadItemImplDelegate
   // (Note that |GetBrowserContext| are present in both interfaces.)
   void DetermineDownloadTarget(DownloadItemImpl* item,