Make ToolbarActionsModel own ComponentToolbarActionsFactory

chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc

Index: chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc
diff --git a/chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc b/chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc
index c39f29ddc3d5d4d4f0456de878752f52404d80ce..026629040a6ba83e1f5fc11d2bfe9a1b46155c26 100644
--- a/chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc
+++ b/chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc
@@ -123,10 +123,6 @@ class ToolbarActionsModelUnitTest
   ~ToolbarActionsModelUnitTest() override {}
 
  protected:
-  // Initialize the ExtensionService, ToolbarActionsModel, and
-  // ExtensionSystem.
-  void Init();
-
   enum class MigrationStatus {
     // The feature is enabled without any extension to migrate.
     FEATURE_ENABLED_NO_EXTENSION,
@@ -141,11 +137,24 @@ class ToolbarActionsModelUnitTest
     FEATURE_DISABLED_WITH_PREF_TRUE,
   };
 
-  // Initialize the ExtensionService, ToolbarActionsModel, and ExtensionSystem,
+  // Initializes the ExtensionService, ToolbarActionsModel, and ExtensionSystem.
+  void Init();
+
+  // Initializes the ExtensionService, ToolbarActionsModel, and ExtensionSystem,
+  // making ToolbarActionsModel use a MockComponentToolbarActionsFactory.
+  void InitWithMockActionsFactory();
+
+  // Initializes the ExtensionService, ToolbarActionsModel, and ExtensionSystem,
   // and an action extension to migrate to a component.  |migration_status|
   // is used to configure the user's initial migration status.
   void InitForMigrationTest(MigrationStatus migration_status);
 
+  // Initializes ExtensionSystem and triggers ToolbarActionsModel::OnReady() to
+  // be called with |mock_actions_factory| in use. The ownership of the actions
+  // factory gets transferred to |toolbar_model_|.
+  void ReadyWithMockActionsFactory(
+      MockComponentToolbarActionsFactory* mock_actions_factory);
+
   void TearDown() override;
 
   // Adds or removes the given |extension| and verify success.
@@ -164,7 +173,8 @@ class ToolbarActionsModelUnitTest
   testing::AssertionResult AddActionExtensions() WARN_UNUSED_RESULT;
 
   // Creates an extension that is to be migrated to a component action.
-  void CreateMigratedActionExtension();
+  void CreateMigratedActionExtension(
+      MockComponentToolbarActionsFactory* mock_actions_factory);
 
   // Returns the action's id at the given index in the toolbar model, or empty
   // if one does not exist.
@@ -177,8 +187,6 @@ class ToolbarActionsModelUnitTest
   // Returns true if the |toobar_model_| has an action with the given |id|.
   bool ModelHasActionForId(const std::string& id) const;
 
-  void SetMockActionsFactory(MockComponentToolbarActionsFactory* factory);
-
   ToolbarActionsModel* toolbar_model() { return toolbar_model_; }
 
   const ToolbarActionsModelTestObserver* observer() const {
@@ -237,8 +245,6 @@ class ToolbarActionsModelUnitTest
   scoped_refptr<const extensions::Extension> page_action_extension_;
   scoped_refptr<const extensions::Extension> no_action_extension_;
 
-  std::unique_ptr<MockComponentToolbarActionsFactory> mock_actions_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(ToolbarActionsModelUnitTest);
 };
 
@@ -250,45 +256,69 @@ void ToolbarActionsModelUnitTest::Init() {
   model_observer_.reset(new ToolbarActionsModelTestObserver(toolbar_model_));
 }
 
