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

chrome/browser/download/download_item.cc

 // Copyright (c) 2011 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.
 
 #include "chrome/browser/download/download_item.h"
 
 #include "base/basictypes.h"
 #include "base/file_util.h"
 #include "base/format_macros.h"
 #include "base/i18n/case_conversion.h"
@@ skipping 113 matching lines @@
       original_mime_type_(info.original_mime_type),
       total_bytes_(info.total_bytes),
       received_bytes_(info.received_bytes),
       start_tick_(base::TimeTicks()),
       state_(static_cast<DownloadState>(info.state)),
       start_time_(info.start_time),
       db_handle_(info.db_handle),
       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),
       target_name_(info.original_name),
       save_as_(false),
       is_otr_(false),
       is_extension_install_(info.is_extension_install),
       is_temporary_(false),
       all_data_saved_(false),
       opened_(false) {
@@ skipping 18 matching lines @@
       total_bytes_(info.total_bytes),
       received_bytes_(0),
       last_os_error_(0),
       start_tick_(base::TimeTicks::Now()),
       state_(IN_PROGRESS),
       start_time_(info.start_time),
       db_handle_(DownloadHistory::kUninitializedHandle),
       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,
                                  info.is_dangerous_url)),
       auto_opened_(false),
       target_name_(info.original_name),
       process_handle_(info.process_handle),
       save_as_(info.prompt_user_for_save_location),
       is_otr_(is_otr),
       is_extension_install_(info.is_extension_install),
@@ skipping 18 matching lines @@
       total_bytes_(0),
       received_bytes_(0),
       last_os_error_(0),
       start_tick_(base::TimeTicks::Now()),
       state_(IN_PROGRESS),
       start_time_(base::Time::Now()),
       db_handle_(DownloadHistory::kUninitializedHandle),
       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),
       save_as_(false),
       is_otr_(is_otr),
       is_extension_install_(false),
       is_temporary_(false),
       all_data_saved_(false),
       opened_(false) {
   Init(true /* start progress timer */);
@@ skipping 29 matching lines @@
   DownloadPrefs* prefs = download_manager_->download_prefs();
   if (open)
     prefs->EnableAutoOpenBasedOnExtension(GetUserVerifiedFilePath());
   else
     prefs->DisableAutoOpenBasedOnExtension(GetUserVerifiedFilePath());
 }
 
 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()) {
       download_util::OpenChromeExtension(download_manager_->profile(),
                                          download_manager_,
                                          *this);
       return;
     }
 #if defined(OS_MACOSX)
     // Mac OS X requires opening downloads on the UI thread.
@@ skipping 79 matching lines @@
   UpdateObservers();
 }
 
 void DownloadItem::OnAllDataSaved(int64 size) {
   DCHECK(!all_data_saved_);
   all_data_saved_ = true;
   UpdateSize(size);
   StopProgressTimer();
 }
 
+void DownloadItem::OnDownloadedFileRemoved() {
+  file_externally_removed_ = true;
+  UpdateObservers();
+}
+
 void DownloadItem::Completed() {
   VLOG(20) << __FUNCTION__ << "() " << DebugString(false);
 
   download_util::RecordDownloadCount(download_util::COMPLETED_COUNT);
 
   // Handle chrome extensions explicitly and skip the shell execute.
   if (is_extension_install()) {
     download_util::OpenChromeExtension(download_manager_->profile(),
                                        download_manager_,
                                        *this);
@@ skipping 274 matching lines @@
         is_otr() ? 'T' : 'F',
         DebugSafetyStateString(safety_state()),
         url_list.c_str(),
         target_name_.value().c_str(),
         full_path().value().c_str());
   } else {
     description += base::StringPrintf(" url = \"%s\"", url_list.c_str());
   }
   return description;
 }