Support for adding save page download items into downloads history.

chrome/browser/download/download_manager.cc

 // Copyright (c) 2011 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 "base/callback.h"
 #include "base/file_util.h"
 #include "base/i18n/case_conversion.h"
 #include "base/logging.h"
@@ skipping 24 matching lines @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/pref_names.h"
 #include "content/browser/browser_thread.h"
 #include "content/browser/renderer_host/render_process_host.h"
 #include "content/browser/renderer_host/render_view_host.h"
 #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"
 #include "net/base/mime_util.h"
 #include "net/base/net_util.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 
 DownloadManager::DownloadManager(DownloadStatusUpdater* status_updater)
     : 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);
 }
 
 DownloadManager::~DownloadManager() {
   DCHECK(!shutdown_needed_);
   if (status_updater_)
     status_updater_->RemoveDelegate(this);
 }
 
@@ skipping 47 matching lines @@
   // anything left.
 
   // Copy downloads_ to separate container so as not to set off checks
   // in DownloadItem destruction.
   DownloadSet downloads_to_delete;
   downloads_to_delete.swap(downloads_);
 
   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
   // it returns.
   if (select_file_dialog_.get())
     select_file_dialog_->ListenerDestroyed();
 
   download_history_.reset();
   download_prefs_.reset();
 
   shutdown_needed_ = false;
 }
 
 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();
        it != history_downloads_.end(); ++it) {
     if (it->second->is_temporary() &&
         it->second->full_path().DirName() == dir_path)
       result->push_back(it->second);
   }
 }
 
 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();
        it != history_downloads_.end(); ++it) {
     if (!it->second->is_temporary() &&
         (dir_path.empty() || it->second->full_path().DirName() == dir_path))
       result->push_back(it->second);
   }
 }
 
 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();
        it != history_downloads_.end(); ++it) {
     DownloadItem* item =it->second;
     // Skip temporary items.
     if (item->is_temporary())
       continue;
     // Skip items that have all their data, and are OK to save.
     if (!item->IsPartialDownload() &&
         (item->safety_state() != DownloadItem::DANGEROUS))
       continue;
     // Skip items that don't match |dir_path|.
     // If |dir_path| is empty, all remaining items match.
     if (!dir_path.empty() && (it->second->full_path().DirName() != dir_path))
       continue;
 
     result->push_back(item);
   }
 
   // If we have a parent profile, let it add its downloads to the results.
   Profile* original_profile = profile_->GetOriginalProfile();
   if (original_profile != profile_)
     original_profile->GetDownloadManager()->GetCurrentDownloads(dir_path,
                                                                 result);
 }
 
 void DownloadManager::SearchDownloads(const string16& query,
-                                      std::vector<DownloadItem*>* result) {
+                                      DownloadVec* result) {
   DCHECK(result);
 
   string16 query_lower(base::i18n::ToLower(query));
 
   for (DownloadMap::iterator it = history_downloads_.begin();
        it != history_downloads_.end(); ++it) {
     DownloadItem* download_item = it->second;
 
     if (download_item->is_temporary() || download_item->is_extension_install())
       continue;
@@ skipping 684 matching lines @@
       BrowserThread::FILE, FROM_HERE,
       NewRunnableMethod(
           file_manager_, &DownloadFileManager::CancelDownload, download_id));
 }
 
 void DownloadManager::UpdateAppIcon() {
   if (status_updater_)
     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())
     return;
 
   // Make history update.
   DownloadItem* download = it->second;
   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,
                                             const base::Time remove_end) {
   download_history_->RemoveEntriesBetween(remove_begin, remove_end);
 
   // 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) &&
         (download->IsComplete() ||
          download->IsCancelled() ||
          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) {
   return RemoveDownloadsBetween(remove_begin, base::Time());
 }
 
 int DownloadManager::RemoveAllDownloads() {
   if (this != profile_->GetOriginalProfile()->GetDownloadManager()) {
     // This is an incognito downloader. Clear All should clear main download
     // manager as well.
     profile_->GetOriginalProfile()->GetDownloadManager()->RemoveAllDownloads();
   }
   // The null times make the date range unbounded.
   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.
 void DownloadManager::DownloadUrl(const GURL& url,
                                   const GURL& referrer,
                                   const std::string& referrer_charset,
                                   TabContents* tab_contents) {
   DownloadUrlToFile(url, referrer, referrer_charset, DownloadSaveInfo(),
                     tab_contents);
 }
@@ skipping 173 matching lines @@
 
   // TODO(rdsmith): Convert to DCHECK() when http://crbug.com/84508
   // is fixed.
   CHECK_NE(DownloadHistory::kUninitializedHandle, db_handle);
 
   DCHECK(download->db_handle() == DownloadHistory::kUninitializedHandle);
   download->set_db_handle(db_handle);
 
   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
 // service, we associate the DownloadItem with the db handle, update our
 // 'history_downloads_' map and inform observers.
 void DownloadManager::OnCreateDownloadEntryComplete(int32 download_id,
                                                     int64 db_handle) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DownloadItem* download = GetActiveDownloadItem(download_id);
   if (!download)
     return;
 
   VLOG(20) << __FUNCTION__ << "()" << " db_handle = " << db_handle
            << " download_id = " << download_id
            << " download = " << download->DebugString(true);
 
   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
   // received the DB handle.  We 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->IsInProgress()) {
     MaybeCompleteDownload(download);
   } else {
@@ skipping 53 matching lines @@
   DownloadItem* download = active_downloads_[download_id];
   DCHECK(download != NULL);
   return download;
 }
 
 // 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 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;
     std::insert_iterator<DownloadSet> insert_it(remainder, remainder.begin());
     std::set_difference(all_sets[i]->begin(), all_sets[i]->end(),
                         downloads_.begin(), downloads_.end(),
                         insert_it);
     DCHECK(remainder.empty());
     std::insert_iterator<DownloadSet>
         insert_union(downloads_union, downloads_union.end());
@@ skipping 34 matching lines @@
     observed_download_manager_->RemoveObserver(this);
 }
 
 void DownloadManager::OtherDownloadManagerObserver::ModelChanged() {
   observing_download_manager_->NotifyModelChanged();
 }
 
 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_++;
+}
+