Make DownloadHistory observe manager, items

chrome/browser/download/save_page_browsertest.cc

 // Copyright (c) 2012 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 "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/path_service.h"
 #include "base/scoped_temp_dir.h"
 #include "base/test/test_file_util.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/api/prefs/pref_member.h"
 #include "chrome/browser/download/chrome_download_manager_delegate.h"
 #include "chrome/browser/download/download_history.h"
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/download/download_service.h"
 #include "chrome/browser/download/download_service_factory.h"
+#include "chrome/browser/history/download_persistent_store_info.h"
 #include "chrome/browser/net/url_request_mock_util.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/download_manager.h"
-#include "content/public/browser/download_persistent_store_info.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/test_utils.h"
 #include "content/test/net/url_request_mock_http_job.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/download/save_package_file_picker_chromeos.h"
 #else
 #include "chrome/browser/download/save_package_file_picker.h"
 #endif
 
 using content::BrowserContext;
 using content::BrowserThread;
 using content::DownloadItem;
 using content::DownloadManager;
-using content::DownloadPersistentStoreInfo;
 using content::URLRequestMockHTTPJob;
 using content::WebContents;
 
+class DownloadPersistedObserver : public DownloadHistory::Observer {

[Randy Smith (Not in Mondays), 2012/11/09 21:36:39]

A sentence or two in a comment about how to use this?  An observer that triggers
(returns?) if any persisted download item causes the passed filter to return
true?

[benjhayden, 2012/11/12 18:44:16]

Done.

+ public:
+  typedef base::Callback<bool(
+      DownloadItem* item,
+      const DownloadPersistentStoreInfo&)> PersistedFilter;
+
+  DownloadPersistedObserver(Profile* profile, const PersistedFilter& filter)
+    : profile_(profile),
+      filter_(filter) {
+    DownloadServiceFactory::GetForProfile(profile_)->
+      GetDownloadHistory()->AddObserver(this);
+  }
+
+  virtual ~DownloadPersistedObserver() {
+    DownloadService* service = DownloadServiceFactory::GetForProfile(profile_);
+    if (service && service->GetDownloadManagerDelegate())

[Randy Smith (Not in Mondays), 2012/11/09 21:36:39]

Why is the download manager delegate relevant?

[benjhayden, 2012/11/12 18:44:16]

Done.

+      service->GetDownloadHistory()->RemoveObserver(this);
+  }
+
+  bool WaitForPersisted() {
+    if (persisted_)
+      return true;
+    waiting_ = true;
+    content::RunMessageLoop();
+    waiting_ = false;
+    return persisted_;
+  }
+
+  virtual void OnDownloadStored(DownloadItem* item,
+                                const DownloadPersistentStoreInfo& info) {
+    persisted_ = filter_.Run(item, info);
+    if (persisted_ && waiting_)
+      MessageLoopForUI::current()->Quit();
+  }
+
+ private:
+  Profile* profile_;
+  DownloadItem* item_;
+  PersistedFilter filter_;
+  bool waiting_;
+  bool persisted_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadPersistedObserver);
+};
+
+class DownloadRemovedObserver : public DownloadPersistedObserver {
+ public:
+  DownloadRemovedObserver(Profile* profile, int32 download_id)
+      : DownloadPersistedObserver(profile, PersistedFilter()),
+        removed_(false),
+        waiting_(false),
+        download_id_(download_id) {
+  }
+  virtual ~DownloadRemovedObserver() {}
+
+  bool WaitForRemoved() {
+    if (removed_)
+      return true;
+    waiting_ = true;
+    content::RunMessageLoop();
+    waiting_ = false;
+    return removed_;
+  }
+
+  virtual void OnDownloadStored(DownloadItem* item,
+                                const DownloadPersistentStoreInfo& info) {
+  }
+
+  virtual void OnDownloadsRemoved(const DownloadHistory::IdSet& ids) {
+    removed_ = ids.find(download_id_) != ids.end();
+    if (removed_ && waiting_)
+      MessageLoopForUI::current()->Quit();
+  }
+
+ private:
+  bool removed_;
+  bool waiting_;
+  int32 download_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadRemovedObserver);
+};
+
+bool DownloadStoredProperly(
+    const GURL& expected_url,
+    const FilePath& expected_path,
+    int64 num_files,
+    DownloadItem::DownloadState expected_state,
+    DownloadItem* item,
+    const DownloadPersistentStoreInfo& info) {
+  if (info.path != expected_path) {

[Randy Smith (Not in Mondays), 2012/11/09 21:36:39]

Why not EXPECT_EQ?  Seems more in line with how tests are usually written.  You
could have the set of expectations, then one big && return at the end for the
return value.

[benjhayden, 2012/11/12 18:44:16]

DownloadStoredProperly may be called multiple times, so returning false doesn't
necessarily mean that the test failed, it might just mean that the test hasn't
passed yet.
Changed these to VLOG(20)s and added a comment.

[Randy Smith (Not in Mondays), 2012/11/12 19:24:30]

Whoops--good point.

+    LOG(ERROR) << __FUNCTION__ << " " << info.path.value()
+               << " != " << expected_path.value();
+    return false;
+  }
+  if (info.url != expected_url) {
+    LOG(ERROR) << __FUNCTION__ << " " << info.url.spec()
+               << " != " << expected_url.spec();
+    return false;
+  }
+  if ((num_files >= 0) && (info.received_bytes != num_files)) {
+    LOG(ERROR) << __FUNCTION__ << " " << num_files
+               << " != " << info.received_bytes;
+    return false;
+  }
+  if (info.state != expected_state) {
+    LOG(ERROR) << __FUNCTION__ << " " << info.state
+               << " != " << expected_state;
+    return false;
+  }
+  return true;
+}
+
 const FilePath::CharType kTestDir[] = FILE_PATH_LITERAL("save_page");
 
