diff options
| -rw-r--r-- | chrome/browser/download/download_browsertest.cc | 453 | ||||
| -rw-r--r-- | chrome/browser/download/download_service.cc | 23 | ||||
| -rw-r--r-- | chrome/browser/download/download_service.h | 6 | ||||
| -rw-r--r-- | chrome/browser/download/download_test_observer.cc | 297 | ||||
| -rw-r--r-- | chrome/browser/download/download_test_observer.h | 181 | ||||
| -rw-r--r-- | chrome/browser/ui/browser.cc | 162 | ||||
| -rw-r--r-- | chrome/browser/ui/browser.h | 27 | ||||
| -rw-r--r-- | chrome/browser/ui/browser_close_browsertest.cc | 551 | ||||
| -rw-r--r-- | chrome/browser/ui/browser_list.cc | 21 | ||||
| -rw-r--r-- | chrome/browser/ui/gtk/download/download_in_progress_dialog_gtk.cc | 15 | ||||
| -rw-r--r-- | chrome/browser/ui/views/download/download_in_progress_dialog_view.cc | 15 | ||||
| -rw-r--r-- | chrome/chrome_tests.gypi | 2 |
12 files changed, 1238 insertions, 515 deletions
diff --git a/chrome/browser/download/download_browsertest.cc b/chrome/browser/download/download_browsertest.cc index 2731a6c..8668631 100644 --- a/chrome/browser/download/download_browsertest.cc +++ b/chrome/browser/download/download_browsertest.cc @@ -21,6 +21,7 @@ #include "chrome/browser/download/download_service.h" #include "chrome/browser/download/download_service_factory.h" #include "chrome/browser/download/download_shelf.h" +#include "chrome/browser/download/download_test_observer.h" #include "chrome/browser/download/download_util.h" #include "chrome/browser/extensions/extension_install_ui.h" #include "chrome/browser/extensions/extension_service.h" @@ -83,359 +84,6 @@ void DenyDangerousDownload(scoped_refptr<DownloadManager> download_manager, download->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); } -// Construction of this class defines a system state, based on some number -// of downloads being seen in a particular state + other events that -// may occur in the download system. That state will be recorded if it -// 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: - // 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. - - // TODO(rdsmith): Consider rewriting the interface to take a list of events - // to treat as completion events. - DownloadsObserver(DownloadManager* download_manager, - size_t wait_count, - 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_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_NE(DownloadItem::REMOVING, download_finished_state) - << "Waiting for REMOVING is not supported. Try COMPLETE."; - } - - ~DownloadsObserver() { - std::set<DownloadItem*>::iterator it = downloads_observed_.begin(); - for (; it != downloads_observed_.end(); ++it) - (*it)->RemoveObserver(this); - - download_manager_->RemoveObserver(this); - } - - // State accessors. - bool select_file_dialog_seen() { return select_file_dialog_seen_; } - - // Wait for whatever state was specified in the constructor. - void WaitForFinished() { - if (!IsFinished()) { - waiting_ = true; - ui_test_utils::RunMessageLoop(); - waiting_ = false; - } - } - - // Return true if everything's happened that we're configured for. - bool IsFinished() { - if (finished_downloads_.size() - finished_downloads_at_construction_ - >= wait_count_) - return true; - return (finish_on_select_file_ && select_file_dialog_seen_); - } - - // DownloadItem::Observer - virtual void OnDownloadUpdated(DownloadItem* download) { - // The REMOVING state indicates that the download is being destroyed. - // Stop observing. Do not do anything with it, as it is about to be gone. - if (download->state() == DownloadItem::REMOVING) { - std::set<DownloadItem*>::iterator it = downloads_observed_.find(download); - ASSERT_TRUE(it != downloads_observed_.end()); - downloads_observed_.erase(it); - download->RemoveObserver(this); - return; - } - - // Real UI code gets the user's response after returning from the observer. - if (download->safety_state() == DownloadItem::DANGEROUS && - !ContainsKey(dangerous_downloads_seen_, download->id())) { - dangerous_downloads_seen_.insert(download->id()); - - // Calling DangerousDownloadValidated() at this point will - // cause the download to be completed twice. Do what the real UI - // code does: make the call as a delayed task. - switch (dangerous_download_action_) { - case ON_DANGEROUS_DOWNLOAD_ACCEPT: - // Fake user click on "Accept". Delay the actual click, as the - // real UI would. - BrowserThread::PostTask( - BrowserThread::UI, FROM_HERE, - base::Bind(&AcceptDangerousDownload, download_manager_, - download->id())); - break; - - case ON_DANGEROUS_DOWNLOAD_DENY: - // Fake a user click on "Deny". Delay the actual click, as the - // real UI would. - BrowserThread::PostTask( - BrowserThread::UI, FROM_HERE, - base::Bind(&DenyDangerousDownload, download_manager_, - download->id())); - break; - - case ON_DANGEROUS_DOWNLOAD_FAIL: - ADD_FAILURE() << "Unexpected dangerous download item."; - break; - - default: - NOTREACHED(); - } - } - - if (download->state() == download_finished_state_) - DownloadInFinalState(download); - } - - virtual void OnDownloadOpened(DownloadItem* download) {} - - // DownloadManager::Observer - virtual void ModelChanged() { - // Regenerate DownloadItem observers. If there are any download items - // in our final state, note them in |finished_downloads_| - // (done by |OnDownloadUpdated()|). - std::vector<DownloadItem*> downloads; - download_manager_->GetAllDownloads(FilePath(), &downloads); - - std::vector<DownloadItem*>::iterator it = downloads.begin(); - for (; it != downloads.end(); ++it) { - OnDownloadUpdated(*it); // Safe to call multiple times; checks state. - - std::set<DownloadItem*>::const_iterator - finished_it(finished_downloads_.find(*it)); - std::set<DownloadItem*>::iterator - observed_it(downloads_observed_.find(*it)); - - // If it isn't finished and we're aren't observing it, start. - if (finished_it == finished_downloads_.end() && - observed_it == downloads_observed_.end()) { - (*it)->AddObserver(this); - downloads_observed_.insert(*it); - continue; - } - - // If it is finished and we are observing it, stop. - if (finished_it != finished_downloads_.end() && - observed_it != downloads_observed_.end()) { - (*it)->RemoveObserver(this); - downloads_observed_.erase(observed_it); - continue; - } - } - } - - virtual void SelectFileDialogDisplayed(int32 /* id */) { - select_file_dialog_seen_ = true; - SignalIfFinished(); - } - - virtual size_t NumDangerousDownloadsSeen() const { - return dangerous_downloads_seen_.size(); - } - - 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; 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. - return; - } - - // Record the transition. - finished_downloads_.insert(download); - - SignalIfFinished(); - } - - void SignalIfFinished() { - if (waiting_ && IsFinished()) - MessageLoopForUI::current()->Quit(); - } - - // The observed 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 - // reaches wait_count_, we're done. - std::set<DownloadItem*> finished_downloads_; - - // The set of DownloadItem's we are currently observing. Generally there - // won't be any overlap with the above; once we see the final state - // on a DownloadItem, we'll stop observing it. - std::set<DownloadItem*> downloads_observed_; - - // The number of downloads to wait on completing. - size_t wait_count_; - - // The number of downloads entered in final state in initial - // 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 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 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. - bool finish_on_select_file_; - - // True if we've seen the select file dialog. - bool select_file_dialog_seen_; - - // Action to take if a dangerous download is encountered. - DangerousDownloadAction dangerous_download_action_; - - // Holds the download ids which were dangerous. - std::set<int32> dangerous_downloads_seen_; - - 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, - base::Bind(&DownloadsFlushObserver::PingFileThread, this, 2)); - } - } - } - - void PingFileThread(int cycle) { - BrowserThread::PostTask( - BrowserThread::IO, FROM_HERE, - base::Bind(&DownloadsFlushObserver::PingIOThread, this, cycle)); - } - - void PingIOThread(int cycle) { - if (--cycle) { - BrowserThread::PostTask( - BrowserThread::UI, FROM_HERE, - base::Bind(&DownloadsFlushObserver::PingFileThread, this, 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 @@ -722,39 +370,39 @@ class DownloadTest : public InProcessBrowserTest { return GetDownloadPrefs(browser)->download_path(); } - // Create a DownloadsObserver that will wait for the + // Create a DownloadTestObserver that will wait for the // specified number of downloads to finish. - DownloadsObserver* CreateWaiter(Browser* browser, int num_downloads) { + DownloadTestObserver* CreateWaiter(Browser* browser, int num_downloads) { DownloadManager* download_manager = DownloadManagerForBrowser(browser); - return new DownloadsObserver( + return new DownloadTestObserver( download_manager, num_downloads, DownloadItem::COMPLETE, // Really done - true, // Bail on select file - ON_DANGEROUS_DOWNLOAD_FAIL); + true, // Bail on select file + DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL); } - // Create a DownloadsObserver that will wait for the + // Create a DownloadTestObserver that will wait for the // specified number of downloads to start. - DownloadsObserver* CreateInProgressWaiter(Browser* browser, + DownloadTestObserver* CreateInProgressWaiter(Browser* browser, int num_downloads) { DownloadManager* download_manager = DownloadManagerForBrowser(browser); - return new DownloadsObserver( + return new DownloadTestObserver( download_manager, num_downloads, DownloadItem::IN_PROGRESS, // Has started true, // Bail on select file - ON_DANGEROUS_DOWNLOAD_FAIL); + DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL); } - // Create a DownloadsObserver that will wait for the + // Create a DownloadTestObserver that will wait for the // specified number of downloads to finish, or for // a dangerous download warning to be shown. - DownloadsObserver* DangerousInstallWaiter( + DownloadTestObserver* DangerousInstallWaiter( Browser* browser, int num_downloads, DownloadItem::DownloadState final_state, - DangerousDownloadAction dangerous_download_action) { + DownloadTestObserver::DangerousDownloadAction dangerous_download_action) { DownloadManager* download_manager = DownloadManagerForBrowser(browser); - return new DownloadsObserver( + return new DownloadTestObserver( download_manager, num_downloads, final_state, true, // Bail on select file @@ -776,7 +424,7 @@ class DownloadTest : public InProcessBrowserTest { SelectExpectation expectation, int browser_test_flags) { // Setup notification, navigate, and block. - scoped_ptr<DownloadsObserver> observer(CreateWaiter(browser, 1)); + scoped_ptr<DownloadTestObserver> observer(CreateWaiter(browser, 1)); // This call will block until the condition specified by // |browser_test_flags|, but will not wait for the download to finish. ui_test_utils::NavigateToURLWithDisposition(browser, @@ -868,7 +516,7 @@ class DownloadTest : public InProcessBrowserTest { // Download a partial web page in a background tab and wait. // The mock system will not complete until it gets a special URL. - scoped_ptr<DownloadsObserver> observer(CreateWaiter(browser, 1)); + scoped_ptr<DownloadTestObserver> observer(CreateWaiter(browser, 1)); ui_test_utils::NavigateToURL(browser, url); // TODO(ahendrickson): check download status text before downloading. @@ -1076,13 +724,13 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadMimeTypeSelect) { // Download the file and wait. We expect the Select File dialog to appear // due to the MIME type, but we still wait until the download completes. - scoped_ptr<DownloadsObserver> observer( - new DownloadsObserver( + scoped_ptr<DownloadTestObserver> observer( + new DownloadTestObserver( DownloadManagerForBrowser(browser()), 1, DownloadItem::COMPLETE, // Really done false, // Continue on select file. - ON_DANGEROUS_DOWNLOAD_FAIL)); + DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL)); ui_test_utils::NavigateToURLWithDisposition( browser(), url, CURRENT_TAB, ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION); @@ -1503,7 +1151,8 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, MultiDownload) { // Create a download, wait until it's started, and confirm // we're in the expected state. - scoped_ptr<DownloadsObserver> observer1(CreateInProgressWaiter(browser(), 1)); + scoped_ptr<DownloadTestObserver> observer1( + CreateInProgressWaiter(browser(), 1)); ui_test_utils::NavigateToURL( browser(), GURL(URLRequestSlowDownloadJob::kUnknownSizeUrl)); observer1->WaitForFinished(); @@ -1537,7 +1186,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, MultiDownload) { // Allow the first request to finish. We do this by loading a third URL // in a separate tab. - scoped_ptr<DownloadsObserver> observer2(CreateWaiter(browser(), 1)); + scoped_ptr<DownloadTestObserver> observer2(CreateWaiter(browser(), 1)); GURL finish_url(URLRequestSlowDownloadJob::kFinishDownloadUrl); ui_test_utils::NavigateToURLWithDisposition( browser(), @@ -1572,11 +1221,11 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadCancelled) { 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 + // DownloadTestObserver 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 + // DownloadTestObserver, 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 @@ -1589,7 +1238,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadCancelled) { // Create a download, wait until it's started, and confirm // we're in the expected state. - scoped_ptr<DownloadsObserver> observer( + scoped_ptr<DownloadTestObserver> observer( CreateInProgressWaiter(browser(), 1)); ui_test_utils::NavigateToURL( browser(), GURL(URLRequestSlowDownloadJob::kUnknownSizeUrl)); @@ -1604,8 +1253,9 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadCancelled) { // Cancel the download and wait for download system quiesce. downloads[0]->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); - scoped_refptr<DownloadsFlushObserver> flush_observer( - new DownloadsFlushObserver(DownloadManagerForBrowser(browser()))); + scoped_refptr<DownloadTestFlushObserver> flush_observer( + new DownloadTestFlushObserver( + DownloadManagerForBrowser(browser()))); flush_observer->WaitForFlush(); // Get the important info from other threads and check it. @@ -1719,7 +1369,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, AnchorDownloadTag) { // Create a download, wait until it's complete, and confirm // we're in the expected state. - scoped_ptr<DownloadsObserver> observer(CreateWaiter(browser(), 1)); + scoped_ptr<DownloadTestObserver> observer(CreateWaiter(browser(), 1)); ui_test_utils::NavigateToURL(browser(), url); observer->WaitForFinished(); @@ -1765,11 +1415,10 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, CrxDenyInstall) { ASSERT_TRUE(InitialSetup(false)); GURL extension_url(URLRequestMockHTTPJob::GetMockUrl(kGoodCrxPath)); - scoped_ptr<DownloadsObserver> observer( - DangerousInstallWaiter(browser(), - 1, - DownloadItem::CANCELLED, - ON_DANGEROUS_DOWNLOAD_DENY)); + scoped_ptr<DownloadTestObserver> observer( + DangerousInstallWaiter( + browser(), 1, DownloadItem::CANCELLED, + DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_DENY)); ui_test_utils::NavigateToURL(browser(), extension_url); observer->WaitForFinished(); @@ -1795,11 +1444,10 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, CrxInstallDenysPermissions) { download_crx_util::SetMockInstallUIForTesting( new MockAbortExtensionInstallUI()); - scoped_ptr<DownloadsObserver> observer( - DangerousInstallWaiter(browser(), - 1, - DownloadItem::COMPLETE, - ON_DANGEROUS_DOWNLOAD_ACCEPT)); + scoped_ptr<DownloadTestObserver> observer( + DangerousInstallWaiter( + browser(), 1, DownloadItem::COMPLETE, + DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_ACCEPT)); ui_test_utils::NavigateToURL(browser(), extension_url); observer->WaitForFinished(); @@ -1825,11 +1473,10 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, CrxInstallAcceptPermissions) { download_crx_util::SetMockInstallUIForTesting( new MockAutoConfirmExtensionInstallUI(browser()->profile())); - scoped_ptr<DownloadsObserver> observer( - DangerousInstallWaiter(browser(), - 1, - DownloadItem::COMPLETE, - ON_DANGEROUS_DOWNLOAD_ACCEPT)); + scoped_ptr<DownloadTestObserver> observer( + DangerousInstallWaiter( + browser(), 1, DownloadItem::COMPLETE, + DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_ACCEPT)); ui_test_utils::NavigateToURL(browser(), extension_url); observer->WaitForFinished(); @@ -1856,11 +1503,10 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, CrxInvalid) { download_crx_util::SetMockInstallUIForTesting( new MockAutoConfirmExtensionInstallUI(browser()->profile())); - scoped_ptr<DownloadsObserver> observer( - DangerousInstallWaiter(browser(), - 1, - DownloadItem::COMPLETE, - ON_DANGEROUS_DOWNLOAD_ACCEPT)); + scoped_ptr<DownloadTestObserver> observer( + DangerousInstallWaiter( + browser(), 1, DownloadItem::COMPLETE, + DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_ACCEPT)); ui_test_utils::NavigateToURL(browser(), extension_url); observer->WaitForFinished(); @@ -1881,11 +1527,10 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, CrxLargeTheme) { download_crx_util::SetMockInstallUIForTesting( new MockAutoConfirmExtensionInstallUI(browser()->profile())); - scoped_ptr<DownloadsObserver> observer( - DangerousInstallWaiter(browser(), - 1, - DownloadItem::COMPLETE, - ON_DANGEROUS_DOWNLOAD_ACCEPT)); + scoped_ptr<DownloadTestObserver> observer( + DangerousInstallWaiter( + browser(), 1, DownloadItem::COMPLETE, + DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_ACCEPT)); ui_test_utils::NavigateToURL(browser(), extension_url); observer->WaitForFinished(); diff --git a/chrome/browser/download/download_service.cc b/chrome/browser/download/download_service.cc index 659af4f..6b5fcb2 100644 --- a/chrome/browser/download/download_service.cc +++ b/chrome/browser/download/download_service.cc @@ -6,7 +6,9 @@ #include "chrome/browser/browser_process.h" #include "chrome/browser/download/chrome_download_manager_delegate.h" +#include "chrome/browser/download/download_service_factory.h" #include "chrome/browser/profiles/profile.h" +#include "chrome/browser/profiles/profile_manager.h" #include "content/browser/download/download_manager.h" DownloadService::DownloadService(Profile* profile) @@ -34,6 +36,27 @@ bool DownloadService::HasCreatedDownloadManager() { return download_manager_created_; } +int DownloadService::DownloadCount() const { + return download_manager_created_ ? manager_->in_progress_count() : 0; +} + +// static +int DownloadService::DownloadCountAllProfiles() { + std::vector<Profile*> profiles( + g_browser_process->profile_manager()->GetLoadedProfiles()); + + int count = 0; + for (std::vector<Profile*>::iterator it = profiles.begin(); + it < profiles.end(); ++it) { + count += DownloadServiceFactory::GetForProfile(*it)->DownloadCount(); + if ((*it)->HasOffTheRecordProfile()) + count += DownloadServiceFactory::GetForProfile( + (*it)->GetOffTheRecordProfile())->DownloadCount(); + } + + return count; +} + void DownloadService::SetDownloadManagerDelegateForTesting( ChromeDownloadManagerDelegate* new_delegate) { // Guarantee everything is properly initialized. diff --git a/chrome/browser/download/download_service.h b/chrome/browser/download/download_service.h index 4d754b0..2cc6570 100644 --- a/chrome/browser/download/download_service.h +++ b/chrome/browser/download/download_service.h @@ -29,6 +29,12 @@ class DownloadService : public ProfileKeyedService { // Has a download manager been created? (By calling above function.) bool HasCreatedDownloadManager(); + // Number of downloads associated with this instance of the service. + int DownloadCount() const; + + // Number of downloads associated with all profiles. + static int DownloadCountAllProfiles(); + // Sets the DownloadManagerDelegate associated with this object and // its DownloadManager. Takes ownership of |delegate|, and destroys // the previous delegate. For testing. diff --git a/chrome/browser/download/download_test_observer.cc b/chrome/browser/download/download_test_observer.cc new file mode 100644 index 0000000..1d0c5b5 --- /dev/null +++ b/chrome/browser/download/download_test_observer.cc @@ -0,0 +1,297 @@ +// 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. + +#include <vector> + +#include "base/logging.h" +#include "base/message_loop.h" +#include "base/stl_util.h" +#include "base/task.h" +#include "chrome/browser/download/download_test_observer.h" +#include "chrome/test/base/ui_test_utils.h" +#include "content/browser/browser_thread.h" + +// These functions take scoped_refptr's to DownloadManager because they +// are posted to message queues, and hence may execute arbitrarily after +// their actual posting. Once posted, there is no connection between +// these routines and the DownloadTestObserver class from which they came, +// so the DownloadTestObserver's reference to the DownloadManager cannot +// be counted on to keep the DownloadManager around. + +// Fake user click on "Accept". +void AcceptDangerousDownload(scoped_refptr<DownloadManager> download_manager, + int32 download_id) { + DownloadItem* download = download_manager->GetDownloadItem(download_id); + download->DangerousDownloadValidated(); +} + +// Fake user click on "Deny". +void DenyDangerousDownload(scoped_refptr<DownloadManager> download_manager, + int32 download_id) { + DownloadItem* download = download_manager->GetDownloadItem(download_id); + ASSERT_TRUE(download->IsPartialDownload()); + download->Cancel(true); + download->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); +} + +DownloadTestObserver::DownloadTestObserver( + DownloadManager* download_manager, + size_t wait_count, + 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_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_NE(DownloadItem::REMOVING, download_finished_state) + << "Waiting for REMOVING is not supported. Try COMPLETE."; + } + +DownloadTestObserver::~DownloadTestObserver() { + for (DownloadSet::iterator it = downloads_observed_.begin(); + it != downloads_observed_.end(); ++it) + (*it)->RemoveObserver(this); + + download_manager_->RemoveObserver(this); +} + +void DownloadTestObserver::WaitForFinished() { + if (!IsFinished()) { + waiting_ = true; + ui_test_utils::RunMessageLoop(); + waiting_ = false; + } +} + +bool DownloadTestObserver::IsFinished() const { + if (finished_downloads_.size() - finished_downloads_at_construction_ >= + wait_count_) + return true; + return (finish_on_select_file_ && select_file_dialog_seen_); +} + +void DownloadTestObserver::OnDownloadUpdated(DownloadItem* download) { + // The REMOVING state indicates that the download is being destroyed. + // Stop observing. Do not do anything with it, as it is about to be gone. + if (download->state() == DownloadItem::REMOVING) { + DownloadSet::iterator it = downloads_observed_.find(download); + ASSERT_TRUE(it != downloads_observed_.end()); + downloads_observed_.erase(it); + download->RemoveObserver(this); + return; + } + + // Real UI code gets the user's response after returning from the observer. + if (download->safety_state() == DownloadItem::DANGEROUS && + !ContainsKey(dangerous_downloads_seen_, download->id())) { + dangerous_downloads_seen_.insert(download->id()); + + // Calling DangerousDownloadValidated() at this point will + // cause the download to be completed twice. Do what the real UI + // code does: make the call as a delayed task. + switch (dangerous_download_action_) { + case ON_DANGEROUS_DOWNLOAD_ACCEPT: + // Fake user click on "Accept". Delay the actual click, as the + // real UI would. + BrowserThread::PostTask( + BrowserThread::UI, FROM_HERE, + NewRunnableFunction( + &AcceptDangerousDownload, + download_manager_, + download->id())); + break; + + case ON_DANGEROUS_DOWNLOAD_DENY: + // Fake a user click on "Deny". Delay the actual click, as the + // real UI would. + BrowserThread::PostTask( + BrowserThread::UI, FROM_HERE, + NewRunnableFunction( + &DenyDangerousDownload, + download_manager_, + download->id())); + break; + + case ON_DANGEROUS_DOWNLOAD_FAIL: + ADD_FAILURE() << "Unexpected dangerous download item."; + break; + + default: + NOTREACHED(); + } + } + + if (download->state() == download_finished_state_) { + DownloadInFinalState(download); + } +} + +void DownloadTestObserver::ModelChanged() { + // Regenerate DownloadItem observers. If there are any download items + // in our final state, note them in |finished_downloads_| + // (done by |OnDownloadUpdated()|). + std::vector<DownloadItem*> downloads; + download_manager_->GetAllDownloads(FilePath(), &downloads); + + for (std::vector<DownloadItem*>::iterator it = downloads.begin(); + it != downloads.end(); ++it) { + OnDownloadUpdated(*it); // Safe to call multiple times; checks state. + + DownloadSet::const_iterator finished_it(finished_downloads_.find(*it)); + DownloadSet::iterator observed_it(downloads_observed_.find(*it)); + + // If it isn't finished and we're aren't observing it, start. + if (finished_it == finished_downloads_.end() && + observed_it == downloads_observed_.end()) { + (*it)->AddObserver(this); + downloads_observed_.insert(*it); + continue; + } + + // If it is finished and we are observing it, stop. + if (finished_it != finished_downloads_.end() && + observed_it != downloads_observed_.end()) { + (*it)->RemoveObserver(this); + downloads_observed_.erase(observed_it); + continue; + } + } +} + +void DownloadTestObserver::SelectFileDialogDisplayed(int32 /* id */) { + select_file_dialog_seen_ = true; + SignalIfFinished(); +} + +size_t DownloadTestObserver::NumDangerousDownloadsSeen() const { + return dangerous_downloads_seen_.size(); +} + +void DownloadTestObserver::DownloadInFinalState(DownloadItem* download) { + if (finished_downloads_.find(download) != finished_downloads_.end()) { + // We've already seen terminal state on this download. + return; + } + + // Record the transition. + finished_downloads_.insert(download); + + SignalIfFinished(); +} + +void DownloadTestObserver::SignalIfFinished() { + if (waiting_ && IsFinished()) + MessageLoopForUI::current()->Quit(); +} + +DownloadTestFlushObserver::DownloadTestFlushObserver( + DownloadManager* download_manager) + : download_manager_(download_manager), + waiting_for_zero_inprogress_(true) {} + +void DownloadTestFlushObserver::WaitForFlush() { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + download_manager_->AddObserver(this); + ui_test_utils::RunMessageLoop(); +} + +void DownloadTestFlushObserver::ModelChanged() { + // Model has changed, so there may be more DownloadItems to observe. + CheckDownloadsInProgress(true); +} + +void DownloadTestFlushObserver::OnDownloadUpdated(DownloadItem* download) { + // The REMOVING state indicates that the download is being destroyed. + // Stop observing. Do not do anything with it, as it is about to be gone. + if (download->state() == DownloadItem::REMOVING) { + DownloadSet::iterator it = downloads_observed_.find(download); + ASSERT_TRUE(it != downloads_observed_.end()); + downloads_observed_.erase(it); + download->RemoveObserver(this); + return; + } + + // No change in DownloadItem set on manager. + CheckDownloadsInProgress(false); +} + +DownloadTestFlushObserver::~DownloadTestFlushObserver() { + download_manager_->RemoveObserver(this); + for (DownloadSet::iterator it = downloads_observed_.begin(); + it != downloads_observed_.end(); ++it) { + (*it)->RemoveObserver(this); + } +} + +// 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 DownloadTestFlushObserver::CheckDownloadsInProgress( + bool observe_downloads) { + if (waiting_for_zero_inprogress_) { + int count = 0; + + std::vector<DownloadItem*> downloads; + download_manager_->SearchDownloads(string16(), &downloads); + for (std::vector<DownloadItem*>::iterator it = downloads.begin(); + 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); + downloads_observed_.insert(*it); + } + // 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 (DownloadSet::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, + &DownloadTestFlushObserver::PingFileThread, 2)); + } + } +} + +void DownloadTestFlushObserver::PingFileThread(int cycle) { + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + NewRunnableMethod(this, &DownloadTestFlushObserver::PingIOThread, + cycle)); +} + +void DownloadTestFlushObserver::PingIOThread(int cycle) { + if (--cycle) { + BrowserThread::PostTask( + BrowserThread::UI, FROM_HERE, + NewRunnableMethod(this, &DownloadTestFlushObserver::PingFileThread, + cycle)); + } else { + BrowserThread::PostTask( + BrowserThread::UI, FROM_HERE, new MessageLoop::QuitTask()); + } +} diff --git a/chrome/browser/download/download_test_observer.h b/chrome/browser/download/download_test_observer.h new file mode 100644 index 0000000..01c03c6 --- /dev/null +++ b/chrome/browser/download/download_test_observer.h @@ -0,0 +1,181 @@ +// 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 CHROME_BROWSER_DOWNLOAD_DOWNLOAD_TEST_OBSERVER_H_ +#define CHROME_BROWSER_DOWNLOAD_DOWNLOAD_TEST_OBSERVER_H_ +#pragma once + +#include <set> + +#include "base/basictypes.h" +#include "base/memory/ref_counted.h" +#include "content/browser/download/download_item.h" +#include "content/browser/download/download_manager.h" + +// Construction of this class defines a system state, based on some number +// of downloads being seen in a particular state + other events that +// may occur in the download system. That state will be recorded if it +// 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 DownloadTestObserver : public DownloadManager::Observer, + public DownloadItem::Observer { + public: + // Action an observer should take if a dangerous download is encountered. + enum DangerousDownloadAction { + ON_DANGEROUS_DOWNLOAD_ACCEPT, // Accept the download + ON_DANGEROUS_DOWNLOAD_DENY, // Deny the download + ON_DANGEROUS_DOWNLOAD_FAIL // Fail if a dangerous download is seen + }; + + // 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. + + // TODO(rdsmith): Consider rewriting the interface to take a list of events + // to treat as completion events. + DownloadTestObserver(DownloadManager* download_manager, + size_t wait_count, + DownloadItem::DownloadState download_finished_state, + bool finish_on_select_file, + DangerousDownloadAction dangerous_download_action); + + virtual ~DownloadTestObserver(); + + // State accessors. + bool select_file_dialog_seen() const { return select_file_dialog_seen_; } + + // Wait for whatever state was specified in the constructor. + void WaitForFinished(); + + // Return true if everything's happened that we're configured for. + bool IsFinished() const; + + // DownloadItem::Observer + virtual void OnDownloadUpdated(DownloadItem* download) OVERRIDE; + virtual void OnDownloadOpened(DownloadItem* download) OVERRIDE {} + + // DownloadManager::Observer + virtual void ModelChanged() OVERRIDE; + + virtual void SelectFileDialogDisplayed(int32 id) OVERRIDE; + + size_t NumDangerousDownloadsSeen() const; + + private: + typedef std::set<DownloadItem*> DownloadSet; + + // 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; 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); + + void SignalIfFinished(); + + // The observed 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 + // reaches wait_count_, we're done. + DownloadSet finished_downloads_; + + // The set of DownloadItem's we are currently observing. Generally there + // won't be any overlap with the above; once we see the final state + // on a DownloadItem, we'll stop observing it. + DownloadSet downloads_observed_; + + // The number of downloads to wait on completing. + size_t wait_count_; + + // The number of downloads entered in final state in initial + // 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 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 state on which to consider the DownloadItem finished. + DownloadItem::DownloadState download_finished_state_; + + // True if we should transition the DownloadTestObserver to finished if + // the select file dialog comes up. + bool finish_on_select_file_; + + // True if we've seen the select file dialog. + bool select_file_dialog_seen_; + + // Action to take if a dangerous download is encountered. + DangerousDownloadAction dangerous_download_action_; + + // Holds the download ids which were dangerous. + std::set<int32> dangerous_downloads_seen_; + + DISALLOW_COPY_AND_ASSIGN(DownloadTestObserver); +}; + +// The WaitForFlush() method on this class 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 DownloadTestFlushObserver + : public DownloadManager::Observer, + public DownloadItem::Observer, + public base::RefCountedThreadSafe<DownloadTestFlushObserver> { + public: + explicit DownloadTestFlushObserver(DownloadManager* download_manager); + + void WaitForFlush(); + + // DownloadsManager observer methods. + virtual void ModelChanged() OVERRIDE; + + // DownloadItem observer methods. + virtual void OnDownloadUpdated(DownloadItem* download) OVERRIDE; + virtual void OnDownloadOpened(DownloadItem* download) OVERRIDE {} + + protected: + friend class base::RefCountedThreadSafe<DownloadTestFlushObserver>; + + virtual ~DownloadTestFlushObserver(); + + private: + typedef std::set<DownloadItem*> DownloadSet; + + // 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); + + void PingFileThread(int cycle); + + void PingIOThread(int cycle); + + DownloadManager* download_manager_; + DownloadSet downloads_observed_; + bool waiting_for_zero_inprogress_; + + DISALLOW_COPY_AND_ASSIGN(DownloadTestFlushObserver); +}; + +#endif // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_TEST_OBSERVER_H_ diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc index 1b62ba3..2a9fbe7 100644 --- a/chrome/browser/ui/browser.cc +++ b/chrome/browser/ui/browser.cc @@ -19,8 +19,8 @@ #include "base/metrics/histogram.h" #include "base/path_service.h" #include "base/string_number_conversions.h" -#include "base/stringprintf.h" #include "base/string_util.h" +#include "base/stringprintf.h" #include "base/threading/thread.h" #include "base/threading/thread_restrictions.h" #include "base/time.h" @@ -75,6 +75,7 @@ #include "chrome/browser/printing/print_preview_tab_controller.h" #include "chrome/browser/printing/print_view_manager.h" #include "chrome/browser/profiles/profile.h" +#include "chrome/browser/profiles/profile_manager.h" #include "chrome/browser/sessions/restore_tab_helper.h" #include "chrome/browser/sessions/session_service.h" #include "chrome/browser/sessions/session_service_factory.h" @@ -1059,6 +1060,64 @@ void Browser::InProgressDownloadResponse(bool cancel_downloads) { ShowDownloadsTab(); } +Browser::DownloadClosePreventionType Browser::OkToCloseWithInProgressDownloads( + int* num_downloads_blocking) const { + DCHECK(num_downloads_blocking); + *num_downloads_blocking = 0; + + if (is_attempting_to_close_browser_) + return DOWNLOAD_CLOSE_OK; + + // If we're not running a full browser process with a profile manager + // (testing), it's ok to close the browser. + if (!g_browser_process->profile_manager()) + return DOWNLOAD_CLOSE_OK; + + int total_download_count = DownloadService::DownloadCountAllProfiles(); + if (total_download_count == 0) + return DOWNLOAD_CLOSE_OK; // No downloads; can definitely close. + + // Figure out how many windows are open total, and associated with this + // profile, that are relevant for the ok-to-close decision. + int profile_window_count = 0; + int total_window_count = 0; + for (BrowserList::const_iterator iter = BrowserList::begin(); + iter != BrowserList::end(); ++iter) { + // Don't count this browser window or any other in the process of closing. + Browser* const browser = *iter; + // Check is_attempting_to_close_browser_ as window closing may be + // delayed, and windows that are in the process of closing don't + // count against our totals. + if (browser == this || browser->is_attempting_to_close_browser_) + continue; + + if ((*iter)->profile() == profile()) + profile_window_count++; + total_window_count++; + } + + // If there aren't any other windows, we're at browser shutdown, + // which would cancel all current downloads. + if (total_window_count == 0) { + *num_downloads_blocking = total_download_count; + return DOWNLOAD_CLOSE_BROWSER_SHUTDOWN; + } + + // If there aren't any other windows on our profile, and we're an incognito + // profile, and there are downloads associated with that profile, + // those downloads would be cancelled by our window (-> profile) close. + DownloadService* download_service = + DownloadServiceFactory::GetForProfile(profile()); + if (profile_window_count == 0 && download_service->DownloadCount() > 0 && + profile()->IsOffTheRecord()) { + *num_downloads_blocking = download_service->DownloadCount(); + return DOWNLOAD_CLOSE_LAST_WINDOW_IN_INCOGNITO_PROFILE; + } + + // Those are the only conditions under which we will block shutdown. + return DOWNLOAD_CLOSE_OK; +} + //////////////////////////////////////////////////////////////////////////////// // Browser, TabStripModel pass-thrus: @@ -5075,100 +5134,19 @@ void Browser::ClearUnloadState(TabContents* tab, bool process_now) { /////////////////////////////////////////////////////////////////////////////// // Browser, In-progress download termination handling (private): -void Browser::CheckDownloadsInProgress(bool* normal_downloads_are_present, - bool* incognito_downloads_are_present) { - *normal_downloads_are_present = false; - *incognito_downloads_are_present = false; - - // If there are no download in-progress, our job is done. - DownloadManager* download_manager = NULL; - DownloadService* download_service = - DownloadServiceFactory::GetForProfile(profile()); - // But first we need to check for the existence of the download manager, as - // GetDownloadManager() will unnecessarily try to create one if it does not - // exist. - if (download_service->HasCreatedDownloadManager()) - download_manager = download_service->GetDownloadManager(); - if (profile()->IsOffTheRecord()) { - // Browser is incognito and so download_manager if present is for incognito - // downloads. - *incognito_downloads_are_present = - (download_manager && download_manager->in_progress_count() != 0); - // Check original profile. - DownloadService* download_service = DownloadServiceFactory::GetForProfile( - profile()->GetOriginalProfile()); - if (download_service->HasCreatedDownloadManager()) - download_manager = download_service->GetDownloadManager(); - } - - *normal_downloads_are_present = - (download_manager && download_manager->in_progress_count() != 0); -} - bool Browser::CanCloseWithInProgressDownloads() { - if (cancel_download_confirmation_state_ != NOT_PROMPTED) { - if (cancel_download_confirmation_state_ == WAITING_FOR_RESPONSE) { - // We need to hear from the user before we can close. - return false; - } - // RESPONSE_RECEIVED case, the user decided to go along with the closing. - return true; - } - // Indicated that normal (non-incognito) downloads are pending. - bool normal_downloads_are_present = false; - bool incognito_downloads_are_present = false; - CheckDownloadsInProgress(&normal_downloads_are_present, - &incognito_downloads_are_present); - if (!normal_downloads_are_present && !incognito_downloads_are_present) - return true; - - if (is_attempting_to_close_browser_) - return true; - - if ((!normal_downloads_are_present && !profile()->IsOffTheRecord()) || - (!incognito_downloads_are_present && profile()->IsOffTheRecord())) - return true; - - // Let's figure out if we are the last window for our profile. - // Note that we cannot just use BrowserList::GetBrowserCount as browser - // windows closing is delayed and the returned count might include windows - // that are being closed. - // The browser allowed to be closed only if: - // 1. It is a regular browser and there are no regular downloads present or - // this is not the last regular browser window. - // 2. It is an incognito browser and there are no incognito downloads present - // or this is not the last incognito browser window. - int count = 0; - for (BrowserList::const_iterator iter = BrowserList::begin(); - iter != BrowserList::end(); ++iter) { - // Don't count this browser window or any other in the process of closing. - // Only consider tabbed browser windows, not popups. - Browser* const browser = *iter; - if (browser == this - || browser->is_attempting_to_close_browser_ - || !browser->is_type_tabbed()) - continue; - - // Verify that this is not the last non-incognito or incognito browser, - // depending on the pending downloads. - if (normal_downloads_are_present && !profile()->IsOffTheRecord() && - browser->profile()->IsOffTheRecord()) - continue; - if (incognito_downloads_are_present && profile()->IsOffTheRecord() && - !browser->profile()->IsOffTheRecord()) - continue; - - // We test the original profile, because an incognito browser window keeps - // the original profile alive (and its DownloadManager). - // We also need to test explicitly the profile directly so that 2 incognito - // profiles count as a match. - if ((*iter)->profile() == profile() || - (*iter)->profile()->GetOriginalProfile() == profile()) - count++; - } - if (count > 0) + // If we've prompted, we need to hear from the user before we + // can close. + if (cancel_download_confirmation_state_ != NOT_PROMPTED) + return cancel_download_confirmation_state_ != WAITING_FOR_RESPONSE; + + int num_downloads_blocking; + if (DOWNLOAD_CLOSE_OK == + OkToCloseWithInProgressDownloads(&num_downloads_blocking)) return true; + // Closing this window will kill some downloads; prompt to make sure + // that's ok. cancel_download_confirmation_state_ = WAITING_FOR_RESPONSE; window_->ConfirmBrowserCloseWithPendingDownloads(); diff --git a/chrome/browser/ui/browser.h b/chrome/browser/ui/browser.h index 595fe67..90e33f2 100644 --- a/chrome/browser/ui/browser.h +++ b/chrome/browser/ui/browser.h @@ -110,6 +110,21 @@ class Browser : public TabHandlerDelegate, FEATURE_DOWNLOADSHELF = 128 }; + // The context for a download blocked notification from + // OkToCloseWithInProgressDownloads. + enum DownloadClosePreventionType { + // Browser close is not blocked by download state. + DOWNLOAD_CLOSE_OK, + + // The browser is shutting down and there are active downloads + // that would be cancelled. + DOWNLOAD_CLOSE_BROWSER_SHUTDOWN, + + // There are active downloads associated with this incognito profile + // that would be canceled. + DOWNLOAD_CLOSE_LAST_WINDOW_IN_INCOGNITO_PROFILE, + }; + struct CreateParams { CreateParams(Type type, Profile* profile); @@ -342,16 +357,20 @@ class Browser : public TabHandlerDelegate, // In-progress download termination handling ///////////////////////////////// - // Are normal and/or incognito downloads in progress? - void CheckDownloadsInProgress(bool* normal_downloads, - bool* incognito_downloads); - // Called when the user has decided whether to proceed or not with the browser // closure. |cancel_downloads| is true if the downloads should be canceled // and the browser closed, false if the browser should stay open and the // downloads running. void InProgressDownloadResponse(bool cancel_downloads); + // Indicates whether or not this browser window can be closed, or + // would be blocked by in-progress downloads. + // If executing downloads would be cancelled by this window close, + // then |*num_downloads_blocking| is updated with how many downloads + // would be canceled if the close continued. + DownloadClosePreventionType OkToCloseWithInProgressDownloads( + int* num_downloads_blocking) const; + // TabStripModel pass-thrus ///////////////////////////////////////////////// TabStripModel* tabstrip_model() const { diff --git a/chrome/browser/ui/browser_close_browsertest.cc b/chrome/browser/ui/browser_close_browsertest.cc new file mode 100644 index 0000000..be7386c --- /dev/null +++ b/chrome/browser/ui/browser_close_browsertest.cc @@ -0,0 +1,551 @@ +// 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. + +#include "base/command_line.h" +#include "base/logging.h" +#include "base/path_service.h" +#include "base/stringprintf.h" +#include "chrome/browser/browser_process.h" +#include "chrome/browser/download/download_service.h" +#include "chrome/browser/download/download_service_factory.h" +#include "chrome/browser/download/download_test_observer.h" +#include "chrome/browser/net/url_request_mock_util.h" +#include "chrome/browser/prefs/pref_service.h" +#include "chrome/browser/profiles/profile.h" +#include "chrome/browser/profiles/profile_manager.h" +#include "chrome/browser/ui/browser.h" +#include "chrome/browser/ui/browser_window.h" +#include "chrome/browser/ui/webui/active_downloads_ui.h" +#include "chrome/common/chrome_paths.h" +#include "chrome/common/pref_names.h" +#include "chrome/common/url_constants.h" +#include "chrome/test/base/in_process_browser_test.h" +#include "chrome/test/base/ui_test_utils.h" +#include "content/browser/download/download_item.h" +#include "content/browser/net/url_request_slow_download_job.h" +#include "content/browser/tab_contents/tab_contents.h" +#include "content/public/common/page_transition_types.h" + +class BrowserCloseTest : public InProcessBrowserTest { + public: + // Structure defining test cases for DownloadsCloseCheck. + struct DownloadsCloseCheckCase { + std::string DebugString() const; + + // Input + struct { + struct { + int windows; + int downloads; + } regular; + struct { + int windows; + int downloads; + } incognito; + } profile_a; + + struct { + struct { + int windows; + int downloads; + } regular; + struct { + int windows; + int downloads; + } incognito; + } profile_b; + + // We always probe a window in profile A. + enum { REGULAR = 0, INCOGNITO = 1 } window_to_probe; + + // Output + Browser::DownloadClosePreventionType type; + + // Unchecked if type == DOWNLOAD_CLOSE_OK. + int num_blocking; + }; + + protected: + virtual void SetUpOnMainThread() OVERRIDE { + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + base::Bind(&chrome_browser_net::SetUrlRequestMocksEnabled, true)); + } + + // Create a second profile to work within multi-profile. + Profile* CreateSecondProfile() { + FilePath user_data_dir; + PathService::Get(chrome::DIR_USER_DATA, &user_data_dir); + + if (!second_profile_data_dir_.CreateUniqueTempDirUnderPath(user_data_dir)) + return NULL; + + Profile* second_profile = + g_browser_process->profile_manager()->GetProfile( + second_profile_data_dir_.path()); + if (!second_profile) + return NULL; + + bool result = second_profile_downloads_dir_.CreateUniqueTempDir(); + if (!result) + return NULL; + second_profile->GetPrefs()->SetFilePath( + prefs::kDownloadDefaultDirectory, + second_profile_downloads_dir_.path()); + + return second_profile; + } + + // Create |num_downloads| number of downloads that are stalled + // (will quickly get to a place where the server won't + // provide any more data) so that we can test closing the + // browser with active downloads. + void CreateStalledDownloads(Browser* browser, int num_downloads) { + GURL url(URLRequestSlowDownloadJob::kKnownSizeUrl); + + if (num_downloads == 0) + return; + + // Setup an observer waiting for the given number of downloads + // to get to IN_PROGRESS. + DownloadManager* download_manager = + browser->profile()->GetDownloadManager(); + scoped_ptr<DownloadTestObserver> observer( + new DownloadTestObserver( + download_manager, num_downloads, + DownloadItem::IN_PROGRESS, + true, // Bail on select file. + DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL)); + + // Set of that number of downloads. + while (num_downloads--) + ui_test_utils::NavigateToURLWithDisposition( + browser, url, NEW_BACKGROUND_TAB, + ui_test_utils::BROWSER_TEST_NONE); + + // Wait for them. + observer->WaitForFinished(); + } + + // All all downloads created in CreateStalledDownloads() to + // complete, and block in this routine until they do complete. + void CompleteAllDownloads(Browser* browser) { + GURL finish_url(URLRequestSlowDownloadJob::kFinishDownloadUrl); + ui_test_utils::NavigateToURL(browser, finish_url); + + // Go through and, for every single profile, wait until there are + // no active downloads on that download manager. + std::vector<Profile*> profiles( + g_browser_process->profile_manager()->GetLoadedProfiles()); + for (std::vector<Profile*>::const_iterator pit = profiles.begin(); + pit != profiles.end(); ++pit) { + DownloadService* download_service = + DownloadServiceFactory::GetForProfile(*pit); + if (download_service->HasCreatedDownloadManager()) { + DownloadManager *mgr = download_service->GetDownloadManager(); + scoped_refptr<DownloadTestFlushObserver> observer( + new DownloadTestFlushObserver(mgr)); + observer->WaitForFlush(); + } + if ((*pit)->HasOffTheRecordProfile()) { + DownloadService* incognito_download_service = + DownloadServiceFactory::GetForProfile( + (*pit)->GetOffTheRecordProfile()); + if (incognito_download_service->HasCreatedDownloadManager()) { + DownloadManager *mgr = + incognito_download_service->GetDownloadManager(); + scoped_refptr<DownloadTestFlushObserver> observer( + new DownloadTestFlushObserver(mgr)); + observer->WaitForFlush(); + } + } + } + } + + // Create a Browser (with associated window) on the specified profile. + Browser* CreateBrowserOnProfile(Profile* profile) { + Browser* new_browser = Browser::Create(profile); + new_browser->AddSelectedTabWithURL(GURL(chrome::kAboutBlankURL), + content::PAGE_TRANSITION_START_PAGE); + ui_test_utils::WaitForLoadStop( + new_browser->GetSelectedTabContents()); + new_browser->window()->Show(); + return new_browser; + } + + // Adjust the number of browsers and associated windows up or down + // to |num_windows|. This routine assumes that there is only a single + // browser associated with the profile on entry. |*base_browser| contains + // this browser, and the profile is derived from that browser. On output, + // if |*base_browser| was destroyed (because |num_windows == 0|), NULL + // is assigned to that memory location. + bool AdjustBrowsersOnProfile(Browser** base_browser, int num_windows) { + int num_downloads_blocking; + if (num_windows == 0) { + if (Browser::DOWNLOAD_CLOSE_OK != + (*base_browser)->OkToCloseWithInProgressDownloads( + &num_downloads_blocking)) + return false; + (*base_browser)->window()->Close(); + *base_browser = 0; + return true; + } + + // num_windows > 0 + Profile* profile((*base_browser)->profile()); + for (int w = 1; w < num_windows; ++w) { + CreateBrowserOnProfile(profile); + } + return true; + } + + int TotalUnclosedBrowsers() { + int count = 0; + for (BrowserList::const_iterator iter = BrowserList::begin(); + iter != BrowserList::end(); ++iter) + if (!(*iter)->IsAttemptingToCloseBrowser()) { + count++; + } + return count; + } + + // Note that this is invalid to call if TotalUnclosedBrowsers() == 0. + Browser* FirstUnclosedBrowser() { + for (BrowserList::const_iterator iter = BrowserList::begin(); + iter != BrowserList::end(); ++iter) + if (!(*iter)->IsAttemptingToCloseBrowser()) + return (*iter); + return NULL; + } + + bool SetupForDownloadCloseCheck() { + first_profile_ = browser()->profile(); + + bool result = first_profile_downloads_dir_.CreateUniqueTempDir(); + EXPECT_TRUE(result); + if (!result) return false; + first_profile_->GetPrefs()->SetFilePath( + prefs::kDownloadDefaultDirectory, + first_profile_downloads_dir_.path()); + + second_profile_ = CreateSecondProfile(); + EXPECT_TRUE(second_profile_); + if (!second_profile_) return false; + + return true; + } + + // Test a specific DownloadsCloseCheckCase. Returns false if + // an assertion has failed and the test should be aborted. + bool ExecuteDownloadCloseCheckCase(size_t i) { + const DownloadsCloseCheckCase& check_case(download_close_check_cases[i]); + + // Test invariant: so that we don't actually try and close the browser, + // we always enter the function with a single browser window open on the + // main profile. That means we need to exit the function the same way. + // So we setup everything except for the |first_profile_| regular, and then + // flip the bit on the main window. + // Note that this means that browser() is unreliable in the context + // of this function or its callers; we'll be killing that main window + // and recreating it fairly frequently. + int unclosed_browsers = TotalUnclosedBrowsers(); + EXPECT_EQ(1, unclosed_browsers); + if (1 != unclosed_browsers) + return false; + + Browser* entry_browser = FirstUnclosedBrowser(); + EXPECT_EQ(first_profile_, entry_browser->profile()) + << "Case" << i + << ": " << check_case.DebugString(); + if (first_profile_ != entry_browser->profile()) + return false; + int total_download_count = DownloadService::DownloadCountAllProfiles(); + EXPECT_EQ(0, total_download_count) + << "Case " << i + << ": " << check_case.DebugString(); + if (0 != total_download_count) + return false; + + Profile* first_profile_incognito = first_profile_->GetOffTheRecordProfile(); + Profile* second_profile_incognito = + second_profile_->GetOffTheRecordProfile(); + + // For simplicty of coding, we create a window on each profile so that + // we can easily create downloads, then we destroy or create windows + // as necessary. + Browser* browser_a_regular(CreateBrowserOnProfile(first_profile_)); + Browser* browser_a_incognito( + CreateBrowserOnProfile(first_profile_incognito)); + Browser* browser_b_regular(CreateBrowserOnProfile(second_profile_)); + Browser* browser_b_incognito( + CreateBrowserOnProfile(second_profile_incognito)); + + // Kill our entry browser. + entry_browser->window()->Close(); + entry_browser = NULL; + + // Create all downloads needed. + CreateStalledDownloads( + browser_a_regular, check_case.profile_a.regular.downloads); + CreateStalledDownloads( + browser_a_incognito, check_case.profile_a.incognito.downloads); + CreateStalledDownloads( + browser_b_regular, check_case.profile_b.regular.downloads); + CreateStalledDownloads( + browser_b_incognito, check_case.profile_b.incognito.downloads); + + // Adjust the windows + Browser** browsers[] = { + &browser_a_regular, &browser_a_incognito, + &browser_b_regular, &browser_b_incognito + }; + int window_counts[] = { + check_case.profile_a.regular.windows, + check_case.profile_a.incognito.windows, + check_case.profile_b.regular.windows, + check_case.profile_b.incognito.windows, + }; + for (size_t j = 0; j < arraysize(browsers); ++j) { + bool result = AdjustBrowsersOnProfile(browsers[j], window_counts[j]); + EXPECT_TRUE(result); + if (!result) + return false; + } + ui_test_utils::RunAllPendingInMessageLoop(); + +#if defined(OS_CHROMEOS) + // Get rid of downloads panel on ChromeOS + Browser* panel = NULL; +#if defined(TOUCH_UI) + ActiveDownloadsUI::GetPopup(&panel); +#else + panel = ActiveDownloadsUI::GetPopup(); +#endif + if (panel) + panel->CloseWindow(); + ui_test_utils::RunAllPendingInMessageLoop(); +#endif + + // All that work, for this one little test. + EXPECT_TRUE((check_case.window_to_probe == + DownloadsCloseCheckCase::REGULAR) || + (check_case.window_to_probe == + DownloadsCloseCheckCase::INCOGNITO)); + if (!((check_case.window_to_probe == + DownloadsCloseCheckCase::REGULAR) || + (check_case.window_to_probe == + DownloadsCloseCheckCase::INCOGNITO))) + return false; + + int num_downloads_blocking; + Browser* browser_to_probe = + (check_case.window_to_probe == DownloadsCloseCheckCase::REGULAR ? + browser_a_regular : + browser_a_incognito); + Browser::DownloadClosePreventionType type = + browser_to_probe->OkToCloseWithInProgressDownloads( + &num_downloads_blocking); + EXPECT_EQ(check_case.type, type) << "Case " << i + << ": " << check_case.DebugString(); + if (type != Browser::DOWNLOAD_CLOSE_OK) + EXPECT_EQ(check_case.num_blocking, num_downloads_blocking) + << "Case " << i + << ": " << check_case.DebugString(); + + // Release all the downloads. + CompleteAllDownloads(browser_to_probe); + + // Create a new main window and kill everything else. + entry_browser = CreateBrowserOnProfile(first_profile_); + for (BrowserList::const_iterator bit = BrowserList::begin(); + bit != BrowserList::end(); ++bit) { + if ((*bit) != entry_browser) { + EXPECT_TRUE((*bit)->window()); + if (!(*bit)->window()) + return false; + (*bit)->window()->Close(); + } + } + ui_test_utils::RunAllPendingInMessageLoop(); + + return true; + } + + static const DownloadsCloseCheckCase download_close_check_cases[]; + + // DownloadCloseCheck variables. + Profile* first_profile_; + Profile* second_profile_; + + ScopedTempDir first_profile_downloads_dir_; + ScopedTempDir second_profile_data_dir_; + ScopedTempDir second_profile_downloads_dir_; +}; + +const BrowserCloseTest::DownloadsCloseCheckCase +BrowserCloseTest::download_close_check_cases[] = { + // Top level nesting is {profile_a, profile_b} + // Second level nesting is {regular, incognito + // Third level (inner) nesting is {windows, downloads} + + // Last window (incognito) triggers browser close warning. + {{{0, 0}, {1, 1}}, {{0, 0}, {0, 0}}, + BrowserCloseTest::DownloadsCloseCheckCase::INCOGNITO, + Browser::DOWNLOAD_CLOSE_BROWSER_SHUTDOWN, 1}, + + // Last incognito window triggers incognito close warning. + {{{1, 0}, {1, 1}}, {{0, 0}, {0, 0}}, + BrowserCloseTest::DownloadsCloseCheckCase::INCOGNITO, + Browser::DOWNLOAD_CLOSE_LAST_WINDOW_IN_INCOGNITO_PROFILE, 1}, + + // Last incognito window with no downloads triggers no warning. + {{{0, 0}, {1, 0}}, {{0, 0}, {0, 0}}, + BrowserCloseTest::DownloadsCloseCheckCase::INCOGNITO, + Browser::DOWNLOAD_CLOSE_OK}, + + // Last incognito window with window+download on another incognito profile + // triggers no warning. + {{{0, 0}, {1, 0}}, {{0, 0}, {1, 1}}, + BrowserCloseTest::DownloadsCloseCheckCase::INCOGNITO, + Browser::DOWNLOAD_CLOSE_OK}, + + // Non-last incognito window triggers no warning. + {{{0, 0}, {2, 1}}, {{0, 0}, {0, 0}}, + BrowserCloseTest::DownloadsCloseCheckCase::INCOGNITO, + Browser::DOWNLOAD_CLOSE_OK}, + + // Non-last regular window triggers no warning. + {{{2, 1}, {0, 0}}, {{0, 0}, {0, 0}}, + BrowserCloseTest::DownloadsCloseCheckCase::REGULAR, + Browser::DOWNLOAD_CLOSE_OK}, + + // Last regular window triggers browser close. + {{{1, 1}, {0, 0}}, {{0, 0}, {0, 0}}, + BrowserCloseTest::DownloadsCloseCheckCase::REGULAR, + Browser::DOWNLOAD_CLOSE_BROWSER_SHUTDOWN, 1}, + + // Last regular window triggers browser close for download on different + // profile. + {{{1, 0}, {0, 0}}, {{0, 1}, {0, 0}}, + BrowserCloseTest::DownloadsCloseCheckCase::REGULAR, + Browser::DOWNLOAD_CLOSE_BROWSER_SHUTDOWN, 1}, + + // Last regular window triggers no warning if incognito + // active (http://crbug.com/61257). + {{{1, 0}, {1, 1}}, {{0, 0}, {0, 0}}, + BrowserCloseTest::DownloadsCloseCheckCase::REGULAR, + Browser::DOWNLOAD_CLOSE_OK}, + + // Last regular window triggers no warning if other profile window active. + {{{1, 1}, {0, 0}}, {{1, 0}, {0, 0}}, + BrowserCloseTest::DownloadsCloseCheckCase::REGULAR, + Browser::DOWNLOAD_CLOSE_OK}, + + // Last regular window triggers no warning if other incognito window + // active. + {{{1, 0}, {0, 0}}, {{0, 0}, {1, 1}}, + BrowserCloseTest::DownloadsCloseCheckCase::REGULAR, + Browser::DOWNLOAD_CLOSE_OK}, + + // Last regular window triggers no warning if incognito active. + {{{1, 1}, {1, 0}}, {{0, 0}, {0, 0}}, + BrowserCloseTest::DownloadsCloseCheckCase::REGULAR, + Browser::DOWNLOAD_CLOSE_OK}, + + // Test plural for regular. + {{{1, 2}, {0, 0}}, {{0, 0}, {0, 0}}, + BrowserCloseTest::DownloadsCloseCheckCase::REGULAR, + Browser::DOWNLOAD_CLOSE_BROWSER_SHUTDOWN, 2}, + + // Test plural for incognito. + {{{1, 0}, {1, 2}}, {{0, 0}, {0, 0}}, + BrowserCloseTest::DownloadsCloseCheckCase::INCOGNITO, + Browser::DOWNLOAD_CLOSE_LAST_WINDOW_IN_INCOGNITO_PROFILE, 2}, +}; + +std::string BrowserCloseTest::DownloadsCloseCheckCase::DebugString() const { + std::string result; + result += "{"; + if (profile_a.regular.windows || profile_a.regular.downloads) + result += base::StringPrintf("Regular profile A: (%d w, %d d), ", + profile_a.regular.windows, + profile_a.regular.downloads); + if (profile_a.incognito.windows || profile_a.incognito.downloads) + result += base::StringPrintf("Incognito profile A: (%d w, %d d), ", + profile_a.incognito.windows, + profile_a.incognito.downloads); + if (profile_b.regular.windows || profile_b.regular.downloads) + result += base::StringPrintf("Regular profile B: (%d w, %d d), ", + profile_b.regular.windows, + profile_b.regular.downloads); + if (profile_b.incognito.windows || profile_b.incognito.downloads) + result += base::StringPrintf("Incognito profile B: (%d w, %d d), ", + profile_b.incognito.windows, + profile_b.incognito.downloads); + result += (window_to_probe == REGULAR ? "Probe regular" : + window_to_probe == INCOGNITO ? "Probe incognito" : + "Probe unknown"); + result += "} -> "; + if (type == Browser::DOWNLOAD_CLOSE_OK) { + result += "No warning"; + } else { + result += base::StringPrintf( + "%s (%d downloads) warning", + (type == Browser::DOWNLOAD_CLOSE_BROWSER_SHUTDOWN ? "Browser shutdown" : + type == Browser::DOWNLOAD_CLOSE_LAST_WINDOW_IN_INCOGNITO_PROFILE ? + "Incognito close" : "Unknown"), + num_blocking); + } + return result; +} + +// The following test is split into three chunks to reduce the chance +// of hitting the 25s timeout. + +IN_PROC_BROWSER_TEST_F(BrowserCloseTest, DownloadsCloseCheck_0) { + ASSERT_TRUE(SetupForDownloadCloseCheck()); + for (size_t i = 0; i < arraysize(download_close_check_cases) / 6; ++i) { + ExecuteDownloadCloseCheckCase(i); + } +} + +IN_PROC_BROWSER_TEST_F(BrowserCloseTest, DownloadsCloseCheck_1) { + ASSERT_TRUE(SetupForDownloadCloseCheck()); + for (size_t i = arraysize(download_close_check_cases) / 6; + i < 2 * arraysize(download_close_check_cases) / 6; ++i) { + ExecuteDownloadCloseCheckCase(i); + } +} + +IN_PROC_BROWSER_TEST_F(BrowserCloseTest, DownloadsCloseCheck_2) { + ASSERT_TRUE(SetupForDownloadCloseCheck()); + for (size_t i = 2 * arraysize(download_close_check_cases) / 6; + i < 3 * arraysize(download_close_check_cases) / 6; ++i) { + ExecuteDownloadCloseCheckCase(i); + } +} + +IN_PROC_BROWSER_TEST_F(BrowserCloseTest, DownloadsCloseCheck_3) { + ASSERT_TRUE(SetupForDownloadCloseCheck()); + for (size_t i = 3 * arraysize(download_close_check_cases) / 6; + i < 4 * arraysize(download_close_check_cases) / 6; ++i) { + ExecuteDownloadCloseCheckCase(i); + } +} + +IN_PROC_BROWSER_TEST_F(BrowserCloseTest, DownloadsCloseCheck_4) { + ASSERT_TRUE(SetupForDownloadCloseCheck()); + for (size_t i = 4 * arraysize(download_close_check_cases) / 6; + i < 5 * arraysize(download_close_check_cases) / 6; ++i) { + ExecuteDownloadCloseCheckCase(i); + } +} + +IN_PROC_BROWSER_TEST_F(BrowserCloseTest, DownloadsCloseCheck_5) { + ASSERT_TRUE(SetupForDownloadCloseCheck()); + for (size_t i = 5 * arraysize(download_close_check_cases) / 6; + i < 6 * arraysize(download_close_check_cases) / 6; ++i) { + ExecuteDownloadCloseCheckCase(i); + } +} diff --git a/chrome/browser/ui/browser_list.cc b/chrome/browser/ui/browser_list.cc index 416784d..f5e7f46 100644 --- a/chrome/browser/ui/browser_list.cc +++ b/chrome/browser/ui/browser_list.cc @@ -10,6 +10,7 @@ #include "build/build_config.h" #include "chrome/browser/browser_process.h" #include "chrome/browser/browser_shutdown.h" +#include "chrome/browser/download/download_service.h" #include "chrome/browser/metrics/thread_watcher.h" #include "chrome/browser/prefs/pref_service.h" #include "chrome/browser/printing/background_printing_manager.h" @@ -188,15 +189,17 @@ printing::BackgroundPrintingManager* GetBackgroundPrintingManager() { // This currently checks if there is pending download, or if it needs to // handle unload handler. bool AreAllBrowsersCloseable() { - for (BrowserList::const_iterator i = BrowserList::begin(); - i != BrowserList::end(); ++i) { - bool normal_downloads_are_present = false; - bool incognito_downloads_are_present = false; - (*i)->CheckDownloadsInProgress(&normal_downloads_are_present, - &incognito_downloads_are_present); - if (normal_downloads_are_present || - incognito_downloads_are_present || - (*i)->TabsNeedBeforeUnloadFired()) + BrowserList::const_iterator browser_it = BrowserList::begin(); + if (browser_it == BrowserList::end()) + return true; + + // If there are any downloads active, all browsers are not closeable. + if (DownloadService::DownloadCountAllProfiles() > 0) + return false; + + // Check TabsNeedBeforeUnloadFired(). + for (; browser_it != BrowserList::end(); ++browser_it) { + if ((*browser_it)->TabsNeedBeforeUnloadFired()) return false; } return true; diff --git a/chrome/browser/ui/gtk/download/download_in_progress_dialog_gtk.cc b/chrome/browser/ui/gtk/download/download_in_progress_dialog_gtk.cc index 289b585..43d4600 100644 --- a/chrome/browser/ui/gtk/download/download_in_progress_dialog_gtk.cc +++ b/chrome/browser/ui/gtk/download/download_in_progress_dialog_gtk.cc @@ -21,9 +21,18 @@ DownloadInProgressDialogGtk::DownloadInProgressDialogGtk(Browser* browser) : browser_(browser) { - int download_count = - DownloadServiceFactory::GetForProfile( - browser->profile())->GetDownloadManager()->in_progress_count(); + int download_count; + Browser::DownloadClosePreventionType type = + browser_->OkToCloseWithInProgressDownloads(&download_count); + + // This dialog should have been created within the same thread invocation + // as the original test that lead to us, so it should always not be ok + // to close. + DCHECK_NE(Browser::DOWNLOAD_CLOSE_OK, type); + + // TODO(rdsmith): This dialog should be different depending on whether we're + // closing the last incognito window of a profile or doing browser shutdown. + // See http://crbug.com/88421. std::string warning_text; std::string explanation_text; diff --git a/chrome/browser/ui/views/download/download_in_progress_dialog_view.cc b/chrome/browser/ui/views/download/download_in_progress_dialog_view.cc index 5fe1b6c..933460a 100644 --- a/chrome/browser/ui/views/download/download_in_progress_dialog_view.cc +++ b/chrome/browser/ui/views/download/download_in_progress_dialog_view.cc @@ -27,9 +27,18 @@ DownloadInProgressDialogView::DownloadInProgressDialogView(Browser* browser) : browser_(browser), product_name_(l10n_util::GetStringUTF16(IDS_PRODUCT_NAME)) { - int download_count = - DownloadServiceFactory::GetForProfile( - browser->profile())->GetDownloadManager()->in_progress_count(); + int download_count; + Browser::DownloadClosePreventionType type = + browser_->OkToCloseWithInProgressDownloads(&download_count); + + // This dialog should have been created within the same thread invocation + // as the original test that lead to us, so it should always not be ok + // to close. + DCHECK_NE(Browser::DOWNLOAD_CLOSE_OK, type); + + // TODO(rdsmith): This dialog should be different depending on whether we're + // closing the last incognito window of a profile or doing browser shutdown. + // See http://crbug.com/88421. string16 warning_text; string16 explanation_text; diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index 21dfbbd..15479b5 100644 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -2338,6 +2338,7 @@ 'browser/crash_recovery_browsertest.cc', 'browser/custom_handlers/protocol_handler_registry_browsertest.cc', 'browser/download/download_browsertest.cc', + 'browser/download/download_test_observer.cc', 'browser/download/save_page_browsertest.cc', 'browser/errorpage_browsertest.cc', 'browser/extensions/alert_apitest.cc', @@ -2484,6 +2485,7 @@ 'browser/task_manager/task_manager_notification_browsertest.cc', 'browser/translate/translate_manager_browsertest.cc', 'browser/ui/browser_browsertest.cc', + 'browser/ui/browser_close_browsertest.cc', 'browser/ui/browser_init_browsertest.cc', 'browser/ui/browser_navigator_browsertest.cc', 'browser/ui/browser_navigator_browsertest.h', |