diff options
29 files changed, 311 insertions, 765 deletions
diff --git a/chrome/browser/automation/testing_automation_provider.cc b/chrome/browser/automation/testing_automation_provider.cc index 28c1f53..71f1eac 100644 --- a/chrome/browser/automation/testing_automation_provider.cc +++ b/chrome/browser/automation/testing_automation_provider.cc @@ -3127,7 +3127,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 a0addef..13efb9e 100644 --- a/chrome/browser/download/download_browsertest.cc +++ b/chrome/browser/download/download_browsertest.cc @@ -60,7 +60,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 }; @@ -76,7 +75,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); } @@ -95,10 +94,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. @@ -107,21 +104,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."; } @@ -200,16 +196,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); } } @@ -312,8 +304,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. @@ -331,36 +323,121 @@ class DownloadsObserver : public DownloadManager::Observer, DISALLOW_COPY_AND_ASSIGN(DownloadsObserver); }; -// Ping through an arbitrary set of threads. Must be run from the UI -// thread. -class ThreadPinger : public base::RefCountedThreadSafe<ThreadPinger> { +// WaitForFlush() returns after: +// * There are no IN_PROGRESS download items remaining on the +// DownloadManager. +// * There have been two round trip messages through the file and +// IO threads. +// This almost certainly means that a Download cancel has propagated through +// the system. +class DownloadsFlushObserver + : public DownloadManager::Observer, + public DownloadItem::Observer, + public base::RefCountedThreadSafe<DownloadsFlushObserver> { public: - ThreadPinger(const BrowserThread::ID ids[], size_t num_ids) : - next_thread_index_(0u), - ids_(ids, ids + num_ids) {} + explicit DownloadsFlushObserver(DownloadManager* download_manager) + : download_manager_(download_manager), + waiting_for_zero_inprogress_(true) {} - void Ping() { - if (ids_.size() == 0) - return; - NextThread(); + void WaitForFlush() { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + download_manager_->AddObserver(this); ui_test_utils::RunMessageLoop(); } + // DownloadsManager observer methods. + virtual void ModelChanged() { + // Model has changed, so there may be more DownloadItems to observe. + CheckDownloadsInProgress(true); + } + + // DownloadItem observer methods. + virtual void OnDownloadUpdated(DownloadItem* download) { + // No change in DownloadItem set on manager. + CheckDownloadsInProgress(false); + } + virtual void OnDownloadOpened(DownloadItem* download) {} + + protected: + friend class base::RefCountedThreadSafe<DownloadsFlushObserver>; + + virtual ~DownloadsFlushObserver() { + download_manager_->RemoveObserver(this); + for (std::set<DownloadItem*>::iterator it = downloads_observed_.begin(); + it != downloads_observed_.end(); ++it) { + (*it)->RemoveObserver(this); + } + } + private: - void NextThread() { - if (next_thread_index_ == ids_.size()) { + // If we're waiting for that flush point, check the number + // of downloads in the IN_PROGRESS state and take appropriate + // action. If requested, also observes all downloads while iterating. + void CheckDownloadsInProgress(bool observe_downloads) { + if (waiting_for_zero_inprogress_) { + int count = 0; + + std::vector<DownloadItem*> downloads; + download_manager_->SearchDownloads(string16(), &downloads); + std::vector<DownloadItem*>::iterator it = downloads.begin(); + for (; it != downloads.end(); ++it) { + if ((*it)->state() == DownloadItem::IN_PROGRESS) + count++; + if (observe_downloads) { + if (downloads_observed_.find(*it) == downloads_observed_.end()) { + (*it)->AddObserver(this); + } + // Download items are forever, and we don't want to make + // assumptions about future state transitions, so once we + // start observing them, we don't stop until destruction. + } + } + + if (count == 0) { + waiting_for_zero_inprogress_ = false; + // Stop observing DownloadItems. We maintain the observation + // of DownloadManager so that we don't have to independently track + // whether we are observing it for conditional destruction. + for (std::set<DownloadItem*>::iterator it = downloads_observed_.begin(); + it != downloads_observed_.end(); ++it) { + (*it)->RemoveObserver(this); + } + downloads_observed_.clear(); + + // Trigger next step. We need to go past the IO thread twice, as + // there's a self-task posting in the IO thread cancel path. + BrowserThread::PostTask( + BrowserThread::FILE, FROM_HERE, + NewRunnableMethod(this, + &DownloadsFlushObserver::PingFileThread, 2)); + } + } + } + + void PingFileThread(int cycle) { + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + NewRunnableMethod(this, &DownloadsFlushObserver::PingIOThread, + cycle)); + } + + void PingIOThread(int cycle) { + if (--cycle) { BrowserThread::PostTask( - BrowserThread::UI, FROM_HERE, new MessageLoop::QuitTask()); + BrowserThread::UI, FROM_HERE, + NewRunnableMethod(this, &DownloadsFlushObserver::PingFileThread, + cycle)); } else { - BrowserThread::ID next_id(ids_[next_thread_index_++]); BrowserThread::PostTask( - next_id, FROM_HERE, - NewRunnableMethod(this, &ThreadPinger::NextThread)); + BrowserThread::UI, FROM_HERE, new MessageLoop::QuitTask()); } } - size_t next_thread_index_; - std::vector<BrowserThread::ID> ids_; + DownloadManager* download_manager_; + std::set<DownloadItem*> downloads_observed_; + bool waiting_for_zero_inprogress_; + + DISALLOW_COPY_AND_ASSIGN(DownloadsFlushObserver); }; // Collect the information from FILE and IO threads needed for the Cancel @@ -431,39 +508,6 @@ class PickSuggestedFileDelegate : public ChromeDownloadManagerDelegate { } }; -// Don't respond to ChooseDownloadPath until a cancel is requested -// out of band. Can handle only one outstanding request at a time -// for a download path. -class BlockCancelFileDelegate : public ChromeDownloadManagerDelegate { - public: - explicit BlockCancelFileDelegate(Profile* profile) - : ChromeDownloadManagerDelegate(profile), - choose_download_path_called_(false), - choose_download_path_data_(NULL) { - SetDownloadManager(profile->GetDownloadManager()); - } - - virtual void ChooseDownloadPath(TabContents* tab_contents, - const FilePath& suggested_path, - void* data) OVERRIDE { - CHECK(!choose_download_path_called_); - choose_download_path_called_ = true; - choose_download_path_data_ = data; - } - - void CancelOutstandingDownloadPathRequest() { - if (choose_download_path_called_) { - if (download_manager_) - download_manager_->FileSelectionCanceled(choose_download_path_data_); - choose_download_path_called_ = false; - choose_download_path_data_ = NULL; - } - } - private: - bool choose_download_path_called_; - void *choose_download_path_data_; -}; - // Get History Information. class DownloadsHistoryDataCollector { public: @@ -581,25 +625,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, -}; - -static const BrowserThread::ID kIOFileX2ThreadList[] = { - BrowserThread::IO, BrowserThread::FILE, - BrowserThread::IO, BrowserThread::FILE }; - -static const BrowserThread::ID kExternalTerminationThreadList[] = { - BrowserThread::IO, BrowserThread::IO, BrowserThread::FILE }; - -// Not in anonymous namespace so that friend class from DownloadManager -// can target it. class DownloadTest : public InProcessBrowserTest { public: enum SelectExpectation { @@ -682,12 +707,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()); @@ -704,29 +723,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) { @@ -734,11 +736,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 @@ -749,13 +749,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); } @@ -910,35 +908,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); - } - - size_t NumberInProgressDownloads(Browser* browser) { - std::vector<DownloadItem*> downloads; - browser->profile()->GetDownloadManager()->GetInProgressDownloads( - &downloads); - return downloads.size(); - } - - void WaitForIOFileX2() { - scoped_refptr<ThreadPinger> pinger( - new ThreadPinger(kIOFileX2ThreadList, arraysize(kIOFileX2ThreadList))); - pinger->Ping(); - } - // 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). @@ -1006,39 +981,6 @@ class DownloadTest : public InProcessBrowserTest { browser->profile()->SetDownloadManagerDelegate(new_delegate); } - // Arrange for select file calls on the given browser from the - // download manager to block until explicitly released. - // Note that the returned pointer is non-owning; the lifetime - // of the object will be that of the profile. - BlockCancelFileDelegate* BlockSelectFile(Browser* browser) { - BlockCancelFileDelegate* new_delegate = - new BlockCancelFileDelegate(browser->profile()); - - DownloadManager* manager = browser->profile()->GetDownloadManager(); - - new_delegate->SetDownloadManager(manager); - manager->set_delegate(new_delegate); - - // Gives ownership to Profile. - browser->profile()->SetDownloadManagerDelegate(new_delegate); - - return new_delegate; - } - - // Wait for an external termination (e.g. signalling - // URLRequestSlowDownloadJob to return an error) to propagate through - // this sytem. This involves hopping over to the IO thread (twice, - // because of possible self-posts from URLRequestJob) then - // chasing the (presumed) notification through the download - // system (FILE->UI). - void WaitForExternalTermination() { - scoped_refptr<ThreadPinger> pinger( - new ThreadPinger( - kExternalTerminationThreadList, - arraysize(kExternalTerminationThreadList))); - pinger->Ping(); - } - private: // Location of the test data. FilePath test_dir_; @@ -1105,8 +1047,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( @@ -1115,168 +1056,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, CancelAtFileSelection) { - ASSERT_TRUE(InitialSetup(true)); - FilePath file(FILE_PATH_LITERAL("download-test1.lib")); - GURL url(URLRequestMockHTTPJob::GetMockUrl(file)); - - BlockCancelFileDelegate* delegate = BlockSelectFile(browser()); - - // 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()); - - // Return from file selection to release allocated data. - delegate->CancelOutstandingDownloadPathRequest(); -} - -// 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 as if the user had hit cancel. -IN_PROC_BROWSER_TEST_F(DownloadTest, CancelFromFileSelection) { - ASSERT_TRUE(InitialSetup(true)); - FilePath file(FILE_PATH_LITERAL("download-test1.lib")); - GURL url(URLRequestMockHTTPJob::GetMockUrl(file)); - - BlockCancelFileDelegate* delegate = BlockSelectFile(browser()); - - // 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()); - - // (Effectively) Click the Cancel button. This should remove the download - // as it hasn't yet been entered into the history. - delegate->CancelOutstandingDownloadPathRequest(); - 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)); - - BlockCancelFileDelegate* delegate = BlockSelectFile(browser()); - - // 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()); - - // Return from file selection to release allocated data. - delegate->CancelOutstandingDownloadPathRequest(); -} - -// 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); - - BlockCancelFileDelegate* delegate = BlockSelectFile(browser()); - - // 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(); - WaitForExternalTermination(); - - // 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()); - - // Return from file selection to release allocated data. - delegate->CancelOutstandingDownloadPathRequest(); -} - // Access a file with a viewable mime-type, verify that a download // did not initiate. IN_PROC_BROWSER_TEST_F(DownloadTest, NoDownload) { @@ -1676,37 +1461,25 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, NewWindow) { CheckDownload(browser(), file, file); } -// Create a download, wait until it's visible on the DownloadManager, -// then cancel it. IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadCancelled) { ASSERT_TRUE(InitialSetup(false)); EXPECT_EQ(1, browser()->tab_count()); - // The code below is slightly fragile. Currently, - // the DownloadsObserver will only finish when the new download has - // reached the state of being entered into the history and being - // user-visible. However, by the pure semantics of + // TODO(rdsmith): Fragile code warning! The code below relies on the + // DownloadsObserver only finishing when the new download has reached + // the state of being entered into the history and being user-visible + // (that's what's required for the Remove to be valid and for the + // download shelf to be visible). By the pure semantics of // DownloadsObserver, that's not guaranteed; DownloadItems are created // in the IN_PROGRESS state and made known to the DownloadManager // immediately, so any ModelChanged event on the DownloadManager after - // navigation would allow the observer to return. At some point we'll - // probably change the DownloadManager to fire a ModelChanged event - // earlier in download processing. This test should continue to work, - // as a cancel is valid at any point during download process. However, - // at that point it'll be testing two different things, depending on - // what state the download item has reached when it is cancelled: - // a) cancelling from a pre-history entry when the only record of a - // download item is on the active_downloads_ queue, and b) cancelling - // from a post-history entry when the download is present on the - // history_downloads_ list. - // - // Note that the above is why we don't do any UI testing here--we don't - // know whether or not the download item has been made visible to the user. - - // TODO(rdsmith): After we fire ModelChanged events at finer granularity, - // split this test into subtests that tests canceling from each separate - // download item state. Also include UI testing as appropriate in those - // split tests. + // navigation would allow the observer to return. However, the only + // ModelChanged() event the code will currently fire is in + // OnCreateDownloadEntryComplete, at which point the download item will + // be in the state we need. + // The right way to fix this is to create finer grained states on the + // DownloadItem, and wait for the state that indicates the item has been + // entered in the history and made visible in the UI. // Create a download, wait until it's started, and confirm // we're in the expected state. @@ -1717,199 +1490,27 @@ 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()); // Cancel the download and wait for download system quiesce. downloads[0]->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); - ASSERT_EQ(0u, NumberInProgressDownloads(browser())); - WaitForIOFileX2(); + scoped_refptr<DownloadsFlushObserver> flush_observer( + new DownloadsFlushObserver(browser()->profile()->GetDownloadManager())); + flush_observer->WaitForFlush(); // Get the important info from other threads and check it. scoped_refptr<CancelTestDataCollector> info(new CancelTestDataCollector()); info->WaitForDataCollected(); EXPECT_EQ(0, info->rdh_pending_requests()); EXPECT_EQ(0, info->dfm_pending_downloads()); -} - -// 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(); - - DownloadItem* download1; - std::vector<DownloadItem*> downloads; - GetPersistentDownloads(browser(), &downloads); - ASSERT_EQ(1u, downloads.size()); - download1 = downloads[0]; - ASSERT_EQ(DownloadItem::IN_PROGRESS, download1->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(); - WaitForExternalTermination(); - - // Should still be visible, with INTERRUPTED state. - GetPersistentDownloads(browser(), &downloads); - ASSERT_EQ(1u, downloads.size()); - DownloadItem* download = downloads[0]; - ASSERT_EQ(download, download1); - 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()); + // Using "DownloadItem::Remove" follows the discard dangerous download path, + // which completely removes the browser from the shelf and closes the shelf + // if it was there. Download panel stays open on ChromeOS. CheckDownloadUI(browser(), false, true, FilePath()); } @@ -1927,7 +1528,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(); @@ -2039,7 +1640,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 4cf61f8..aa80b2e 100644 --- a/chrome/browser/ui/panels/panel_browsertest.cc +++ b/chrome/browser/ui/panels/panel_browsertest.cc @@ -901,7 +901,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 f319af6..d37cd51 100644 --- a/content/browser/download/download_item.cc +++ b/content/browser/download/download_item.cc @@ -353,27 +353,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() { @@ -432,22 +429,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(); } @@ -469,32 +454,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) { @@ -525,14 +498,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 55ba657..f1c52d4 100644 --- a/content/browser/download/download_manager.cc +++ b/content/browser/download/download_manager.cc @@ -112,7 +112,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); } } @@ -274,7 +275,7 @@ void DownloadManager::RestartDownload( TabContents* contents = request_handle.GetTabContents(); // |id_ptr| will be deleted in either FileSelected() or - // FileSelectionCanceled(). + // FileSelectionCancelled(). int32* id_ptr = new int32; *id_ptr = download_id; @@ -504,19 +505,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); @@ -538,7 +543,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) { @@ -555,6 +562,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(); } @@ -584,9 +605,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)); @@ -736,7 +762,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 ---------------------------- @@ -807,22 +838,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 @@ -838,10 +855,27 @@ void DownloadManager::OnDownloadItemAddedToPersistentStore(int32 download_id, 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) { @@ -880,20 +914,8 @@ 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) { - if (!ContainsKey(active_downloads_, download_id)) - return NULL; - + DCHECK(ContainsKey(active_downloads_, download_id)); DownloadItem* download = active_downloads_[download_id]; DCHECK(download != NULL); return download; diff --git a/content/browser/download/download_manager.h b/content/browser/download/download_manager.h index dcf4ab2..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 @@ -266,7 +264,7 @@ class CONTENT_EXPORT DownloadManager void SavePageDownloadFinished(DownloadItem* download); // Get the download item from the active map. Useful when the item is not - // yet in the history map. Returns NULL if no such active download. + // yet in the history map. DownloadItem* GetActiveDownloadItem(int id); DownloadManagerDelegate* delegate() const { return delegate_; } @@ -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_; }; |