-const char kAppendedExtension[] =
+static const char kAppendedExtension[] =
 #if defined(OS_WIN)
     ".htm";
 #else
     ".html";
 #endif
 
 // Loosely based on logic in DownloadTestObserver.
 class DownloadItemCreatedObserver : public DownloadManager::Observer {
  public:
   explicit DownloadItemCreatedObserver(DownloadManager* manager)
@@ skipping 22 matching lines @@
       waiting_ = true;
       content::RunMessageLoop();
       waiting_ = false;
     }
 
     *items_seen = items_seen_;
     return;
   }
 
  private:
-
   // DownloadManager::Observer
   virtual void OnDownloadCreated(
       DownloadManager* manager, DownloadItem* item) OVERRIDE {
     DCHECK_EQ(manager, manager_);
     items_seen_.push_back(item);
 
     if (waiting_)
       MessageLoopForUI::current()->Quit();
   }
 
   virtual void ManagerGoingDown(DownloadManager* manager) OVERRIDE {
     manager_->RemoveObserver(this);
     manager_ = NULL;
     if (waiting_)
       MessageLoopForUI::current()->Quit();
   }
 
   bool waiting_;
   DownloadManager* manager_;
   std::vector<DownloadItem*> items_seen_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadItemCreatedObserver);
 };
 
-class DownloadPersistedObserver : public DownloadItem::Observer {
- public:
-  explicit DownloadPersistedObserver(DownloadItem* item)
-      : waiting_(false), item_(item) {
-    item->AddObserver(this);
-  }
-
-  ~DownloadPersistedObserver() {
-    if (item_)
-      item_->RemoveObserver(this);
-  }
-
-  // Wait for download item to get the persisted bit set.
-  // Note that this class provides no protection against the download
-  // being destroyed between creation and return of WaitForPersisted();
-  // the caller must guarantee that in some other fashion.
-  void WaitForPersisted() {
-    // In combination with OnDownloadDestroyed() below, verify the
-    // above interface contract.
-    DCHECK(item_);
-
-    if (item_->IsPersisted())
-      return;
-
-    waiting_ = true;
-    content::RunMessageLoop();
-    waiting_ = false;
-
-    return;
-  }
-
- private:
-  // DownloadItem::Observer
-  virtual void OnDownloadUpdated(DownloadItem* item) OVERRIDE {
-    DCHECK_EQ(item, item_);
-
-    if (waiting_ && item->IsPersisted())
-      MessageLoopForUI::current()->Quit();
-  }
-
-  virtual void OnDownloadDestroyed(DownloadItem* item) OVERRIDE {
-    if (item != item_)
-      return;
-
-    item_->RemoveObserver(this);
-    item_ = NULL;
-  }
-
-  bool waiting_;
-  DownloadItem* item_;
-
-  DISALLOW_COPY_AND_ASSIGN(DownloadPersistedObserver);
-};
-
 class SavePageBrowserTest : public InProcessBrowserTest {
  public:
   SavePageBrowserTest() {}
   virtual ~SavePageBrowserTest();
 
  protected:
   void SetUp() OVERRIDE {
     ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_dir_));
     ASSERT_TRUE(save_dir_.CreateUniqueTempDir());
     InProcessBrowserTest::SetUp();
