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 36 matching lines @@
 #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());

[Paweł Hajdan Jr., 2011/07/27 16:53:51]

nit: "const int" inside a method is generally an overkill.

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

Do you feel strongly about this? I like const auto vars as it makes the code
easier to read (you can focus on the variables that change). 

+  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);

[Paweł Hajdan Jr., 2011/07/27 16:53:51]

nit: Better use the idiom below:

if (!download) {
  NOTREACHED();
  return;
}

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

Done.

+  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());
+  }
+}
+
+int32 DownloadManager::GetNextSavePageId() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  return next_save_page_id_++;
+}
+