Adding active_downloads_ map.

chrome/browser/download/download_manager.cc

Index: chrome/browser/download/download_manager.cc
diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index da878b0f2687bf7823138c1062daaa22da64b5f1..3cc33cde3b67730023450029c18ed9d384eaa2c2 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -120,6 +120,7 @@ void DownloadManager::Shutdown() {
 
   // And clear all non-owning containers.
   in_progress_.clear();
+  active_downloads_.clear();
 #if !defined(NDEBUG)
   save_page_as_downloads_.clear();
 #endif
@@ -427,7 +428,9 @@ void DownloadManager::CreateDownloadItem(DownloadCreateInfo* info) {
   DownloadItem* download = new DownloadItem(this, *info,
                                             profile_->IsOffTheRecord());
   DCHECK(!ContainsKey(in_progress_, info->download_id));
+  DCHECK(!ContainsKey(active_downloads_, info->download_id));
   downloads_.insert(download);
+  active_downloads_[info->download_id] = download;
 }
 
 void DownloadManager::AttachDownloadItem(DownloadCreateInfo* info,
@@ -437,22 +440,14 @@ void DownloadManager::AttachDownloadItem(DownloadCreateInfo* info,
   scoped_ptr<DownloadCreateInfo> infop(info);
   info->path = target_path;
 
-  // NOTE(ahendrickson) We will be adding a new map |active_downloads_|, into
-  // which we will be adding the download as soon as it's created.  This will
-  // make this loop unnecessary.
-  // Eventually |active_downloads_| will replace |in_progress_|, but we don't
-  // want to change the semantics yet.
+  // NOTE(ahendrickson) Eventually |active_downloads_| will replace
+  // |in_progress_|, but we don't want to change the semantics yet.
   DCHECK(!ContainsKey(in_progress_, info->download_id));
-  DownloadItem* download = NULL;
-  for (std::set<DownloadItem*>::iterator i = downloads_.begin();
-       i != downloads_.end(); ++i) {
-    DownloadItem* item = (*i);
-    if (item && (item->id() == info->download_id)) {
-      download = item;
-      break;
-    }
-  }
+  DCHECK(ContainsKey(active_downloads_, info->download_id));
+  DownloadItem* download = active_downloads_[info->download_id];
   DCHECK(download != NULL);
+  DCHECK(downloads_.find(download) != downloads_.end());
+
   download->SetFileCheckResults(info->path,
                                 info->is_dangerous,
                                 info->path_uniquifier,
@@ -506,8 +501,9 @@ void DownloadManager::AttachDownloadItem(DownloadCreateInfo* info,
 }
 
 void DownloadManager::UpdateDownload(int32 download_id, int64 size) {
-  DownloadMap::iterator it = in_progress_.find(download_id);
-  if (it != in_progress_.end()) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DownloadMap::iterator it = active_downloads_.find(download_id);
+  if (it != active_downloads_.end()) {

[Randy Smith (Not in Mondays), 2011/01/03 22:02:22]

This is a semantic change, i.e. that we'll keep updating the download observers
and history after we've removed the download from in_progress_.  Are you fairly
comfortable that either this produces no user-visible change or that the
user-visible change it produces is benign?

[ahendrickson, 2011/01/04 16:51:38]

My understanding is that nothing will be visible to the user until the download
appears in the |in_progress_| map, so there should not be any user-visible
change.

     DownloadItem* download = it->second;
     download->Update(size);
     download_history_->UpdateEntry(download);
@@ -580,6 +576,12 @@ void DownloadManager::OnAllDataSaved(int32 download_id, int64 size) {
   download->OnSafeDownloadFinished(file_manager_);
 }
 
+void DownloadManager::OnDownloadFileCompleted(int32 download_id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  active_downloads_.erase(download_id);
+}
+
 void DownloadManager::DownloadRenamedToFinalName(int download_id,
                                                  const FilePath& full_path) {
   VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id
@@ -649,6 +651,7 @@ void DownloadManager::DangerousDownloadRenamed(int64 download_handle,
 }
 
 void DownloadManager::DownloadCancelled(int32 download_id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DownloadMap::iterator it = in_progress_.find(download_id);
   if (it == in_progress_.end())
     return;
@@ -661,6 +664,7 @@ void DownloadManager::DownloadCancelled(int32 download_id) {
   // don't have a valid db_handle yet.
   if (download->db_handle() != DownloadHistory::kUninitializedHandle) {
     in_progress_.erase(it);
+    active_downloads_.erase(download_id);
     download_history_->UpdateEntry(download);
   }
 
@@ -959,6 +963,7 @@ void DownloadManager::OnQueryDownloadEntriesComplete(
 void DownloadManager::OnCreateDownloadEntryComplete(
     DownloadCreateInfo info,
     int64 db_handle) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DownloadMap::iterator it = in_progress_.find(info.download_id);
   DCHECK(it != in_progress_.end());
 
@@ -983,7 +988,7 @@ void DownloadManager::OnCreateDownloadEntryComplete(
          history_downloads_.end());
   history_downloads_[download->db_handle()] = download;
 
-  // Show in the appropropriate browser UI.
+  // Show in the appropriate browser UI.
   ShowDownloadInBrowser(info, download);
 
   // Inform interested objects about the new download.
@@ -995,6 +1000,7 @@ void DownloadManager::OnCreateDownloadEntryComplete(
   // observers so that they get more than just the start notification.
   if (download->state() != DownloadItem::IN_PROGRESS) {
     in_progress_.erase(it);
+    active_downloads_.erase(info.download_id);

[Randy Smith (Not in Mondays), 2011/01/03 22:02:22]

I don't think that this is correct, but I'm not certain what is.  The COMPLETE
state is reached when the last byte arrives, but there's a lot of download
processing that occurs afterwards, so based on what I'd like to see
active_downloads_ mean (== download there until no further processing in the
downloads system) testing for moving the state out of IN_PROGRESS isn't right. 
But there isn't a state to test for (currently; one of my CLs is adding one),
and this callback could occur arbitrarily long after the download actually
compelete/is auto-opened.  

I think given all that, I'll suggest you leave this code the way it is, but put
a TODO comment in that we need to fix it when we either have the final state to
test against or the history insertion is done inline.  Then whichever of my CLs
I commit first I'll fix it in.

[ahendrickson, 2011/01/04 16:51:38]

Added TODO.

     download_history_->UpdateEntry(download);
     download->UpdateObservers();
   }
@@ -1048,7 +1054,7 @@ void DownloadManager::AssertContainersConsistent() const {
 #if !defined(NDEBUG)
   // Turn everything into sets.
   DownloadSet in_progress_set, history_set;
-  const DownloadMap* input_maps[] = {&in_progress_, &history_downloads_};
+  const DownloadMap* input_maps[] = {&active_downloads_, &history_downloads_};
   DownloadSet* local_sets[] = {&in_progress_set, &history_set};
   DCHECK_EQ(ARRAYSIZE_UNSAFE(input_maps), ARRAYSIZE_UNSAFE(local_sets));
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(input_maps); i++) {