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

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: @r168573 Created 8 years, 1 month 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
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 edc733d3a46ebcbaf5e5ed8086f5b3e22045c9a9..9f2fca4e705dbab2edd18577def57508ceb2aa2f 100644
--- a/chrome/browser/download/save_page_browsertest.cc
+++ b/chrome/browser/download/save_page_browsertest.cc
@@ -17,6 +17,7 @@
#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_row.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 +33,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,13 +50,130 @@ using content::BrowserContext;
using content::BrowserThread;
using content::DownloadItem;
using content::DownloadManager;
-using content::DownloadPersistentStoreInfo;
using content::URLRequestMockHTTPJob;
using content::WebContents;
+// Waits for an item record in the downloads database to match |filter|. See
+// DownloadStoredProperly() below for an example filter.
+class DownloadPersistedObserver : public DownloadHistory::Observer {
+ public:
+ typedef base::Callback<bool(
+ DownloadItem* item,
+ const history::DownloadRow&)> 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->GetDownloadHistory())
+ service->GetDownloadHistory()->RemoveObserver(this);
+ }
+
+ bool WaitForPersisted() {
+ if (persisted_)
+ return true;
+ waiting_ = true;
+ content::RunMessageLoop();
+ waiting_ = false;
+ return persisted_;
+ }
+
+ virtual void OnDownloadStored(DownloadItem* item,
+ const history::DownloadRow& 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);
+};
+
+// Waits for an item record to be removed from the downloads database.
+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 history::DownloadRow& 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 history::DownloadRow& info) {
+ // This function may be called multiple times for a given test. Returning
+ // false doesn't necessarily mean that the test has failed or will fail, it
+ // might just mean that the test hasn't passed yet.
+ if (info.path != expected_path) {
+ VLOG(20) << __FUNCTION__ << " " << info.path.value()
+ << " != " << expected_path.value();
+ return false;
+ }
+ if (info.url != expected_url) {
+ VLOG(20) << __FUNCTION__ << " " << info.url.spec()
+ << " != " << expected_url.spec();
+ return false;
+ }
+ if ((num_files >= 0) && (info.received_bytes != num_files)) {
+ VLOG(20) << __FUNCTION__ << " " << num_files
+ << " != " << info.received_bytes;
+ return false;
+ }
+ if (info.state != expected_state) {
+ VLOG(20) << __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
@@ -99,7 +216,6 @@ class DownloadItemCreatedObserver : public DownloadManager::Observer {
}
private:
-
// DownloadManager::Observer
virtual void OnDownloadCreated(
DownloadManager* manager, DownloadItem* item) OVERRIDE {
@@ -124,60 +240,6 @@ 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);
- }
-
- // 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() {}
@@ -219,7 +281,11 @@ class SavePageBrowserTest : public InProcessBrowserTest {
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|.
+ bool WaitForSavePackageToFinish(
+ Browser* browser,
+ const GURL& expected_url) const {
content::WindowedNotificationObserver observer(
content::NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED,
content::NotificationService::AllSources());
@@ -241,24 +307,23 @@ class SavePageBrowserTest : public InProcessBrowserTest {
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();
-
- *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());
@@ -266,92 +331,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
- << " 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_;
@@ -370,17 +349,14 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveHTMLOnly) {
FilePath full_file_name, dir;
GetDestinationPaths("a", &full_file_name, &dir);
+ DownloadPersistedObserver persisted(browser()->profile(), base::Bind(
+ &DownloadStoredProperly, url, full_file_name, 1,
+ DownloadItem::COMPLETE));
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));
+ persisted.WaitForPersisted();
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(
@@ -398,31 +374,29 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveHTMLOnlyCancel) {
FilePath full_file_name, dir;
GetDestinationPaths("a", &full_file_name, &dir);
DownloadItemCreatedObserver creation_observer(manager);
+ DownloadPersistedObserver persisted(browser()->profile(), 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.
+
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);
+ ASSERT_EQ(1UL, items.size());
+ ASSERT_EQ(url.spec(), items[0]->GetOriginalUrl().spec());
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);
- // -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);
+ persisted.WaitForPersisted();
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));
+ // TODO(benjhayden): Figure out how to safely wait for SavePackage's finished
+ // notification, then expect the contents of the downloaded file.
}
IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveHTMLOnlyTabDestroy) {
@@ -459,17 +433,15 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveViewSourceHTMLOnly) {
FilePath full_file_name, dir;
GetDestinationPaths("a", &full_file_name, &dir);
+ DownloadPersistedObserver persisted(browser()->profile(), base::Bind(
+ &DownloadStoredProperly, actual_page_url, full_file_name, 1,
+ DownloadItem::COMPLETE));
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));
+ persisted.WaitForPersisted();
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));
@@ -483,16 +455,15 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveCompleteHTML) {
FilePath full_file_name, dir;
GetDestinationPaths("b", &full_file_name, &dir);
+ DownloadPersistedObserver persisted(browser()->profile(), base::Bind(
+ &DownloadStoredProperly, url, full_file_name, 3,
+ DownloadItem::COMPLETE));
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));
+ persisted.WaitForPersisted();
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));
@@ -531,9 +502,7 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest,
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));
// Confirm download shelf is visible.
EXPECT_TRUE(incognito->window()->IsDownloadShelfVisible());
@@ -557,16 +526,16 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, FileNameFromPageTitle) {
std::string("Test page for saving page feature") + kAppendedExtension);
FilePath dir = save_dir_.path().AppendASCII(
"Test page for saving page feature_files");
+ DownloadPersistedObserver persisted(browser()->profile(), base::Bind(
+ &DownloadStoredProperly, url, full_file_name, 3,
+ DownloadItem::COMPLETE));
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));
+ persisted.WaitForPersisted();
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));
@@ -586,25 +555,26 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, RemoveFromList) {
FilePath full_file_name, dir;
GetDestinationPaths("a", &full_file_name, &dir);
+ DownloadPersistedObserver persisted(browser()->profile(), base::Bind(
+ &DownloadStoredProperly, url, full_file_name, 1,
+ DownloadItem::COMPLETE));
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));
+ persisted.WaitForPersisted();
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());
+
+ EXPECT_EQ(manager->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());
+ removed.WaitForRemoved();
EXPECT_TRUE(file_util::PathExists(full_file_name));
EXPECT_FALSE(file_util::PathExists(dir));
@@ -672,11 +642,12 @@ IN_PROC_BROWSER_TEST_F(SavePageAsMHTMLBrowserTest, SavePageAsMHTML) {
#else
SavePackageFilePicker::SetShouldPromptUser(false);
#endif
+ DownloadPersistedObserver persisted(browser()->profile(), base::Bind(
+ &DownloadStoredProperly, url, full_file_name, -1,
+ DownloadItem::COMPLETE));
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));
+ persisted.WaitForPersisted();
EXPECT_TRUE(file_util::PathExists(full_file_name));
int64 actual_file_size = -1;

Powered by Google App Engine
This is Rietveld 408576698