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

Issues Search
    My Issues | Starred     Open | Closed | All

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

Issue 10867065: Remove DownloadFileManager in favor of direct ownership of DownloadFiles. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src
Patch Set: Fixed SavePageBrowserTest to always wait for DI to be persisted." Created 8 years, 4 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
« no previous file with comments | « chrome/browser/download/download_browsertest.cc ('k') | content/browser/browser_context.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 d3f737bcfdf7a22d064c104a78db8c2b440e6cf1..e9689377a9fe9d5cf50840a0f6bc0103ea1b2f51 100644
--- a/chrome/browser/download/save_page_browsertest.cc
+++ b/chrome/browser/download/save_page_browsertest.cc
@@ -124,6 +124,60 @@ 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
+ void OnDownloadUpdated(DownloadItem* item) {
+ DCHECK_EQ(item, item_);
+
+ if (waiting_ && item->IsPersisted())
+ MessageLoopForUI::current()->Quit();
+ }
+
+ void OnDownloadDestroyed(DownloadItem* item) {
+ if (item != item_)
+ return;
+
+ item_->RemoveObserver(this);
+ item_ = NULL;
+ }
+
+ bool waiting_;
+ DownloadItem* item_;
+
+ DISALLOW_COPY_AND_ASSIGN(DownloadPersistedObserver);
+};
+
class SavePageBrowserTest : public InProcessBrowserTest {
public:
SavePageBrowserTest() {}
@@ -165,7 +219,6 @@ class SavePageBrowserTest : public InProcessBrowserTest {
return current_tab;
}
-
GURL WaitForSavePackageToFinish() const {
content::WindowedNotificationObserver observer(
content::NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED,
@@ -175,6 +228,13 @@ class SavePageBrowserTest : public InProcessBrowserTest {
GetOriginalUrl();
}
+ void WaitForPersistedDownloadItem() const {
benjhayden 2012/08/26 16:17:03 Why not inline this in CheckDownloadHistory()?
Randy Smith (Not in Mondays) 2012/08/27 15:02:50 Quite right. Done.
+ std::vector<DownloadItem*> downloads;
+ GetDownloadManager()->SearchDownloads(string16(), &downloads);
+ ASSERT_EQ(1u, downloads.size());
+ DownloadPersistedObserver(downloads[0]).WaitForPersisted();
+ }
+
DownloadManager* GetDownloadManager() const {
DownloadManager* download_manager =
BrowserContext::GetDownloadManager(browser()->profile());
@@ -209,11 +269,12 @@ class SavePageBrowserTest : public InProcessBrowserTest {
DownloadPersistentStoreInfoMatch(const GURL& url,
const FilePath& path,
- int64 num_files)
+ int64 num_files,
+ DownloadItem::DownloadState state)
: url_(url),
path_(path),
- num_files_(num_files) {
- }
+ num_files_(num_files),
+ state_(state) {}
bool operator() (const DownloadPersistentStoreInfo& info) const {
return info.url == url_ &&
@@ -223,22 +284,25 @@ class SavePageBrowserTest : public InProcessBrowserTest {
((num_files_ < 0) ||
(info.received_bytes == num_files_)) &&
info.total_bytes == 0 &&
- info.state == DownloadItem::COMPLETE;
+ info.state == state_;
}
GURL url_;
FilePath path_;
int64 num_files_;
+ DownloadItem::DownloadState state_;
};
void CheckDownloadHistory(const GURL& url,
const FilePath& path,
- int64 num_files) {
+ int64 num_files,
+ DownloadItem::DownloadState state) {
QueryDownloadHistory();
std::vector<DownloadPersistentStoreInfo>::iterator found =
std::find_if(history_entries_.begin(), history_entries_.end(),
- DownloadPersistentStoreInfoMatch(url, path, num_files));
+ DownloadPersistentStoreInfoMatch(url, path, num_files,
+ state));
if (found == history_entries_.end()) {
LOG(ERROR) << "Missing url=" << url.spec()
@@ -280,9 +344,11 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveHTMLOnly) {
content::SAVE_PAGE_TYPE_AS_ONLY_HTML));
EXPECT_EQ(url, WaitForSavePackageToFinish());
+ WaitForPersistedDownloadItem();
EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
- CheckDownloadHistory(url, full_file_name, 1); // a.htm is 1 file.
+ // 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));
@@ -309,9 +375,14 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveHTMLOnlyCancel) {
// TODO(rdsmith): Fix DII::Cancel() to actually cancel the save package.
// Currently it's ignored.
EXPECT_EQ(url, WaitForSavePackageToFinish());
+ WaitForPersistedDownloadItem();
+ DownloadPersistedObserver(item).WaitForPersisted();
+ // -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());
- CheckDownloadHistory(url, full_file_name, 1); // a.htm is 1 file.
EXPECT_TRUE(file_util::PathExists(full_file_name));
EXPECT_FALSE(file_util::PathExists(dir));
@@ -356,9 +427,12 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveViewSourceHTMLOnly) {
content::SAVE_PAGE_TYPE_AS_ONLY_HTML));
EXPECT_EQ(actual_page_url, WaitForSavePackageToFinish());
+ WaitForPersistedDownloadItem();
EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
- CheckDownloadHistory(actual_page_url, full_file_name, 1); // a.htm is 1 file.
+ // 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));
@@ -376,9 +450,11 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveCompleteHTML) {
full_file_name, dir, content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML));
EXPECT_EQ(url, WaitForSavePackageToFinish());
+ WaitForPersistedDownloadItem();
EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
- CheckDownloadHistory(url, full_file_name, 3); // b.htm is 3 files.
+ // 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));
@@ -409,9 +485,11 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, FileNameFromPageTitle) {
full_file_name, dir, content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML));
EXPECT_EQ(url, WaitForSavePackageToFinish());
+ WaitForPersistedDownloadItem();
EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
- CheckDownloadHistory(url, full_file_name, 3); // b.htm is 3 files.
+ // 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));
@@ -435,16 +513,19 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, RemoveFromList) {
content::SAVE_PAGE_TYPE_AS_ONLY_HTML));
EXPECT_EQ(url, WaitForSavePackageToFinish());
+ WaitForPersistedDownloadItem();
EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
- CheckDownloadHistory(url, full_file_name, 1); // a.htm is 1 file.
+ // 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)),
+ DownloadPersistentStoreInfoMatch(
+ url, full_file_name, 1, DownloadItem::COMPLETE)),
history_entries_.end());
EXPECT_TRUE(file_util::PathExists(full_file_name));
@@ -518,7 +599,7 @@ IN_PROC_BROWSER_TEST_F(SavePageAsMHTMLBrowserTest, SavePageAsMHTML) {
content::NotificationService::AllSources());
chrome::SavePage(browser());
observer.Wait();
- CheckDownloadHistory(url, full_file_name, -1);
+ CheckDownloadHistory(url, full_file_name, -1, DownloadItem::COMPLETE);
EXPECT_TRUE(file_util::PathExists(full_file_name));
int64 actual_file_size = -1;
« no previous file with comments | « chrome/browser/download/download_browsertest.cc ('k') | content/browser/browser_context.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698