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..ad4ada025e92f92d1dacf0599cfa92f626c46bb3 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.

[Randy Smith (Not in Mondays), 2010/12/29 15:58:11]

Just FYI: I think we'll be in a position to yank in_progress after my history
deflake CL; that simplifies and makes clearer what's happening with in_progress.

   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,8 @@ 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()) {
+  DownloadMap::iterator it = active_downloads_.find(download_id);
+  if (it != active_downloads_.end()) {
     DownloadItem* download = it->second;
     download->Update(size);
     download_history_->UpdateEntry(download);
@@ -553,6 +548,7 @@ void DownloadManager::OnAllDataSaved(int32 download_id, int64 size) {
   // don't have a valid db_handle yet.
   if (download->db_handle() != DownloadHistory::kUninitializedHandle) {
     in_progress_.erase(it);
+    active_downloads_.erase(download_id);

[Randy Smith (Not in Mondays), 2010/12/29 15:58:11]

It looks like the semantics for active_downloads_ is for the download to enter
it when we create the download item, and to leave it whenever we'd normally
remove from in_progress_.  But in_progress_ is left when we've got all the data
and have completed the UI stuff (soon to include the history insertion :-}), and
the download isn't really done at that point.  For non-dangerous downloads we go
through and rename the file for the last time and then possibly auto-open the
darn thing.  For dangerous downloads we (may) wait until the user validates them
and then do the same thing.  My current preference is to keep the download in
active_downloads_ until we are well and truly done with it--i.e. it's gotten
it's final file name and it's been auto-opened if it's going to be.  I'm not
certain this is the right thing--I'm specifically worried it'll make our life
harder when we go to remove in_progress_ around the downloads we show as visible
in the DOM.  And it doesn't make sense in terms of the map aspect of
active_downloads_, as once the IO->FILE pipe is finished, we're never going to
get that download_id referenced from a DownloadItem external source again.  But
if we keep the thing around, the active_downloads_ container can be defined as a
container of all the downloads the download system is actively thinking about,
which appeals to my simple categorization approach to these containers.  

If you actively disagree with my preference, I'm ok with it going in as is.  But
if you don't care, could you shift it over to being cleaned up in something like
DownloadManager::DownloadFinished?  We can always shift it back pretty easily if
we decide that's what's needed.

[ahendrickson, 2011/01/01 18:15:59]

Done.

     download_history_->UpdateEntry(download);
   }
 
@@ -661,6 +657,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);
   }
 
@@ -983,7 +980,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 +992,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);
     download_history_->UpdateEntry(download);
     download->UpdateObservers();
   }
@@ -1048,7 +1046,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++) {