Adding active_downloads_ map.

chrome/browser/download/download_manager.cc

 // Copyright (c) 2010 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_manager.h"
 
 #include "app/l10n_util.h"
 #include "app/resource_bundle.h"
 #include "base/callback.h"
 #include "base/file_util.h"
@@ skipping 102 matching lines @@
     }
   }
 
   // At this point, all dangerous downloads have had their files removed
   // and all in progress downloads have been cancelled.  We can now delete
   // anything left.
   STLDeleteElements(&downloads_);
 
   // And clear all non-owning containers.
   in_progress_.clear();
+  active_downloads_.clear();
 #if !defined(NDEBUG)
   save_page_as_downloads_.clear();
 #endif
 
   file_manager_ = NULL;
 
   // Make sure the save as dialog doesn't notify us back if we're gone before
   // it returns.
   if (select_file_dialog_.get())
     select_file_dialog_->ListenerDestroyed();
@@ skipping 287 matching lines @@
     AttachDownloadItem(info, info->suggested_path);
   }
 }
 
 void DownloadManager::CreateDownloadItem(DownloadCreateInfo* info) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   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,
                                          const FilePath& target_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   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,
                                 info->prompt_user_for_save_location,
                                 info->is_extension_install,
                                 info->original_name);
   in_progress_[info->download_id] = download;
 
   bool download_finished = ContainsKey(pending_finished_downloads_,
                                        info->download_id);
@@ skipping 33 matching lines @@
                    pending_finished_downloads_[info->download_id]);
   }
 
   download_history_->AddEntry(*info, download,
       NewCallback(this, &DownloadManager::OnCreateDownloadEntryComplete));
 
   UpdateAppIcon();
 }
 
 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);
   }
   UpdateAppIcon();
 }
 
 void DownloadManager::OnAllDataSaved(int32 download_id, int64 size) {
   VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id
            << " size = " << size;
@@ skipping 25 matching lines @@
 
   VLOG(20) << __FUNCTION__ << "()"
            << " download = " << download->DebugString(true);
 
   download->OnAllDataSaved(size);
 
   // Clean up will happen when the history system create callback runs if we
   // 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);
   }
 
   UpdateAppIcon();
 
   // If this a dangerous download not yet validated by the user, don't do
   // anything. When the user notifies us, it will trigger a call to
   // ProceedWithFinishedDangerousDownload.
   if (download->safety_state() == DownloadItem::DANGEROUS) {
     return;
@@ skipping 88 matching lines @@
     return;
   DownloadItem* download = it->second;
 
   VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id
            << " download = " << download->DebugString(true);
 
   // Clean up will happen when the history system create callback runs if we
   // 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);
   }
 
   DownloadCancelledInternal(download_id,
                             download->render_process_id(),
                             download->request_id());
   UpdateAppIcon();
 }
 
 void DownloadManager::DownloadCancelledInternal(int download_id,
@@ skipping 302 matching lines @@
     db_handle = download_history_->GetNextFakeDbHandle();
 
   DCHECK(download->db_handle() == DownloadHistory::kUninitializedHandle);
   download->set_db_handle(db_handle);
 
   // Insert into our full map.
   DCHECK(history_downloads_.find(download->db_handle()) ==
          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.
   NotifyModelChanged();
 
   // If this download has been completed before we've received the db handle,
   // post one final message to the history service so that it can be properly
   // in sync with the DownloadItem's completion status, and also inform any
   // 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();
   }
 
   UpdateAppIcon();
 }
 
 void DownloadManager::ShowDownloadInBrowser(const DownloadCreateInfo& info,
                                             DownloadItem* download) {
   // The 'contents' may no longer exist if the user closed the tab before we
@@ skipping 33 matching lines @@
   }
   return NULL;
 }
 
 // Confirm that everything in all maps is also in |downloads_|, and that
 // everything in |downloads_| is also in some other map.
 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++) {
     for (DownloadMap::const_iterator it = input_maps[i]->begin();
          it != input_maps[i]->end(); it++) {
       local_sets[i]->insert(&*it->second);
     }
   }
 
   // Check if each set is fully present in downloads, and create a union.
@@ skipping 46 matching lines @@
     observed_download_manager_->RemoveObserver(this);
 }
 
 void DownloadManager::OtherDownloadManagerObserver::ModelChanged() {
   observing_download_manager_->NotifyModelChanged();
 }
 
 void DownloadManager::OtherDownloadManagerObserver::ManagerGoingDown() {
   observed_download_manager_ = NULL;
 }