BrowserContext should simply own DownloadManager

This patch changes BrowserContext so it store a DownloadManager
directly (as UserData), instead of storing a refptr to it.

Client code (mostly tests) that used refptrs was changed to use either
raw pointers or to use scoped_ptr/ScopedVector in case of
DownloadManagers created by it.

The Bind() in DownloadManagerImpl::CheckForFileRemoval() is now using
a weak pointer to itself.

BUG=237871
TEST=content_unittests and {unit,browser}_tests with *Download* and *SavePage*

Review URL: https://chromiumcodereview.appspot.com/15701007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@202743 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/drive/download_handler_unittest.cc

@@ -60,7 +60,7 @@ class DownloadHandlerTest : public testing::Test {
     file_write_helper_.reset(new FileWriteHelper(&file_system_));
     download_handler_.reset(
         new DownloadHandler(file_write_helper_.get(), &file_system_));
-    download_handler_->Initialize(download_manager_, temp_dir_.path());
+    download_handler_->Initialize(download_manager_.get(), temp_dir_.path());
   }
 
   virtual void TearDown() OVERRIDE {
@@ -70,7 +70,7 @@ class DownloadHandlerTest : public testing::Test {
   base::ScopedTempDir temp_dir_;
   base::MessageLoopForUI message_loop_;
   content::TestBrowserThread ui_thread_;
-  scoped_refptr<content::MockDownloadManager> download_manager_;
+  scoped_ptr<content::MockDownloadManager> download_manager_;
   MockFileSystem file_system_;
   scoped_ptr<FileWriteHelper> file_write_helper_;
   scoped_ptr<DownloadHandler> download_handler_;

chrome/browser/download/all_download_item_notifier_unittest.cc

@@ -70,7 +70,7 @@ class AllDownloadItemNotifierTest : public testing::Test {
 
  private:
   NiceMock<content::MockDownloadItem> item_;
-  scoped_refptr<content::MockDownloadManager> download_manager_;
+  scoped_ptr<content::MockDownloadManager> download_manager_;
   scoped_ptr<AllDownloadItemNotifier> notifier_;
   NiceMock<MockNotifierObserver> observer_;
 

chrome/browser/download/chrome_download_manager_delegate.h

@@ -104,7 +104,7 @@ class ChromeDownloadManagerDelegate
 
   // So that test classes that inherit from this for override purposes
   // can call back into the DownloadManager.
-  scoped_refptr<content::DownloadManager> download_manager_;
+  content::DownloadManager* download_manager_;
 
   virtual safe_browsing::DownloadProtectionService*
       GetDownloadProtectionService();

chrome/browser/download/chrome_download_manager_delegate_unittest.cc

@@ -169,7 +169,7 @@ class ChromeDownloadManagerDelegateTest :
   base::ScopedTempDir test_download_dir_;
   content::TestBrowserThread ui_thread_;
   content::TestBrowserThread file_thread_;
-  scoped_refptr<content::MockDownloadManager> download_manager_;
+  scoped_ptr<content::MockDownloadManager> download_manager_;
   scoped_refptr<TestChromeDownloadManagerDelegate> delegate_;
   MockWebContentsDelegate web_contents_delegate_;
 };

chrome/browser/download/download_browsertest.cc

@@ -2642,8 +2642,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, HiddenDownload) {
   base::FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
   GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
 
-  scoped_refptr<DownloadManager> download_manager =
-      DownloadManagerForBrowser(browser());
+  DownloadManager* download_manager = DownloadManagerForBrowser(browser());
   scoped_ptr<content::DownloadTestObserver> observer(
       new content::DownloadTestObserverTerminal(
           download_manager,

chrome/browser/download/download_history_unittest.cc

@@ -413,7 +413,7 @@ class DownloadHistoryTest : public testing::Test {
   base::MessageLoopForUI loop_;
   content::TestBrowserThread ui_thread_;
   std::vector<NiceMockDownloadItem*> items_;
-  scoped_refptr<content::MockDownloadManager> manager_;
+  scoped_ptr<content::MockDownloadManager> manager_;
   FakeHistoryAdapter* history_;
   scoped_ptr<DownloadHistory> download_history_;
   content::DownloadManager::Observer* manager_observer_;

chrome/browser/download/download_shelf_unittest.cc

@@ -43,7 +43,7 @@ class DownloadShelfTest : public testing::Test {
   base::MessageLoopForUI message_loop_;
   content::TestBrowserThread ui_thread_;
   scoped_ptr<content::MockDownloadItem> download_item_;
-  scoped_refptr<content::MockDownloadManager> download_manager_;
+  scoped_ptr<content::MockDownloadManager> download_manager_;
   TestDownloadShelf shelf_;
 };
 
@@ -63,7 +63,8 @@ DownloadShelfTest::DownloadShelfTest()
   ON_CALL(*download_item_, ShouldOpenFileBasedOnExtension())
       .WillByDefault(Return(false));
 
-  download_manager_ = new ::testing::NiceMock<content::MockDownloadManager>();
+  download_manager_.reset(
+      new ::testing::NiceMock<content::MockDownloadManager>());
   ON_CALL(*download_manager_, GetDownload(_))
       .WillByDefault(Return(download_item_.get()));
 

chrome/browser/download/download_status_updater_unittest.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_vector.h"
 #include "base/memory/weak_ptr.h"
 #include "base/message_loop.h"
 #include "base/stl_util.h"
@@ -79,7 +79,7 @@ class DownloadStatusUpdaterTest : public testing::Test {
     for (int i = 0; i < manager_count; ++i) {
       content::MockDownloadManager* mgr =
           new StrictMock<content::MockDownloadManager>;
-      managers_.push_back(make_scoped_refptr(mgr));
+      managers_.push_back(mgr);
     }
   }
 
@@ -89,7 +89,7 @@ class DownloadStatusUpdaterTest : public testing::Test {
 
   // Hook the specified manager into the updater.
   void LinkManager(int i) {
-    content::MockDownloadManager* mgr = managers_[i].get();
+    content::MockDownloadManager* mgr = managers_[i];
     manager_observer_index_ = i;
     while (manager_observers_.size() <= static_cast<size_t>(i)) {
       manager_observers_.push_back(NULL);
@@ -103,7 +103,7 @@ class DownloadStatusUpdaterTest : public testing::Test {
   // Add some number of Download items to a particular manager.
   void AddItems(int manager_index, int item_count, int in_progress_count) {
     DCHECK_GT(managers_.size(), static_cast<size_t>(manager_index));
-    content::MockDownloadManager* manager = managers_[manager_index].get();
+    content::MockDownloadManager* manager = managers_[manager_index];
 
     if (manager_items_.size() <= static_cast<size_t>(manager_index))
       manager_items_.resize(manager_index+1);
@@ -127,7 +127,7 @@ class DownloadStatusUpdaterTest : public testing::Test {
   // Return the specified manager.
   content::MockDownloadManager* Manager(int manager_index) {
     DCHECK_GT(managers_.size(), static_cast<size_t>(manager_index));
-    return managers_[manager_index].get();
+    return managers_[manager_index];
   }
 
   // Return the specified item.
@@ -163,16 +163,16 @@ class DownloadStatusUpdaterTest : public testing::Test {
 
   // Verify and clear all mocks expectations.
   void VerifyAndClearExpectations() {
-    for (std::vector<scoped_refptr<content::MockDownloadManager> >::iterator it
+    for (ScopedVector<content::MockDownloadManager>::iterator it
              = managers_.begin(); it != managers_.end(); ++it)
-      Mock::VerifyAndClearExpectations(it->get());
+      Mock::VerifyAndClearExpectations(*it);
     for (std::vector<Items>::iterator it = manager_items_.begin();
          it != manager_items_.end(); ++it)
       for (Items::iterator sit = it->begin(); sit != it->end(); ++sit)
         Mock::VerifyAndClearExpectations(*sit);
   }
 
-  std::vector<scoped_refptr<content::MockDownloadManager> > managers_;
+  ScopedVector<content::MockDownloadManager> managers_;
   // DownloadItem so that it can be assigned to the result of SearchDownloads.
   typedef std::vector<content::DownloadItem*> Items;
   std::vector<Items> manager_items_;
@@ -210,7 +210,7 @@ TEST_F(DownloadStatusUpdaterTest, OneManagerNoItems) {
 
   float progress = -1;
   int download_count = -1;
-  EXPECT_CALL(*managers_[0].get(), GetAllDownloads(_))
+  EXPECT_CALL(*managers_[0], GetAllDownloads(_))
       .WillRepeatedly(SetArgPointee<0>(manager_items_[0]));
   EXPECT_TRUE(updater_->GetProgress(&progress, &download_count));
   EXPECT_FLOAT_EQ(0.0f, progress);

chrome/browser/download/download_ui_controller_unittest.cc

@@ -69,7 +69,7 @@ class DownloadUIControllerTest : public testing::Test {
   content::DownloadItem* received_item() { return received_item_; }
 
  private:
-  scoped_refptr<MockDownloadManager> manager_;
+  scoped_ptr<MockDownloadManager> manager_;
   content::DownloadManager::Observer* manager_observer_;
   content::DownloadItem::Observer* item_observer_;
   content::DownloadItem* received_item_;
@@ -85,7 +85,7 @@ DownloadUIControllerTest::DownloadUIControllerTest()
 }
 
 void DownloadUIControllerTest::SetUp() {
-  manager_ = new testing::StrictMock<MockDownloadManager>();
+  manager_.reset(new testing::StrictMock<MockDownloadManager>());
   EXPECT_CALL(*manager_, AddObserver(_))
       .WillOnce(SaveArg<0>(&manager_observer_));
   EXPECT_CALL(*manager_, RemoveObserver(_))

content/browser/browser_context.cc

@@ -156,18 +156,18 @@ DownloadManager* BrowserContext::GetDownloadManager(
   if (!context->GetUserData(kDownloadManagerKeyName)) {
     ResourceDispatcherHostImpl* rdh = ResourceDispatcherHostImpl::Get();
     DCHECK(rdh);
-    scoped_refptr<DownloadManager> download_manager =
+    DownloadManager* download_manager =
         new DownloadManagerImpl(
             GetContentClient()->browser()->GetNetLog(), context);
 
     context->SetUserData(
         kDownloadManagerKeyName,
-        new UserDataAdapter<DownloadManager>(download_manager));
+        download_manager);
     download_manager->SetDelegate(context->GetDownloadManagerDelegate());
   }
 
-  return UserDataAdapter<DownloadManager>::Get(
-      context, kDownloadManagerKeyName);
+  return static_cast<DownloadManager*>(
+      context->GetUserData(kDownloadManagerKeyName));
 }
 
 // static

content/browser/download/download_id_unittest.cc

@@ -9,7 +9,7 @@
 #include <set>
 #include <vector>
 
-#include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_vector.h"
 #include "content/browser/browser_thread_impl.h"
 #include "content/public/test/mock_download_manager.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -20,34 +20,22 @@ class DownloadIdTest : public testing::Test {
  public:
   DownloadIdTest()
       : ui_thread_(BrowserThread::UI, &message_loop_) {
-    num_managers_ = ARRAYSIZE_UNSAFE(download_managers_);
-    std::vector<MockDownloadManager*> managers;
-    managers.resize(num_managers_);
-    size_t i;
-    // Create the download managers.
-    for (i = 0; i < num_managers_; ++i) {
-      managers[i] = new MockDownloadManager();
+    const size_t num_managers_ = 2;
+    for (size_t i = 0; i < num_managers_; ++i) {
+      download_managers_.push_back(new MockDownloadManager());
     }
-    // Sort by pointer value.
-    std::sort(managers.begin(), managers.end());
-    // Assign to |download_managers_|.
-    for (i = 0; i < num_managers_; ++i) {
-      download_managers_[i] = managers[i];
-      managers[i] = NULL;
-    }
-  }
 
-  virtual ~DownloadIdTest() {
-    for (size_t i = 0; i < num_managers_; ++i)
-      download_managers_[i] = NULL;  // Releases & deletes.
+    // These pointers will be used as Domains for DownloadIds. Domain affects
+    // the comparison of two DownloadIds, so we keep them sorted and use
+    // that property when testing.
+    std::sort(download_managers_.begin(), download_managers_.end());
   }
 
  protected:
-  scoped_refptr<DownloadManager> download_managers_[2];
+  ScopedVector<DownloadManager> download_managers_;
   base::MessageLoopForUI message_loop_;
   // Necessary to delete |DownloadManager|s.
   BrowserThreadImpl ui_thread_;
-  size_t num_managers_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadIdTest);
 };
@@ -89,7 +77,7 @@ TEST_F(DownloadIdTest, NotEqualsIndex) {
 
 TEST_F(DownloadIdTest, NotEqualsManager) {
   // Because it's sorted above, &download_managers_[1] > &download_managers_[0].
-  EXPECT_LT(download_managers_[0].get(), download_managers_[1].get());
+  EXPECT_LT(download_managers_[0], download_managers_[1]);
   DownloadId id1(download_managers_[0], 23);
   DownloadId id2(download_managers_[1], 23);
   DownloadId id3(download_managers_[1], 22);

content/browser/download/download_manager_impl.cc

@@ -239,7 +239,8 @@ DownloadManagerImpl::DownloadManagerImpl(
       shutdown_needed_(true),
       browser_context_(browser_context),
       delegate_(NULL),
-      net_log_(net_log) {
+      net_log_(net_log),
+      weak_factory_(this) {
   DCHECK(browser_context);
 }
 
@@ -449,7 +450,7 @@ void DownloadManagerImpl::CheckForFileRemoval(DownloadItemImpl* download_item) {
     delegate_->CheckForFileExistence(
         download_item,
         base::Bind(&DownloadManagerImpl::OnFileExistenceChecked,
-                   this, download_item->GetId()));
+                   weak_factory_.GetWeakPtr(), download_item->GetId()));
   }
 }
 

content/browser/download/download_manager_impl.h

@@ -36,6 +36,7 @@ class CONTENT_EXPORT DownloadManagerImpl : public DownloadManager,
   // Caller guarantees that |net_log| will remain valid
   // for the lifetime of DownloadManagerImpl (until Shutdown() is called).
   DownloadManagerImpl(net::NetLog* net_log, BrowserContext* browser_context);
+  virtual ~DownloadManagerImpl();
 
   // Implementation functions (not part of the DownloadManager interface).
 
@@ -102,10 +103,6 @@ class CONTENT_EXPORT DownloadManagerImpl : public DownloadManager,
   friend class DownloadManagerTest;
   friend class DownloadTest;
 
-  friend class base::RefCountedThreadSafe<DownloadManagerImpl>;
-
-  virtual ~DownloadManagerImpl();
-
   // Create a new active item based on the info.  Separate from
   // StartDownload() for testing.
   DownloadItemImpl* CreateActiveItem(DownloadId id,
@@ -166,6 +163,8 @@ class CONTENT_EXPORT DownloadManagerImpl : public DownloadManager,
 
   net::NetLog* net_log_;
 
+  base::WeakPtrFactory<DownloadManagerImpl> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(DownloadManagerImpl);
 };
 

content/browser/download/download_manager_impl_unittest.cc

@@ -444,7 +444,7 @@ class DownloadManagerTest : public testing::Test {
   // then create a DownloadManager that points
   // at all of those.
   virtual void SetUp() {
-    DCHECK(!download_manager_.get());
+    DCHECK(!download_manager_);
 
     mock_download_item_factory_ = (new MockDownloadItemFactory())->AsWeakPtr();
     mock_download_file_factory_ = (new MockDownloadFileFactory())->AsWeakPtr();
@@ -456,8 +456,8 @@ class DownloadManagerTest : public testing::Test {
     EXPECT_CALL(*mock_browser_context_.get(), IsOffTheRecord())
         .WillRepeatedly(Return(false));
 
-    download_manager_ = new DownloadManagerImpl(
-        NULL, mock_browser_context_.get());
+    download_manager_.reset(new DownloadManagerImpl(
+                                NULL, mock_browser_context_.get()));
     download_manager_->SetDownloadItemFactoryForTesting(
         scoped_ptr<DownloadItemFactory>(
             mock_download_item_factory_.get()).Pass());
@@ -481,7 +481,7 @@ class DownloadManagerTest : public testing::Test {
         .WillOnce(Return());
 
     download_manager_->Shutdown();
-    download_manager_ = NULL;
+    download_manager_.reset();
     message_loop_.RunUntilIdle();
     ASSERT_EQ(NULL, mock_download_item_factory_.get());
     ASSERT_EQ(NULL, mock_download_file_factory_.get());
@@ -555,7 +555,7 @@ class DownloadManagerTest : public testing::Test {
 
  protected:
   // Key test variable; we'll keep it available to sub-classes.
-  scoped_refptr<DownloadManagerImpl> download_manager_;
+  scoped_ptr<DownloadManagerImpl> download_manager_;
   base::WeakPtr<MockDownloadFileFactory> mock_download_file_factory_;
 
   // Target detetermined callback.

content/public/browser/download_manager.h

@@ -56,9 +56,10 @@ struct DownloadCreateInfo;
 struct DownloadRetrieveInfo;
 
 // Browser's download manager: manages all downloads and destination view.
-class CONTENT_EXPORT DownloadManager
-    : public base::RefCountedThreadSafe<DownloadManager> {
+class CONTENT_EXPORT DownloadManager : public base::SupportsUserData::Data {
  public:
+  virtual ~DownloadManager() {}
+
   // Sets/Gets the delegate for this DownloadManager. The delegate has to live
   // past its Shutdown method being called (by the DownloadManager).
   virtual void SetDelegate(DownloadManagerDelegate* delegate) = 0;
@@ -169,12 +170,6 @@ class CONTENT_EXPORT DownloadManager
   // Get the download item for |id| if present, no matter what type of download
   // it is or state it's in.
   virtual DownloadItem* GetDownload(int id) = 0;
-
- protected:
-  virtual ~DownloadManager() {}
-
- private:
-  friend class base::RefCountedThreadSafe<DownloadManager>;
 };
 
 }  // namespace content

content/public/test/mock_download_manager.h

@@ -60,6 +60,7 @@ class MockDownloadManager : public DownloadManager {
   };
 
   MockDownloadManager();
+  virtual ~MockDownloadManager();
 
   // DownloadManager:
   MOCK_METHOD1(SetDelegate, void(DownloadManagerDelegate* delegate));
@@ -115,9 +116,6 @@ class MockDownloadManager : public DownloadManager {
   MOCK_METHOD1(SavePageDownloadFinished, void(DownloadItem* download));
   MOCK_METHOD1(GetActiveDownloadItem, DownloadItem*(int id));
   MOCK_METHOD1(GetActiveDownload, DownloadItem*(int32 download_id));
-
- protected:
-  virtual ~MockDownloadManager();
 };
 
 }  // namespace content

content/public/test/test_file_error_injector.cc

@@ -338,12 +338,11 @@ void DownloadFileWithErrorsFactory::ClearErrors() {
 }
 
 TestFileErrorInjector::TestFileErrorInjector(
-    scoped_refptr<DownloadManager> download_manager)
+    DownloadManager* download_manager)
     : created_factory_(NULL),
       // This code is only used for browser_tests, so a
       // DownloadManager is always a DownloadManagerImpl.
-      download_manager_(
-          static_cast<DownloadManagerImpl*>(download_manager.get())) {
+      download_manager_(static_cast<DownloadManagerImpl*>(download_manager)) {
   // Record the value of the pointer, for later validation.
   created_factory_ =
       new DownloadFileWithErrorsFactory(
@@ -446,7 +445,7 @@ void TestFileErrorInjector::RecordDownloadFileDestruction(const GURL& url) {
 
 // static
 scoped_refptr<TestFileErrorInjector> TestFileErrorInjector::Create(
-    scoped_refptr<DownloadManager> download_manager) {
+    DownloadManager* download_manager) {
   static bool visited = false;
   DCHECK(!visited);  // Only allowed to be called once.
   visited = true;

content/public/test/test_file_error_injector.h

@@ -74,7 +74,7 @@ class TestFileErrorInjector
   // creator goes out of scope.
   // TODO(rdsmith): Allow multiple calls for different download managers.
   static scoped_refptr<TestFileErrorInjector> Create(
-      scoped_refptr<DownloadManager> download_manager);
+      DownloadManager* download_manager);
 
   // Adds an error.
   // Must be called before |InjectErrors()| for a particular download file.
@@ -112,8 +112,7 @@ class TestFileErrorInjector
 
   typedef std::set<GURL> FileSet;
 
-  TestFileErrorInjector(
-      scoped_refptr<DownloadManager> download_manager);
+  explicit TestFileErrorInjector(DownloadManager* download_manager);
 
   virtual ~TestFileErrorInjector();
 
@@ -140,7 +139,7 @@ class TestFileErrorInjector
   DownloadFileWithErrorsFactory* created_factory_;
 
   // The download manager we set the factory on.
-  scoped_refptr<DownloadManagerImpl> download_manager_;
+  DownloadManagerImpl* download_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(TestFileErrorInjector);
 };