@@ skipping 21 matching lines @@
     *full_file_name = save_dir_.path().AppendASCII(prefix + ".htm");
     *dir = save_dir_.path().AppendASCII(prefix + "_files");
   }
 
   WebContents* GetCurrentTab(Browser* browser) const {
     WebContents* current_tab = chrome::GetActiveWebContents(browser);
     EXPECT_TRUE(current_tab);
     return current_tab;
   }
 
-  bool WaitForSavePackageToFinish(Browser* browser, GURL* url_at_finish) const {
+  // Returns true if and when there was a single download created, and its url
+  // is |expected_url|.

[Randy Smith (Not in Mondays), 2012/11/09 21:36:39]

This doesn't feel like an accurate description.  "Returns after a download item
has been created and persisted (as determined by the filter).  Returns true if
that download matches |expected_url|"?

Actually, the description needs to specifically call out that a null filter
disables waiting for download item persistence.

Mind you, written out like that, the interface contract doesn't make a lot of
sense, and it might be reasonable to change it.  Maybe there's a more useful
interface contract?  Maybe separating out "finish" and "confirming that download
was stored properly" (running the filter)?  

[benjhayden, 2012/11/12 18:44:16]

Done.

+  bool WaitForSavePackageToFinish(
+      Browser* browser,
+      const GURL& expected_url,
+      const DownloadPersistedObserver::PersistedFilter& filter) const {
     content::WindowedNotificationObserver observer(
         content::NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED,
         content::NotificationService::AllSources());
     observer.Wait();
 
     // Generally, there should only be one download item created
     // in all of these tests.  If it's already here, grab it; if not,
     // wait for it to show up.
     std::vector<DownloadItem*> items;
     DownloadManager* manager(
         BrowserContext::GetDownloadManager(browser->profile()));
     manager->GetAllDownloads(&items);
     if (items.size() == 0u) {
       DownloadItemCreatedObserver(manager).WaitForDownloadItem(&items);
     }
 
     EXPECT_EQ(1u, items.size());
     if (1u != items.size())
       return false;
     DownloadItem* download_item(items[0]);
 
-    // Note on synchronization:
-    //
-    // For each Save Page As operation, we create a corresponding shell
-    // DownloadItem to display progress to the user.  That DownloadItem
-    // goes through its own state transitions, including being persisted
-    // out to the history database, and the download shelf is not shown
-    // until after the persistence occurs.  Save Package completion (and
-    // marking the DownloadItem as completed) occurs asynchronously from
-    // persistence.  Thus if we want to examine either UI state or DB
-    // state, we need to wait until both the save package operation is
-    // complete and the relevant download item has been persisted.
-    DownloadPersistedObserver(download_item).WaitForPersisted();
+    if (!filter.is_null() && !DownloadHistory::IsPersisted(download_item))
+      DownloadPersistedObserver(browser->profile(), filter).WaitForPersisted();

[Randy Smith (Not in Mondays), 2012/11/09 21:36:39]

This feels ... wrong to me.  Specifically, the behavior of this routine is now
race-dependent--if the download_item is persisted before this point, filter is
not checked, and if it's not, filter is gating.  Could you write this code so
the results aren't dependent on the winner of a race?

[benjhayden, 2012/11/12 18:44:16]

Done.

 
-    *url_at_finish = content::Details<DownloadItem>(observer.details()).ptr()->
-        GetOriginalUrl();
-    return true;
+    return ((expected_url == download_item->GetOriginalUrl()) &&
+            (expected_url == content::Details<DownloadItem>(
+                observer.details()).ptr()->GetOriginalUrl()));
   }
 
+  // Note on synchronization:
+  //
+  // For each Save Page As operation, we create a corresponding shell
+  // DownloadItem to display progress to the user.  That DownloadItem goes
+  // through its own state transitions, including being persisted out to the
+  // history database, and the download shelf is not shown until after the
+  // persistence occurs.  Save Package completion (and marking the DownloadItem
+  // as completed) occurs asynchronously from persistence.  Thus if we want to
+  // examine either UI state or DB state, we need to wait until both the save
+  // package operation is complete and the relevant download item has been
+  // persisted.
+
   DownloadManager* GetDownloadManager() const {
     DownloadManager* download_manager =
         BrowserContext::GetDownloadManager(browser()->profile());
     EXPECT_TRUE(download_manager);
     return download_manager;
   }
 
-  void QueryDownloadHistory() {
-    // Query the history system.
-    ChromeDownloadManagerDelegate* delegate =
-      static_cast<ChromeDownloadManagerDelegate*>(
-          GetDownloadManager()->GetDelegate());
-    delegate->download_history()->Load(
-        base::Bind(&SavePageBrowserTest::OnQueryDownloadEntriesComplete,
-                   base::Unretained(this)));
-
-    // Run message loop until a quit message is sent from
-    // OnQueryDownloadEntriesComplete().
-    content::RunMessageLoop();
-  }
-
-  void OnQueryDownloadEntriesComplete(
-      std::vector<DownloadPersistentStoreInfo>* entries) {
-    history_entries_ = *entries;
-
-    // Indicate thet we have received the history and can continue.
-    MessageLoopForUI::current()->Quit();
-  }
-
-  struct DownloadPersistentStoreInfoMatch
-    : public std::unary_function<DownloadPersistentStoreInfo, bool> {
-
-    DownloadPersistentStoreInfoMatch(const GURL& url,
-                                     const FilePath& path,
-                                     int64 num_files,
-                                     DownloadItem::DownloadState state)
-      : url_(url),
-        path_(path),
-        num_files_(num_files),
-        state_(state) {}
-
-    bool operator() (const DownloadPersistentStoreInfo& info) const {
-      return info.url == url_ &&
-             info.path == path_ &&
-             // For non-MHTML save packages, received_bytes is actually the
-             // number of files.
-             ((num_files_ < 0) ||
-              (info.received_bytes == num_files_)) &&
-             info.total_bytes == 0 &&
-             info.state == state_;
-    }
-
-    GURL url_;
-    FilePath path_;
-    int64 num_files_;
-    DownloadItem::DownloadState state_;
-  };
-
-  void CheckDownloadHistory(const GURL& url,
-                            const FilePath& path,
-                            int64 num_files,
-                            DownloadItem::DownloadState state) {
-    // Make sure the relevant download item made it into the history.
-    std::vector<DownloadItem*> downloads;
-    GetDownloadManager()->GetAllDownloads(&downloads);
-    ASSERT_EQ(1u, downloads.size());
-
-    QueryDownloadHistory();
-
-    std::vector<DownloadPersistentStoreInfo>::iterator found =
-      std::find_if(history_entries_.begin(), history_entries_.end(),
-                   DownloadPersistentStoreInfoMatch(url, path, num_files,
-                                                    state));
-
-    if (found == history_entries_.end()) {
-      LOG(ERROR) << "Missing url=" << url.spec()
-                 << " path=" << path.value()
-                 << " received=" << num_files
-                 << " state=" << state;
-      for (size_t index = 0; index < history_entries_.size(); ++index) {
-        LOG(ERROR) << "History@" << index << ": url="
-                   << history_entries_[index].url.spec()
-                   << " path=" << history_entries_[index].path.value()
-                   << " received=" << history_entries_[index].received_bytes
-                   << " total=" << history_entries_[index].total_bytes
-                   << " state=" << history_entries_[index].state;
-      }
-      EXPECT_TRUE(false);
-    }
-  }
-
-  std::vector<DownloadPersistentStoreInfo> history_entries_;
-
   // Path to directory containing test data.
   FilePath test_dir_;
 
   // Temporary directory we will save pages to.
   ScopedTempDir save_dir_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(SavePageBrowserTest);
 };
 
 SavePageBrowserTest::~SavePageBrowserTest() {
 }
 
 IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveHTMLOnly) {
   GURL url = NavigateToMockURL("a");
 
   FilePath full_file_name, dir;
   GetDestinationPaths("a", &full_file_name, &dir);
   ASSERT_TRUE(GetCurrentTab(browser())->SavePage(full_file_name, dir,
                                         content::SAVE_PAGE_TYPE_AS_ONLY_HTML));
-
-  GURL output_url;
-  ASSERT_TRUE(WaitForSavePackageToFinish(browser(), &output_url));
-  EXPECT_EQ(url, output_url);
-
+  ASSERT_TRUE(WaitForSavePackageToFinish(browser(), url, base::Bind(
+      &DownloadStoredProperly, url, full_file_name, 1,
+      DownloadItem::COMPLETE)));
   EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
-  // a.htm is 1 file.
-  CheckDownloadHistory(url, full_file_name, 1, DownloadItem::COMPLETE);
-
   EXPECT_TRUE(file_util::PathExists(full_file_name));
   EXPECT_FALSE(file_util::PathExists(dir));
   EXPECT_TRUE(file_util::ContentsEqual(
       test_dir_.Append(FilePath(kTestDir)).Append(FILE_PATH_LITERAL("a.htm")),
       full_file_name));
 }
 
 IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveHTMLOnlyCancel) {
   GURL url = NavigateToMockURL("a");
   DownloadManager* manager(GetDownloadManager());
   std::vector<DownloadItem*> downloads;
   manager->GetAllDownloads(&downloads);
   ASSERT_EQ(0u, downloads.size());
 
   FilePath full_file_name, dir;
   GetDestinationPaths("a", &full_file_name, &dir);
   DownloadItemCreatedObserver creation_observer(manager);
   ASSERT_TRUE(GetCurrentTab(browser())->SavePage(full_file_name, dir,
                                         content::SAVE_PAGE_TYPE_AS_ONLY_HTML));
   std::vector<DownloadItem*> items;
   creation_observer.WaitForDownloadItem(&items);
   ASSERT_TRUE(items.size() == 1);
   items[0]->Cancel(true);
 
   // TODO(rdsmith): Fix DII::Cancel() to actually cancel the save package.
   // Currently it's ignored.
-  GURL output_url;
-  ASSERT_TRUE(WaitForSavePackageToFinish(browser(), &output_url));
-  EXPECT_EQ(url, output_url);
-
+  ASSERT_TRUE(WaitForSavePackageToFinish(browser(), url, base::Bind(
+      &DownloadStoredProperly, url, full_file_name, -1,
+      DownloadItem::CANCELLED)));
   // -1 to disable number of files check; we don't update after cancel, and
   // we don't know when the single file completed in relationship to
   // the cancel.
-  CheckDownloadHistory(url, full_file_name, -1, DownloadItem::CANCELLED);
 
   EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
 
   EXPECT_TRUE(file_util::PathExists(full_file_name));
   EXPECT_FALSE(file_util::PathExists(dir));
   EXPECT_TRUE(file_util::ContentsEqual(
       test_dir_.Append(FilePath(kTestDir)).Append(FILE_PATH_LITERAL("a.htm")),
       full_file_name));
 }
 
