Make ToolbarActionsModel own ComponentToolbarActionsFactory

chrome/browser/ui/toolbar/toolbar_actions_model.h

Index: chrome/browser/ui/toolbar/toolbar_actions_model.h
diff --git a/chrome/browser/ui/toolbar/toolbar_actions_model.h b/chrome/browser/ui/toolbar/toolbar_actions_model.h
index e19df0e001739661e9d8b47930ba5cb732c6d9f2..54b1ea4f7e24eed038151ec9c82eac8549fb3870 100644
--- a/chrome/browser/ui/toolbar/toolbar_actions_model.h
+++ b/chrome/browser/ui/toolbar/toolbar_actions_model.h
@@ -14,6 +14,7 @@
 #include "chrome/browser/extensions/api/extension_action/extension_action_api.h"
 #include "chrome/browser/extensions/component_migration_helper.h"

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

nit: remove this include and use a forward declaration instead, if possible

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

Done.

 #include "chrome/browser/extensions/extension_action.h"
+#include "chrome/browser/ui/toolbar/component_toolbar_actions_factory.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/prefs/pref_change_registrar.h"
 #include "extensions/browser/extension_prefs.h"
@@ -133,6 +134,8 @@ class ToolbarActionsModel
   // "show all actions".
   void SetVisibleIconCount(size_t count);
 
+  extensions::ComponentMigrationHelper* GetComponentMigrationHelper();

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

Is this necessary if the ComponentToolbarActionsFactory is exposed, and that
exposes the migration helper? If so, nit: add a comment here

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

Removed migration-related code.

+
   // Note that this (and all_icons_visible()) are the global default, but are
   // inappropriate for determining a specific window's state - for that, use
   // the ToolbarActionsBar.
@@ -163,10 +166,6 @@ class ToolbarActionsModel
     return is_highlighting() ? highlighted_items_ : toolbar_items_;
   }
 
-  extensions::ComponentMigrationHelper* component_migration_helper() {
-    return component_migration_helper_.get();
-  }
-
   bool is_highlighting() const { return highlight_type_ != HIGHLIGHT_NONE; }
   HighlightType highlight_type() const { return highlight_type_; }
 
@@ -175,6 +174,10 @@ class ToolbarActionsModel
     has_active_bubble_ = has_active_bubble;
   }
 
+  ComponentToolbarActionsFactory* component_actions_factory() {
+    return component_actions_factory_.get();
+  }
+
   void SetActionVisibility(const std::string& action_id, bool visible);
 
   // ComponentMigrationHelper::ComponentActionDelegate:
@@ -202,6 +205,10 @@ class ToolbarActionsModel
   std::unique_ptr<extensions::ExtensionMessageBubbleController>
   GetExtensionMessageBubbleController(Browser* browser);
 
+  // Swaps |component_actions_factory_| with |mock_factory|. The ownership of

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

nit: s/Swaps/Replaces/ or something like: "Set the component action factory for
this object; transfers ownership."

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

Done.

+  // |mock_factory| is transferred to |this|.

[Devlin, 2017/01/10 23:07:11]

if ownership is transferred, use a unique_ptr (and then the comment is
unnecessary).

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

Done.

+  void SetMockActionsFactory(ComponentToolbarActionsFactory* mock_factory);

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

nit: append ForTest[ing] to function name (or remove 'mock' from param name)

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

Ownership transfers should usually be done with unique_ptr.

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

Done.

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

Done.

+
  private:
   // Callback when actions are ready.
   void OnReady();
@@ -291,9 +298,7 @@ class ToolbarActionsModel
   // The ExtensionActionManager, cached for convenience.
   extensions::ExtensionActionManager* extension_action_manager_;
 
-  // The ComponentMigrationHelper.
-  std::unique_ptr<extensions::ComponentMigrationHelper>
-      component_migration_helper_;
+  std::unique_ptr<ComponentToolbarActionsFactory> component_actions_factory_;
 
   // True if we've handled the initial EXTENSIONS_READY notification.
   bool actions_initialized_;