Merge 79180 - Reverted 76780 -- Adding Save Page downloads to downloads history.

This fixes issue 76963, in which a tab closure while a page is being saved results in a crash.  See that issue for details.

BUG=76963
TEST=Relevant tests pass, can't repro 76963.

Review URL: http://codereview.chromium.org/6708075

TBR=rdsmith@chromium.org
Review URL: http://codereview.chromium.org/6778024

git-svn-id: svn://svn.chromium.org/chrome/branches/696/src@79862 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 25 insertions, 127 deletions

diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index a22985b..a59349e 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -855,22 +855,6 @@ int DownloadManager::RemoveAllDownloads() {
   return RemoveDownloadsBetween(base::Time(), base::Time());
 }
 
-void DownloadManager::AddDownloadItemToHistory(DownloadItem* item,
-                                               int64 db_handle) {
-  // 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(item->db_handle() == DownloadHistory::kUninitializedHandle);
-  item->set_db_handle(db_handle);
-  DCHECK(!ContainsKey(history_downloads_, db_handle));
-  history_downloads_[db_handle] = item;
-}
-
 void DownloadManager::SavePageAsDownloadStarted(DownloadItem* download_item) {
 #if !defined(NDEBUG)
   save_page_as_downloads_.insert(download_item);
@@ -1022,7 +1006,20 @@ void DownloadManager::OnCreateDownloadEntryComplete(
            << " download_id = " << info.download_id
            << " download = " << download->DebugString(true);
 
-  AddDownloadItemToHistory(download, db_handle);
+  // 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(!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.
diff --git a/chrome/browser/download/download_manager.h b/chrome/browser/download/download_manager.h
index b7fc417..e45c2ac 100644
--- a/chrome/browser/download/download_manager.h
+++ b/chrome/browser/download/download_manager.h
@@ -153,10 +153,6 @@ class DownloadManager
   // Remove the download with id |download_id| from |active_downloads_|.
   void RemoveFromActiveList(int32 download_id);
 
-  // Add DownloadItem to history, validate |db_handle| and store
-  // it in the DownloadItem.
-  void AddDownloadItemToHistory(DownloadItem* item, int64 db_handle);
-
   // Called when a Save Page As download is started. Transfers ownership
   // of |download_item| to the DownloadManager.
   void SavePageAsDownloadStarted(DownloadItem* download_item);
diff --git a/chrome/browser/download/save_package.cc b/chrome/browser/download/save_package.cc
index edf2ace..f32c656 100644
--- a/chrome/browser/download/save_package.cc
+++ b/chrome/browser/download/save_package.cc
@@ -19,7 +19,6 @@
 #include "base/task.h"
 #include "base/threading/thread.h"
 #include "chrome/browser/browser_process.h"
-#include "chrome/browser/download/download_history.h"
 #include "chrome/browser/download/download_item.h"
 #include "chrome/browser/download/download_item_model.h"
 #include "chrome/browser/download/download_manager.h"
@@ -314,10 +313,17 @@ bool SavePackage::Init() {
 
   request_context_getter_ = profile->GetRequestContext();
 
-  // Create the fake DownloadItem and add it to the download history.
-  CreateDownloadItem(saved_main_file_path_,
-                     page_url_,
-                     profile->IsOffTheRecord());
+  // Create the fake DownloadItem and display the view.
+  DownloadManager* download_manager =
+      tab_contents()->profile()->GetDownloadManager();
+  download_ = new DownloadItem(download_manager,
+                               saved_main_file_path_,
+                               page_url_,
+                               profile->IsOffTheRecord());
+
+  // Transfer the ownership to the download manager. We need the DownloadItem
+  // to be alive as long as the Profile is alive.
+  download_manager->SavePageAsDownloadStarted(download_);
 
   tab_contents()->OnStartDownload(download_);
 
@@ -346,41 +352,6 @@ bool SavePackage::Init() {
   return true;
 }
 
-void SavePackage::OnDownloadEntryAdded(DownloadCreateInfo info,
-                                       int64 db_handle) {
-  DownloadManager* download_manager =
-      tab_contents()->profile()->GetDownloadManager();
-
-  download_manager->AddDownloadItemToHistory(download_, db_handle);
-}
-
-void SavePackage::CreateDownloadItem(const FilePath& path,
-                                     const GURL& url,
-                                     bool is_otr) {
-  DownloadManager* download_manager =
-      tab_contents()->profile()->GetDownloadManager();
-
-  download_ = new DownloadItem(download_manager, path, url, is_otr);
-
-  // Transfer the ownership to the download manager. We need the DownloadItem
-  // to be alive as long as the Profile is alive.
-  download_manager->SavePageAsDownloadStarted(download_);
-
-  // Copy over the fields used by the history service.
-  DownloadCreateInfo info(download_->full_path(),
-                          download_->url(),
-                          download_->start_time(),
-                          0, 0,
-                          download_->state(),
-                          download_->id(),
-                          false);
-
-  // Add entry to the history service.
-  DownloadHistory* download_history = download_manager->download_history();
-  download_history->AddEntry(info, download_,
-      NewCallback(this, &SavePackage::OnDownloadEntryAdded));
-}
-
 // On POSIX, the length of |pure_file_name| + |file_name_ext| is further
 // restricted by NAME_MAX. The maximum allowed path looks like:
 // '/path/to/save_dir' + '/' + NAME_MAX.
@@ -719,9 +690,6 @@ void SavePackage::Stop() {
 
   // Inform the DownloadItem we have canceled whole save page job.
   download_->Cancel(false);
-  DownloadManager* download_manager =
-      tab_contents()->profile()->GetDownloadManager();
-  download_manager->download_history()->UpdateEntry(download_);
 }
 
 void SavePackage::CheckFinish() {
@@ -776,16 +744,10 @@ void SavePackage::Finish() {
 
   download_->OnAllDataSaved(all_save_items_count_);
   download_->MarkAsComplete();
-
   // Notify download observers that we are complete (the call
   // to OnReadyToFinish() set the state to complete but did not notify).
   download_->UpdateObservers();
 
-  // Update the download history.
-  DownloadManager* download_manager =
-      tab_contents()->profile()->GetDownloadManager();
-  download_manager->download_history()->UpdateEntry(download_);
-
   NotificationService::current()->Notify(
       NotificationType::SAVE_PACKAGE_SUCCESSFULLY_FINISHED,
       Source<SavePackage>(this),
diff --git a/chrome/browser/download/save_package.h b/chrome/browser/download/save_package.h
index 2f6a99a..917da70 100644
--- a/chrome/browser/download/save_package.h
+++ b/chrome/browser/download/save_package.h
@@ -16,7 +16,6 @@
 #include "base/hash_tables.h"
 #include "base/ref_counted.h"
 #include "base/task.h"
-#include "chrome/browser/history/download_create_info.h"
 #include "chrome/browser/ui/shell_dialogs.h"
 #include "content/browser/tab_contents/tab_contents_observer.h"
 #include "googleurl/src/gurl.h"
@@ -164,12 +163,6 @@ class SavePackage : public base::RefCountedThreadSafe<SavePackage>,
   void SaveNextFile(bool process_all_remainder_items);
   void DoSavingProcess();
 
-  // Called when Save Page As entry is commited to the history system.
-  void OnDownloadEntryAdded(DownloadCreateInfo info, int64 db_handle);
-
-  // Called when a Save Page As download is started.
-  void CreateDownloadItem(const FilePath& path, const GURL& url, bool is_otr);
-
   // TabContentsObserver implementation.
   virtual bool OnMessageReceived(const IPC::Message& message);
 
diff --git a/chrome/browser/download/save_page_browsertest.cc b/chrome/browser/download/save_page_browsertest.cc
index 7b47a58..7f83c84 100644
--- a/chrome/browser/download/save_page_browsertest.cc
+++ b/chrome/browser/download/save_page_browsertest.cc
@@ -8,10 +8,7 @@
 #include "base/scoped_temp_dir.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/browser_window.h"
-#include "chrome/browser/download/download_history.h"
-#include "chrome/browser/download/download_manager.h"
 #include "chrome/browser/net/url_request_mock_http_job.h"
-#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/notification_service.h"
@@ -48,37 +45,6 @@ class SavePageBrowserTest : public InProcessBrowserTest {
     return *Details<GURL>(observer.details()).ptr();
   }
 
-  void QueryDownloadHistory(TabContents* current_tab) {
-    DownloadManager* download_manager =
-      current_tab->profile()->GetDownloadManager();
-
-    // Query the history system.
-    download_manager->download_history()->Load(
-      NewCallback(this,
-        &SavePageBrowserTest::OnQueryDownloadEntriesComplete));
-
-    // Run message loop until a quit message is sent from
-    // OnQueryDownloadEntriesComplete().
-    ui_test_utils::RunMessageLoop();
-  }
-
-  void OnQueryDownloadEntriesComplete(
-    std::vector<DownloadCreateInfo>* entries) {
-
-    // Make a copy of the URLs returned by the history system.
-    history_urls_.clear();
-    for (size_t i = 0; i < entries->size(); ++i) {
-      history_urls_.push_back(entries->at(i).url);
-    }
-
-    // Indicate thet we have received the history and
-    // can continue.
-    MessageLoopForUI::current()->Quit();
-  }
-
-  // URLs found in the history.
-  std::vector<GURL> history_urls_;
-
   // Path to directory containing test data.
   FilePath test_dir_;
 
@@ -105,10 +71,6 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveHTMLOnly) {
   if (browser()->SupportsWindowFeature(Browser::FEATURE_DOWNLOADSHELF))
     EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
 
-  QueryDownloadHistory(current_tab);
-  EXPECT_TRUE(std::find(history_urls_.begin(), history_urls_.end(),
-                        url) != history_urls_.end());
-
   EXPECT_TRUE(file_util::PathExists(full_file_name));
   EXPECT_FALSE(file_util::PathExists(dir));
   EXPECT_TRUE(file_util::ContentsEqual(
@@ -138,10 +100,6 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveViewSourceHTMLOnly) {
   if (browser()->SupportsWindowFeature(Browser::FEATURE_DOWNLOADSHELF))
     EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
 
-  QueryDownloadHistory(current_tab);
-  EXPECT_TRUE(std::find(history_urls_.begin(), history_urls_.end(),
-                        actual_page_url) != history_urls_.end());
-
   EXPECT_TRUE(file_util::PathExists(full_file_name));
   EXPECT_FALSE(file_util::PathExists(dir));
   EXPECT_TRUE(file_util::ContentsEqual(
@@ -168,10 +126,6 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveCompleteHTML) {
   if (browser()->SupportsWindowFeature(Browser::FEATURE_DOWNLOADSHELF))
     EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
 
-  QueryDownloadHistory(current_tab);
-  EXPECT_TRUE(std::find(history_urls_.begin(), history_urls_.end(),
-                        url) != history_urls_.end());
-
   EXPECT_TRUE(file_util::PathExists(full_file_name));
   EXPECT_TRUE(file_util::PathExists(dir));
   EXPECT_TRUE(file_util::TextContentsEqual(
@@ -214,10 +168,6 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, FileNameFromPageTitle) {
   if (browser()->SupportsWindowFeature(Browser::FEATURE_DOWNLOADSHELF))
     EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
 
-  QueryDownloadHistory(current_tab);
-  EXPECT_TRUE(std::find(history_urls_.begin(), history_urls_.end(),
-                        url) != history_urls_.end());
-
   EXPECT_TRUE(file_util::PathExists(full_file_name));
   EXPECT_TRUE(file_util::PathExists(dir));
   EXPECT_TRUE(file_util::TextContentsEqual(