Support restricting browsing data removal for downloads by origin.

content/browser/download/download_manager_impl.h

 // 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.
 
 #ifndef CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_MANAGER_IMPL_H_
 #define CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_MANAGER_IMPL_H_
 
 #include <map>
 #include <set>
 #include <string>
@@ skipping 48 matching lines @@
 
   // DownloadManager functions.
   void SetDelegate(DownloadManagerDelegate* delegate) override;
   DownloadManagerDelegate* GetDelegate() const override;
   void Shutdown() override;
   void GetAllDownloads(DownloadVector* result) override;
   void StartDownload(
       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;
   void DownloadUrl(scoped_ptr<DownloadUrlParameters> params) override;
   void AddObserver(Observer* observer) override;
   void RemoveObserver(Observer* observer) override;
   content::DownloadItem* CreateDownloadItem(
       uint32 id,
       const base::FilePath& current_path,
       const base::FilePath& target_path,
@@ skipping 55 matching lines @@
 
   // Get next download id. |callback| is called on the UI thread and may
   // be called synchronously.
   void GetNextId(const DownloadIdCallback& callback);
 
   // Called with the result of DownloadManagerDelegate::CheckForFileExistence.
   // Updates the state of the file and then notifies this update to the file's
   // 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,
                                const DownloadTargetCallback& callback) override;
   bool ShouldCompleteDownload(DownloadItemImpl* item,
                               const base::Closure& complete_callback) override;
   bool ShouldOpenFileBasedOnExtension(const base::FilePath& path) override;
   bool ShouldOpenDownload(DownloadItemImpl* item,
                           const ShouldOpenDownloadCallback& callback) override;
   void CheckForFileRemoval(DownloadItemImpl* download_item) override;
@@ skipping 33 matching lines @@
   net::NetLog* net_log_;
 
   base::WeakPtrFactory<DownloadManagerImpl> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadManagerImpl);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_MANAGER_IMPL_H_