diff options
Diffstat (limited to 'chrome/browser/download')
8 files changed, 63 insertions, 50 deletions
diff --git a/chrome/browser/download/chrome_download_manager_delegate.cc b/chrome/browser/download/chrome_download_manager_delegate.cc index 0d1c0c5..efcc701 100644 --- a/chrome/browser/download/chrome_download_manager_delegate.cc +++ b/chrome/browser/download/chrome_download_manager_delegate.cc @@ -216,6 +216,8 @@ ChromeDownloadManagerDelegate::ChromeDownloadManagerDelegate(Profile* profile) } ChromeDownloadManagerDelegate::~ChromeDownloadManagerDelegate() { + // If a DownloadManager was set for this, Shutdown() must be called. + DCHECK(!download_manager_); } void ChromeDownloadManagerDelegate::SetDownloadManager(DownloadManager* dm) { @@ -225,6 +227,13 @@ void ChromeDownloadManagerDelegate::SetDownloadManager(DownloadManager* dm) { void ChromeDownloadManagerDelegate::Shutdown() { download_prefs_.reset(); weak_ptr_factory_.InvalidateWeakPtrs(); + download_manager_ = NULL; +} + +content::DownloadIdCallback +ChromeDownloadManagerDelegate::GetDownloadIdReceiverCallback() { + return base::Bind(&ChromeDownloadManagerDelegate::SetNextId, + weak_ptr_factory_.GetWeakPtr()); } void ChromeDownloadManagerDelegate::SetNextId(uint32 next_id) { diff --git a/chrome/browser/download/chrome_download_manager_delegate.h b/chrome/browser/download/chrome_download_manager_delegate.h index 38b91f7..095f391 100644 --- a/chrome/browser/download/chrome_download_manager_delegate.h +++ b/chrome/browser/download/chrome_download_manager_delegate.h @@ -7,7 +7,6 @@ #include "base/compiler_specific.h" #include "base/containers/hash_tables.h" -#include "base/memory/ref_counted.h" #include "base/memory/scoped_ptr.h" #include "base/memory/weak_ptr.h" #include "chrome/browser/download/download_path_reservation_tracker.h" @@ -48,12 +47,12 @@ struct hash<extensions::CrxInstaller*> { // This is the Chrome side helper for the download system. class ChromeDownloadManagerDelegate - : public base::RefCountedThreadSafe<ChromeDownloadManagerDelegate>, - public content::DownloadManagerDelegate, + : public content::DownloadManagerDelegate, public content::NotificationObserver, public DownloadTargetDeterminerDelegate { public: explicit ChromeDownloadManagerDelegate(Profile* profile); + virtual ~ChromeDownloadManagerDelegate(); // Should be called before the first call to ShouldCompleteDownload() to // disable SafeBrowsing checks for |item|. @@ -61,9 +60,9 @@ class ChromeDownloadManagerDelegate void SetDownloadManager(content::DownloadManager* dm); - // Callbacks passed to GetNextId() will not be called until SetNextId() is - // called. - void SetNextId(uint32 next_id); + // Callbacks passed to GetNextId() will not be called until the returned + // callback is called. + content::DownloadIdCallback GetDownloadIdReceiverCallback(); // content::DownloadManagerDelegate virtual void Shutdown() OVERRIDE; @@ -105,9 +104,6 @@ class ChromeDownloadManagerDelegate DownloadPrefs* download_prefs() { return download_prefs_.get(); } protected: - // So that test classes can inherit from this for override purposes. - virtual ~ChromeDownloadManagerDelegate(); - // So that test classes that inherit from this for override purposes // can call back into the DownloadManager. content::DownloadManager* download_manager_; @@ -165,6 +161,8 @@ class ChromeDownloadManagerDelegate uint32 download_id, const base::Closure& user_complete_callback); + void SetNextId(uint32 id); + void ReturnNextId(const content::DownloadIdCallback& callback); void OnDownloadTargetDetermined( diff --git a/chrome/browser/download/chrome_download_manager_delegate_unittest.cc b/chrome/browser/download/chrome_download_manager_delegate_unittest.cc index 1e96837..7ef30fc 100644 --- a/chrome/browser/download/chrome_download_manager_delegate_unittest.cc +++ b/chrome/browser/download/chrome_download_manager_delegate_unittest.cc @@ -75,6 +75,8 @@ class TestChromeDownloadManagerDelegate : public ChromeDownloadManagerDelegate { : ChromeDownloadManagerDelegate(profile) { } + virtual ~TestChromeDownloadManagerDelegate() {} + virtual safe_browsing::DownloadProtectionService* GetDownloadProtectionService() OVERRIDE { return NULL; @@ -118,9 +120,6 @@ class TestChromeDownloadManagerDelegate : public ChromeDownloadManagerDelegate { content::DownloadItem*, const base::FilePath&, const DownloadTargetDeterminerDelegate::FileSelectedCallback&)); - - private: - ~TestChromeDownloadManagerDelegate() {} }; class ChromeDownloadManagerDelegateTest : @@ -164,7 +163,7 @@ class ChromeDownloadManagerDelegateTest : TestingPrefServiceSyncable* pref_service_; base::ScopedTempDir test_download_dir_; scoped_ptr<content::MockDownloadManager> download_manager_; - scoped_refptr<TestChromeDownloadManagerDelegate> delegate_; + scoped_ptr<TestChromeDownloadManagerDelegate> delegate_; MockWebContentsDelegate web_contents_delegate_; }; @@ -176,7 +175,7 @@ void ChromeDownloadManagerDelegateTest::SetUp() { ChromeRenderViewHostTestHarness::SetUp(); CHECK(profile()); - delegate_ = new TestChromeDownloadManagerDelegate(profile()); + delegate_.reset(new TestChromeDownloadManagerDelegate(profile())); delegate_->SetDownloadManager(download_manager_.get()); pref_service_ = profile()->GetTestingPrefService(); web_contents()->SetDelegate(&web_contents_delegate_); diff --git a/chrome/browser/download/download_service.cc b/chrome/browser/download/download_service.cc index a33ed163..13503dc 100644 --- a/chrome/browser/download/download_service.cc +++ b/chrome/browser/download/download_service.cc @@ -43,7 +43,7 @@ ChromeDownloadManagerDelegate* DownloadService::GetDownloadManagerDelegate() { // In case the delegate has already been set by // SetDownloadManagerDelegateForTesting. if (!manager_delegate_.get()) - manager_delegate_ = new ChromeDownloadManagerDelegate(profile_); + manager_delegate_.reset(new ChromeDownloadManagerDelegate(profile_)); manager_delegate_->SetDownloadManager(manager); @@ -55,8 +55,8 @@ ChromeDownloadManagerDelegate* DownloadService::GetDownloadManagerDelegate() { if (!profile_->IsOffTheRecord()) { HistoryService* history = HistoryServiceFactory::GetForProfile( profile_, Profile::EXPLICIT_ACCESS); - history->GetNextDownloadId(base::Bind( - &ChromeDownloadManagerDelegate::SetNextId, manager_delegate_)); + history->GetNextDownloadId( + manager_delegate_->GetDownloadIdReceiverCallback()); download_history_.reset(new DownloadHistory( manager, scoped_ptr<DownloadHistory::HistoryAdapter>( @@ -135,17 +135,13 @@ void DownloadService::CancelAllDownloads() { } void DownloadService::SetDownloadManagerDelegateForTesting( - ChromeDownloadManagerDelegate* new_delegate) { - // Set the new delegate first so that if BrowserContext::GetDownloadManager() - // causes a new download manager to be created, we won't create a redundant - // ChromeDownloadManagerDelegate(). - manager_delegate_ = new_delegate; - // Guarantee everything is properly initialized. + scoped_ptr<ChromeDownloadManagerDelegate> new_delegate) { + manager_delegate_.swap(new_delegate); DownloadManager* dm = BrowserContext::GetDownloadManager(profile_); - if (dm->GetDelegate() != new_delegate) { - dm->SetDelegate(new_delegate); - new_delegate->SetDownloadManager(dm); - } + dm->SetDelegate(manager_delegate_.get()); + manager_delegate_->SetDownloadManager(dm); + if (new_delegate) + new_delegate->Shutdown(); } bool DownloadService::IsShelfEnabled() { @@ -169,6 +165,6 @@ void DownloadService::Shutdown() { #if !defined(OS_ANDROID) extension_event_router_.reset(); #endif - manager_delegate_ = NULL; + manager_delegate_.reset(); download_history_.reset(); } diff --git a/chrome/browser/download/download_service.h b/chrome/browser/download/download_service.h index c382486..72b0de0 100644 --- a/chrome/browser/download/download_service.h +++ b/chrome/browser/download/download_service.h @@ -7,7 +7,6 @@ #include "base/basictypes.h" #include "base/callback_forward.h" -#include "base/memory/ref_counted.h" #include "base/memory/scoped_ptr.h" #include "components/browser_context_keyed_service/browser_context_keyed_service.h" @@ -58,7 +57,7 @@ class DownloadService : public BrowserContextKeyedService { // its DownloadManager. Takes ownership of |delegate|, and destroys // the previous delegate. For testing. void SetDownloadManagerDelegateForTesting( - ChromeDownloadManagerDelegate* delegate); + scoped_ptr<ChromeDownloadManagerDelegate> delegate); // Will be called to release references on other services as part // of Profile shutdown. @@ -75,7 +74,7 @@ class DownloadService : public BrowserContextKeyedService { // ChromeDownloadManagerDelegate may be the target of callbacks from // the history service/DB thread and must be kept alive for those // callbacks. - scoped_refptr<ChromeDownloadManagerDelegate> manager_delegate_; + scoped_ptr<ChromeDownloadManagerDelegate> manager_delegate_; scoped_ptr<DownloadHistory> download_history_; diff --git a/chrome/browser/download/download_test_file_activity_observer.cc b/chrome/browser/download/download_test_file_activity_observer.cc index 5768726..db68b6f 100644 --- a/chrome/browser/download/download_test_file_activity_observer.cc +++ b/chrome/browser/download/download_test_file_activity_observer.cc @@ -24,11 +24,15 @@ class DownloadTestFileActivityObserver::MockDownloadManagerDelegate explicit MockDownloadManagerDelegate(Profile* profile) : ChromeDownloadManagerDelegate(profile), file_chooser_enabled_(false), - file_chooser_displayed_(false) { + file_chooser_displayed_(false), + weak_ptr_factory_(this) { if (!profile->IsOffTheRecord()) - SetNextId(content::DownloadItem::kInvalidId + 1); + GetDownloadIdReceiverCallback().Run( + content::DownloadItem::kInvalidId + 1); } + virtual ~MockDownloadManagerDelegate() {} + void EnableFileChooser(bool enable) { file_chooser_enabled_ = enable; } @@ -39,6 +43,10 @@ class DownloadTestFileActivityObserver::MockDownloadManagerDelegate return did_show; } + base::WeakPtr<MockDownloadManagerDelegate> GetWeakPtr() { + return weak_ptr_factory_.GetWeakPtr(); + } + protected: virtual void PromptUserForDownloadPath(content::DownloadItem* item, @@ -54,26 +62,30 @@ class DownloadTestFileActivityObserver::MockDownloadManagerDelegate virtual void OpenDownload(content::DownloadItem* item) OVERRIDE {} private: - virtual ~MockDownloadManagerDelegate() {} - bool file_chooser_enabled_; bool file_chooser_displayed_; + base::WeakPtrFactory<MockDownloadManagerDelegate> weak_ptr_factory_; }; DownloadTestFileActivityObserver::DownloadTestFileActivityObserver( Profile* profile) { - test_delegate_ = new MockDownloadManagerDelegate(profile); + scoped_ptr<MockDownloadManagerDelegate> mock_delegate( + new MockDownloadManagerDelegate(profile)); + test_delegate_ = mock_delegate->GetWeakPtr(); DownloadServiceFactory::GetForBrowserContext(profile)-> - SetDownloadManagerDelegateForTesting(test_delegate_.get()); + SetDownloadManagerDelegateForTesting( + mock_delegate.PassAs<ChromeDownloadManagerDelegate>()); } DownloadTestFileActivityObserver::~DownloadTestFileActivityObserver() { } void DownloadTestFileActivityObserver::EnableFileChooser(bool enable) { - test_delegate_->EnableFileChooser(enable); + if (test_delegate_.get()) + test_delegate_->EnableFileChooser(enable); } bool DownloadTestFileActivityObserver::TestAndResetDidShowFileChooser() { - return test_delegate_->TestAndResetDidShowFileChooser(); + return test_delegate_.get() && + test_delegate_->TestAndResetDidShowFileChooser(); } diff --git a/chrome/browser/download/download_test_file_activity_observer.h b/chrome/browser/download/download_test_file_activity_observer.h index 736b9fd..28e07d8 100644 --- a/chrome/browser/download/download_test_file_activity_observer.h +++ b/chrome/browser/download/download_test_file_activity_observer.h @@ -5,7 +5,7 @@ #ifndef CHROME_BROWSER_DOWNLOAD_DOWNLOAD_TEST_FILE_ACTIVITY_OBSERVER_H_ #define CHROME_BROWSER_DOWNLOAD_DOWNLOAD_TEST_FILE_ACTIVITY_OBSERVER_H_ -#include "base/memory/ref_counted.h" +#include "base/memory/weak_ptr.h" class Profile; @@ -34,7 +34,7 @@ class DownloadTestFileActivityObserver { private: class MockDownloadManagerDelegate; - scoped_refptr<MockDownloadManagerDelegate> test_delegate_; + base::WeakPtr<MockDownloadManagerDelegate> test_delegate_; }; #endif // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_TEST_FILE_ACTIVITY_OBSERVER_H_ diff --git a/chrome/browser/download/save_page_browsertest.cc b/chrome/browser/download/save_page_browsertest.cc index 70636b9..8eb7341 100644 --- a/chrome/browser/download/save_page_browsertest.cc +++ b/chrome/browser/download/save_page_browsertest.cc @@ -448,26 +448,28 @@ class DelayingDownloadManagerDelegate : public ChromeDownloadManagerDelegate { explicit DelayingDownloadManagerDelegate(Profile* profile) : ChromeDownloadManagerDelegate(profile) { } + virtual ~DelayingDownloadManagerDelegate() {} + virtual bool ShouldCompleteDownload( content::DownloadItem* item, const base::Closure& user_complete_callback) OVERRIDE { return false; } - protected: - virtual ~DelayingDownloadManagerDelegate() {} - private: DISALLOW_COPY_AND_ASSIGN(DelayingDownloadManagerDelegate); }; IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveHTMLOnlyTabDestroy) { GURL url = NavigateToMockURL("a"); - DownloadManager* manager(GetDownloadManager()); - scoped_refptr<DelayingDownloadManagerDelegate> delaying_delegate( + scoped_ptr<DelayingDownloadManagerDelegate> delaying_delegate( new DelayingDownloadManagerDelegate(browser()->profile())); - delaying_delegate->SetNextId(content::DownloadItem::kInvalidId + 1); - manager->SetDelegate(delaying_delegate.get()); + delaying_delegate->GetDownloadIdReceiverCallback().Run( + content::DownloadItem::kInvalidId + 1); + DownloadServiceFactory::GetForBrowserContext(browser()->profile())-> + SetDownloadManagerDelegateForTesting( + delaying_delegate.PassAs<ChromeDownloadManagerDelegate>()); + DownloadManager* manager(GetDownloadManager()); std::vector<DownloadItem*> downloads; manager->GetAllDownloads(&downloads); ASSERT_EQ(0u, downloads.size()); @@ -487,8 +489,6 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveHTMLOnlyTabDestroy) { EXPECT_FALSE(base::PathExists(full_file_name)); EXPECT_FALSE(base::PathExists(dir)); - - manager->SetDelegate(NULL); } // Disabled on Windows due to flakiness. http://crbug.com/162323 |