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

chrome/browser/ui/browser.cc

Index: chrome/browser/ui/browser.cc
diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index 5cbc61600ad23e5a3403514fe9b59d715b3fa82b..fced7c87eeb5c55a58c405189758bf617a684df7 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -2453,6 +2453,13 @@ void Browser::UpdateUIForNavigationInTab(TabContentsWrapper* contents,
     contents->tab_contents()->Focus();
 }
 
+void Browser::CheckForFilesRemoval() {
+  if (profile_->HasCreatedDownloadManager()) {
+    DownloadManager* download_manager = profile_->GetDownloadManager();
+    download_manager->CheckForFilesRemoval();
+  }
+}
+
 GURL Browser::GetHomePage() const {
   // --homepage overrides any preferences.
   const CommandLine& command_line = *CommandLine::ForCurrentProcess();
@@ -3220,6 +3227,8 @@ void Browser::OnStartDownload(DownloadItem* download, TabContents* tab) {
   if (!window())
     return;
 
+  CheckForFilesRemoval();

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

I apologize--I know I recommended this location, but now that I actually see it
here, my brain kicks into gear, and I don't think this function call belongs
here.  Where we put the probe points is completely up to us--I agree that it's
good to "clean up" old history that no longer has the downloads around, but I
don't think it's worth a lot of effort to try and keep current downloads up to
date.  So I think the right place for the probe point is in
DownloadManager::OnQueryDownloadEntriesComplete(), and basically no where else
(i.e. once per browser startup, after we've loaded everything from the history).
 If you have other locations you'd like to put it, let me know, but I'd like it
to be in the core downloads code, and not to happen too often.

[haraken1, 2011/05/16 11:42:27]

I agree that we should not include too frequent probe points for rare cases.
However, I think that only at DownloadManager::OnQueryDownloadEntriesComplete()
(i.e. once per browser startup) is a bit too infrequent. I mean, I am afraid
that it is not intuitive for a user that removed files remain displayed as if
they exist even if the user updates the chrome://downloads page (with F5 key).
For example, consider the case where a user removes many files in his download
directory. Then the user opens chrome://downloads page and clicks one of the
files. Then it immediately changes its state to "Removed" and thus the user can
notice that the file is already removed. Next, the user clicks another file.
Then it also immediately changes its state to "Removed" and thus the user can
notice that the file is also already removed. Here the user would wonder that
the chrome://downloads page does not reflect the latest state of the download
directory. So the user presses F5 key to update the chrome://downloads page...
but the states of the page does not change...I think this behavior is not
intuitive. 

Thus, I am not sure but, for the time being, I inserted the probe point only at
DownloadsDOMHandler::ModelChanged(). (In this case, the probe point at
DownloadManager::OnQueryDownloadEntriesComplete() is not necessary because it
eventually calls DownloadsDOMHandler::ModelChanged()). I would like to hear your
opinion. 

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

So I think the use case you describe (a "reload" on the downloads.html page
updating the "removed" state) is a good one, and we should find a way to handle
it.  But I'd rather have the probe point be in
DownloadsDOMHandler::HandleGetDownloads() rather than in ModelChanged(). 
ModelChanged() will be called any time a new download is added to or deleted
from the DownloadManager, but based on the use case, we only want to do a probe
when the downloads.html page is reloaded.  Does that make sense?

(I think I'd still like to keep the call in OnQueryDownloadEntriesComplete,
though I don't feel strongly about it.  Whichever way we go with the above, I
don't think the call in OnQueryDownloadEntriesComplete() will change user facing
behavior, but it'll keep the in-memory entries consistent on a browser-startup
granularity in case we ever put other code in that relies on historical download
entries being updated with regard to file system changes.  But it is an
unnecessary probe point, albeit one that only happens once on browser startup.)

+
 #if defined(OS_CHROMEOS)
   // Don't show content browser for extension/theme downloads from gallery.
   if (download->is_extension_install()) {