Ask download manager to remove a download if it is overwritten by another

chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc

Index: chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc
diff --git a/chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc b/chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc
index 902dbe00145d0c38ef02ba56e5c3cc79ee1ed97d..6c343e5e8a18d90ae5d5be7c5fc0ece846d3480f 100644
--- a/chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc
+++ b/chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc
@@ -13,24 +13,38 @@
 #include "base/memory/ptr_util.h"
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/android/download/download_controller.h"
+#include "chrome/browser/android/download/download_manager_service.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/ui/android/infobars/download_overwrite_infobar.h"
 #include "components/infobars/core/infobar.h"
+#include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/web_contents.h"
 
 namespace {
 
+void DeleteExistingDownloadFileDone(
+    bool is_off_the_record,
+    const base::Closure& callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DownloadManagerService::GetInstance()->CheckForExternallyRemovedDownloads(

[Theresa, 2016/09/15 03:28:30]

The check for externally removed downloads doesn't save state about which files
were externally deleted (right?), so this isn't guaranteed to remove overwritten
file.

e.g.
1. Download file & choose to overwrite an existing file
2. File is deleted, marked as externally removed in temporary state associated
with download item [i]
3. New file is created
4. Chrome is closed and removed from Android recents
5. Chrome is restarted
6. Downloads UI is opened, check for externally deleted files is kicked off

In #6, entries for both of the download items will still exist in the back end.
Since the externally deleted state isn't saved and the file path associated with
the download items still exists (it was created in #3), neither of the download
items will be marked as externally deleted and both will appear in the UI.

Also, the check for externally deleted downloads queues up each file on the
sequenced task runner [ii]. If downloading the file doesn't happen on the same
task runner, we could run into a race condition where the new file is downloaded
and the file path exists before the externally removed check is executed.

One idea for an alternative is to query the download history for items with the
same path as the file being downloaded and remove them if matches are found.

[i]
https://cs.chromium.org/chromium/src/content/browser/download/download_item_i...

[ii]
https://cs.chromium.org/chromium/src/chrome/browser/download/chrome_download_...

[qinmin, 2016/09/15 22:18:56]

I believe CheckForExternallyRemovedDownloads() does save the state.

DownloadHistory inherits from AllDownloadItemNotifier::Observer, as a result,
DownloadHistory::OnDownloadUpdated() will get called whenever
DownloadItemImpl::OnDownloadedFileRemoved() gets called. That will update the
stored download history data, and will be reinstated when browser is
closed/restarted.

There is a possibility that querying the history can take longer than the new
file gets downloaded. However, in ShouldUpdateHistory() method in
download_history.cc,  it compares a lot of information between the old file and
the new file (bytes, last modified, etc). Unless the new file can carry the same
info as the old file, which is very unlikely, this should not cause any race
conditions.

[Theresa, 2016/09/15 22:44:09]

When I was adding the checks for externally downloaded items to do the styling
(before we removed them from the UI entirely), the list of externally removed
items had to be recalculated every time the history service was started.

I don't think the data for externally removed is saved in the database. Here's
the database call for retrieving the history row:
https://cs.chromium.org/chromium/src/chrome/browser/download/download_history...

Here's what the history row contains: 
  DownloadRow(const base::FilePath& current_path,
              const base::FilePath& target_path,
              const std::vector<GURL>& url_chain,
              const GURL& referrer_url,
              const GURL& site_url,
              const GURL& tab_url,
              const GURL& tab_referrer_url,
              const std::string& http_method,
              const std::string& mime_type,
              const std::string& original_mime_type,
              const base::Time& start,
              const base::Time& end,
              const std::string& etag,
              const std::string& last_modified,
              int64_t received,
              int64_t total,
              DownloadState download_state,
              DownloadDangerType danger_type,
              DownloadInterruptReason interrupt_reason,
              const std::string& hash,
              DownloadId id,
              const std::string& guid,
              bool download_opened,
              const std::string& ext_id,
              const std::string& ext_name);




This should be easy to test - with the patch installed, download a file twice,
overwriting the second time. Close Chrome in Android recents to kill the
process. Restart Chrome and go to Downloads. If there's only one entry that's
great! If there are two then the externally removed state isn't getting
propagated.

[qinmin, 2016/09/15 23:14:00]

Yes, I have tested this patch and it works as intended.
Here is what I saw:
1. Download a file. 
2. Go to Downloads Home:
   I saw DownloadManager->GetAllDownloads() return 1 item
3. Download the same file again, choose overwrite
4. Go to Downloads Home:
   DownloadManager->GetAllDownloads() now returns 2 item,
   1 of them GetFileExternallyRemoved() == true
5. Swiping chrome away from recent apps
6. restart Chrome, go to Downloads Home
   DownloadManager->GetAllDownloads() now only returns 1 item, 
   and item->GetFileExternallyRemoved() == false.

Looks like DownloadManager somehow keeps a snapshot of all the downloads, which
may not be equal to what is stored in the history DB. When Chrome restarts,
DownloadManager will restore what's actually stored in the history db.

[Theresa, 2016/09/15 23:50:32]

Yes, that's what I expected. In #5 step, the download item is getting removed by
the Java code in Downloads Home because it sees that it has been externally
removed. If you skip #5 and go straight to #5, what happens in #6? That's the
case I'm concerned about.

[qinmin, 2016/09/16 00:14:59]

Thanks, Theresa. Yes, you are correct. The deletion was caused by our java code,
rather than DownloadHistory.
If I don't open Download home, then the entry will be left over there after
closing/restart chrome.
Updated the CL to remove completed download with the same target_path when we
overwrite a file.

+      nullptr, nullptr, is_off_the_record);
+  callback.Run();
+}
+
 void DeleteExistingDownloadFile(
     const base::FilePath& download_path,
+    bool is_off_the_record,
     const DownloadTargetDeterminerDelegate::FileSelectedCallback& callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
   base::File::Info info;
   if (GetFileInfo(download_path, &info) && !info.is_directory)
     base::DeleteFile(download_path, false);
 
-  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
-                                   base::Bind(callback, download_path));
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&DeleteExistingDownloadFileDone, is_off_the_record,
+                 base::Bind(callback, download_path)));
 }
 
 void CreateNewFileDone(
@@ -89,9 +103,13 @@ ChromeDownloadManagerOverwriteInfoBarDelegate::GetIdentifier() const {
 }
 
 bool ChromeDownloadManagerOverwriteInfoBarDelegate::OverwriteExistingFile() {
+  if (!download_item_)
+    return true;
+
   content::BrowserThread::PostTask(
       content::BrowserThread::FILE, FROM_HERE,
       base::Bind(&DeleteExistingDownloadFile, suggested_download_path_,
+                 download_item_->GetBrowserContext()->IsOffTheRecord(),
                  file_selected_callback_));
   return true;
 }