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

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

When a downloaded file is overwritten by another,
we should ask DownloadManager to check for externally removed files.

BUG=647042
Committed: https://crrev.com/db0d8c5689f4a4d9150ee42f181ef2f9f5af17bc
Cr-Commit-Position: refs/heads/master@{#420381}

[qinmin, 2016-09-14 22:07:47]

qinmin@chromium.org changed reviewers:
+ dfalcantara@chromium.org, twellington@chromium.org

[qinmin, 2016-09-14 22:07:48]

PTAL

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

https://codereview.chromium.org/2344483003/diff/1/chrome/browser/android/down...
File
chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc
(right):

https://codereview.chromium.org/2344483003/diff/1/chrome/browser/android/down...
chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc:30:
DownloadManagerService::GetInstance()->CheckForExternallyRemovedDownloads(
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:57]

https://codereview.chromium.org/2344483003/diff/1/chrome/browser/android/down...
File
chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc
(right):

https://codereview.chromium.org/2344483003/diff/1/chrome/browser/android/down...
chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc:30:
DownloadManagerService::GetInstance()->CheckForExternallyRemovedDownloads(
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]

https://codereview.chromium.org/2344483003/diff/1/chrome/browser/android/down...
File
chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc
(right):

https://codereview.chromium.org/2344483003/diff/1/chrome/browser/android/down...
chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc:30:
DownloadManagerService::GetInstance()->CheckForExternallyRemovedDownloads(
On 2016/09/15 22:18:56, qinmin wrote:
> 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.

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]

https://codereview.chromium.org/2344483003/diff/1/chrome/browser/android/down...
File
chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc
(right):

https://codereview.chromium.org/2344483003/diff/1/chrome/browser/android/down...
chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc:30:
DownloadManagerService::GetInstance()->CheckForExternallyRemovedDownloads(
On 2016/09/15 22:44:09, Theresa Wellington wrote:
> On 2016/09/15 22:18:56, qinmin wrote:
> > 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.
> 
> 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.

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]

https://codereview.chromium.org/2344483003/diff/1/chrome/browser/android/down...
File
chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc
(right):

https://codereview.chromium.org/2344483003/diff/1/chrome/browser/android/down...
chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc:30:
DownloadManagerService::GetInstance()->CheckForExternallyRemovedDownloads(
On 2016/09/15 23:14:00, qinmin wrote:
> On 2016/09/15 22:44:09, Theresa Wellington wrote:
> > On 2016/09/15 22:18:56, qinmin wrote:
> > > 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.
> > 
> > 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.
> 
> 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.

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.

[Theresa, 2016-09-15 23:56:46]

On 2016/09/15 23:50:32, Theresa Wellington wrote:
>
https://codereview.chromium.org/2344483003/diff/1/chrome/browser/android/down...
> File
>
chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc
> (right):
> 
>
https://codereview.chromium.org/2344483003/diff/1/chrome/browser/android/down...
>
chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc:30:
> DownloadManagerService::GetInstance()->CheckForExternallyRemovedDownloads(
> On 2016/09/15 23:14:00, qinmin wrote:
> > On 2016/09/15 22:44:09, Theresa Wellington wrote:
> > > On 2016/09/15 22:18:56, qinmin wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

* In step #4, the download item.... Skip #4 and go straight to #5. Sorry!

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

https://codereview.chromium.org/2344483003/diff/1/chrome/browser/android/down...
File
chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc
(right):

https://codereview.chromium.org/2344483003/diff/1/chrome/browser/android/down...
chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc:30:
DownloadManagerService::GetInstance()->CheckForExternallyRemovedDownloads(
On 2016/09/15 23:50:32, Theresa Wellington wrote:
> On 2016/09/15 23:14:00, qinmin wrote:
> > On 2016/09/15 22:44:09, Theresa Wellington wrote:
> > > On 2016/09/15 22:18:56, qinmin wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

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.

[qinmin, 2016-09-21 17:18:27]

ping, Theresa, is this good to go now? Thanks

[Theresa, 2016-09-21 17:40:44]

https://codereview.chromium.org/2344483003/diff/40001/chrome/browser/android/...
File
chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc
(right):

https://codereview.chromium.org/2344483003/diff/40001/chrome/browser/android/...
chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc:103:
if (!download_item_)
I'm not very familiar with this class, but looking at the comments in the
header, download_item_ is the item requesting the infobar and it could be
deleted while the infobar is showing.

Previously this code would still delete the existing download file and run the
callback even if download_item_ == nullptr, but now it returns immediately. When
this returns true, will the new file no longer be downloaded?

https://codereview.chromium.org/2344483003/diff/40001/chrome/browser/android/...
File chrome/browser/android/download/download_manager_service.cc (right):

https://codereview.chromium.org/2344483003/diff/40001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service.cc:57: if
(item->GetState() == content::DownloadItem::COMPLETE &&
Is it possible to interleave downloading items with the same path e.g. download
really_big_video.mp4 3x in a row, overwriting each time. I'm wondering if it's
possible that checking DownloadItem::COMPLETE will leave pending downloads that
all reference the same path?

https://codereview.chromium.org/2344483003/diff/40001/chrome/browser/android/...
File chrome/browser/android/download/download_manager_service.h (right):

https://codereview.chromium.org/2344483003/diff/40001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service.h:88: void
RemoveExternallyRemovedDownloads(const base::FilePath& path);
Can this be something like RemoveDownloadsForPath()?

// Remove download items associated with |path| from downloads history.

[qinmin, 2016-09-21 21:20:29]

https://codereview.chromium.org/2344483003/diff/40001/chrome/browser/android/...
File
chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc
(right):

https://codereview.chromium.org/2344483003/diff/40001/chrome/browser/android/...
chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc:103:
if (!download_item_)
Yes, if this returns true, the file will no longer be downloaded.
In order to download the file, |file_selected_callback_| has to be called.

The previous code is relies on the weak_ptr in download_target_determiner.cc to
guarantee the safety in case download_item_ is destroyed while the infobar is
shown. The new code just early returns so we don't hit the weak_ptr check in
download_target_determiner.cc


On 2016/09/21 17:40:43, Theresa Wellington wrote:
> I'm not very familiar with this class, but looking at the comments in the
> header, download_item_ is the item requesting the infobar and it could be
> deleted while the infobar is showing.
> 
> Previously this code would still delete the existing download file and run the
> callback even if download_item_ == nullptr, but now it returns immediately.
When
> this returns true, will the new file no longer be downloaded?

https://codereview.chromium.org/2344483003/diff/40001/chrome/browser/android/...
File chrome/browser/android/download/download_manager_service.cc (right):

https://codereview.chromium.org/2344483003/diff/40001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service.cc:57: if
(item->GetState() == content::DownloadItem::COMPLETE &&
On 2016/09/21 17:40:43, Theresa Wellington wrote:
> Is it possible to interleave downloading items with the same path e.g.
download
> really_big_video.mp4 3x in a row, overwriting each time. I'm wondering if it's
> possible that checking DownloadItem::COMPLETE will leave pending downloads
that
> all reference the same path?

If a download starts (or resumes) with the same target path as another ongoing
download, the new comer will fail.
So multiple ongoing downloads cannot have the same target path.

https://codereview.chromium.org/2344483003/diff/40001/chrome/browser/android/...
File chrome/browser/android/download/download_manager_service.h (right):

https://codereview.chromium.org/2344483003/diff/40001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service.h:88: void
RemoveExternallyRemovedDownloads(const base::FilePath& path);
On 2016/09/21 17:40:43, Theresa Wellington wrote:
> Can this be something like RemoveDownloadsForPath()?
> 
> // Remove download items associated with |path| from downloads history.

Done.

[Theresa, 2016-09-21 22:04:19]

lgtm

[qinmin, 2016-09-21 22:04:46]

The CQ bit was checked by qinmin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 22:05:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gone, 2016-09-21 22:20:42]

lgtm

[commit-bot: I haz the power, 2016-09-21 22:46:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 22:46:28]

Dry run: This issue passed the CQ dry run.

[qinmin, 2016-09-22 17:20:57]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2016-09-22 17:21:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 17:30:04]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-22 17:33:31]

Description was changed from

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

When a downloaded file is overwritten by another,
we should ask DownloadManager to check for externally removed files.

BUG=647042
==========

to

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

When a downloaded file is overwritten by another,
we should ask DownloadManager to check for externally removed files.

BUG=647042

Committed: https://crrev.com/db0d8c5689f4a4d9150ee42f181ef2f9f5af17bc
Cr-Commit-Position: refs/heads/master@{#420381}
==========

[commit-bot: I haz the power, 2016-09-22 17:33:34]

Patchset 4 (id:??) landed as
https://crrev.com/db0d8c5689f4a4d9150ee42f181ef2f9f5af17bc
Cr-Commit-Position: refs/heads/master@{#420381}