+void ToolbarActionsModelUnitTest::InitWithMockActionsFactory() {

[msw, 2017/01/10 23:23:59]

nit: this is called once, maybe inline the functionality there?
(or find a way to consolidate some InitForMigrationTest code with this)

[takumif, 2017/02/17 03:22:52]

The test that calls this was already very long, so I didn't want to make it
longer.

+  InitializeEmptyExtensionService();
+  toolbar_model_ = extensions::extension_action_test_util::
+      CreateToolbarModelForProfileWithoutWaitingForReady(profile());
+  MockComponentToolbarActionsFactory* mock_actions_factory =
+      new MockComponentToolbarActionsFactory(profile(), toolbar_model_);

[msw, 2017/01/10 23:23:59]

Use base::MakeUnique

[takumif, 2017/02/17 03:22:52]

Done.

+  ReadyWithMockActionsFactory(mock_actions_factory);
+}
+
+void ToolbarActionsModelUnitTest::ReadyWithMockActionsFactory(
+    MockComponentToolbarActionsFactory* mock_actions_factory) {

[msw, 2017/01/10 23:23:59]

Use unique_ptr for ownership passing

[takumif, 2017/02/17 03:22:52]

Done.

+  mock_actions_factory->RegisterComponentMigrations();
+  toolbar_model_->SetMockActionsFactory(mock_actions_factory);
+
+  // Trigger ToolbarActionsModel::OnReady() after the actions factory has been
+  // swapped out for a mock one.
+  static_cast<extensions::TestExtensionSystem*>(
+      extensions::ExtensionSystem::Get(profile()))
+      ->SetReady();
+  base::RunLoop().RunUntilIdle();
+
+  model_observer_.reset(new ToolbarActionsModelTestObserver(toolbar_model_));
+}
+
 void ToolbarActionsModelUnitTest::InitForMigrationTest(
     MigrationStatus migration_status) {
   InitializeEmptyExtensionService();
-  SetMockActionsFactory(new MockComponentToolbarActionsFactory(nullptr));
-  CreateMigratedActionExtension();
+  toolbar_model_ = extensions::extension_action_test_util::
+      CreateToolbarModelForProfileWithoutWaitingForReady(profile());
+  MockComponentToolbarActionsFactory* mock_actions_factory =
+      new MockComponentToolbarActionsFactory(profile(), toolbar_model_);
+  CreateMigratedActionExtension(mock_actions_factory);
 
   {
     DictionaryPrefUpdate update(profile()->GetPrefs(),
                                 ::prefs::kToolbarMigratedComponentActionStatus);
     switch (migration_status) {
       case MigrationStatus::FEATURE_ENABLED_EXTENSION_INSTALLED:
-        mock_actions_factory_->set_migrated_feature_enabled(true);
+        mock_actions_factory->set_migrated_feature_enabled(true);
         ASSERT_TRUE(AddExtension(browser_action_migrated()));
         break;
       case MigrationStatus::FEATURE_ENABLED_NO_EXTENSION:
-        mock_actions_factory_->set_migrated_feature_enabled(true);
+        mock_actions_factory->set_migrated_feature_enabled(true);
         break;
       case MigrationStatus::FEATURE_ENABLED_WITH_PREF_TRUE:
-        mock_actions_factory_->set_migrated_feature_enabled(true);
+        mock_actions_factory->set_migrated_feature_enabled(true);
         ASSERT_TRUE(AddExtension(browser_action_migrated()));
         update->SetBoolean(component_action_id(), true);
         break;
       case MigrationStatus::FEATURE_ENABLED_WITH_PREF_FALSE:
-        mock_actions_factory_->set_migrated_feature_enabled(true);
+        mock_actions_factory->set_migrated_feature_enabled(true);
         ASSERT_TRUE(AddExtension(browser_action_migrated()));
         update->SetBoolean(component_action_id(), false);
         break;
       case MigrationStatus::FEATURE_DISABLED_WITH_PREF_TRUE:
-        mock_actions_factory_->set_migrated_feature_enabled(false);
+        mock_actions_factory->set_migrated_feature_enabled(false);
         ASSERT_TRUE(AddExtension(browser_action_migrated()));
         update->SetBoolean(component_action_id(), true);
         break;
     }
   }
 
-  toolbar_model_ =
-      extensions::extension_action_test_util::CreateToolbarModelForProfile(
-          profile());
-  model_observer_.reset(new ToolbarActionsModelTestObserver(toolbar_model_));
+  ReadyWithMockActionsFactory(mock_actions_factory);
 }
 
 void ToolbarActionsModelUnitTest::TearDown() {
@@ -368,12 +398,13 @@ ToolbarActionsModelUnitTest::AddBrowserActionExtensions() {
   return AddAndVerifyExtensions(extensions);
 }
 
-void ToolbarActionsModelUnitTest::CreateMigratedActionExtension() {
+void ToolbarActionsModelUnitTest::CreateMigratedActionExtension(
+    MockComponentToolbarActionsFactory* mock_actions_factory) {
   browser_action_migrated_ =
       extensions::extension_action_test_util::CreateActionExtension(
           "browser_actionMigrated",
           extensions::extension_action_test_util::BROWSER_ACTION);
-  mock_actions_factory_->set_migrated_extension_id(
+  mock_actions_factory->set_migrated_extension_id(
       browser_action_migrated_->id());
 }
 
@@ -411,11 +442,6 @@ testing::AssertionResult ToolbarActionsModelUnitTest::AddAndVerifyExtensions(
   return testing::AssertionSuccess();
 }
 
-void ToolbarActionsModelUnitTest::SetMockActionsFactory(
-    MockComponentToolbarActionsFactory* factory) {
-  mock_actions_factory_.reset(factory);
-}
-
 // A basic test for component actions and extensions with browser actions
 // showing up in the toolbar.
 TEST_F(ToolbarActionsModelUnitTest, BasicToolbarActionsModelTest) {
@@ -1314,8 +1340,7 @@ TEST_F(ToolbarActionsModelUnitTest,
        ActionsToolbarReorderAndReinsertWithSwitchAndComponentActions) {
   extensions::FeatureSwitch::ScopedOverride enable_redesign(
       extensions::FeatureSwitch::extension_action_redesign(), true);
-  SetMockActionsFactory(new MockComponentToolbarActionsFactory(nullptr));
-  Init();
+  InitWithMockActionsFactory();
 
   // One component action was added when the model was initialized.
   EXPECT_EQ(1u, num_toolbar_items());