diff options
Diffstat (limited to 'chrome/browser')
22 files changed, 379 insertions, 176 deletions
diff --git a/chrome/browser/automation/automation_provider_observers.cc b/chrome/browser/automation/automation_provider_observers.cc index c9a47a7..15beb50 100644 --- a/chrome/browser/automation/automation_provider_observers.cc +++ b/chrome/browser/automation/automation_provider_observers.cc @@ -1452,11 +1452,9 @@ void AutomationProviderDownloadItemObserver::OnDownloadUpdated( download->Cancel(true); RemoveAndCleanupOnLastEntry(download); } -} -void AutomationProviderDownloadItemObserver::OnDownloadFileCompleted( - DownloadItem* download) { - RemoveAndCleanupOnLastEntry(download); + if (download->IsComplete()) + RemoveAndCleanupOnLastEntry(download); } // We don't want to send multiple messages, as the behavior is undefined. @@ -1528,10 +1526,6 @@ void AutomationProviderDownloadUpdatedObserver::OnDownloadOpened( delete this; } -void AutomationProviderDownloadUpdatedObserver::OnDownloadFileCompleted( - DownloadItem* download) { -} - AutomationProviderDownloadModelChangedObserver:: AutomationProviderDownloadModelChangedObserver( AutomationProvider* provider, diff --git a/chrome/browser/automation/automation_provider_observers.h b/chrome/browser/automation/automation_provider_observers.h index b2b3410..2d71001 100644 --- a/chrome/browser/automation/automation_provider_observers.h +++ b/chrome/browser/automation/automation_provider_observers.h @@ -888,7 +888,6 @@ class AutomationProviderDownloadItemObserver : public DownloadItem::Observer { virtual ~AutomationProviderDownloadItemObserver(); virtual void OnDownloadUpdated(DownloadItem* download); - virtual void OnDownloadFileCompleted(DownloadItem* download); virtual void OnDownloadOpened(DownloadItem* download); private: @@ -915,7 +914,6 @@ class AutomationProviderDownloadUpdatedObserver virtual void OnDownloadUpdated(DownloadItem* download); virtual void OnDownloadOpened(DownloadItem* download); - virtual void OnDownloadFileCompleted(DownloadItem* download); private: base::WeakPtr<AutomationProvider> provider_; diff --git a/chrome/browser/download/download_browsertest.cc b/chrome/browser/download/download_browsertest.cc index b08bdd49..99440ba 100644 --- a/chrome/browser/download/download_browsertest.cc +++ b/chrome/browser/download/download_browsertest.cc @@ -4,11 +4,14 @@ #include "base/file_path.h" #include "base/file_util.h" +#include "base/memory/ref_counted.h" #include "base/memory/scoped_temp_dir.h" #include "base/path_service.h" #include "base/test/test_file_util.h" #include "base/utf_string_conversions.h" +#include "chrome/browser/browser_process.h" #include "chrome/browser/download/download_item.h" +#include "chrome/browser/download/download_file_manager.h" #include "chrome/browser/download/download_manager.h" #include "chrome/browser/download/download_prefs.h" #include "chrome/browser/download/download_shelf.h" @@ -24,6 +27,7 @@ #include "chrome/common/url_constants.h" #include "chrome/test/in_process_browser_test.h" #include "chrome/test/ui_test_utils.h" +#include "content/browser/renderer_host/resource_dispatcher_host.h" #include "content/common/page_transition_types.h" #include "net/base/net_util.h" #include "testing/gtest/include/gtest/gtest.h" @@ -38,14 +42,17 @@ namespace { // occurs at any point after construction. When that state occurs, the class // is considered finished. Callers may either probe for the finished state, or // wait on it. +// +// TODO(rdsmith): Detect manager going down, remove pointer to +// DownloadManager, transition to finished. (For right now we +// just use a scoped_refptr<> to keep it around, but that may cause +// timeouts on waiting if a DownloadManager::Shutdown() occurs which +// cancels our in-progress downloads.) class DownloadsObserver : public DownloadManager::Observer, public DownloadItem::Observer { public: - // The action which should be considered the completion of the download. - enum DownloadFinishedSignal { COMPLETE, FILE_RENAME }; - - // Create an object that will be considered completed when |wait_count| - // download items have entered state |download_finished_signal|. + // Create an object that will be considered finished when |wait_count| + // 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. @@ -57,13 +64,13 @@ class DownloadsObserver : public DownloadManager::Observer, // to treat as completion events. DownloadsObserver(DownloadManager* download_manager, size_t wait_count, - DownloadFinishedSignal download_finished_signal, + DownloadItem::DownloadState download_finished_state, bool finish_on_select_file) : download_manager_(download_manager), wait_count_(wait_count), finished_downloads_at_construction_(0), waiting_(false), - download_finished_signal_(download_finished_signal), + download_finished_state_(download_finished_state), finish_on_select_file_(finish_on_select_file), select_file_dialog_seen_(false) { download_manager_->AddObserver(this); // Will call initial ModelChanged(). @@ -100,12 +107,7 @@ class DownloadsObserver : public DownloadManager::Observer, // DownloadItem::Observer virtual void OnDownloadUpdated(DownloadItem* download) { - if ((download_finished_signal_ == COMPLETE) && download->IsComplete()) - DownloadInFinalState(download); - } - - virtual void OnDownloadFileCompleted(DownloadItem* download) { - if (download_finished_signal_ == FILE_RENAME) + if (download->state() == download_finished_state_) DownloadInFinalState(download); } @@ -114,8 +116,8 @@ class DownloadsObserver : public DownloadManager::Observer, // DownloadManager::Observer virtual void ModelChanged() { // Regenerate DownloadItem observers. If there are any download items - // in the COMPLETE state and that's our final state, note them in - // finished_downloads_ (done by OnDownloadUpdated). + // in our final state, note them in |finished_downloads_| + // (done by |OnDownloadUpdated()|). std::vector<DownloadItem*> downloads; download_manager_->SearchDownloads(string16(), &downloads); @@ -154,9 +156,8 @@ class DownloadsObserver : public DownloadManager::Observer, private: // Called when we know that a download item is in a final state. // Note that this is not the same as it first transitioning in to the - // final state; in the case of DownloadItem::COMPLETE, multiple - // notifications may occur once the item is in that state. So we - // keep our own track of transitions into final. + // final state; multiple notifications may occur once the item is in + // that state. So we keep our own track of transitions into final. void DownloadInFinalState(DownloadItem* download) { if (finished_downloads_.find(download) != finished_downloads_.end()) { // We've already seen terminal state on this download. @@ -175,7 +176,7 @@ class DownloadsObserver : public DownloadManager::Observer, } // The observed download manager. - DownloadManager* download_manager_; + scoped_refptr<DownloadManager> download_manager_; // The set of DownloadItem's that have transitioned to their finished state // since construction of this object. When the size of this array @@ -194,18 +195,18 @@ class DownloadsObserver : public DownloadManager::Observer, // ModelChanged(). We use |finished_downloads_| to track the incoming // transitions to final state we should ignore, and to track the // number of final state transitions that occurred between - // construction and return from wait. But if the final state is the - // COMPLETE state, some downloads may be in it (and thus be entered - // into finished_downloads_) when we construct this class. We don't - // want to count those; + // construction and return from wait. But some downloads may be in our + // final state (and thus be entered into |finished_downloads_|) when we + // construct this class. We don't want to count those in our transition + // to finished. int finished_downloads_at_construction_; // Whether an internal message loop has been started and must be quit upon // all downloads completing. bool waiting_; - // The action on which to consider the DownloadItem finished. - DownloadFinishedSignal download_finished_signal_; + // 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. @@ -217,6 +218,178 @@ class DownloadsObserver : public DownloadManager::Observer, DISALLOW_COPY_AND_ASSIGN(DownloadsObserver); }; +// 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: + explicit DownloadsFlushObserver(DownloadManager* download_manager) + : download_manager_(download_manager), + waiting_for_zero_inprogress_(true) { } + + 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: + // 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, + NewRunnableMethod(this, &DownloadsFlushObserver::PingFileThread, + cycle)); + } else { + BrowserThread::PostTask( + BrowserThread::UI, FROM_HERE, new MessageLoop::QuitTask()); + } + } + + 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 +// Test, specifically the number of outstanding requests on the +// ResourceDispatcherHost and the number of pending downloads on the +// DownloadFileManager. +class CancelTestDataCollector + : public base::RefCountedThreadSafe<CancelTestDataCollector> { + public: + CancelTestDataCollector() + : resource_dispatcher_host_( + g_browser_process->resource_dispatcher_host()), + rdh_pending_requests_(0), + dfm_pending_downloads_(0) { } + + void WaitForDataCollected() { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + NewRunnableMethod(this, &CancelTestDataCollector::IOInfoCollector)); + ui_test_utils::RunMessageLoop(); + } + + int rdh_pending_requests() { return rdh_pending_requests_; } + int dfm_pending_downloads() { return dfm_pending_downloads_; } + + protected: + friend class base::RefCountedThreadSafe<CancelTestDataCollector>; + + virtual ~CancelTestDataCollector() {} + + private: + + void IOInfoCollector() { + download_file_manager_ = resource_dispatcher_host_->download_file_manager(); + rdh_pending_requests_ = resource_dispatcher_host_->pending_requests(); + BrowserThread::PostTask( + BrowserThread::FILE, FROM_HERE, + NewRunnableMethod(this, &CancelTestDataCollector::FileInfoCollector)); + } + + void FileInfoCollector() { + dfm_pending_downloads_ = download_file_manager_->NumberOfActiveDownloads(); + BrowserThread::PostTask( + BrowserThread::UI, FROM_HERE, new MessageLoop::QuitTask()); + } + + ResourceDispatcherHost* resource_dispatcher_host_; + DownloadFileManager* download_file_manager_; + int rdh_pending_requests_; + int dfm_pending_downloads_; + + DISALLOW_COPY_AND_ASSIGN(CancelTestDataCollector); +}; + +} // namespace + class DownloadTest : public InProcessBrowserTest { public: enum SelectExpectation { @@ -282,21 +455,35 @@ class DownloadTest : public InProcessBrowserTest { return download_mananger->download_prefs()->download_path(); } + // Create a DownloadsObserver that will wait for the + // specified number of downloads to finish. DownloadsObserver* CreateWaiter(Browser* browser, int num_downloads) { DownloadManager* download_manager = browser->profile()->GetDownloadManager(); return new DownloadsObserver( download_manager, num_downloads, - DownloadsObserver::FILE_RENAME, // Really done + DownloadItem::COMPLETE, // Really done true); // Bail on select file } + // Create a DownloadsObserver that will wait for the + // specified number of downloads to start. + DownloadsObserver* CreateInProgressWaiter(Browser* browser, + int num_downloads) { + DownloadManager* download_manager = + browser->profile()->GetDownloadManager(); + return new DownloadsObserver( + download_manager, num_downloads, + DownloadItem::IN_PROGRESS, // Has started + true); // Bail on select file + } + // Download |url|, then wait for the download to finish. // |disposition| indicates where the navigation occurs (current tab, new // foreground tab, etc). // |expectation| indicates whether or not a Select File dialog should be // open when the download is finished, or if we don't care. - // If the dialog appears, the test completes. The only effect |expectation| + // If the dialog appears, the routine exits. The only effect |expectation| // has is whether or not the test succeeds. // |browser_test_flags| indicate what to wait for, and is an OR of 0 or more // values in the ui_test_utils::BrowserTestWaitFlags enum. @@ -411,7 +598,7 @@ class DownloadTest : public InProcessBrowserTest { // - Iterate over browser->window()->GetDownloadShelf()'s members // to see if any match the status text we want. Start with the last one. - // Complete sending the request. We do this by loading a second URL in a + // Allow the request to finish. We do this by loading a second URL in a // separate tab. GURL finish_url(URLRequestSlowDownloadJob::kFinishDownloadUrl); ui_test_utils::NavigateToURLWithDisposition( @@ -950,4 +1137,60 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, NewWindow) { CheckDownload(browser(), file, file); } -} // namespace +IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadCancelled) { + ASSERT_TRUE(InitialSetup(false)); + EXPECT_EQ(1, browser()->tab_count()); + + // 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. 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. + scoped_ptr<DownloadsObserver> observer(CreateInProgressWaiter(browser(), 1)); + ui_test_utils::NavigateToURL( + browser(), GURL(URLRequestSlowDownloadJob::kUnknownSizeUrl)); + observer->WaitForFinished(); + + std::vector<DownloadItem*> downloads; + browser()->profile()->GetDownloadManager()->SearchDownloads( + string16(), &downloads); + ASSERT_EQ(1u, downloads.size()); + ASSERT_EQ(DownloadItem::IN_PROGRESS, downloads[0]->state()); + EXPECT_TRUE(IsDownloadUIVisible(browser())); + + // Cancel the download and wait for download system quiesce. + downloads[0]->Remove(/* delete_on_disk = */ true); + 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()); + + // 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. +#if defined(OS_CHROMEOS) + // Except under ChromeOS in which case if we've brought up the file + // picker panel, it stays. + EXPECT_TRUE(IsDownloadUIVisible(browser())); +#else + EXPECT_FALSE(IsDownloadUIVisible(browser())); +#endif +} diff --git a/chrome/browser/download/download_file_manager.cc b/chrome/browser/download/download_file_manager.cc index 33e81fe..2522a98 100644 --- a/chrome/browser/download/download_file_manager.cc +++ b/chrome/browser/download/download_file_manager.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2010 The Chromium Authors. All rights reserved. +// 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. @@ -324,12 +324,13 @@ void DownloadFileManager::RenameInProgressDownloadFile( } // The DownloadManager in the UI thread has provided a final name for the -// download specified by 'id'. Rename the completed download. +// download specified by 'id'. Rename the download that's in the process +// of completing. // // There are 2 possible rename cases where this method can be called: // 1. foo.crdownload -> foo (final, safe) // 2. Unconfirmed.xxx.crdownload -> xxx (final, validated) -void DownloadFileManager::RenameFinishedDownloadFile( +void DownloadFileManager::RenameCompletingDownloadFile( int id, const FilePath& full_path, bool overwrite_existing_file) { VLOG(20) << __FUNCTION__ << "()" << " id = " << id << " overwrite_existing_file = " << overwrite_existing_file @@ -382,8 +383,8 @@ void DownloadFileManager::RenameFinishedDownloadFile( new_path, uniquifier)); } -// Called only from RenameInProgressDownloadFile and RenameFinishedDownloadFile -// on the FILE thread. +// Called only from RenameInProgressDownloadFile and +// RenameCompletingDownloadFile on the FILE thread. void DownloadFileManager::CancelDownloadOnRename(int id) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); diff --git a/chrome/browser/download/download_file_manager.h b/chrome/browser/download/download_file_manager.h index 868ba50..1d96b5d 100644 --- a/chrome/browser/download/download_file_manager.h +++ b/chrome/browser/download/download_file_manager.h @@ -110,9 +110,15 @@ class DownloadFileManager // |overwrite_existing_file| prevents uniquification, and is used for SAFE // downloads, as the user may have decided to overwrite the file. // Sent from the UI thread and run on the FILE thread. - void RenameFinishedDownloadFile(int id, - const FilePath& full_path, - bool overwrite_existing_file); + void RenameCompletingDownloadFile(int id, + const FilePath& full_path, + bool overwrite_existing_file); + + // The number of downloads currently active on the DownloadFileManager. + // Primarily for testing. + int NumberOfActiveDownloads() const { + return downloads_.size(); + } private: friend class base::RefCountedThreadSafe<DownloadFileManager>; @@ -143,7 +149,7 @@ class DownloadFileManager DownloadFile* GetDownloadFile(int id); // Called only from RenameInProgressDownloadFile and - // RenameFinishedDownloadFile on the FILE thread. + // RenameCompletingDownloadFile on the FILE thread. void CancelDownloadOnRename(int id); // Erases the download file with the given the download |id| and removes diff --git a/chrome/browser/download/download_item.cc b/chrome/browser/download/download_item.cc index b6f9c3b..99b6b9d 100644 --- a/chrome/browser/download/download_item.cc +++ b/chrome/browser/download/download_item.cc @@ -33,7 +33,7 @@ // destination file has been determined. // * Entered into the history database. // * Made visible in the download shelf. -// * All data is received. Note that the actual data download occurs +// * All data is saved. Note that the actual data download occurs // in parallel with the above steps, but until those steps are // complete, completion of the data download will be ignored. // * Download file is renamed to its final name, and possibly @@ -246,10 +246,6 @@ void DownloadItem::UpdateObservers() { FOR_EACH_OBSERVER(Observer, observers_, OnDownloadUpdated(this)); } -void DownloadItem::NotifyObserversDownloadFileCompleted() { - FOR_EACH_OBSERVER(Observer, observers_, OnDownloadFileCompleted(this)); -} - bool DownloadItem::CanOpenDownload() { return !Extension::IsExtension(target_name_); } @@ -357,6 +353,7 @@ void DownloadItem::Cancel(bool update_history) { void DownloadItem::MarkAsComplete() { DCHECK(all_data_saved_); state_ = COMPLETE; + UpdateObservers(); } void DownloadItem::OnAllDataSaved(int64 size) { @@ -366,7 +363,7 @@ void DownloadItem::OnAllDataSaved(int64 size) { StopProgressTimer(); } -void DownloadItem::Finished() { +void DownloadItem::Completed() { VLOG(20) << " " << __FUNCTION__ << "() " << DebugString(false); @@ -390,16 +387,13 @@ void DownloadItem::Finished() { auto_opened_ = true; } - // Notify our observers that we are complete (the call to MarkAsComplete() - // set the state to complete but did not notify). - UpdateObservers(); - // The download file is meant to be completed if both the filename is // finalized and the file data is downloaded. The ordering of these two // actions is indeterministic. Thus, if the filename is not finalized yet, // delay the notification. if (name_finalized()) { - NotifyObserversDownloadFileCompleted(); + state_ = COMPLETE; + UpdateObservers(); download_manager_->RemoveFromActiveList(id()); } } @@ -474,17 +468,15 @@ void DownloadItem::OnNameFinalized() { << DebugString(true); name_finalized_ = true; - // The download file is meant to be completed if both the filename is - // finalized and the file data is downloaded. The ordering of these two - // actions is indeterministic. Thus, if we are still in downloading the - // file, delay the notification. - if (IsComplete()) { - NotifyObserversDownloadFileCompleted(); - download_manager_->RemoveFromActiveList(id()); - } + // We can't reach this point in the code without having received all the + // data, so it's safe to move to the COMPLETE state. + DCHECK(all_data_saved_); + state_ = COMPLETE; + UpdateObservers(); + download_manager_->RemoveFromActiveList(id()); } -void DownloadItem::OnDownloadFinished(DownloadFileManager* file_manager) { +void DownloadItem::OnDownloadCompleting(DownloadFileManager* file_manager) { VLOG(20) << " " << __FUNCTION__ << "() " << " needs rename = " << NeedsRename() << " " << DebugString(true); @@ -495,12 +487,14 @@ void DownloadItem::OnDownloadFinished(DownloadFileManager* file_manager) { BrowserThread::PostTask( BrowserThread::FILE, FROM_HERE, NewRunnableMethod( - file_manager, &DownloadFileManager::RenameFinishedDownloadFile, + file_manager, &DownloadFileManager::RenameCompletingDownloadFile, id(), GetTargetFilePath(), safety_state() == SAFE)); return; + } else { + name_finalized_ = true; } - Finished(); + Completed(); BrowserThread::PostTask( BrowserThread::FILE, FROM_HERE, @@ -518,9 +512,7 @@ void DownloadItem::OnDownloadRenamedToFinalName(const FilePath& full_path) { Rename(full_path); OnNameFinalized(); - // This was called from OnDownloadFinished; continue to call - // DownloadFinished. - Finished(); + Completed(); } bool DownloadItem::MatchesQuery(const string16& query) const { diff --git a/chrome/browser/download/download_item.h b/chrome/browser/download/download_item.h index fd81f02..e90dbf51 100644 --- a/chrome/browser/download/download_item.h +++ b/chrome/browser/download/download_item.h @@ -1,4 +1,4 @@ -// Copyright (c) 2010 The Chromium Authors. All rights reserved. +// 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. // @@ -39,16 +39,13 @@ struct DownloadCreateInfo; class DownloadItem { public: enum DownloadState { + // Download is actively progressing. IN_PROGRESS, - // Note that COMPLETE indicates that the download has gotten all of its - // data, has figured out its final destination file, has been entered - // into the history store, and has been shown in the UI. The only - // operations remaining are acceptance by the user of - // a dangerous download (if dangerous), renaming the file - // to the final name, and auto-opening it (if it auto-opens). + // Download is completely finished. COMPLETE, + // Download has been cancelled. CANCELLED, // This state indicates that the download item is about to be destroyed, @@ -82,9 +79,6 @@ class DownloadItem { public: virtual void OnDownloadUpdated(DownloadItem* download) = 0; - // Called when a downloaded file has been completed. - virtual void OnDownloadFileCompleted(DownloadItem* download) = 0; - // Called when a downloaded file has been opened. virtual void OnDownloadOpened(DownloadItem* download) = 0; @@ -115,9 +109,6 @@ class DownloadItem { // Notifies our observers periodically. void UpdateObservers(); - // Notifies our observers the downloaded file has been completed. - void NotifyObserversDownloadFileCompleted(); - // Whether it is OK to open this download. bool CanOpenDownload(); @@ -156,14 +147,10 @@ class DownloadItem { // Called when all data has been saved. Only has display effects. void OnAllDataSaved(int64 size); - // Called when ready to consider the data received and move on to the - // next stage. + // Called by external code (SavePackage) using the DownloadItem interface + // to display progress when the DownloadItem should be considered complete. void MarkAsComplete(); - // Called when the entire download operation (including renaming etc) - // is finished. - void Finished(); - // Download operation had an error. // |size| is the amount of data received so far, and |os_error| is the error // code that the operation received. @@ -210,10 +197,10 @@ class DownloadItem { // Called when the name of the download is finalized. void OnNameFinalized(); - // Called when the download is finished. + // Called when the download is ready to complete. // This may perform final rename if necessary and will eventually call - // DownloadManager::DownloadFinished(). - void OnDownloadFinished(DownloadFileManager* file_manager); + // DownloadItem::Completed(). + void OnDownloadCompleting(DownloadFileManager* file_manager); // Called when the file name for the download is renamed to its final name. void OnDownloadRenamedToFinalName(const FilePath& full_path); @@ -297,6 +284,10 @@ class DownloadItem { // Internal helper for maintaining consistent received and total sizes. void UpdateSize(int64 size); + // Called when the entire download operation (including renaming etc) + // is completed. + void Completed(); + // Start/stop sending periodic updates to our observers void StartProgressTimer(); void StopProgressTimer(); diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc index ccedecf..3d49a46 100644 --- a/chrome/browser/download/download_manager.cc +++ b/chrome/browser/download/download_manager.cc @@ -91,7 +91,7 @@ void DownloadManager::Shutdown() { it++; if (download->safety_state() == DownloadItem::DANGEROUS && - (download->IsPartialDownload() || download->IsComplete())) { + download->IsPartialDownload()) { // The user hasn't accepted it, so we need to remove it // from the disk. This may or may not result in it being // removed from the DownloadManager queues and deleted @@ -505,7 +505,7 @@ void DownloadManager::AttachDownloadItem(DownloadCreateInfo* info) { // The download is a safe download. We need to // rename it to its intermediate '.crdownload' path. The final // name after user confirmation will be set from - // DownloadItem::OnDownloadFinished. + // DownloadItem::OnDownloadCompleting. download_path = download_util::GetCrDownloadPath(info->path); } @@ -644,11 +644,10 @@ void DownloadManager::MaybeCompleteDownload(DownloadItem* download) { UpdateAppIcon(); // Reflect removal from in_progress_. // Final update of download item and history. - download->MarkAsComplete(); download_history_->UpdateEntry(download); // Finish the download. - download->OnDownloadFinished(file_manager_); + download->OnDownloadCompleting(file_manager_); } void DownloadManager::RemoveFromActiveList(int32 download_id) { diff --git a/chrome/browser/download/download_manager.h b/chrome/browser/download/download_manager.h index 3baab3e..84b8399 100644 --- a/chrome/browser/download/download_manager.h +++ b/chrome/browser/download/download_manager.h @@ -99,12 +99,12 @@ class DownloadManager std::vector<DownloadItem*>* result); // Return all non-temporary downloads in the specified directory that are - // are in progress or have finished. + // are in progress or have completed. void GetAllDownloads(const FilePath& dir_path, std::vector<DownloadItem*>* result); // Return all non-temporary downloads in the specified directory that are - // either in-progress or finished but still waiting for user confirmation. + // in-progress (including dangerous downloads waiting for user confirmation). void GetCurrentDownloads(const FilePath& dir_path, std::vector<DownloadItem*>* result); @@ -130,7 +130,7 @@ class DownloadManager void RemoveDownload(int64 download_handle); // Determine if the download is ready for completion, i.e. has had - // all data received, and completed the filename determination and + // all data saved, and completed the filename determination and // history insertion. bool IsDownloadReadyForCompletion(DownloadItem* download); @@ -347,13 +347,14 @@ class DownloadManager // // When a download is created through a user action, the corresponding // DownloadItem* is placed in |active_downloads_| and remains there until the - // download has finished. It is also placed in |in_progress_| and remains - // there until it has received a valid handle from the history system. Once - // it has a valid handle, the DownloadItem* is placed in the - // |history_downloads_| map. When the download is complete, it is removed - // from |in_progress_|. Downloads from past sessions read from a - // persisted state from the history system are placed directly into - // |history_downloads_| since they have valid handles in the history system. + // download is in a terminal state (COMPLETE or CANCELLED). It is also + // placed in |in_progress_| and remains there until it has received a + // valid handle from the history system. Once it has a valid handle, the + // DownloadItem* is placed in the |history_downloads_| map. When the + // download reaches a terminal state, it is removed from |in_progress_|. + // Downloads from past sessions read from a persisted state from the + // history system are placed directly into |history_downloads_| since + // they have valid handles in the history system. typedef std::set<DownloadItem*> DownloadSet; typedef base::hash_map<int64, DownloadItem*> DownloadMap; diff --git a/chrome/browser/download/download_manager_unittest.cc b/chrome/browser/download/download_manager_unittest.cc index baff0b2..9b92a497 100644 --- a/chrome/browser/download/download_manager_unittest.cc +++ b/chrome/browser/download/download_manager_unittest.cc @@ -253,7 +253,7 @@ class ItemObserver : public DownloadItem::Observer { public: explicit ItemObserver(DownloadItem* tracked) : tracked_(tracked), states_hit_(0), - was_updated_(false), was_completed_(false), was_opened_(false) { + was_updated_(false), was_opened_(false) { DCHECK(tracked_); tracked_->AddObserver(this); // Record the initial state. @@ -267,7 +267,6 @@ class ItemObserver : public DownloadItem::Observer { return (1 << state) & states_hit_; } bool was_updated() const { return was_updated_; } - bool was_completed() const { return was_completed_; } bool was_opened() const { return was_opened_; } private: @@ -277,11 +276,6 @@ class ItemObserver : public DownloadItem::Observer { states_hit_ |= (1 << download->state()); was_updated_ = true; } - virtual void OnDownloadFileCompleted(DownloadItem* download) { - DCHECK_EQ(tracked_, download); - states_hit_ |= (1 << download->state()); - was_completed_ = true; - } virtual void OnDownloadOpened(DownloadItem* download) { DCHECK_EQ(tracked_, download); states_hit_ |= (1 << download->state()); @@ -291,7 +285,6 @@ class ItemObserver : public DownloadItem::Observer { DownloadItem* tracked_; int states_hit_; bool was_updated_; - bool was_completed_; bool was_opened_; }; @@ -450,7 +443,6 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) { EXPECT_FALSE(observer->hit_state(DownloadItem::CANCELLED)); EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING)); EXPECT_TRUE(observer->was_updated()); - EXPECT_FALSE(observer->was_completed()); EXPECT_FALSE(observer->was_opened()); download->Cancel(true); @@ -462,7 +454,6 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) { EXPECT_FALSE(observer->hit_state(DownloadItem::CANCELLED)); EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING)); EXPECT_TRUE(observer->was_updated()); - EXPECT_FALSE(observer->was_completed()); EXPECT_FALSE(observer->was_opened()); } @@ -516,7 +507,6 @@ TEST_F(DownloadManagerTest, DownloadCancelTest) { EXPECT_FALSE(observer->hit_state(DownloadItem::COMPLETE)); EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING)); EXPECT_TRUE(observer->was_updated()); - EXPECT_FALSE(observer->was_completed()); EXPECT_FALSE(observer->was_opened()); EXPECT_FALSE(file_util::PathExists(new_path)); @@ -597,7 +587,6 @@ TEST_F(DownloadManagerTest, DownloadOverwriteTest) { EXPECT_TRUE(observer->hit_state(DownloadItem::COMPLETE)); EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING)); EXPECT_TRUE(observer->was_updated()); - EXPECT_TRUE(observer->was_completed()); EXPECT_FALSE(observer->was_opened()); EXPECT_EQ(DownloadItem::COMPLETE, download->state()); diff --git a/chrome/browser/download/drag_download_file.cc b/chrome/browser/download/drag_download_file.cc index e047100..261e3ec 100644 --- a/chrome/browser/download/drag_download_file.cc +++ b/chrome/browser/download/drag_download_file.cc @@ -167,23 +167,17 @@ void DragDownloadFile::ModelChanged() { void DragDownloadFile::OnDownloadUpdated(DownloadItem* download) { AssertCurrentlyOnUIThread(); - if (download->IsCancelled()) { download->RemoveObserver(this); download_manager_->RemoveObserver(this); DownloadCompleted(false); + } else if (download->IsComplete()) { + download->RemoveObserver(this); + download_manager_->RemoveObserver(this); + DownloadCompleted(true); } -} - -void DragDownloadFile::OnDownloadFileCompleted(DownloadItem* download) { - AssertCurrentlyOnUIThread(); - DCHECK(download->IsComplete()); - - download->RemoveObserver(this); - download_manager_->RemoveObserver(this); - - DownloadCompleted(true); + // Ignore other states. } void DragDownloadFile::AssertCurrentlyOnDragThread() { diff --git a/chrome/browser/download/drag_download_file.h b/chrome/browser/download/drag_download_file.h index 01c73cb..581f16d 100644 --- a/chrome/browser/download/drag_download_file.h +++ b/chrome/browser/download/drag_download_file.h @@ -56,7 +56,6 @@ class DragDownloadFile : public ui::DownloadFileProvider, // DownloadItem::Observer methods. // Called on UI thread. virtual void OnDownloadUpdated(DownloadItem* download); - virtual void OnDownloadFileCompleted(DownloadItem* download); virtual void OnDownloadOpened(DownloadItem* download) { } private: diff --git a/chrome/browser/download/mock_download_manager.h b/chrome/browser/download/mock_download_manager.h index 270ae0f..410bdc30 100644 --- a/chrome/browser/download/mock_download_manager.h +++ b/chrome/browser/download/mock_download_manager.h @@ -1,9 +1,9 @@ -// Copyright (c) 2010 The Chromium Authors. All rights reserved. +// 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. -#ifndef MOCK_DOWNLOAD_MANAGER_H_ -#define MOCK_DOWNLOAD_MANAGER_H_ +#ifndef CHROME_BROWSER_DOWNLOAD_MOCK_DOWNLOAD_MANAGER_H_ +#define CHROME_BROWSER_DOWNLOAD_MOCK_DOWNLOAD_MANAGER_H_ #pragma once #include "chrome/browser/download/download_manager.h" @@ -19,7 +19,6 @@ class MockDownloadManager : public DownloadManager { // Override some functions. virtual void UpdateHistoryForDownload(DownloadItem*) { } - virtual void ContinueDownloadFinished(DownloadItem*) { } }; -#endif // MOCK_DOWNLOAD_MANAGER_H_ +#endif // CHROME_BROWSER_DOWNLOAD_MOCK_DOWNLOAD_MANAGER_H_ diff --git a/chrome/browser/download/save_package.cc b/chrome/browser/download/save_package.cc index 9c54006..c120e83 100644 --- a/chrome/browser/download/save_package.cc +++ b/chrome/browser/download/save_package.cc @@ -744,9 +744,6 @@ void SavePackage::Finish() { download_->OnAllDataSaved(all_save_items_count_); download_->MarkAsComplete(); - // Notify download observers that we are complete (the call - // to OnReadyToFinish() set the state to complete but did not notify). - download_->UpdateObservers(); NotificationService::current()->Notify( NotificationType::SAVE_PACKAGE_SUCCESSFULLY_FINISHED, diff --git a/chrome/browser/ui/cocoa/download/download_item_mac.h b/chrome/browser/ui/cocoa/download/download_item_mac.h index a15f670..24d9c4a 100644 --- a/chrome/browser/ui/cocoa/download/download_item_mac.h +++ b/chrome/browser/ui/cocoa/download/download_item_mac.h @@ -38,7 +38,6 @@ class DownloadItemMac : DownloadItem::Observer { // DownloadItem::Observer implementation virtual void OnDownloadUpdated(DownloadItem* download); virtual void OnDownloadOpened(DownloadItem* download); - virtual void OnDownloadFileCompleted(DownloadItem* download) { } BaseDownloadItemModel* download_model() { return download_model_.get(); } diff --git a/chrome/browser/ui/gtk/download/download_item_gtk.cc b/chrome/browser/ui/gtk/download/download_item_gtk.cc index a957edd..f99fbeb 100644 --- a/chrome/browser/ui/gtk/download/download_item_gtk.cc +++ b/chrome/browser/ui/gtk/download/download_item_gtk.cc @@ -176,7 +176,8 @@ DownloadItemGtk::DownloadItemGtk(DownloadShelfGtk* parent_shelf, complete_animation_(this), icon_small_(NULL), icon_large_(NULL), - creation_time_(base::Time::Now()) { + creation_time_(base::Time::Now()), + download_complete_(false) { LoadIcon(); body_.Own(gtk_button_new()); @@ -393,6 +394,11 @@ void DownloadItemGtk::OnDownloadUpdated(DownloadItem* download) { complete_animation_.Show(); break; case DownloadItem::COMPLETE: + if (download_complete_) + // We've already handled the completion specific actions; skip + // doing them again. + break; + if (download->auto_opened()) { parent_shelf_->RemoveDownloadItem(this); // This will delete us! return; @@ -403,6 +409,7 @@ void DownloadItemGtk::OnDownloadUpdated(DownloadItem* download) { DownloadItemDrag::SetSource(body_.get(), get_download(), icon_large_); complete_animation_.Show(); + download_complete_ = true; break; case DownloadItem::IN_PROGRESS: get_download()->is_paused() ? diff --git a/chrome/browser/ui/gtk/download/download_item_gtk.h b/chrome/browser/ui/gtk/download/download_item_gtk.h index 6af9f6c..a8f21b4 100644 --- a/chrome/browser/ui/gtk/download/download_item_gtk.h +++ b/chrome/browser/ui/gtk/download/download_item_gtk.h @@ -49,7 +49,6 @@ class DownloadItemGtk : public DownloadItem::Observer, // DownloadItem::Observer implementation. virtual void OnDownloadUpdated(DownloadItem* download); - virtual void OnDownloadFileCompleted(DownloadItem* download) { } virtual void OnDownloadOpened(DownloadItem* download) { } // ui::AnimationDelegate implementation. @@ -233,6 +232,10 @@ class DownloadItemGtk : public DownloadItem::Observer, // For canceling an in progress icon request. CancelableRequestConsumerT<int, 0> icon_consumer_; + + // Indicates when the download has completed, so we don't redo + // on-completion actions. + bool download_complete_; }; #endif // CHROME_BROWSER_UI_GTK_DOWNLOAD_DOWNLOAD_ITEM_GTK_H_ diff --git a/chrome/browser/ui/views/download/download_item_view.h b/chrome/browser/ui/views/download/download_item_view.h index d15bc00..295d52d 100644 --- a/chrome/browser/ui/views/download/download_item_view.h +++ b/chrome/browser/ui/views/download/download_item_view.h @@ -74,7 +74,6 @@ class DownloadItemView : public views::ButtonListener, // DownloadObserver method virtual void OnDownloadUpdated(DownloadItem* download) OVERRIDE; - virtual void OnDownloadFileCompleted(DownloadItem* download) OVERRIDE { } virtual void OnDownloadOpened(DownloadItem* download) OVERRIDE; // Overridden from views::View: diff --git a/chrome/browser/ui/webui/active_downloads_ui.cc b/chrome/browser/ui/webui/active_downloads_ui.cc index 580193b..b37c280 100644 --- a/chrome/browser/ui/webui/active_downloads_ui.cc +++ b/chrome/browser/ui/webui/active_downloads_ui.cc @@ -115,7 +115,6 @@ class ActiveDownloadsHandler // DownloadItem::Observer interface. virtual void OnDownloadUpdated(DownloadItem* item); - virtual void OnDownloadFileCompleted(DownloadItem* item); virtual void OnDownloadOpened(DownloadItem* item) { } // DownloadManager::Observer interface. @@ -704,10 +703,6 @@ void ActiveDownloadsHandler::OnDownloadUpdated(DownloadItem* item) { *download_util::CreateDownloadItemValue(item, id)); } -void ActiveDownloadsHandler::OnDownloadFileCompleted(DownloadItem* item) { - OnDownloadUpdated(item); -} - void ActiveDownloadsHandler::DeleteFile(const FilePath& path, TaskProxy* task) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); if (!file_util::Delete(path, true)) { diff --git a/chrome/browser/ui/webui/chromeos/imageburner_ui.cc b/chrome/browser/ui/webui/chromeos/imageburner_ui.cc index 30a6a96..1d66791 100644 --- a/chrome/browser/ui/webui/chromeos/imageburner_ui.cc +++ b/chrome/browser/ui/webui/chromeos/imageburner_ui.cc @@ -244,19 +244,18 @@ void ImageBurnHandler::ProgressUpdated(chromeos::BurnLibrary* object, } void ImageBurnHandler::OnDownloadUpdated(DownloadItem* download) { - if (download->IsPartialDownload()) { - scoped_ptr<DictionaryValue> result_value( - download_util::CreateDownloadItemValue(download, 0)); - web_ui_->CallJavascriptFunction("downloadUpdated", *result_value); + if (download->IsCancelled()) { + DownloadCompleted(false); // Should stop observation. + DCHECK(!download_item_observer_added_); + } else if (download->IsComplete()) { + zip_image_file_path_ = download->full_path(); + DownloadCompleted(true); // Should stop observation. + DCHECK(!download_item_observer_added_); + } else if (download->IsPartialDownload()) { + scoped_ptr<DictionaryValue> result_value( + download_util::CreateDownloadItemValue(download, 0)); + web_ui_->CallJavascriptFunction("downloadUpdated", *result_value); } - if (download->IsCancelled()) - DownloadCompleted(false); -} - -void ImageBurnHandler::OnDownloadFileCompleted(DownloadItem* download) { - DCHECK(download->IsComplete()); - zip_image_file_path_ = download->full_path(); - DownloadCompleted(true); } void ImageBurnHandler::OnDownloadOpened(DownloadItem* download) { @@ -590,18 +589,19 @@ void ImageBurnResourceManager::OnDownloadUpdated(DownloadItem* download) { if (download->IsCancelled()) { image_url_.reset(); ConfigFileFetched(false); - } -} -void ImageBurnResourceManager::OnDownloadFileCompleted(DownloadItem* download) { - DCHECK(download->IsComplete()); - std::string image_url; - if (file_util::ReadFileToString(config_file_path_, &image_url)) { - image_url_.reset(new GURL(std::string(kImageBaseURL) + image_url)); - ConfigFileFetched(true); - } else { - image_url_.reset(); - ConfigFileFetched(false); + // ConfigFileFetched should remove observer. + DCHECK(!download_item_observer_added_); + DCHECK(active_download_item_ == NULL); + } else if (download->IsComplete()) { + std::string image_url; + if (file_util::ReadFileToString(config_file_path_, &image_url)) { + image_url_.reset(new GURL(std::string(kImageBaseURL) + image_url)); + ConfigFileFetched(true); + } else { + image_url_.reset(); + ConfigFileFetched(false); + } } } diff --git a/chrome/browser/ui/webui/chromeos/imageburner_ui.h b/chrome/browser/ui/webui/chromeos/imageburner_ui.h index 3a64f84..4dfe9e1 100644 --- a/chrome/browser/ui/webui/chromeos/imageburner_ui.h +++ b/chrome/browser/ui/webui/chromeos/imageburner_ui.h @@ -99,7 +99,6 @@ class ImageBurnResourceManager // DownloadItem::Observer interface virtual void OnDownloadUpdated(DownloadItem* download) OVERRIDE; - virtual void OnDownloadFileCompleted(DownloadItem* download) OVERRIDE; virtual void OnDownloadOpened(DownloadItem* download) OVERRIDE {} // DownloadManager::Observer interface @@ -192,7 +191,6 @@ class ImageBurnHandler : public WebUIMessageHandler, // DownloadItem::Observer interface. virtual void OnDownloadUpdated(DownloadItem* download) OVERRIDE; - virtual void OnDownloadFileCompleted(DownloadItem* download) OVERRIDE; virtual void OnDownloadOpened(DownloadItem* download) OVERRIDE; // DownloadManager::Observer interface. diff --git a/chrome/browser/ui/webui/downloads_dom_handler.h b/chrome/browser/ui/webui/downloads_dom_handler.h index 3778015..bda09ec 100644 --- a/chrome/browser/ui/webui/downloads_dom_handler.h +++ b/chrome/browser/ui/webui/downloads_dom_handler.h @@ -31,7 +31,6 @@ class DownloadsDOMHandler : public WebUIMessageHandler, // DownloadItem::Observer interface virtual void OnDownloadUpdated(DownloadItem* download); - virtual void OnDownloadFileCompleted(DownloadItem* download) { } virtual void OnDownloadOpened(DownloadItem* download) { } // DownloadManager::Observer interface |