Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(4584)

Issues Search
    My Issues | Starred     Open | Closed | All

Unified Diff: chrome/browser/download/save_page_browsertest.cc

Issue 10915180: Make DownloadHistory observe manager, items (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src
Patch Set: . Created 8 years, 3 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« chrome/browser/download/download_history.cc ('K') | « chrome/browser/download/download_status_updater_unittest.cc ('k') | chrome/browser/extensions/api/downloads/downloads_api.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: chrome/browser/download/save_page_browsertest.cc
diff --git a/chrome/browser/download/save_page_browsertest.cc b/chrome/browser/download/save_page_browsertest.cc
index 6a0f33c32adb9a9d672a4a162710d1e256403cdd..0940b65a8fadbd6a3df07ce434df7ab28ee7af97 100644
--- a/chrome/browser/download/save_page_browsertest.cc
+++ b/chrome/browser/download/save_page_browsertest.cc
@@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+#include <sstream>
+
#include "base/bind.h"
#include "base/bind_helpers.h"
#include "base/command_line.h"
@@ -17,6 +19,8 @@
#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/history/history_service_factory.h"
#include "chrome/browser/net/url_request_mock_util.h"
#include "chrome/browser/prefs/pref_service.h"
#include "chrome/browser/profiles/profile.h"
@@ -32,7 +36,6 @@
#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"
@@ -50,11 +53,53 @@ using content::BrowserContext;
using content::BrowserThread;
using content::DownloadItem;
using content::DownloadManager;
-using content::DownloadPersistentStoreInfo;
using content::WebContents;
namespace {
+class DownloadPersistedObserver : public DownloadHistory::Observer {
+ public:
+ typedef base::Callback<bool(const DownloadPersistentStoreInfo&)>
+ PersistedFilter;
+
+ DownloadPersistedObserver(Profile* profile, const PersistedFilter& filter)
+ : profile_(profile),
+ filter_(filter) {
+ DownloadServiceFactory::GetForProfile(profile)->
+ GetDownloadManagerDelegate()->AddHistoryObserver(this);
+ }
+
+ virtual ~DownloadPersistedObserver() {
+ DownloadService* service = DownloadServiceFactory::GetForProfile(profile_);
+ if (service && service->GetDownloadManagerDelegate())
+ service->GetDownloadManagerDelegate()->RemoveHistoryObserver(this);
+ }
+
+ bool WaitForPersisted() {
+ if (persisted_)
+ return true;
+ waiting_ = true;
+ content::RunMessageLoop();
+ waiting_ = false;
+ return persisted_;
+ }
+
+ virtual void OnDownloadStored(const DownloadPersistentStoreInfo& info) {
+ persisted_ = filter_.Run(info);
+ if (persisted_ && waiting_)
+ MessageLoopForUI::current()->Quit();
+ }
+
+ private:
+ Profile* profile_;
+ DownloadItem* item_;
+ PersistedFilter filter_;
+ bool waiting_;
+ bool persisted_;
+
+ DISALLOW_COPY_AND_ASSIGN(DownloadPersistedObserver);
+};
+
static const FilePath::CharType* kTestDir = FILE_PATH_LITERAL("save_page");
static const char* kAppendedExtension =
@@ -105,7 +150,6 @@ class DownloadItemCreatedObserver : public DownloadManager::Observer {
}
private:
-
// DownloadManager::Observer
virtual void OnDownloadCreated(
DownloadManager* manager, DownloadItem* item) OVERRIDE {
@@ -130,59 +174,35 @@ class DownloadItemCreatedObserver : public DownloadManager::Observer {
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);
+bool DownloadStoredProperly(
+ const GURL& expected_url,
+ const FilePath& expected_path,
+ int64 num_files,
+ DownloadItem::DownloadState expected_state,
+ std::stringstream* errors,
+ const DownloadPersistentStoreInfo& info) {
+ if (info.path != expected_path) {
+ *errors << "Expected path " << expected_path.value()
+ << " but received " << info.path.value() << std::endl;
+ return false;
}
-
- // 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;
+ if (info.url != expected_url) {
+ *errors << "Expected url " << expected_url.spec()
+ << " but received " << info.url.spec() << std::endl;
+ return false;
}
-
- private:
- // DownloadItem::Observer
- virtual void OnDownloadUpdated(DownloadItem* item) OVERRIDE {
- DCHECK_EQ(item, item_);
-
- if (waiting_ && item->IsPersisted())
- MessageLoopForUI::current()->Quit();
+ if (num_files >= 0 &&
+ info.received_bytes != num_files) {
+ *errors << "Expected received_bytes " << num_files
+ << " but received " << info.received_bytes << std::endl;
}
-
- virtual void OnDownloadDestroyed(DownloadItem* item) OVERRIDE {
- if (item != item_)
- return;
-
- item_->RemoveObserver(this);
- item_ = NULL;
+ if (info.state != expected_state) {
+ *errors << "Expected state " << expected_state
+ << " but received " << info.state << std::endl;
+ return false;
}
-
- bool waiting_;
- DownloadItem* item_;
-
- DISALLOW_COPY_AND_ASSIGN(DownloadPersistedObserver);
-};
+ return true;
+}
class SavePageBrowserTest : public InProcessBrowserTest {
public:
@@ -227,7 +247,9 @@ class SavePageBrowserTest : public InProcessBrowserTest {
return current_tab;
}
- bool WaitForSavePackageToFinish(GURL* url_at_finish) const {
+ // Returns true if and when there was a single download created, and its url
+ // is |expected_url|.
+ bool WaitForSavePackageToFinish(const GURL& expected_url) const {
content::WindowedNotificationObserver observer(
content::NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED,
content::NotificationService::AllSources());
@@ -244,6 +266,17 @@ class SavePageBrowserTest : public InProcessBrowserTest {
return false;
DownloadItem* download_item(items[0]);
+ return ((expected_url == download_item->GetOriginalUrl()) &&
+ (expected_url == content::Details<DownloadItem>(
+ observer.details()).ptr()->GetOriginalUrl()));
+ }
+
+ void WaitForPersisted(
+ const GURL& expected_url,
+ const FilePath& expected_path,
+ int64 num_files,
+ DownloadItem::DownloadState state) {
+ errors_.reset(new std::stringstream);
// Note on synchronization:
//
// For each Save Page As operation, we create a corresponding shell
@@ -255,11 +288,15 @@ class SavePageBrowserTest : public InProcessBrowserTest {
// 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();
-
- *url_at_finish = content::Details<DownloadItem>(observer.details()).ptr()->
- GetOriginalUrl();
- return true;
+ DownloadPersistedObserver(browser()->profile(), base::Bind(
+ &DownloadStoredProperly,
+ expected_url,
+ expected_path,
+ num_files,
+ state,
+ errors_.get()))
+ .WaitForPersisted();
+ errors_.reset();
}
DownloadManager* GetDownloadManager() const {
@@ -269,91 +306,6 @@ class SavePageBrowserTest : public InProcessBrowserTest {
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;
- 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_;
@@ -362,11 +314,15 @@ class SavePageBrowserTest : public InProcessBrowserTest {
private:
scoped_ptr<DownloadItemCreatedObserver> item_creation_observer_;
+ scoped_ptr<std::stringstream> errors_;
DISALLOW_COPY_AND_ASSIGN(SavePageBrowserTest);
};
SavePageBrowserTest::~SavePageBrowserTest() {
+ if (errors_.get()) {
+ LOG(ERROR) << errors_->str();
+ }
}
IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveHTMLOnly) {
@@ -377,13 +333,10 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveHTMLOnly) {
ASSERT_TRUE(GetCurrentTab()->SavePage(full_file_name, dir,
content::SAVE_PAGE_TYPE_AS_ONLY_HTML));
- GURL output_url;
- ASSERT_TRUE(WaitForSavePackageToFinish(&output_url));
- EXPECT_EQ(url, output_url);
+ ASSERT_TRUE(WaitForSavePackageToFinish(url));
+ WaitForPersisted(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));
@@ -411,14 +364,12 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveHTMLOnlyCancel) {
// TODO(rdsmith): Fix DII::Cancel() to actually cancel the save package.
// Currently it's ignored.
- GURL output_url;
- ASSERT_TRUE(WaitForSavePackageToFinish(&output_url));
- EXPECT_EQ(url, output_url);
+ ASSERT_TRUE(WaitForSavePackageToFinish(url));
// -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);
+ WaitForPersisted(url, full_file_name, -1, DownloadItem::CANCELLED);
EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
@@ -468,14 +419,10 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveViewSourceHTMLOnly) {
ASSERT_TRUE(GetCurrentTab()->SavePage(full_file_name, dir,
content::SAVE_PAGE_TYPE_AS_ONLY_HTML));
- GURL output_url;
- ASSERT_TRUE(WaitForSavePackageToFinish(&output_url));
- EXPECT_EQ(actual_page_url, output_url);
+ ASSERT_TRUE(WaitForSavePackageToFinish(actual_page_url));
EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
- // a.htm is 1 file.
- CheckDownloadHistory(actual_page_url, full_file_name, 1,
- DownloadItem::COMPLETE);
+ WaitForPersisted(actual_page_url, full_file_name, 1, DownloadItem::COMPLETE);
EXPECT_TRUE(file_util::PathExists(full_file_name));
EXPECT_FALSE(file_util::PathExists(dir));
@@ -492,13 +439,10 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveCompleteHTML) {
ASSERT_TRUE(GetCurrentTab()->SavePage(
full_file_name, dir, content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML));
- GURL output_url;
- ASSERT_TRUE(WaitForSavePackageToFinish(&output_url));
- EXPECT_EQ(url, output_url);
+ ASSERT_TRUE(WaitForSavePackageToFinish(url));
EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
- // b.htm is 3 files.
- CheckDownloadHistory(url, full_file_name, 3, DownloadItem::COMPLETE);
+ WaitForPersisted(url, full_file_name, 3, DownloadItem::COMPLETE);
EXPECT_TRUE(file_util::PathExists(full_file_name));
EXPECT_TRUE(file_util::PathExists(dir));
@@ -528,13 +472,10 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, FileNameFromPageTitle) {
ASSERT_TRUE(GetCurrentTab()->SavePage(
full_file_name, dir, content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML));
- GURL output_url;
- ASSERT_TRUE(WaitForSavePackageToFinish(&output_url));
- EXPECT_EQ(url, output_url);
+ ASSERT_TRUE(WaitForSavePackageToFinish(url));
EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
- // b.htm is 3 files.
- CheckDownloadHistory(url, full_file_name, 3, DownloadItem::COMPLETE);
+ WaitForPersisted(url, full_file_name, 3, DownloadItem::COMPLETE);
EXPECT_TRUE(file_util::PathExists(full_file_name));
EXPECT_TRUE(file_util::PathExists(dir));
@@ -557,22 +498,13 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, RemoveFromList) {
ASSERT_TRUE(GetCurrentTab()->SavePage(full_file_name, dir,
content::SAVE_PAGE_TYPE_AS_ONLY_HTML));
- GURL output_url;
- ASSERT_TRUE(WaitForSavePackageToFinish(&output_url));
- EXPECT_EQ(url, output_url);
+ ASSERT_TRUE(WaitForSavePackageToFinish(url));
EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
- // a.htm is 1 file.
- CheckDownloadHistory(url, full_file_name, 1, DownloadItem::COMPLETE);
EXPECT_EQ(GetDownloadManager()->RemoveAllDownloads(), 1);
- // 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());
+ // TODO(benjhayden) Should not be in history.
EXPECT_TRUE(file_util::PathExists(full_file_name));
EXPECT_FALSE(file_util::PathExists(dir));
@@ -646,7 +578,7 @@ IN_PROC_BROWSER_TEST_F(SavePageAsMHTMLBrowserTest, DISABLED_SavePageAsMHTML) {
content::NotificationService::AllSources());
chrome::SavePage(browser());
observer.Wait();
- CheckDownloadHistory(url, full_file_name, -1, DownloadItem::COMPLETE);
+ WaitForPersisted(url, full_file_name, -1, DownloadItem::COMPLETE);
EXPECT_TRUE(file_util::PathExists(full_file_name));
int64 actual_file_size = -1;
« chrome/browser/download/download_history.cc ('K') | « chrome/browser/download/download_status_updater_unittest.cc ('k') | chrome/browser/extensions/api/downloads/downloads_api.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698