Detect removed files and reflect the state in chrome://downloads and the download shelf

chrome/browser/download/download_item.cc

Index: chrome/browser/download/download_item.cc
diff --git a/chrome/browser/download/download_item.cc b/chrome/browser/download/download_item.cc
index 59126a86d43769586c96fbf06678a0fd2ce9b69c..04f50502601c54b4d3e6193906c486825bd5f367 100644
--- a/chrome/browser/download/download_item.cc
+++ b/chrome/browser/download/download_item.cc
@@ -131,6 +131,7 @@ DownloadItem::DownloadItem(DownloadManager* download_manager,
       download_manager_(download_manager),
       is_paused_(false),
       open_when_complete_(false),
+      file_externally_removed_(false),
       safety_state_(SAFE),
       danger_type_(NOT_DANGEROUS),
       auto_opened_(false),
@@ -169,6 +170,7 @@ DownloadItem::DownloadItem(DownloadManager* download_manager,
       download_manager_(download_manager),
       is_paused_(false),
       open_when_complete_(false),
+      file_externally_removed_(false),
       safety_state_(GetSafetyState(info.is_dangerous_file,
                                    info.is_dangerous_url)),
       danger_type_(GetDangerType(info.is_dangerous_file,
@@ -207,6 +209,7 @@ DownloadItem::DownloadItem(DownloadManager* download_manager,
       download_manager_(download_manager),
       is_paused_(false),
       open_when_complete_(false),
+      file_externally_removed_(false),
       safety_state_(SAFE),
       danger_type_(NOT_DANGEROUS),
       auto_opened_(false),
@@ -256,7 +259,8 @@ void DownloadItem::OpenFilesBasedOnExtension(bool open) {
 void DownloadItem::OpenDownload() {
   if (IsPartialDownload()) {
     open_when_complete_ = !open_when_complete_;
-  } else if (IsComplete()) {
+  } else if (IsComplete() && !file_externally_removed()) {
+    download_manager_->CheckForFileRemoval(this);

[Randy Smith (Not in Mondays), 2011/05/16 20:57:12]

I think I'd like a comment here explaining this design choice.  Maybe something
like "Ideally, we'd detecting errors in opening and report them, but we don't
generally have the proper interface for that to the external program that opens
the file.  So instead we spawn a check to update the UI if the file has been
deleted in parallel with the open."?

[Paweł Hajdan Jr., 2011/05/17 20:03:39]

There is an ugliness in a way those objects communicate. DownloadItem calls
DownloadManager here, which is going to ask DownloadItem for its ID and path,
and then it's going to update DownloadItem's file removal flag.

How about doing the check in DownloadItem instead, i.e. making
DownloadManager::CheckForFileRemoval a DownloadItem's method, removing
DownloadItem::OnDownloadedFileRemoved and adding
DownloadManager::OnDownloadedFileRemoved instead to update |history_downloads_|?

[Randy Smith (Not in Mondays), 2011/05/18 19:28:18]

Pawel: I'm confused about what you mean.  Dispatching the check directly from
DownloadItem (i.e. making CheckForFileRemoval a DownloadItem method) makes sense
to me.  But I don't understand the change on the return path--if you get rid of
DownloadItem::OnDownloadFileRemoved(), how do you update the state internal to
the download item?  Or are you thinking of storing that state outside of
DownloadItem?  I presume not--that seems pretty ugly to me.

[Paweł Hajdan Jr., 2011/05/18 20:03:43]

Sorry, I was a bit afraid that comment could be confusing.

I meant to keep the state in DownloadItem, as it currently is in the latest
patch set. Now if we dispatch the check directly from DownloadItem, we should no
longer need a *public* method DownloadItem::OnDownloadFileRemoved() - it can
still exist, just please make it private.

[Randy Smith (Not in Mondays), 2011/05/18 20:13:13]

Ah, thank you!  That makes sense, but it won't work.  In general, you really
don't want to target DownloadItem methods from off-thread, because DownloadItem
can be cancelled or removed at almost any time (think shutdown races).  So all
off-thread communication with the DownloadItem needs to go through the download
manager (which is refcounted for just such an occasion).

[Paweł Hajdan Jr., 2011/05/18 20:23:44]

Oh, I see. Indeed, please ignore my comment about that then.

[haraken1, 2011/06/07 12:49:18]

In conclusion, here I only added the comment that Randy advised me. 

     opened_ = true;
     FOR_EACH_OBSERVER(Observer, observers_, OnDownloadOpened(this));
     if (is_extension_install()) {
@@ -356,6 +360,11 @@ void DownloadItem::OnAllDataSaved(int64 size) {
   StopProgressTimer();
 }
 
+void DownloadItem::OnDownloadedFileRemoved() {
+  file_externally_removed_ = true;
+  UpdateObservers();
+}
+
 void DownloadItem::Completed() {
   VLOG(20) << __FUNCTION__ << "() " << DebugString(false);