Support for adding save page download items into downloads history.

chrome/browser/download/download_manager.cc

Index: chrome/browser/download/download_manager.cc
===================================================================
--- chrome/browser/download/download_manager.cc	(revision 93070)
+++ chrome/browser/download/download_manager.cc	(working copy)
@@ -54,7 +54,8 @@
     : shutdown_needed_(false),
       profile_(NULL),
       file_manager_(NULL),
-      status_updater_(status_updater->AsWeakPtr()) {
+      status_updater_(status_updater->AsWeakPtr()),
+      next_save_page_id_(0) {
   if (status_updater_)
     status_updater_->AddDelegate(this);
 }
@@ -122,11 +123,10 @@
   in_progress_.clear();
   active_downloads_.clear();
   history_downloads_.clear();
-#if !defined(NDEBUG)
-  save_page_as_downloads_.clear();
-#endif
   STLDeleteElements(&downloads_to_delete);
 
+  DCHECK(save_page_downloads_.empty());
+
   file_manager_ = NULL;
 
   // Make sure the save as dialog doesn't notify us back if we're gone before
@@ -985,16 +985,6 @@
   return RemoveDownloadsBetween(base::Time(), base::Time());
 }
 
-void DownloadManager::SavePageAsDownloadStarted(DownloadItem* download) {
-#if !defined(NDEBUG)
-  save_page_as_downloads_.insert(download);
-#endif
-  downloads_.insert(download);
-  // Add to history and notify observers.
-  AddDownloadItemToHistory(download, DownloadHistory::kUninitializedHandle);
-  NotifyModelChanged();
-}
-
 // Initiate a download of a specific URL. We send the request to the
 // ResourceDispatcherHost, and let it send us responses like a regular
 // download.
@@ -1197,6 +1187,13 @@
 
   DCHECK(!ContainsKey(history_downloads_, download->db_handle()));
   history_downloads_[download->db_handle()] = download;
+
+  // Show in the appropriate browser UI.
+  // This includes buttons to save or cancel, for a dangerous download.
+  ShowDownloadInBrowser(download);
+
+  // Inform interested objects about the new download.
+  NotifyModelChanged();
 }
 
 // Once the new DownloadItem's creation info has been committed to the history
@@ -1215,13 +1212,6 @@
 
   AddDownloadItemToHistory(download, db_handle);
 
-  // Show in the appropriate browser UI.
-  // This includes buttons to save or cancel, for a dangerous download.
-  ShowDownloadInBrowser(download);
-
-  // Inform interested objects about the new download.
-  NotifyModelChanged();
-
   // If the download is still in progress, try to complete it.
   //
   // Otherwise, download has been cancelled or interrupted before we've
@@ -1295,20 +1285,20 @@
 void DownloadManager::AssertContainersConsistent() const {
 #if !defined(NDEBUG)
   // Turn everything into sets.
-  DownloadSet active_set, history_set;
-  const DownloadMap* input_maps[] = {&active_downloads_, &history_downloads_};
-  DownloadSet* local_sets[] = {&active_set, &history_set};
-  DCHECK_EQ(ARRAYSIZE_UNSAFE(input_maps), ARRAYSIZE_UNSAFE(local_sets));
+  const DownloadMap* input_maps[] = {&active_downloads_,
+                                     &history_downloads_,
+                                     &save_page_downloads_};
+  DownloadSet active_set, history_set, save_page_set;
+  DownloadSet* all_sets[] = {&active_set, &history_set, &save_page_set};
+  DCHECK_EQ(ARRAYSIZE_UNSAFE(input_maps), ARRAYSIZE_UNSAFE(all_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);
+         it != input_maps[i]->end(); ++it) {
+      all_sets[i]->insert(&*it->second);
     }
   }
 
   // Check if each set is fully present in downloads, and create a union.
-  const DownloadSet* all_sets[] = {&active_set, &history_set,
-                                   &save_page_as_downloads_};
   DownloadSet downloads_union;
   for (int i = 0; i < static_cast<int>(ARRAYSIZE_UNSAFE(all_sets)); i++) {
     DownloadSet remainder;
@@ -1363,3 +1353,52 @@
 void DownloadManager::OtherDownloadManagerObserver::ManagerGoingDown() {
   observed_download_manager_ = NULL;
 }
+
+void DownloadManager::SavePageDownloadStarted(DownloadItem* download) {
+  DCHECK(!ContainsKey(save_page_downloads_, download->id()));
+  downloads_.insert(download);
+  save_page_downloads_[download->id()] = download;
+
+  // Add this entry to the history service.
+  // Additionally, the UI is notified in the callback.
+  download_history_->AddEntry(download,
+      NewCallback(this, &DownloadManager::OnSavePageDownloadEntryAdded));
+}
+
+void DownloadManager::OnSavePageDownloadEntryAdded(int32 download_id,
+                                                   int64 db_handle) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  DownloadMap::const_iterator it = save_page_downloads_.find(download_id);
+  // This can happen if SavePageDownloadFinished happens before the history
+  // callback. In that case, the download is stuck in pending downloads
+  // state and never finishes. This will be fixed with the integration of
+  // SavePackage into DownloadManager.

[Randy Smith (Not in Mondays), 2011/07/25 18:59:31]

This comment is no longer correct and should be updated.

[achuithb, 2011/07/27 01:04:57]

I updated the comment. This can now only happen if shutdown happened before
the history callback.

+  if (it == save_page_downloads_.end())
+    return;
+
+  DownloadItem* download = it->second;
+  DCHECK(download);
+  if (!download)
+    return;
+
+  AddDownloadItemToHistory(download, db_handle);
+
+  // Finalize this download if it terminated before the history callback.
+  if (!download->IsInProgress())
+    SavePageDownloadFinished(download);
+}
+
+void DownloadManager::SavePageDownloadFinished(DownloadItem* download) {
+  if (download->db_handle() != DownloadHistory::kUninitializedHandle) {
+    download_history_->UpdateEntry(download);
+    DCHECK(ContainsKey(save_page_downloads_, download->id()));
+    save_page_downloads_.erase(download->id());
+  }
+}
+
+int32 DownloadManager::GetNextSavePageId() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  return next_save_page_id_++;
+}
+