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.
   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()) {
+  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);
   }
   UpdateAppIcon();
 }
 
 void DownloadManager::OnAllDataSaved(int32 download_id, int64 size) {
   VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id
            << " size = " << size;
@@ skipping 52 matching lines @@
         NewRunnableMethod(
             this, &DownloadManager::ProceedWithFinishedDangerousDownload,
             download->db_handle(),
             download->full_path(), download->target_name()));
     return;
   }
 
   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
            << " full_path = \"" << full_path.value() << "\"";
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DownloadItem* item = GetDownloadItem(download_id);
   if (!item)
     return;
   item->OnDownloadRenamedToFinalName(full_path);
 }
@@ skipping 49 matching lines @@
     // name when calling GetFileNameToReportUser().
     download->set_path_uniquifier(new_path_uniquifier);
     RenameDownload(download, new_path);
   }
 
   // Continue the download finished sequence.
   download->Finished();
 }
 
 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;
   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 278 matching lines @@
   }
   NotifyModelChanged();
 }
 
 // Once the new DownloadItem's creation info has been committed to the history
 // service, we associate the DownloadItem with the db handle, update our
 // 'history_downloads_' map and inform observers.
 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());
 
   DownloadItem* download = it->second;
   VLOG(20) << __FUNCTION__ << "()" << " db_handle = " << db_handle
            << " download_id = " << info.download_id
            << " download = " << download->DebugString(true);
 
   // It's not immediately obvious, but HistoryBackend::CreateDownload() can
   // call this function with an invalid |db_handle|. For instance, this can
   // happen when the history database is offline. We cannot have multiple
   // DownloadItems with the same invalid db_handle, so we need to assign a
   // unique |db_handle| here.
   if (db_handle == DownloadHistory::kUninitializedHandle)
     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);

[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();
   }
 
   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;
 }