chrome/browser/download/save_page_browsertest.cc - Issue 10918024: Revert 154294 - Remove DownloadFileManager in favor of direct ownership of DownloadFiles.

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

Issue 10918024: Revert 154294 - Remove DownloadFileManager in favor of direct ownership of DownloadFiles. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src/

Patch Set: 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

Index: chrome/browser/download/save_page_browsertest.cc

===================================================================

--- chrome/browser/download/save_page_browsertest.cc (revision 154444)

+++ chrome/browser/download/save_page_browsertest.cc (working copy)

@@ -64,16 +64,13 @@

".html";

#endif

-void NullFunction() {

} // namespace

// Loosely based on logic in DownloadTestObserver.

class DownloadItemCreatedObserver : public DownloadManager::Observer {

public:

explicit DownloadItemCreatedObserver(DownloadManager* manager)

- : waiting_(false), manager_(manager) {

+ : waiting_(false), manager_(manager), created_item_(NULL) {

manager->AddObserver(this);

}

@@ -86,37 +83,34 @@

// Note that this class provides no protection against the download

// being destroyed between creation and return of WaitForNewDownloadItem();

// the caller must guarantee that in some other fashion.

- void WaitForDownloadItem(std::vector<DownloadItem*>* items_seen) {

+ DownloadItem* WaitForNewDownloadItem() {

if (!manager_) {

// The manager went away before we were asked to wait; return

// what we have, even if it's null.

- *items_seen = items_seen_;

- return;

+ return created_item_;

}

- if (items_seen_.empty()) {

+ if (!created_item_) {

waiting_ = true;

content::RunMessageLoop();

waiting_ = false;

}

- *items_seen = items_seen_;

- return;

+ return created_item_;

}

private:

// DownloadManager::Observer

- virtual void OnDownloadCreated(

- DownloadManager* manager, DownloadItem* item) OVERRIDE {

+ void OnDownloadCreated(DownloadManager* manager, DownloadItem* item) {

DCHECK_EQ(manager, manager_);

- items_seen_.push_back(item);

+ if (!created_item_)

+ created_item_ = item;

if (waiting_)

MessageLoopForUI::current()->Quit();

}

- virtual void ManagerGoingDown(DownloadManager* manager) OVERRIDE {

+ void ManagerGoingDownload(DownloadManager* manager) {

manager_->RemoveObserver(this);

manager_ = NULL;

if (waiting_)

@@ -125,65 +119,11 @@

bool waiting_;

DownloadManager* manager_;

- std::vector<DownloadItem*> items_seen_;

+ DownloadItem* created_item_;

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() {}

@@ -202,8 +142,6 @@

BrowserThread::PostTask(

BrowserThread::IO, FROM_HERE,

base::Bind(&chrome_browser_net::SetUrlRequestMocksEnabled, true));

- item_creation_observer_.reset(

- new DownloadItemCreatedObserver(GetDownloadManager()));

}

GURL NavigateToMockURL(const std::string& prefix) {

@@ -227,34 +165,12 @@

return current_tab;

}

GURL WaitForSavePackageToFinish() 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. Wait for it, and wait for it to

- // be persisted.

- std::vector<DownloadItem*> items;

- item_creation_observer_->WaitForDownloadItem(&items);

- DCHECK_EQ(1u, items.size());

- 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();

return content::Details<DownloadItem>(observer.details()).ptr()->

GetOriginalUrl();

}

@@ -293,12 +209,11 @@

DownloadPersistentStoreInfoMatch(const GURL& url,

const FilePath& path,

- int64 num_files,

- DownloadItem::DownloadState state)

+ int64 num_files)

: url_(url),

path_(path),

- num_files_(num_files),

- state_(state) {}

+ num_files_(num_files) {

+ }

bool operator() (const DownloadPersistentStoreInfo& info) const {

return info.url == url_ &&

@@ -308,30 +223,22 @@

((num_files_ < 0) ||

(info.received_bytes == num_files_)) &&

info.total_bytes == 0 &&

- info.state == state_;

+ info.state == DownloadItem::COMPLETE;

}

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()->SearchDownloads(string16(), &downloads);

- ASSERT_EQ(1u, downloads.size());

+ int64 num_files) {

QueryDownloadHistory();

std::vector<DownloadPersistentStoreInfo>::iterator found =

std::find_if(history_entries_.begin(), history_entries_.end(),

- DownloadPersistentStoreInfoMatch(url, path, num_files,

- state));

+ DownloadPersistentStoreInfoMatch(url, path, num_files));

if (found == history_entries_.end()) {

LOG(ERROR) << "Missing url=" << url.spec()

@@ -358,8 +265,6 @@

ScopedTempDir save_dir_;

private:

- scoped_ptr<DownloadItemCreatedObserver> item_creation_observer_;

DISALLOW_COPY_AND_ASSIGN(SavePageBrowserTest);

};

@@ -377,8 +282,7 @@

EXPECT_EQ(url, WaitForSavePackageToFinish());

EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());

- // a.htm is 1 file.

- CheckDownloadHistory(url, full_file_name, 1, DownloadItem::COMPLETE);

+ 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));

