diff options
29 files changed, 186 insertions, 575 deletions
diff --git a/chrome/browser/automation/testing_automation_provider.cc b/chrome/browser/automation/testing_automation_provider.cc index 6e45cf2..c3d2fd9 100644 --- a/chrome/browser/automation/testing_automation_provider.cc +++ b/chrome/browser/automation/testing_automation_provider.cc @@ -3120,7 +3120,7 @@ void TestingAutomationProvider::PerformActionOnDownload( } else if (action == "cancel") { selected_item->AddObserver(new AutomationProviderDownloadUpdatedObserver( this, reply_message, false)); - selected_item->Cancel(); + selected_item->Cancel(true); } else { AutomationJSONReply(this, reply_message) .SendError(StringPrintf("Invalid action '%s' given.", action.c_str())); diff --git a/chrome/browser/download/chrome_download_manager_delegate.cc b/chrome/browser/download/chrome_download_manager_delegate.cc index 89d72ab..e3055bc 100644 --- a/chrome/browser/download/chrome_download_manager_delegate.cc +++ b/chrome/browser/download/chrome_download_manager_delegate.cc @@ -220,8 +220,8 @@ void ChromeDownloadManagerDelegate::UpdatePathForItemInPersistentStore( } void ChromeDownloadManagerDelegate::RemoveItemFromPersistentStore( - int64 db_handle) { - download_history_->RemoveEntry(db_handle); + DownloadItem* item) { + download_history_->RemoveEntry(item); } void ChromeDownloadManagerDelegate::RemoveItemsFromPersistentStoreBetween( diff --git a/chrome/browser/download/chrome_download_manager_delegate.h b/chrome/browser/download/chrome_download_manager_delegate.h index e5d7197..b006e53 100644 --- a/chrome/browser/download/chrome_download_manager_delegate.h +++ b/chrome/browser/download/chrome_download_manager_delegate.h @@ -68,7 +68,7 @@ class ChromeDownloadManagerDelegate virtual void UpdatePathForItemInPersistentStore( DownloadItem* item, const FilePath& new_path) OVERRIDE; - virtual void RemoveItemFromPersistentStore(int64 db_handle) OVERRIDE; + virtual void RemoveItemFromPersistentStore(DownloadItem* item) OVERRIDE; virtual void RemoveItemsFromPersistentStoreBetween( const base::Time remove_begin, const base::Time remove_end) OVERRIDE; diff --git a/chrome/browser/download/download_browsertest.cc b/chrome/browser/download/download_browsertest.cc index 1b494af..f100c01 100644 --- a/chrome/browser/download/download_browsertest.cc +++ b/chrome/browser/download/download_browsertest.cc @@ -58,7 +58,6 @@ const FilePath kLargeThemePath(FILE_PATH_LITERAL("extensions/theme2.crx")); enum DangerousDownloadAction { ON_DANGEROUS_DOWNLOAD_ACCEPT, // Accept the download ON_DANGEROUS_DOWNLOAD_DENY, // Deny the download - ON_DANGEROUS_DOWNLOAD_IGNORE, // Don't do anything; calling code will handle. ON_DANGEROUS_DOWNLOAD_FAIL // Fail if a dangerous download is seen }; @@ -74,7 +73,7 @@ void DenyDangerousDownload(scoped_refptr<DownloadManager> download_manager, int32 download_id) { DownloadItem* download = download_manager->GetDownloadItem(download_id); ASSERT_TRUE(download->IsPartialDownload()); - download->Cancel(); + download->Cancel(true); download->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); } @@ -93,10 +92,8 @@ void DenyDangerousDownload(scoped_refptr<DownloadManager> download_manager, class DownloadsObserver : public DownloadManager::Observer, public DownloadItem::Observer { public: - typedef std::set<DownloadItem::DownloadState> StateSet; - // Create an object that will be considered finished when |wait_count| - // download items have entered any states in |download_finished_states|. + // download items have entered state |download_finished_state|. // If |finish_on_select_file| is true, the object will also be // considered finished if the DownloadManager raises a // SelectFileDialogDisplayed() notification. @@ -105,21 +102,20 @@ class DownloadsObserver : public DownloadManager::Observer, // to treat as completion events. DownloadsObserver(DownloadManager* download_manager, size_t wait_count, - StateSet download_finished_states, + DownloadItem::DownloadState download_finished_state, bool finish_on_select_file, DangerousDownloadAction dangerous_download_action) : download_manager_(download_manager), wait_count_(wait_count), finished_downloads_at_construction_(0), waiting_(false), - download_finished_states_(download_finished_states), + download_finished_state_(download_finished_state), finish_on_select_file_(finish_on_select_file), select_file_dialog_seen_(false), dangerous_download_action_(dangerous_download_action) { download_manager_->AddObserver(this); // Will call initial ModelChanged(). finished_downloads_at_construction_ = finished_downloads_.size(); - EXPECT_TRUE(download_finished_states.find(DownloadItem::REMOVING) == - download_finished_states.end()) + EXPECT_NE(DownloadItem::REMOVING, download_finished_state) << "Waiting for REMOVING is not supported. Try COMPLETE."; } @@ -198,16 +194,12 @@ class DownloadsObserver : public DownloadManager::Observer, ADD_FAILURE() << "Unexpected dangerous download item."; break; - case ON_DANGEROUS_DOWNLOAD_IGNORE: - break; - default: NOTREACHED(); } } - if (download_finished_states_.find(download->state()) != - download_finished_states_.end()) { + if (download->state() == download_finished_state_) { DownloadInFinalState(download); } } @@ -310,8 +302,8 @@ class DownloadsObserver : public DownloadManager::Observer, // all downloads completing. bool waiting_; - // The states on which to consider the DownloadItem finished. - StateSet download_finished_states_; + // The state on which to consider the DownloadItem finished. + DownloadItem::DownloadState download_finished_state_; // True if we should transition the DownloadsObserver to finished if // the select file dialog comes up. @@ -631,18 +623,6 @@ class MockDownloadOpeningObserver : public DownloadManager::Observer { DISALLOW_COPY_AND_ASSIGN(MockDownloadOpeningObserver); }; -static const DownloadItem::DownloadState kTerminalStates[] = { - DownloadItem::CANCELLED, - DownloadItem::INTERRUPTED, - DownloadItem::COMPLETE, -}; - -static const DownloadItem::DownloadState kInProgressStates[] = { - DownloadItem::IN_PROGRESS, -}; - -// Not in anonymous namespace so that friend class from DownloadManager -// can target it. class DownloadTest : public InProcessBrowserTest { public: enum SelectExpectation { @@ -719,12 +699,6 @@ class DownloadTest : public InProcessBrowserTest { return true; } - // For tests that want to test system reaction to files - // going away underneath them. - void DeleteDownloadsDirectory() { - EXPECT_TRUE(downloads_directory_.Delete()); - } - DownloadPrefs* GetDownloadPrefs(Browser* browser) { return DownloadPrefs::FromDownloadManager( browser->profile()->GetDownloadManager()); @@ -741,29 +715,12 @@ class DownloadTest : public InProcessBrowserTest { browser->profile()->GetDownloadManager(); return new DownloadsObserver( download_manager, num_downloads, - DownloadsObserver::StateSet( - kTerminalStates, kTerminalStates + arraysize(kTerminalStates)), + DownloadItem::COMPLETE, // Really done true, // Bail on select file ON_DANGEROUS_DOWNLOAD_FAIL); } // Create a DownloadsObserver that will wait for the - // specified number of downloads to finish, and is - // ok with dangerous downloads. Note that use of this - // waiter is conditional on accepting the dangerous download. - DownloadsObserver* CreateDangerousWaiter( - Browser* browser, int num_downloads) { - DownloadManager* download_manager = - browser->profile()->GetDownloadManager(); - return new DownloadsObserver( - download_manager, num_downloads, - DownloadsObserver::StateSet( - kTerminalStates, kTerminalStates + arraysize(kTerminalStates)), - true, // Bail on select file - ON_DANGEROUS_DOWNLOAD_IGNORE); - } - - // Create a DownloadsObserver that will wait for the // specified number of downloads to start. DownloadsObserver* CreateInProgressWaiter(Browser* browser, int num_downloads) { @@ -771,11 +728,9 @@ class DownloadTest : public InProcessBrowserTest { browser->profile()->GetDownloadManager(); return new DownloadsObserver( download_manager, num_downloads, - DownloadsObserver::StateSet( - kInProgressStates, - kInProgressStates + arraysize(kInProgressStates)), + DownloadItem::IN_PROGRESS, // Has started true, // Bail on select file - ON_DANGEROUS_DOWNLOAD_IGNORE); + ON_DANGEROUS_DOWNLOAD_FAIL); } // Create a DownloadsObserver that will wait for the @@ -786,13 +741,11 @@ class DownloadTest : public InProcessBrowserTest { int num_downloads, DownloadItem::DownloadState final_state, DangerousDownloadAction dangerous_download_action) { - DownloadsObserver::StateSet states; - states.insert(final_state); DownloadManager* download_manager = browser->profile()->GetDownloadManager(); return new DownloadsObserver( download_manager, num_downloads, - states, + final_state, true, // Bail on select file dangerous_download_action); } @@ -947,22 +900,12 @@ class DownloadTest : public InProcessBrowserTest { return true; } - void GetPersistentDownloads(Browser* browser, - std::vector<DownloadItem*>* downloads) { + void GetDownloads(Browser* browser, std::vector<DownloadItem*>* downloads) { DCHECK(downloads); - downloads->clear(); DownloadManager* manager = browser->profile()->GetDownloadManager(); manager->SearchDownloads(string16(), downloads); } - void GetInProgressDownloads(Browser* browser, - std::vector<DownloadItem*>* downloads) { - downloads->clear(); - DCHECK(downloads); - DownloadManager* manager = browser->profile()->GetDownloadManager(); - manager->GetInProgressDownloads(downloads); - } - // Check that the download UI (shelf on non-chromeos or panel on chromeos) // is visible or not as expected. Additionally, check that the filename // is present in the UI (currently only on chromeos). @@ -1096,8 +1039,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadMimeTypeSelect) { new DownloadsObserver( browser()->profile()->GetDownloadManager(), 1, - DownloadsObserver::StateSet( - kTerminalStates, kTerminalStates + arraysize(kTerminalStates)), + DownloadItem::COMPLETE, // Really done false, // Continue on select file. ON_DANGEROUS_DOWNLOAD_FAIL)); ui_test_utils::NavigateToURLWithDisposition( @@ -1106,115 +1048,12 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadMimeTypeSelect) { observer->WaitForFinished(); EXPECT_TRUE(observer->select_file_dialog_seen()); + // Check state. EXPECT_EQ(1, browser()->tab_count()); CheckDownload(browser(), file, file); CheckDownloadUI(browser(), true, true, file); } -// Put up a Select File dialog when the file is downloaded, due to -// prompt_for_download==true argument to InitialSetup(). -// Confirm that we can cancel the download in that state. -IN_PROC_BROWSER_TEST_F(DownloadTest, CancelFromFileSelection) { - ASSERT_TRUE(InitialSetup(true)); - FilePath file(FILE_PATH_LITERAL("download-test1.lib")); - GURL url(URLRequestMockHTTPJob::GetMockUrl(file)); - - // Download the file and wait. We expect the Select File dialog to appear. - DownloadAndWait(browser(), url, EXPECT_SELECT_DIALOG); - - std::vector<DownloadItem*> active_downloads, history_downloads; - GetInProgressDownloads(browser(), &active_downloads); - ASSERT_EQ(1u, active_downloads.size()); - EXPECT_EQ(DownloadItem::IN_PROGRESS, active_downloads[0]->state()); - GetPersistentDownloads(browser(), &history_downloads); - EXPECT_EQ(0u, history_downloads.size()); - - // This should remove the download as it hasn't yet been entered into - // the history. - active_downloads[0]->Cancel(); - MessageLoopForUI::current()->RunAllPending(); - - GetInProgressDownloads(browser(), &active_downloads); - EXPECT_EQ(0u, active_downloads.size()); - GetPersistentDownloads(browser(), &history_downloads); - EXPECT_EQ(0u, history_downloads.size()); - - // Check state. - EXPECT_EQ(1, browser()->tab_count()); - // Since we exited while the Select File dialog was visible, there should not - // be anything in the download shelf and so it should not be visible. - CheckDownloadUI(browser(), false, false, FilePath()); -} - -// Put up a Select File dialog when the file is downloaded, due to -// prompt_for_download==true argument to InitialSetup(). -// Confirm that we can remove the download in that state. -IN_PROC_BROWSER_TEST_F(DownloadTest, RemoveFromFileSelection) { - ASSERT_TRUE(InitialSetup(true)); - FilePath file(FILE_PATH_LITERAL("download-test1.lib")); - GURL url(URLRequestMockHTTPJob::GetMockUrl(file)); - - // Download the file and wait. We expect the Select File dialog to appear. - DownloadAndWait(browser(), url, EXPECT_SELECT_DIALOG); - - std::vector<DownloadItem*> active_downloads, history_downloads; - GetInProgressDownloads(browser(), &active_downloads); - ASSERT_EQ(1u, active_downloads.size()); - EXPECT_EQ(DownloadItem::IN_PROGRESS, active_downloads[0]->state()); - GetPersistentDownloads(browser(), &history_downloads); - EXPECT_EQ(0u, history_downloads.size()); - - // Confirm the file can be successfully removed from the select file - // dialog blocked state. - active_downloads[0]->Remove(); - MessageLoopForUI::current()->RunAllPending(); - - GetInProgressDownloads(browser(), &active_downloads); - EXPECT_EQ(0u, active_downloads.size()); - GetPersistentDownloads(browser(), &history_downloads); - EXPECT_EQ(0u, history_downloads.size()); - - EXPECT_EQ(1, browser()->tab_count()); - // Since we exited while the Select File dialog was visible, there should not - // be anything in the download shelf and so it should not be visible. - CheckDownloadUI(browser(), false, false, FilePath()); -} - -// Put up a Select File dialog when the file is downloaded, due to -// prompt_for_download==true argument to InitialSetup(). -// Confirm that an error coming in from the network works properly -// when in that state. -IN_PROC_BROWSER_TEST_F(DownloadTest, InterruptFromFileSelection) { - ASSERT_TRUE(InitialSetup(true)); - GURL url(URLRequestSlowDownloadJob::kKnownSizeUrl); - - // Download the file and wait. We expect the Select File dialog to appear. - DownloadAndWait(browser(), url, EXPECT_SELECT_DIALOG); - - std::vector<DownloadItem*> active_downloads, history_downloads; - GetInProgressDownloads(browser(), &active_downloads); - ASSERT_EQ(1u, active_downloads.size()); - EXPECT_EQ(DownloadItem::IN_PROGRESS, active_downloads[0]->state()); - GetPersistentDownloads(browser(), &history_downloads); - EXPECT_EQ(0u, history_downloads.size()); - - // Complete the download with error. - GURL error_url(URLRequestSlowDownloadJob::kErrorFinishDownloadUrl); - ui_test_utils::NavigateToURL(browser(), error_url); - MessageLoopForUI::current()->RunAllPending(); - - // Confirm that a download error before entry into history - // deletes the download. - GetInProgressDownloads(browser(), &active_downloads); - EXPECT_EQ(0u, active_downloads.size()); - GetPersistentDownloads(browser(), &history_downloads); - EXPECT_EQ(0u, history_downloads.size()); - - // Since we exited while the Select File dialog was visible, there should not - // be anything in the download shelf and so it should not be visible. - CheckDownloadUI(browser(), false, false, FilePath()); -} - // Access a file with a viewable mime-type, verify that a download // did not initiate. IN_PROC_BROWSER_TEST_F(DownloadTest, NoDownload) { @@ -1643,7 +1482,8 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadCancelled) { observer->WaitForFinished(); std::vector<DownloadItem*> downloads; - GetPersistentDownloads(browser(), &downloads); + browser()->profile()->GetDownloadManager()->SearchDownloads( + string16(), &downloads); ASSERT_EQ(1u, downloads.size()); ASSERT_EQ(DownloadItem::IN_PROGRESS, downloads[0]->state()); CheckDownloadUI(browser(), true, true, FilePath()); @@ -1666,182 +1506,6 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadCancelled) { CheckDownloadUI(browser(), false, true, FilePath()); } -// Do a dangerous download and confirm that the download does -// not complete until user accept, and that all states are -// correct along the way. -IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadDangerous) { - ASSERT_TRUE(InitialSetup(false)); - FilePath file(FILE_PATH_LITERAL("download-dangerous.jar")); - GURL url(URLRequestMockHTTPJob::GetMockUrl(file)); - - EXPECT_EQ(1, browser()->tab_count()); - - scoped_ptr<DownloadsObserver> observer( - CreateInProgressWaiter(browser(), 1)); - ui_test_utils::NavigateToURL(browser(), url); - observer->WaitForFinished(); - - // We should have one download, in history, and it should - // still be dangerous. - std::vector<DownloadItem*> downloads; - GetPersistentDownloads(browser(), &downloads); - ASSERT_EQ(1u, downloads.size()); - DownloadItem* download = downloads[0]; - EXPECT_EQ(DownloadItem::IN_PROGRESS, download->state()); - EXPECT_EQ(DownloadItem::DANGEROUS, download->safety_state()); - EXPECT_EQ(DownloadItem::DANGEROUS_FILE, download->GetDangerType()); - // In ChromeOS, popup will be up, but file name will be unrecognizable. - CheckDownloadUI(browser(), true, true, FilePath()); - - // See if accepting completes the download and changes the safety - // state. - scoped_ptr<DownloadsObserver> completion_observer( - CreateDangerousWaiter(browser(), 1)); - AcceptDangerousDownload(browser()->profile()->GetDownloadManager(), - download->id()); - completion_observer->WaitForFinished(); - - GetPersistentDownloads(browser(), &downloads); - ASSERT_EQ(1u, downloads.size()); - ASSERT_EQ(downloads[0], download); - EXPECT_EQ(DownloadItem::COMPLETE, download->state()); - EXPECT_EQ(DownloadItem::DANGEROUS_BUT_VALIDATED, download->safety_state()); - CheckDownloadUI(browser(), true, true, file); -} - -// Confirm that a dangerous download that gets a file error before -// completion ends in the right state (currently cancelled because file -// errors are non-resumable). Note that this is really testing -// to make sure errors from the final rename are propagated properly. -IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadDangerousFileError) { - ASSERT_TRUE(InitialSetup(false)); - FilePath file(FILE_PATH_LITERAL("download-dangerous.jar")); - GURL url(URLRequestMockHTTPJob::GetMockUrl(file)); - - EXPECT_EQ(1, browser()->tab_count()); - - scoped_ptr<DownloadsObserver> observer( - CreateInProgressWaiter(browser(), 1)); - ui_test_utils::NavigateToURL(browser(), url); - observer->WaitForFinished(); - - // We should have one download, in history, and it should - // still be dangerous. - std::vector<DownloadItem*> downloads; - GetPersistentDownloads(browser(), &downloads); - ASSERT_EQ(1u, downloads.size()); - DownloadItem* download = downloads[0]; - EXPECT_EQ(DownloadItem::IN_PROGRESS, download->state()); - EXPECT_EQ(DownloadItem::DANGEROUS, download->safety_state()); - EXPECT_EQ(DownloadItem::DANGEROUS_FILE, download->GetDangerType()); - // In ChromeOS, popup will be up, but file name will be unrecognizable. - CheckDownloadUI(browser(), true, true, FilePath()); - - // Accept it after nuking the directory into which it's being downloaded; - // that should complete the download with an error. - DeleteDownloadsDirectory(); - scoped_ptr<DownloadsObserver> completion_observer( - CreateDangerousWaiter(browser(), 1)); - AcceptDangerousDownload(browser()->profile()->GetDownloadManager(), - download->id()); - completion_observer->WaitForFinished(); - - GetPersistentDownloads(browser(), &downloads); - ASSERT_EQ(1u, downloads.size()); - ASSERT_EQ(downloads[0], download); - EXPECT_EQ(DownloadItem::INTERRUPTED, download->state()); - EXPECT_EQ(DownloadItem::DANGEROUS_BUT_VALIDATED, download->safety_state()); - // In ChromeOS, popup will still be up, but the file will have been - // deleted. - CheckDownloadUI(browser(), true, true, FilePath()); -} - -// Confirm that declining a dangerous download erases it from living memory. -IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadDangerousDecline) { - ASSERT_TRUE(InitialSetup(false)); - FilePath file(FILE_PATH_LITERAL("download-dangerous.jar")); - GURL url(URLRequestMockHTTPJob::GetMockUrl(file)); - - EXPECT_EQ(1, browser()->tab_count()); - - scoped_ptr<DownloadsObserver> observer( - CreateInProgressWaiter(browser(), 1)); - ui_test_utils::NavigateToURL(browser(), url); - observer->WaitForFinished(); - - // We should have one download, in history, and it should - // still be dangerous. - std::vector<DownloadItem*> downloads; - GetPersistentDownloads(browser(), &downloads); - ASSERT_EQ(1u, downloads.size()); - DownloadItem* download = downloads[0]; - EXPECT_EQ(DownloadItem::IN_PROGRESS, download->state()); - EXPECT_EQ(DownloadItem::DANGEROUS, download->safety_state()); - EXPECT_EQ(DownloadItem::DANGEROUS_FILE, download->GetDangerType()); - CheckDownloadUI(browser(), true, true, FilePath()); - - DenyDangerousDownload(browser()->profile()->GetDownloadManager(), - download->id()); - - GetPersistentDownloads(browser(), &downloads); - ASSERT_EQ(0u, downloads.size()); - CheckDownloadUI(browser(), false, true, FilePath()); -} - -// Fail a download with a network error partway through, and make sure the -// state is INTERRUPTED and the error is propagated. -IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadInterrupted) { - ASSERT_TRUE(InitialSetup(false)); - GURL url(URLRequestSlowDownloadJob::kKnownSizeUrl); - - scoped_ptr<DownloadsObserver> observer( - CreateInProgressWaiter(browser(), 1)); - ui_test_utils::NavigateToURL(browser(), url); - observer->WaitForFinished(); - - std::vector<DownloadItem*> downloads; - GetPersistentDownloads(browser(), &downloads); - ASSERT_EQ(1u, downloads.size()); - ASSERT_EQ(DownloadItem::IN_PROGRESS, downloads[0]->state()); - FilePath filename; - net::FileURLToFilePath(url, &filename); - CheckDownloadUI(browser(), true, true, - download_util::GetCrDownloadPath(filename.BaseName())); - - // Fail the download - GURL error_url(URLRequestSlowDownloadJob::kErrorFinishDownloadUrl); - ui_test_utils::NavigateToURL(browser(), error_url); - MessageLoopForUI::current()->RunAllPending(); - - // Should still be visible, with INTERRUPTED state. - GetPersistentDownloads(browser(), &downloads); - ASSERT_EQ(1u, downloads.size()); - DownloadItem* download = downloads[0]; - ASSERT_EQ(DownloadItem::INTERRUPTED, download->state()); - // TODO(rdsmith): Confirm error provided by URLRequest is shown - // in DownloadItem. - CheckDownloadUI(browser(), true, true, FilePath()); - - // Confirm cancel does nothing. - download->Cancel(); - MessageLoopForUI::current()->RunAllPending(); - - GetPersistentDownloads(browser(), &downloads); - ASSERT_EQ(1u, downloads.size()); - ASSERT_EQ(download, downloads[0]); - ASSERT_EQ(DownloadItem::INTERRUPTED, download->state()); - CheckDownloadUI(browser(), true, true, FilePath()); - - // Confirm remove gets rid of it. - download->Remove(); - download = NULL; - MessageLoopForUI::current()->RunAllPending(); - - GetPersistentDownloads(browser(), &downloads); - ASSERT_EQ(0u, downloads.size()); - CheckDownloadUI(browser(), false, true, FilePath()); -} - // Confirm a download makes it into the history properly. IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadHistoryCheck) { ASSERT_TRUE(InitialSetup(false)); @@ -1856,7 +1520,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadHistoryCheck) { // Get details of what downloads have just happened. std::vector<DownloadItem*> downloads; - GetPersistentDownloads(browser(), &downloads); + GetDownloads(browser(), &downloads); ASSERT_EQ(1u, downloads.size()); int64 db_handle = downloads[0]->db_handle(); @@ -1971,7 +1635,8 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, AutoOpen) { // Find the download and confirm it was opened. std::vector<DownloadItem*> downloads; - GetPersistentDownloads(browser(), &downloads); + browser()->profile()->GetDownloadManager()->SearchDownloads( + string16(), &downloads); ASSERT_EQ(1u, downloads.size()); EXPECT_EQ(DownloadItem::COMPLETE, downloads[0]->state()); EXPECT_TRUE(downloads[0]->opened()); diff --git a/chrome/browser/download/download_history.cc b/chrome/browser/download/download_history.cc index 9c9a26e..3da198b 100644 --- a/chrome/browser/download/download_history.cc +++ b/chrome/browser/download/download_history.cc @@ -116,14 +116,14 @@ void DownloadHistory::UpdateDownloadPath(DownloadItem* download_item, hs->UpdateDownloadPath(new_path, download_item->db_handle()); } -void DownloadHistory::RemoveEntry(int64 db_handle) { +void DownloadHistory::RemoveEntry(DownloadItem* download_item) { // No update necessary if the download was initiated while in incognito mode. - if (db_handle <= DownloadItem::kUninitializedHandle) + if (download_item->db_handle() <= DownloadItem::kUninitializedHandle) return; HistoryService* hs = profile_->GetHistoryService(Profile::EXPLICIT_ACCESS); if (hs) - hs->RemoveDownload(db_handle); + hs->RemoveDownload(download_item->db_handle()); } void DownloadHistory::RemoveEntriesBetween(const base::Time remove_begin, diff --git a/chrome/browser/download/download_history.h b/chrome/browser/download/download_history.h index 9ef445c..6d6b237 100644 --- a/chrome/browser/download/download_history.h +++ b/chrome/browser/download/download_history.h @@ -47,10 +47,8 @@ class DownloadHistory { void UpdateDownloadPath(DownloadItem* download_item, const FilePath& new_path); - // Removes the download identified by |db_handle| from the history database. - // |db_handle| is used instead of the DownloadItem pointer to allow - // removal after deletion of the download item. - void RemoveEntry(int64 db_handle); + // Removes |download_item| from the history database. + void RemoveEntry(DownloadItem* download_item); // Removes download-related history entries in the given time range. void RemoveEntriesBetween(const base::Time remove_begin, diff --git a/chrome/browser/download/download_item_model.cc b/chrome/browser/download/download_item_model.cc index f5fec1b..891a5ed 100644 --- a/chrome/browser/download/download_item_model.cc +++ b/chrome/browser/download/download_item_model.cc @@ -26,7 +26,7 @@ DownloadItemModel::DownloadItemModel(DownloadItem* download) } void DownloadItemModel::CancelTask() { - download_->Cancel(); + download_->Cancel(true /* update history service */); } string16 DownloadItemModel::GetStatusText() { diff --git a/chrome/browser/download/download_manager_unittest.cc b/chrome/browser/download/download_manager_unittest.cc index 6a2974b..3380046 100644 --- a/chrome/browser/download/download_manager_unittest.cc +++ b/chrome/browser/download/download_manager_unittest.cc @@ -534,7 +534,7 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) { simple_size, simple_total)); - download->Cancel(); + download->Cancel(true); EXPECT_TRUE(observer->hit_state(DownloadItem::IN_PROGRESS)); EXPECT_TRUE(observer->hit_state(DownloadItem::INTERRUPTED)); @@ -630,7 +630,7 @@ TEST_F(DownloadManagerTest, DownloadFileErrorTest) { download_item_model->GetStatusText()); // Clean up. - download->Cancel(); + download->Cancel(true); message_loop_.RunAllPending(); } @@ -676,10 +676,10 @@ TEST_F(DownloadManagerTest, DownloadCancelTest) { download_file->AppendDataToFile(kTestData, kTestDataLen); - download->Cancel(); + download->Cancel(false); message_loop_.RunAllPending(); - EXPECT_TRUE(GetActiveDownloadItem(0) == NULL); + EXPECT_TRUE(GetActiveDownloadItem(0) != NULL); EXPECT_TRUE(observer->hit_state(DownloadItem::IN_PROGRESS)); EXPECT_TRUE(observer->hit_state(DownloadItem::CANCELLED)); EXPECT_FALSE(observer->hit_state(DownloadItem::INTERRUPTED)); diff --git a/chrome/browser/ui/cocoa/download/download_item_controller.mm b/chrome/browser/ui/cocoa/download/download_item_controller.mm index d89cfdc..eb854ba 100644 --- a/chrome/browser/ui/cocoa/download/download_item_controller.mm +++ b/chrome/browser/ui/cocoa/download/download_item_controller.mm @@ -353,7 +353,7 @@ class DownloadShelfContextMenuMac : public DownloadShelfContextMenu { base::Time::Now() - creationTime_); DownloadItem* download = bridge_->download_model()->download(); if (download->IsPartialDownload()) - download->Cancel(); + download->Cancel(true); download->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); // WARNING: we are deleted at this point. Don't access 'this'. } diff --git a/chrome/browser/ui/gtk/download/download_item_gtk.cc b/chrome/browser/ui/gtk/download/download_item_gtk.cc index b0d61a1..5f9218e 100644 --- a/chrome/browser/ui/gtk/download/download_item_gtk.cc +++ b/chrome/browser/ui/gtk/download/download_item_gtk.cc @@ -878,6 +878,6 @@ void DownloadItemGtk::OnDangerousDecline(GtkWidget* button) { UMA_HISTOGRAM_LONG_TIMES("clickjacking.discard_download", base::Time::Now() - creation_time_); if (get_download()->IsPartialDownload()) - get_download()->Cancel(); + get_download()->Cancel(true); get_download()->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); } diff --git a/chrome/browser/ui/panels/panel_browsertest.cc b/chrome/browser/ui/panels/panel_browsertest.cc index b060934..ec4661a 100644 --- a/chrome/browser/ui/panels/panel_browsertest.cc +++ b/chrome/browser/ui/panels/panel_browsertest.cc @@ -674,7 +674,7 @@ class DownloadObserver : public DownloadManager::Observer { return; EXPECT_EQ(1U, downloads.size()); - downloads.front()->Cancel(); // Don't actually need to download it. + downloads.front()->Cancel(false); // Don't actually need to download it. saw_download_ = true; EXPECT_TRUE(waiting_); diff --git a/chrome/browser/ui/views/download/download_item_view.cc b/chrome/browser/ui/views/download/download_item_view.cc index 79ecc01..02010f7 100644 --- a/chrome/browser/ui/views/download/download_item_view.cc +++ b/chrome/browser/ui/views/download/download_item_view.cc @@ -654,7 +654,7 @@ void DownloadItemView::ButtonPressed( UMA_HISTOGRAM_LONG_TIMES("clickjacking.discard_download", base::Time::Now() - creation_time_); if (download_->IsPartialDownload()) - download_->Cancel(); + download_->Cancel(true); download_->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); // WARNING: we are deleted at this point. Don't access 'this'. } else if (sender == save_button_) { diff --git a/chrome/browser/ui/webui/active_downloads_ui.cc b/chrome/browser/ui/webui/active_downloads_ui.cc index 5114b9f..3d5af17 100644 --- a/chrome/browser/ui/webui/active_downloads_ui.cc +++ b/chrome/browser/ui/webui/active_downloads_ui.cc @@ -270,7 +270,7 @@ void ActiveDownloadsHandler::HandleCancelDownload(const ListValue* args) { DownloadItem* item = GetDownloadById(args); if (item) { if (item->IsPartialDownload()) - item->Cancel(); + item->Cancel(true); item->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); } } diff --git a/chrome/browser/ui/webui/chromeos/imageburner/imageburner_ui.cc b/chrome/browser/ui/webui/chromeos/imageburner/imageburner_ui.cc index 1130256..9e54398 100644 --- a/chrome/browser/ui/webui/chromeos/imageburner/imageburner_ui.cc +++ b/chrome/browser/ui/webui/chromeos/imageburner/imageburner_ui.cc @@ -509,7 +509,7 @@ void WebUIHandler::ProcessError(int message_id) { // We don't want to process Download Cancelled signal. active_download_item_->RemoveObserver(this); if (active_download_item_->IsPartialDownload()) - active_download_item_->Cancel(); + active_download_item_->Cancel(true); active_download_item_->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); active_download_item_ = NULL; CleanupDownloadObjects(); diff --git a/chrome/browser/ui/webui/downloads_dom_handler.cc b/chrome/browser/ui/webui/downloads_dom_handler.cc index 5f74dd2..52e986c 100644 --- a/chrome/browser/ui/webui/downloads_dom_handler.cc +++ b/chrome/browser/ui/webui/downloads_dom_handler.cc @@ -314,7 +314,7 @@ void DownloadsDOMHandler::HandleCancel(const ListValue* args) { CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_CANCEL); DownloadItem* file = GetDownloadByValue(args); if (file) - file->Cancel(); + file->Cancel(true); } void DownloadsDOMHandler::HandleClearAll(const ListValue* args) { diff --git a/chrome/test/data/download-dangerous.jar b/chrome/test/data/download-dangerous.jar deleted file mode 100644 index 28f58bc..0000000 --- a/chrome/test/data/download-dangerous.jar +++ /dev/null @@ -1,2 +0,0 @@ -Contents of file not important; will be marked as dangerous based on -content-type and extension. diff --git a/chrome/test/data/download-dangerous.jar.mock-http-headers b/chrome/test/data/download-dangerous.jar.mock-http-headers deleted file mode 100644 index e675398..0000000 --- a/chrome/test/data/download-dangerous.jar.mock-http-headers +++ /dev/null @@ -1,6 +0,0 @@ -HTTP/1.1 200 OK -Content-Type: application/x-jar -Content-Disposition: attachment; filename="download-dangerous.jar" -Content-Length: 97 -Date: Mon, 13 Nov 2006 21:38:09 GMT -Expires: Tue, 14 Nov 2006 19:23:58 GMT diff --git a/content/browser/download/download_item.cc b/content/browser/download/download_item.cc index a88a438..0809872 100644 --- a/content/browser/download/download_item.cc +++ b/content/browser/download/download_item.cc @@ -352,27 +352,24 @@ void DownloadItem::Update(int64 bytes_so_far) { UpdateObservers(); } -void DownloadItem::Cancel() { +// Triggered by a user action. +void DownloadItem::Cancel(bool update_history) { // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved. CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true); - // Small downloads might be complete before we have a chance to run. - if (!IsInProgress()) + VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true); + if (!IsPartialDownload()) { + // Small downloads might be complete before this method has + // a chance to run. return; - - TransitionTo(CANCELLED); + } download_stats::RecordDownloadCount(download_stats::CANCELLED_COUNT); - // History insertion is the point at which we have finalized download - // details and persist them if something goes wrong. Before history - // insertion, interrupt or cancel results in download removal. - if (db_handle() == DownloadItem::kUninitializedHandle) { - download_manager_->RemoveDownload(this); - // We are now deleted; no further code should be executed on this - // object. - } + TransitionTo(CANCELLED); + StopProgressTimer(); + if (update_history) + download_manager_->DownloadCancelledInternal(this); } void DownloadItem::MarkAsComplete() { @@ -431,22 +428,10 @@ void DownloadItem::Completed() { } void DownloadItem::TransitionTo(DownloadState new_state) { - DownloadState old_state = state_; - if (old_state == new_state) + if (state_ == new_state) return; - // Check for disallowed state transitions. - CHECK(!(old_state == IN_PROGRESS && new_state == REMOVING)); - state_ = new_state; - - // Do special operations for transitions from an active state. - if (old_state == IN_PROGRESS && - (new_state == CANCELLED || new_state == INTERRUPTED)) { - download_manager_->DownloadStopped(this); - StopProgressTimer(); - } - UpdateObservers(); } @@ -468,32 +453,20 @@ void DownloadItem::UpdateTarget() { state_info_.target_name = full_path_.BaseName(); } -void DownloadItem::Interrupt(int64 size, net::Error net_error) { +void DownloadItem::Interrupted(int64 size, net::Error net_error) { // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved. CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true); - // Small downloads might be complete before we have a chance to run. if (!IsInProgress()) return; - UpdateSize(size); last_error_ = net_error; - - TransitionTo(INTERRUPTED); - + UpdateSize(size); + StopProgressTimer(); download_stats::RecordDownloadInterrupted(net_error, received_bytes_, total_bytes_); - - // History insertion is the point at which we have finalized download - // details and persist them if something goes wrong. Before history - // insertion, interrupt or cancel results in download removal. - if (db_handle() == DownloadItem::kUninitializedHandle) { - download_manager_->RemoveDownload(this); - // We are now deleted; no further code should be executed on this - // object. - } + TransitionTo(INTERRUPTED); } void DownloadItem::Delete(DeleteReason reason) { @@ -524,14 +497,11 @@ void DownloadItem::Remove() { CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); download_manager_->AssertQueueStateConsistent(this); - if (IsInProgress()) { - TransitionTo(CANCELLED); - download_stats::RecordDownloadCount(download_stats::CANCELLED_COUNT); - } + Cancel(true); download_manager_->AssertQueueStateConsistent(this); - download_stats::RecordDownloadCount(download_stats::REMOVED_COUNT); - download_manager_->RemoveDownload(this); + TransitionTo(REMOVING); + download_manager_->RemoveDownload(db_handle_); // We have now been deleted. } diff --git a/content/browser/download/download_item.h b/content/browser/download/download_item.h index 7a81aa0..07c43fe 100644 --- a/content/browser/download/download_item.h +++ b/content/browser/download/download_item.h @@ -157,11 +157,16 @@ class CONTENT_EXPORT DownloadItem { // Received a new chunk of data void Update(int64 bytes_so_far); - // Cancel the download operation. This may be called at any time; if - // it is called before all download attributes have been finalized and - // the download entered into the history, it will remove the download from - // the system. - void Cancel(); + // Cancel the download operation. We need to distinguish between cancels at + // exit (DownloadManager destructor) from user interface initiated cancels + // because at exit, the history system may not exist, and any updates to it + // require AddRef'ing the DownloadManager in the destructor which results in + // a DCHECK failure. Set 'update_history' to false when canceling from at + // exit to prevent this crash. This may result in a difference between the + // downloaded file's size on disk, and what the history system's last record + // of it is. At worst, we'll end up re-downloading a small portion of the file + // when resuming a download (assuming the server supports byte ranges). + void Cancel(bool update_history); // Called by external code (SavePackage) using the DownloadItem interface // to display progress when the DownloadItem should be considered complete. @@ -177,14 +182,10 @@ class CONTENT_EXPORT DownloadItem { // Called when the downloaded file is removed. void OnDownloadedFileRemoved(); - // Download operation had an error; call to interrupt the processing. - // Note that if the download attributes haven't yet been finalized and - // the download entered into the history (which implies that it hasn't - // yet been made visible in the UI), this call will remove the - // download from the system. - // |size| is the amount of data received so far, and |net_error| is the - // error code that the operation received. - void Interrupt(int64 size, net::Error net_error); + // Download operation had an error. + // |size| is the amount of data received at interruption. + // |error| is the network error code that the operation received. + void Interrupted(int64 size, net::Error error); // Deletes the file from disk and removes the download from the views and // history. |user| should be true if this is the result of the user clicking @@ -356,8 +357,7 @@ class CONTENT_EXPORT DownloadItem { void StartProgressTimer(); void StopProgressTimer(); - // Does the actual work of transition state; all state - // transitions should go through this. + // Call to transition state; all state transitions should go through this. void TransitionTo(DownloadState new_state); // Called when safety_state_ should be recomputed from is_dangerous_file diff --git a/content/browser/download/download_manager.cc b/content/browser/download/download_manager.cc index 2681e72..06bf42f5 100644 --- a/content/browser/download/download_manager.cc +++ b/content/browser/download/download_manager.cc @@ -105,7 +105,8 @@ void DownloadManager::Shutdown() { // from all queues. download->Delete(DownloadItem::DELETE_DUE_TO_BROWSER_SHUTDOWN); } else if (download->IsPartialDownload()) { - download->Cancel(); + download->Cancel(false); + delegate_->UpdateItemInPersistentStore(download); } } @@ -497,19 +498,23 @@ void DownloadManager::OnDownloadRenamedToFinalName(int download_id, delegate_->UpdatePathForItemInPersistentStore(item, full_path); } -void DownloadManager::DownloadStopped(DownloadItem* download) { +void DownloadManager::CancelDownload(int32 download_id) { + DownloadItem* download = GetActiveDownload(download_id); + // A cancel at the right time could remove the download from the + // |active_downloads_| map before we get here. + if (!download) + return; + + download->Cancel(true); +} + +void DownloadManager::DownloadCancelledInternal(DownloadItem* download) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - CHECK(ContainsKey(active_downloads_, download->id())); VLOG(20) << __FUNCTION__ << "()" << " download = " << download->DebugString(true); - in_progress_.erase(download->id()); - active_downloads_.erase(download->id()); - UpdateDownloadProgress(); // Reflect removal from in_progress_. - if (download->db_handle() != DownloadItem::kUninitializedHandle) - delegate_->UpdateItemInPersistentStore(download); - + RemoveFromActiveList(download); // This function is called from the DownloadItem, so DI state // should already have been updated. AssertQueueStateConsistent(download); @@ -531,7 +536,9 @@ void DownloadManager::OnDownloadError(int32 download_id, << " size = " << size << " download = " << download->DebugString(true); - download->Interrupt(size, error); + RemoveFromActiveList(download); + download->Interrupted(size, error); + download->OffThreadCancel(file_manager_); } DownloadItem* DownloadManager::GetActiveDownload(int32 download_id) { @@ -548,6 +555,20 @@ DownloadItem* DownloadManager::GetActiveDownload(int32 download_id) { return download; } +void DownloadManager::RemoveFromActiveList(DownloadItem* download) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + DCHECK(download); + + // Clean up will happen when the history system create callback runs if we + // don't have a valid db_handle yet. + if (download->db_handle() != DownloadItem::kUninitializedHandle) { + in_progress_.erase(download->id()); + active_downloads_.erase(download->id()); + UpdateDownloadProgress(); // Reflect removal from in_progress_. + delegate_->UpdateItemInPersistentStore(download); + } +} + void DownloadManager::UpdateDownloadProgress() { delegate_->DownloadProgressUpdated(); } @@ -577,9 +598,14 @@ int DownloadManager::RemoveDownloadItems( return num_deleted; } -void DownloadManager::RemoveDownload(DownloadItem* download) { - // Make history update. Ignores if db_handle isn't in history. - delegate_->RemoveItemFromPersistentStore(download->db_handle()); +void DownloadManager::RemoveDownload(int64 download_handle) { + DownloadMap::iterator it = history_downloads_.find(download_handle); + if (it == history_downloads_.end()) + return; + + // Make history update. + DownloadItem* download = it->second; + delegate_->RemoveItemFromPersistentStore(download); // Remove from our tables and delete. int downloads_count = RemoveDownloadItems(DownloadVector(1, download)); @@ -729,7 +755,12 @@ void DownloadManager::FileSelectionCanceled(void* params) { VLOG(20) << __FUNCTION__ << "()" << " download = " << download->DebugString(true); - download->Cancel(); + // TODO(ahendrickson) -- This currently has no effect, as the download is + // not put on the active list until the file selection is complete. Need + // to put it on the active list earlier in the process. + RemoveFromActiveList(download); + + download->OffThreadCancel(file_manager_); } // Operations posted to us from the history service ---------------------------- @@ -800,22 +831,8 @@ void DownloadManager::OnDownloadItemAddedToPersistentStore(int32 download_id, int64 db_handle) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DownloadItem* download = GetActiveDownloadItem(download_id); - if (!download) { - // The download was cancelled while the persistent store was entering it. - // We resolve this race by turning around and deleting it in the - // persistent store (implicitly treating it as a failure in download - // initiation, which is appropriate as the only places the cancel could - // have come from were in resolving issues (like the file name) which - // we need to have resolved for persistent store insertion). - - // Make sure we haven't already been shutdown (the callback raced - // with shutdown), as that would mean that we no longer have access - // to the persistent store. In that case, the history will be cleaned up - // on next persistent store query. - if (shutdown_needed_) - delegate_->RemoveItemFromPersistentStore(db_handle); + if (!download) return; - } VLOG(20) << __FUNCTION__ << "()" << " db_handle = " << db_handle << " download_id = " << download_id @@ -826,10 +843,27 @@ void DownloadManager::OnDownloadItemAddedToPersistentStore(int32 download_id, base::debug::Alias(&largest_handle); CHECK(!ContainsKey(history_downloads_, db_handle)); - CHECK(download->IsInProgress()); AddDownloadItemToHistory(download, db_handle); - MaybeCompleteDownload(download); + // If the download is still in progress, try to complete it. + // + // Otherwise, download has been cancelled or interrupted before we've + // received the DB handle. We post one final message to the history + // service so that it can be properly in sync with the DownloadItem's + // completion status, and also inform any observers so that they get + // more than just the start notification. + if (download->IsInProgress()) { + MaybeCompleteDownload(download); + } else { + // TODO(rdsmith): Convert to DCHECK() when http://crbug.com/84508 + // is fixed. + CHECK(download->IsCancelled()) + << " download = " << download->DebugString(true); + in_progress_.erase(download_id); + active_downloads_.erase(download_id); + delegate_->UpdateItemInPersistentStore(download); + download->UpdateObservers(); + } } void DownloadManager::ShowDownloadInBrowser(DownloadItem* download) { @@ -868,16 +902,6 @@ DownloadItem* DownloadManager::GetDownloadItem(int download_id) { return NULL; } -void DownloadManager::GetInProgressDownloads( - std::vector<DownloadItem*>* result) { - DCHECK(result); - - for (DownloadMap::iterator it = active_downloads_.begin(); - it != active_downloads_.end(); ++it) { - result->push_back(it->second); - } -} - DownloadItem* DownloadManager::GetActiveDownloadItem(int download_id) { DCHECK(ContainsKey(active_downloads_, download_id)); DownloadItem* download = active_downloads_[download_id]; diff --git a/content/browser/download/download_manager.h b/content/browser/download/download_manager.h index 965bdcd..06803ce 100644 --- a/content/browser/download/download_manager.h +++ b/content/browser/download/download_manager.h @@ -122,24 +122,22 @@ class CONTENT_EXPORT DownloadManager void OnResponseCompleted(int32 download_id, int64 size, const std::string& hash); + // Offthread target for cancelling a particular download. Will be a no-op + // if the download has already been cancelled. + void CancelDownload(int32 download_id); + // Called when there is an error in the download. // |download_id| is the ID of the download. // |size| is the number of bytes that are currently downloaded. // |error| is a download error code. Indicates what caused the interruption. void OnDownloadError(int32 download_id, int64 size, net::Error error); - // This routine is called from the DownloadItem when a - // request is cancelled or interrupted. It removes the download - // from all internal queues holding in-progress work, and takes care - // of the off-thread aspects of the cancel (stopping the request, - // cancelling the download on the file thread). - void DownloadStopped(DownloadItem* download); + // Called from DownloadItem to handle the DownloadManager portion of a + // Cancel; should not be called from other locations. + void DownloadCancelledInternal(DownloadItem* download); - // Called from DownloadItem when the download is being removed. - // Takes care of all history operations, modifying internal queues, - // and notifying DownloadManager observers, and actually deletes - // the DownloadItem. - void RemoveDownload(DownloadItem* download); + // Called from a view when a user clicks a UI button or link. + void RemoveDownload(int64 download_handle); // Determine if the download is ready for completion, i.e. has had // all data saved, and completed the filename determination and @@ -275,16 +273,16 @@ class CONTENT_EXPORT DownloadManager typedef std::set<DownloadItem*> DownloadSet; typedef base::hash_map<int64, DownloadItem*> DownloadMap; - friend struct BrowserThread::DeleteOnThread<BrowserThread::UI>; - friend class DeleteTask<DownloadManager>; - friend class base::RefCountedThreadSafe<DownloadManager, - BrowserThread::DeleteOnUIThread>; - // For testing. friend class DownloadManagerTest; friend class MockDownloadManager; friend class DownloadTest; + friend class base::RefCountedThreadSafe<DownloadManager, + BrowserThread::DeleteOnUIThread>; + friend struct BrowserThread::DeleteOnThread<BrowserThread::UI>; + friend class DeleteTask<DownloadManager>; + void set_delegate(DownloadManagerDelegate* delegate) { delegate_ = delegate; } virtual ~DownloadManager(); @@ -307,18 +305,17 @@ class CONTENT_EXPORT DownloadManager // Returns NULL if the download is not active. DownloadItem* GetActiveDownload(int32 download_id); + // Removes |download| from the active and in progress maps. + // Called when the download is cancelled or has an error. + // Does nothing if the download is not in the history DB. + void RemoveFromActiveList(DownloadItem* download); + // Updates the delegate about the overall download progress. void UpdateDownloadProgress(); // Inform observers that the model has changed. void NotifyModelChanged(); - // Return all in progress downloads. This includes downloads that - // have not yet been entered into the history (all other accessors - // only return downloads that have been entered into the history). - // This is intended to be used for testing only. - void GetInProgressDownloads(std::vector<DownloadItem*>* result); - // Debugging routine to confirm relationship between below // containers; no-op if NDEBUG. void AssertContainersConsistent() const; diff --git a/content/browser/download/download_manager_delegate.h b/content/browser/download/download_manager_delegate.h index be72f11..55d25f6 100644 --- a/content/browser/download/download_manager_delegate.h +++ b/content/browser/download/download_manager_delegate.h @@ -84,7 +84,7 @@ class DownloadManagerDelegate { // Notifies the delegate that it should remove the download item from its // persistent store. - virtual void RemoveItemFromPersistentStore(int64 db_handle) = 0; + virtual void RemoveItemFromPersistentStore(DownloadItem* item) = 0; // Notifies the delegate to remove downloads from the given time range. virtual void RemoveItemsFromPersistentStoreBetween( diff --git a/content/browser/download/download_request_handle.h b/content/browser/download/download_request_handle.h index aba8dc1..c65ebe6 100644 --- a/content/browser/download/download_request_handle.h +++ b/content/browser/download/download_request_handle.h @@ -37,15 +37,11 @@ class DownloadRequestHandle { TabContents* GetTabContents() const; DownloadManager* GetDownloadManager() const; - // Pause or resume the matching URL request. Note that while these - // do not modify the DownloadRequestHandle, they do modify the - // request the handle refers to. + // Pause or resume the matching URL request. void PauseRequest() const; void ResumeRequest() const; - // Cancel the request. Note that while this does not - // modify the DownloadRequestHandle, it does modify the - // request the handle refers to. + // Cancel the request void CancelRequest() const; std::string DebugString() const; diff --git a/content/browser/download/download_stats.h b/content/browser/download/download_stats.h index d80cba2..7ca7a23 100644 --- a/content/browser/download/download_stats.h +++ b/content/browser/download/download_stats.h @@ -64,9 +64,6 @@ enum DownloadCountTypes { // Counts iterations of the BaseFile::AppendDataToFile() loop. WRITE_LOOP_COUNT, - // Downloads that were removed by the user. - REMOVED_COUNT, - DOWNLOAD_COUNT_TYPES_LAST_ENTRY }; diff --git a/content/browser/download/mock_download_manager_delegate.cc b/content/browser/download/mock_download_manager_delegate.cc index dba6b8c..e5b8b66 100644 --- a/content/browser/download/mock_download_manager_delegate.cc +++ b/content/browser/download/mock_download_manager_delegate.cc @@ -65,7 +65,7 @@ void MockDownloadManagerDelegate::UpdatePathForItemInPersistentStore( } void MockDownloadManagerDelegate::RemoveItemFromPersistentStore( - int64 db_handle) { + DownloadItem* item) { } void MockDownloadManagerDelegate::RemoveItemsFromPersistentStoreBetween( diff --git a/content/browser/download/mock_download_manager_delegate.h b/content/browser/download/mock_download_manager_delegate.h index 453f3449..fc3f8f2 100644 --- a/content/browser/download/mock_download_manager_delegate.h +++ b/content/browser/download/mock_download_manager_delegate.h @@ -31,7 +31,7 @@ class MockDownloadManagerDelegate : public DownloadManagerDelegate { virtual void UpdatePathForItemInPersistentStore( DownloadItem* item, const FilePath& new_path) OVERRIDE; - virtual void RemoveItemFromPersistentStore(int64 db_handle) OVERRIDE; + virtual void RemoveItemFromPersistentStore(DownloadItem* item) OVERRIDE; virtual void RemoveItemsFromPersistentStoreBetween( const base::Time remove_begin, const base::Time remove_end) OVERRIDE; diff --git a/content/browser/download/save_package.cc b/content/browser/download/save_package.cc index f0c2be1..1560dd0 100644 --- a/content/browser/download/save_package.cc +++ b/content/browser/download/save_package.cc @@ -626,7 +626,7 @@ void SavePackage::Stop() { // Inform the DownloadItem we have canceled whole save page job. if (download_) { - download_->Cancel(); + download_->Cancel(false); FinalizeDownloadEntry(); } } diff --git a/content/browser/net/url_request_slow_download_job.cc b/content/browser/net/url_request_slow_download_job.cc index 4cb152c..ef1370f 100644 --- a/content/browser/net/url_request_slow_download_job.cc +++ b/content/browser/net/url_request_slow_download_job.cc @@ -10,11 +10,9 @@ #include "base/string_util.h" #include "googleurl/src/gurl.h" #include "net/base/io_buffer.h" -#include "net/base/net_errors.h" #include "net/http/http_response_headers.h" #include "net/url_request/url_request.h" #include "net/url_request/url_request_filter.h" -#include "net/url_request/url_request_status.h" const int kFirstDownloadSize = 1024 * 35; const int kSecondDownloadSize = 1024 * 10; @@ -25,18 +23,9 @@ const char URLRequestSlowDownloadJob::kKnownSizeUrl[] = "http://url.handled.by.slow.download/download-known-size"; const char URLRequestSlowDownloadJob::kFinishDownloadUrl[] = "http://url.handled.by.slow.download/download-finish"; -const char URLRequestSlowDownloadJob::kErrorFinishDownloadUrl[] = - "http://url.handled.by.slow.download/download-error"; std::vector<URLRequestSlowDownloadJob*> - URLRequestSlowDownloadJob::pending_requests_; - -// Return whether this is the finish or error URL. -static bool IsCompletionUrl(const GURL& url) { - if (url.spec() == URLRequestSlowDownloadJob::kFinishDownloadUrl) - return true; - return (url.spec() == URLRequestSlowDownloadJob::kErrorFinishDownloadUrl); -} + URLRequestSlowDownloadJob::kPendingRequests; void URLRequestSlowDownloadJob::Start() { MessageLoop::current()->PostTask( @@ -54,8 +43,6 @@ void URLRequestSlowDownloadJob::AddUrlHandler() { &URLRequestSlowDownloadJob::Factory); filter->AddUrlHandler(GURL(kFinishDownloadUrl), &URLRequestSlowDownloadJob::Factory); - filter->AddUrlHandler(GURL(kErrorFinishDownloadUrl), - &URLRequestSlowDownloadJob::Factory); } /*static */ @@ -63,23 +50,19 @@ net::URLRequestJob* URLRequestSlowDownloadJob::Factory( net::URLRequest* request, const std::string& scheme) { URLRequestSlowDownloadJob* job = new URLRequestSlowDownloadJob(request); - if (!IsCompletionUrl(request->url())) - URLRequestSlowDownloadJob::pending_requests_.push_back(job); + if (request->url().spec() != kFinishDownloadUrl) + URLRequestSlowDownloadJob::kPendingRequests.push_back(job); return job; } /* static */ -void URLRequestSlowDownloadJob::FinishPendingRequests(bool error) { +void URLRequestSlowDownloadJob::FinishPendingRequests() { typedef std::vector<URLRequestSlowDownloadJob*> JobList; - for (JobList::iterator it = pending_requests_.begin(); it != - pending_requests_.end(); ++it) { - if (error) { - (*it)->set_should_error_download(); - } else { - (*it)->set_should_finish_download(); - } + for (JobList::iterator it = kPendingRequests.begin(); it != + kPendingRequests.end(); ++it) { + (*it)->set_should_finish_download(); } - pending_requests_.clear(); + kPendingRequests.clear(); } URLRequestSlowDownloadJob::URLRequestSlowDownloadJob(net::URLRequest* request) @@ -87,21 +70,19 @@ URLRequestSlowDownloadJob::URLRequestSlowDownloadJob(net::URLRequest* request) first_download_size_remaining_(kFirstDownloadSize), should_finish_download_(false), should_send_second_chunk_(false), - should_error_download_(false), ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {} void URLRequestSlowDownloadJob::StartAsync() { - if (IsCompletionUrl(request_->url())) { - URLRequestSlowDownloadJob::FinishPendingRequests( - request_->url().spec() == kErrorFinishDownloadUrl); - } + if (LowerCaseEqualsASCII(kFinishDownloadUrl, request_->url().spec().c_str())) + URLRequestSlowDownloadJob::FinishPendingRequests(); NotifyHeadersComplete(); } bool URLRequestSlowDownloadJob::ReadRawData(net::IOBuffer* buf, int buf_size, int *bytes_read) { - if (IsCompletionUrl(request_->url())) { + if (LowerCaseEqualsASCII(kFinishDownloadUrl, + request_->url().spec().c_str())) { *bytes_read = 0; return true; } @@ -151,9 +132,6 @@ void URLRequestSlowDownloadJob::CheckDoneStatus() { should_send_second_chunk_ = true; SetStatus(net::URLRequestStatus()); NotifyReadComplete(kSecondDownloadSize); - } else if (should_error_download_) { - NotifyDone( - net::URLRequestStatus(net::URLRequestStatus::FAILED, net::ERR_FAILED)); } else { MessageLoop::current()->PostDelayedTask( FROM_HERE, @@ -176,7 +154,8 @@ void URLRequestSlowDownloadJob::GetResponseInfoConst( net::HttpResponseInfo* info) const { // Send back mock headers. std::string raw_headers; - if (IsCompletionUrl(request_->url())) { + if (LowerCaseEqualsASCII(kFinishDownloadUrl, + request_->url().spec().c_str())) { raw_headers.append( "HTTP/1.1 200 OK\n" "Content-type: text/plain\n"); @@ -186,7 +165,7 @@ void URLRequestSlowDownloadJob::GetResponseInfoConst( "Content-type: application/octet-stream\n" "Cache-Control: max-age=0\n"); - if (request_->url().spec() == kKnownSizeUrl) { + if (LowerCaseEqualsASCII(kKnownSizeUrl, request_->url().spec().c_str())) { raw_headers.append(base::StringPrintf( "Content-Length: %d\n", kFirstDownloadSize + kSecondDownloadSize)); diff --git a/content/browser/net/url_request_slow_download_job.h b/content/browser/net/url_request_slow_download_job.h index 66eff9a6..9cbde81 100644 --- a/content/browser/net/url_request_slow_download_job.h +++ b/content/browser/net/url_request_slow_download_job.h @@ -1,13 +1,9 @@ // Copyright (c) 2011 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -// This class simulates a slow download. This is used in browser tests -// to test the download manager. Requests to |kUnknownSizeUrl| and -// |kKnownSizeUrl| start downloads that pause after the first chunk of -// data is delivered. A later request to |kFinishDownloadUrl| will finish -// these downloads. A later request to |kErrorFinishDownloadUrl| will -// cause these downloads to error with |net::ERR_FAILED|. -// TODO(rdsmith): Update to allow control of returned error. +// This class simulates a slow download. This used in a UI test to test the +// download manager. Requests to |kUnknownSizeUrl| and |kKnownSizeUrl| start +// downloads that pause after the first #ifndef CONTENT_BROWSER_NET_URL_REQUEST_SLOW_DOWNLOAD_JOB_H_ #define CONTENT_BROWSER_NET_URL_REQUEST_SLOW_DOWNLOAD_JOB_H_ @@ -41,7 +37,6 @@ class URLRequestSlowDownloadJob : public net::URLRequestJob { static const char kUnknownSizeUrl[]; static const char kKnownSizeUrl[]; static const char kFinishDownloadUrl[]; - static const char kErrorFinishDownloadUrl[]; // Adds the testing URLs to the net::URLRequestFilter. CONTENT_EXPORT static void AddUrlHandler(); @@ -52,19 +47,17 @@ class URLRequestSlowDownloadJob : public net::URLRequestJob { void GetResponseInfoConst(net::HttpResponseInfo* info) const; // Mark all pending requests to be finished. We keep track of pending - // requests in |pending_requests_|. - static void FinishPendingRequests(bool error); - static std::vector<URLRequestSlowDownloadJob*> pending_requests_; + // requests in |kPendingRequests|. + static void FinishPendingRequests(); + static std::vector<URLRequestSlowDownloadJob*> kPendingRequests; void StartAsync(); void set_should_finish_download() { should_finish_download_ = true; } - void set_should_error_download() { should_error_download_ = true; } int first_download_size_remaining_; bool should_finish_download_; bool should_send_second_chunk_; - bool should_error_download_; ScopedRunnableMethodFactory<URLRequestSlowDownloadJob> method_factory_; }; |