@@ skipping 26 matching lines @@
   GURL view_source_url = URLRequestMockHTTPJob::GetMockViewSourceUrl(
       FilePath(kTestDir).Append(file_name));
   GURL actual_page_url = URLRequestMockHTTPJob::GetMockUrl(
       FilePath(kTestDir).Append(file_name));
   ui_test_utils::NavigateToURL(browser(), view_source_url);
 
   FilePath full_file_name, dir;
   GetDestinationPaths("a", &full_file_name, &dir);
   ASSERT_TRUE(GetCurrentTab(browser())->SavePage(full_file_name, dir,
                                         content::SAVE_PAGE_TYPE_AS_ONLY_HTML));
-
-  GURL output_url;
-  ASSERT_TRUE(WaitForSavePackageToFinish(browser(), &output_url));
-  EXPECT_EQ(actual_page_url, output_url);
+  ASSERT_TRUE(WaitForSavePackageToFinish(browser(), actual_page_url, base::Bind(
+      &DownloadStoredProperly, actual_page_url, full_file_name, 1,
+      DownloadItem::COMPLETE)));
 
   EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
-  // a.htm is 1 file.
-  CheckDownloadHistory(actual_page_url, full_file_name, 1,
-                       DownloadItem::COMPLETE);
 
   EXPECT_TRUE(file_util::PathExists(full_file_name));
   EXPECT_FALSE(file_util::PathExists(dir));
   EXPECT_TRUE(file_util::ContentsEqual(
       test_dir_.Append(FilePath(kTestDir)).Append(file_name),
       full_file_name));
 }
 
 IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveCompleteHTML) {
   GURL url = NavigateToMockURL("b");
 
   FilePath full_file_name, dir;
   GetDestinationPaths("b", &full_file_name, &dir);
   ASSERT_TRUE(GetCurrentTab(browser())->SavePage(
       full_file_name, dir, content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML));
-
-  GURL output_url;
-  ASSERT_TRUE(WaitForSavePackageToFinish(browser(), &output_url));
-  EXPECT_EQ(url, output_url);
+  ASSERT_TRUE(WaitForSavePackageToFinish(browser(), url, base::Bind(
+      &DownloadStoredProperly, url, full_file_name, 3,
+      DownloadItem::COMPLETE)));
 
   EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
-  // b.htm is 3 files.
-  CheckDownloadHistory(url, full_file_name, 3, DownloadItem::COMPLETE);
 
   EXPECT_TRUE(file_util::PathExists(full_file_name));
   EXPECT_TRUE(file_util::PathExists(dir));
   EXPECT_TRUE(file_util::TextContentsEqual(
       test_dir_.Append(FilePath(kTestDir)).AppendASCII("b.saved1.htm"),
       full_file_name));
   EXPECT_TRUE(file_util::ContentsEqual(
       test_dir_.Append(FilePath(kTestDir)).AppendASCII("1.png"),
       dir.AppendASCII("1.png")));
   EXPECT_TRUE(file_util::ContentsEqual(
@@ skipping 18 matching lines @@
       FilePath(kTestDir).AppendASCII("b.htm"));
   NavigateToURLWithDisposition(incognito, url, NEW_FOREGROUND_TAB,
                                ui_test_utils::BROWSER_TEST_WAIT_FOR_TAB);
 
   // Save the page before completion.
   FilePath full_file_name, dir;
   GetDestinationPaths("b", &full_file_name, &dir);
   ASSERT_TRUE(GetCurrentTab(incognito)->SavePage(
       full_file_name, dir, content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML));
 
-  GURL output_url;
-  WaitForSavePackageToFinish(incognito, &output_url);
-  EXPECT_EQ(url, output_url);
+  ASSERT_TRUE(WaitForSavePackageToFinish(
+      incognito, url, DownloadPersistedObserver::PersistedFilter()));
 
   // Confirm download shelf is visible.
   EXPECT_TRUE(incognito->window()->IsDownloadShelfVisible());
 
   // We can't check more than this because SavePackage is racing with
   // the page load.  If the page load won the race, then SavePackage
   // might have completed. If the page load lost the race, then
   // SavePackage will cancel because there aren't any resources to
   // save.
 }
 
 IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, NoSave) {
   ui_test_utils::NavigateToURL(browser(), GURL(chrome::kAboutBlankURL));
   EXPECT_FALSE(chrome::CanSavePage(browser()));
 }
 
 IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, FileNameFromPageTitle) {
   GURL url = NavigateToMockURL("b");
 
   FilePath full_file_name = save_dir_.path().AppendASCII(
       std::string("Test page for saving page feature") + kAppendedExtension);
   FilePath dir = save_dir_.path().AppendASCII(
       "Test page for saving page feature_files");
   ASSERT_TRUE(GetCurrentTab(browser())->SavePage(
       full_file_name, dir, content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML));
 
-  GURL output_url;
-  ASSERT_TRUE(WaitForSavePackageToFinish(browser(), &output_url));
-  EXPECT_EQ(url, output_url);
+  ASSERT_TRUE(WaitForSavePackageToFinish(browser(), url, base::Bind(
+      &DownloadStoredProperly, url, full_file_name, 3,
+      DownloadItem::COMPLETE)));
 
   EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
-  // b.htm is 3 files.
-  CheckDownloadHistory(url, full_file_name, 3, DownloadItem::COMPLETE);
 
   EXPECT_TRUE(file_util::PathExists(full_file_name));
   EXPECT_TRUE(file_util::PathExists(dir));
   EXPECT_TRUE(file_util::TextContentsEqual(
       test_dir_.Append(FilePath(kTestDir)).AppendASCII("b.saved2.htm"),
       full_file_name));
   EXPECT_TRUE(file_util::ContentsEqual(
       test_dir_.Append(FilePath(kTestDir)).AppendASCII("1.png"),
       dir.AppendASCII("1.png")));
   EXPECT_TRUE(file_util::ContentsEqual(
       test_dir_.Append(FilePath(kTestDir)).AppendASCII("1.css"),
       dir.AppendASCII("1.css")));
 }
 
 IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, RemoveFromList) {
   GURL url = NavigateToMockURL("a");
 
   FilePath full_file_name, dir;
   GetDestinationPaths("a", &full_file_name, &dir);
   ASSERT_TRUE(GetCurrentTab(browser())->SavePage(full_file_name, dir,
                                         content::SAVE_PAGE_TYPE_AS_ONLY_HTML));
 
-  GURL output_url;
-  ASSERT_TRUE(WaitForSavePackageToFinish(browser(), &output_url));
-  EXPECT_EQ(url, output_url);
+  ASSERT_TRUE(WaitForSavePackageToFinish(browser(), url, base::Bind(
+      &DownloadStoredProperly, url, full_file_name, 1,
+      DownloadItem::COMPLETE)));
 
   EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
-  // a.htm is 1 file.
-  CheckDownloadHistory(url, full_file_name, 1, DownloadItem::COMPLETE);
 
-  EXPECT_EQ(GetDownloadManager()->RemoveAllDownloads(), 1);
+  DownloadManager* manager(GetDownloadManager());
+  std::vector<DownloadItem*> downloads;
+  manager->GetAllDownloads(&downloads);
+  ASSERT_EQ(1UL, downloads.size());
+  DownloadRemovedObserver removed(browser()->profile(), downloads[0]->GetId());
 
-  // Should not be in history.
-  QueryDownloadHistory();
-  EXPECT_EQ(std::find_if(history_entries_.begin(), history_entries_.end(),
-                         DownloadPersistentStoreInfoMatch(
-                             url, full_file_name, 1, DownloadItem::COMPLETE)),
-      history_entries_.end());
+  EXPECT_EQ(manager->RemoveAllDownloads(), 1);
+
+  removed.WaitForRemoved();
 
   EXPECT_TRUE(file_util::PathExists(full_file_name));
   EXPECT_FALSE(file_util::PathExists(dir));
   EXPECT_TRUE(file_util::ContentsEqual(
       test_dir_.Append(FilePath(kTestDir)).Append(FILE_PATH_LITERAL("a.htm")),
       full_file_name));
 }
 
 // This tests that a webpage with the title "test.exe" is saved as
 // "test.exe.htm".
@@ skipping 48 matching lines @@
   FilePath download_dir = DownloadPrefs::FromDownloadManager(
       GetDownloadManager())->DownloadPath();
   FilePath full_file_name = download_dir.AppendASCII(std::string(
       "Test page for saving page feature.mhtml"));
 #if defined(OS_CHROMEOS)
   SavePackageFilePickerChromeOS::SetShouldPromptUser(false);
 #else
   SavePackageFilePicker::SetShouldPromptUser(false);
 #endif
   chrome::SavePage(browser());
-  GURL output_url;
-  ASSERT_TRUE(WaitForSavePackageToFinish(browser(), &output_url));
-  EXPECT_EQ(url, output_url);
-  CheckDownloadHistory(url, full_file_name, -1, DownloadItem::COMPLETE);
+  ASSERT_TRUE(WaitForSavePackageToFinish(browser(), url, base::Bind(
+      &DownloadStoredProperly, url, full_file_name, -1,
+      DownloadItem::COMPLETE)));
 
   EXPECT_TRUE(file_util::PathExists(full_file_name));
   int64 actual_file_size = -1;
   EXPECT_TRUE(file_util::GetFileSize(full_file_name, &actual_file_size));
   EXPECT_LE(kFileSizeMin, actual_file_size);
 }