@@ -399,21 +303,15 @@

DownloadItemCreatedObserver creation_observer(manager);

ASSERT_TRUE(GetCurrentTab()->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);

+ DownloadItem* item = creation_observer.WaitForNewDownloadItem();

+ item->Cancel(true);

// TODO(rdsmith): Fix DII::Cancel() to actually cancel the save package.

// Currently it's ignored.

EXPECT_EQ(url, WaitForSavePackageToFinish());

- // -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));

@@ -436,13 +334,11 @@

DownloadItemCreatedObserver creation_observer(manager);

ASSERT_TRUE(GetCurrentTab()->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);

+ DownloadItem* item = creation_observer.WaitForNewDownloadItem();

// Close the tab; does this cancel the download?

GetCurrentTab()->Close();

- EXPECT_TRUE(items[0]->IsCancelled());

+ EXPECT_TRUE(item->IsCancelled());

EXPECT_FALSE(file_util::PathExists(full_file_name));

EXPECT_FALSE(file_util::PathExists(dir));

@@ -464,9 +360,7 @@

EXPECT_EQ(actual_page_url, WaitForSavePackageToFinish());

EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());

- // a.htm is 1 file.

- CheckDownloadHistory(actual_page_url, full_file_name, 1,

- DownloadItem::COMPLETE);

+ CheckDownloadHistory(actual_page_url, full_file_name, 1); // a.htm is 1 file.

EXPECT_TRUE(file_util::PathExists(full_file_name));

EXPECT_FALSE(file_util::PathExists(dir));

@@ -486,8 +380,7 @@

EXPECT_EQ(url, WaitForSavePackageToFinish());

EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());

- // b.htm is 3 files.

- CheckDownloadHistory(url, full_file_name, 3, DownloadItem::COMPLETE);

+ CheckDownloadHistory(url, full_file_name, 3); // b.htm is 3 files.

EXPECT_TRUE(file_util::PathExists(full_file_name));

EXPECT_TRUE(file_util::PathExists(dir));

@@ -520,8 +413,7 @@

EXPECT_EQ(url, WaitForSavePackageToFinish());

EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());

- // b.htm is 3 files.

- CheckDownloadHistory(url, full_file_name, 3, DownloadItem::COMPLETE);

+ CheckDownloadHistory(url, full_file_name, 3); // b.htm is 3 files.

EXPECT_TRUE(file_util::PathExists(full_file_name));

EXPECT_TRUE(file_util::PathExists(dir));

@@ -547,16 +439,14 @@

EXPECT_EQ(url, WaitForSavePackageToFinish());

EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());

- // a.htm is 1 file.

- CheckDownloadHistory(url, full_file_name, 1, DownloadItem::COMPLETE);

+ CheckDownloadHistory(url, full_file_name, 1); // a.htm is 1 file.

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)),

+ DownloadPersistentStoreInfoMatch(url, full_file_name, 1)),

history_entries_.end());

EXPECT_TRUE(file_util::PathExists(full_file_name));

@@ -630,7 +520,7 @@

content::NotificationService::AllSources());

chrome::SavePage(browser());

observer.Wait();

- CheckDownloadHistory(url, full_file_name, -1, DownloadItem::COMPLETE);

+ CheckDownloadHistory(url, full_file_name, -1);

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') | chrome/test/functional/downloads.py » ('j') | no next file with comments »