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 37eab12071580e2d57907af1f281d373cb91162a..6407a8f8de409d5d9c582b7a17cd2274b694109d 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_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" |
@@ -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,124 @@ 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 th
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 te
benjhayden
2012/11/12 18:44:16
DownloadStoredProperly may be called multiple time
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 |
@@ -99,7 +210,6 @@ class DownloadItemCreatedObserver : public DownloadManager::Observer { |
} |
private: |
- |
// DownloadManager::Observer |
virtual void OnDownloadCreated( |
DownloadManager* manager, DownloadItem* item) OVERRIDE { |
@@ -124,60 +234,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 +275,12 @@ 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|. |
Randy Smith (Not in Mondays)
2012/11/09 21:36:39
This doesn't feel like an accurate description. "
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()); |
@@ -241,24 +302,26 @@ 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; |
+ 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 beh
benjhayden
2012/11/12 18:44:16
Done.
|
+ |
+ 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 +329,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_; |
@@ -372,15 +349,10 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveHTMLOnly) { |
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( |
@@ -407,14 +379,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(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()); |
@@ -461,15 +431,11 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveViewSourceHTMLOnly) { |
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)); |
@@ -485,14 +451,11 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveCompleteHTML) { |
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)); |
@@ -531,9 +494,8 @@ 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, DownloadPersistedObserver::PersistedFilter())); |
// Confirm download shelf is visible. |
EXPECT_TRUE(incognito->window()->IsDownloadShelfVisible()); |
@@ -560,13 +522,11 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, FileNameFromPageTitle) { |
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)); |
@@ -589,22 +549,21 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, RemoveFromList) { |
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()); |
+ |
+ 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)); |
@@ -673,10 +632,9 @@ IN_PROC_BROWSER_TEST_F(SavePageAsMHTMLBrowserTest, SavePageAsMHTML) { |
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; |