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

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

Issue 10823406: Remove DownloadFileManager in favor of direct ownership of DownloadFiles. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src
Patch Set: Incorporated comments and merged to LKGR. 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..85c61743dd7f766236f57383b1c29e397f5a29dc 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() {}
@@ -209,11 +263,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 +278,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()
@@ -282,7 +340,8 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveHTMLOnly) {
EXPECT_EQ(url, WaitForSavePackageToFinish());
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 +368,13 @@ 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());
+ 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));
@@ -358,7 +421,9 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveViewSourceHTMLOnly) {
EXPECT_EQ(actual_page_url, WaitForSavePackageToFinish());
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));
@@ -378,7 +443,8 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveCompleteHTML) {
EXPECT_EQ(url, WaitForSavePackageToFinish());
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));
@@ -411,7 +477,8 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, FileNameFromPageTitle) {
EXPECT_EQ(url, WaitForSavePackageToFinish());
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));
@@ -437,14 +504,16 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, RemoveFromList) {
EXPECT_EQ(url, WaitForSavePackageToFinish());
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 +587,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