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 94123)
+++ chrome/browser/download/download_manager.cc	(working copy)
@@ -42,6 +42,7 @@
 #include "content/browser/renderer_host/resource_dispatcher_host.h"
 #include "content/browser/tab_contents/tab_contents.h"
 #include "content/common/content_notification_types.h"
+#include "content/common/notification_service.h"
 #include "googleurl/src/gurl.h"
 #include "grit/generated_resources.h"
 #include "grit/theme_resources.h"
@@ -54,7 +55,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 +124,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
@@ -141,7 +142,7 @@
 }
 
 void DownloadManager::GetTemporaryDownloads(
-    const FilePath& dir_path, std::vector<DownloadItem*>* result) {
+    const FilePath& dir_path, DownloadVec* result) {
   DCHECK(result);
 
   for (DownloadMap::iterator it = history_downloads_.begin();
@@ -153,7 +154,7 @@
 }
 
 void DownloadManager::GetAllDownloads(
-    const FilePath& dir_path, std::vector<DownloadItem*>* result) {
+    const FilePath& dir_path, DownloadVec* result) {
   DCHECK(result);
 
   for (DownloadMap::iterator it = history_downloads_.begin();
@@ -165,7 +166,7 @@
 }
 
 void DownloadManager::GetCurrentDownloads(
-    const FilePath& dir_path, std::vector<DownloadItem*>* result) {
+    const FilePath& dir_path, DownloadVec* result) {
   DCHECK(result);
 
   for (DownloadMap::iterator it = history_downloads_.begin();
@@ -194,7 +195,7 @@
 }
 
 void DownloadManager::SearchDownloads(const string16& query,
-                                      std::vector<DownloadItem*>* result) {
+                                      DownloadVec* result) {
   DCHECK(result);
 
   string16 query_lower(base::i18n::ToLower(query));
@@ -899,6 +900,28 @@
     status_updater_->Update();
 }
 
+int DownloadManager::RemoveDownloadItems(const DownloadVec& pending_deletes) {
+  if (pending_deletes.empty())
+    return 0;
+
+  // Delete from internal maps.
+  for (size_t i = 0; i < pending_deletes.size(); ++i) {
+    DownloadItem* download = pending_deletes[i];
+    DCHECK(download);
+    history_downloads_.erase(download->db_handle());
+    save_page_downloads_.erase(download->id());
+    downloads_.erase(download);
+  }
+
+  // Tell observers to refresh their views.
+  NotifyModelChanged();
+
+  // Delete the download items themselves.
+  const int num_deleted = static_cast<int>(pending_deletes.size());
+  STLDeleteContainerPointers(pending_deletes.begin(), pending_deletes.end());
+  return num_deleted;
+}
+
 void DownloadManager::RemoveDownload(int64 download_handle) {
   DownloadMap::iterator it = history_downloads_.find(download_handle);
   if (it == history_downloads_.end())
@@ -909,14 +932,8 @@
   download_history_->RemoveEntry(download);
 
   // Remove from our tables and delete.
-  history_downloads_.erase(it);
-  int downloads_count = downloads_.erase(download);
+  int downloads_count = RemoveDownloadItems(DownloadVec(1, download));
   DCHECK_EQ(1, downloads_count);
-
-  // Tell observers to refresh their views.
-  NotifyModelChanged();
-
-  delete download;
 }
 
 int DownloadManager::RemoveDownloadsBetween(const base::Time remove_begin,
@@ -925,9 +942,10 @@
 
   // All downloads visible to the user will be in the history,
   // so scan that map.
-  DownloadMap::iterator it = history_downloads_.begin();
-  std::vector<DownloadItem*> pending_deletes;
-  while (it != history_downloads_.end()) {
+  DownloadVec pending_deletes;
+  for (DownloadMap::iterator it = history_downloads_.begin();
+      it != history_downloads_.end();
+      ++it) {
     DownloadItem* download = it->second;
     if (download->start_time() >= remove_begin &&
         (remove_end.is_null() || download->start_time() < remove_end) &&
@@ -936,38 +954,10 @@
          download->IsInterrupted())) {
       AssertQueueStateConsistent(download);
 
-      // Remove from the map and move to the next in the list.
-      history_downloads_.erase(it++);
-
-      // Also remove it from any completed dangerous downloads.
       pending_deletes.push_back(download);
-
-      continue;
     }
-
-    ++it;
   }
-
-  // If we aren't deleting anything, we're done.
-  if (pending_deletes.empty())
-    return 0;
-
-  // Remove the chosen downloads from the main owning container.
-  for (std::vector<DownloadItem*>::iterator it = pending_deletes.begin();
-       it != pending_deletes.end(); it++) {
-    downloads_.erase(*it);
-  }
-
-  // Tell observers to refresh their views.
-  NotifyModelChanged();
-
-  // Delete the download items themselves.
-  int num_deleted = static_cast<int>(pending_deletes.size());
-
-  STLDeleteContainerPointers(pending_deletes.begin(), pending_deletes.end());
-  pending_deletes.clear();
-
-  return num_deleted;
+  return RemoveDownloadItems(pending_deletes);
 }
 
 int DownloadManager::RemoveDownloads(const base::Time remove_begin) {
@@ -984,16 +974,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.
@@ -1187,6 +1167,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
@@ -1205,13 +1192,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
@@ -1285,20 +1265,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;
@@ -1353,3 +1333,55 @@
 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 the download manager is shutting down and all maps
+  // have been cleared.
+  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 finished 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());
+
+    NotificationService::current()->Notify(
+        content::NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED,

[Randy Smith (Not in Mondays), 2011/07/27 21:08:35]

I'm not sure what's your change and what's the stuff you've merged in, so I'll
ask the high level question: Are you sure the notification behavior used to be
to notify ("successfully finished") on a save page cancel?

[achuithb, 2011/07/28 00:45:58]

NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED is currently only used in
testing (in testing_automation_provider and save_page_browsertest). The tests
wait for this success notification before proceeding with their checks. In the
case where the history callback occurs after the download finishes, this
notification is sent too early and various assertions/checks fail (both
CheckDownloadUI and CheckDownloadHistory fail). This change makes it so this
notification is sent after the history item is updated. 

I made another change to ensure that this notification is only sent on status
COMPLETE and not on CANCELLED, to address your point. PTAL.

+        Source<DownloadManager>(this),
+        Details<DownloadItem>(download));
+  }
+}
+
+int32 DownloadManager::GetNextSavePageId() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  return next_save_page_id_++;
+}
+