Make ToolbarActionsModel own ComponentToolbarActionsFactory

chrome/browser/ui/toolbar/toolbar_actions_model.cc

Index: chrome/browser/ui/toolbar/toolbar_actions_model.cc
diff --git a/chrome/browser/ui/toolbar/toolbar_actions_model.cc b/chrome/browser/ui/toolbar/toolbar_actions_model.cc
index 768dc9277cc56b985152906beb37211ca1f70345..c8452ad56dbf01f8fc166bda4b54558e785ae495 100644
--- a/chrome/browser/ui/toolbar/toolbar_actions_model.cc
+++ b/chrome/browser/ui/toolbar/toolbar_actions_model.cc
@@ -25,7 +25,6 @@
 #include "chrome/browser/ui/extensions/extension_action_view_controller.h"
 #include "chrome/browser/ui/extensions/extension_message_bubble_factory.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
-#include "chrome/browser/ui/toolbar/component_toolbar_actions_factory.h"
 #include "chrome/browser/ui/toolbar/toolbar_action_view_controller.h"
 #include "chrome/browser/ui/toolbar/toolbar_actions_bar.h"
 #include "chrome/browser/ui/toolbar/toolbar_actions_model_factory.h"
@@ -51,8 +50,8 @@ ToolbarActionsModel::ToolbarActionsModel(
       extension_registry_(extensions::ExtensionRegistry::Get(profile_)),
       extension_action_manager_(
           extensions::ExtensionActionManager::Get(profile_)),
-      component_migration_helper_(
-          new extensions::ComponentMigrationHelper(profile_, this)),
+      component_actions_factory_(
+          base::MakeUnique<ComponentToolbarActionsFactory>(profile_, this)),
       actions_initialized_(false),
       use_redesign_(
           extensions::FeatureSwitch::extension_action_redesign()->IsEnabled()),
@@ -61,8 +60,7 @@ ToolbarActionsModel::ToolbarActionsModel(
       extension_action_observer_(this),
       extension_registry_observer_(this),
       weak_ptr_factory_(this) {
-  ComponentToolbarActionsFactory::GetInstance()->RegisterComponentMigrations(
-      component_migration_helper_.get());
+  component_actions_factory_->RegisterComponentMigrations();
   extensions::ExtensionSystem::Get(profile_)->ready().Post(
       FROM_HERE, base::Bind(&ToolbarActionsModel::OnReady,
                             weak_ptr_factory_.GetWeakPtr()));
@@ -150,6 +148,11 @@ void ToolbarActionsModel::SetVisibleIconCount(size_t count) {
     observer.OnToolbarVisibleCountChanged();
 }
 
+extensions::ComponentMigrationHelper*
+ToolbarActionsModel::GetComponentMigrationHelper() {
+  return component_actions_factory_->migration_helper();
+}
+
 void ToolbarActionsModel::OnExtensionActionUpdated(
     ExtensionAction* extension_action,
     content::WebContents* web_contents,
@@ -195,8 +198,8 @@ ToolbarActionsModel::CreateActionForItem(Browser* browser,
     }
     case COMPONENT_ACTION: {
       DCHECK(use_redesign_);
-      result = ComponentToolbarActionsFactory::GetInstance()
-                   ->GetComponentToolbarActionForId(item.id, browser, bar);
+      result = component_actions_factory_->GetComponentToolbarActionForId(
+          item.id, browser, bar);
       break;
     }
     case UNKNOWN_ACTION:
@@ -278,8 +281,7 @@ void ToolbarActionsModel::OnReady() {
     // Handle component action migrations.  We must make sure that observers are
     // notified of initialization first, so that the associated widgets are
     // created.
-    ComponentToolbarActionsFactory::GetInstance()->HandleComponentMigrations(
-        component_migration_helper_.get(), profile_);
+    component_actions_factory_->HandleComponentMigrations();
   }
 }
 
@@ -447,6 +449,11 @@ ToolbarActionsModel::GetExtensionMessageBubbleController(Browser* browser) {
   return controller;
 }
 
+void ToolbarActionsModel::SetMockActionsFactory(
+    ComponentToolbarActionsFactory* mock_factory) {
+  component_actions_factory_.reset(mock_factory);
+}
+
 void ToolbarActionsModel::RemoveExtension(
     const extensions::Extension* extension) {
   RemoveItem(ToolbarItem(extension->id(), EXTENSION_ACTION));
@@ -500,8 +507,7 @@ void ToolbarActionsModel::Populate() {
 
   // Next, add the component action ids.
   std::set<std::string> component_ids =
-      ComponentToolbarActionsFactory::GetInstance()->GetInitialComponentIds(
-          profile_);
+      component_actions_factory_->GetInitialComponentIds();
   for (const std::string& id : component_ids)
     all_actions.push_back(ToolbarItem(id, COMPONENT_ACTION));
 
@@ -631,12 +637,12 @@ bool ToolbarActionsModel::HasComponentAction(
 }
 
 void ToolbarActionsModel::AddComponentAction(const std::string& action_id) {
+  DCHECK(use_redesign_);
   if (!actions_initialized_) {
-    // TODO(crbug.com/660972): Add these component actions at initialization.
+    component_actions_factory_->OnAddComponentActionBeforeInit(action_id);

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

I'm not entirely sure this is less convoluted than before... it seems like we
still have this strange triangle of
                       ToolbarActionsModel
ComponentToolbarActionsFactory       ComponentMigrationHelper

where each has to know about the other in some way, shape, or form (though they
might be hidden behind delegate interfaces).  I think I'd prefer it if we could
make these a little more unidirectional.

What if instead, we have ComponentToolbarActionsFactory own the
ComponentMigrationHelper (like you have), but also implement its delegate
interface?  And then have the factory call directly into ToolbarActionsModel to
add component actions, and have the model have the caching mechanism.  Then, we
have a little more orderly flow of knowledge.  Does that make sense?  WDYT? 
(I'm brainstorming right now, so lemme know if I'm totally off base :))

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

I think things are a bit less complex after the removal of
CompomentMigrationHelper. ToolbarActionsModel owns
ComponentToolbarActionsFactory, and ComponentToolbarActionsFactory has a
reference to ToolbarActionsModel through ComponentActionDelegate interface.

     return;
   }
 
-  DCHECK(use_redesign_);
   ToolbarItem component_item(action_id, COMPONENT_ACTION);
   DCHECK(!HasItem(component_item));
   AddItem(component_item);
@@ -644,6 +650,10 @@ void ToolbarActionsModel::AddComponentAction(const std::string& action_id) {
 
 void ToolbarActionsModel::RemoveComponentAction(const std::string& action_id) {
   DCHECK(use_redesign_);
+  if (!actions_initialized_) {
+    component_actions_factory_->OnRemoveComponentActionBeforeInit(action_id);
+    return;
+  }
   // If the action was visible and there are overflowed actions, we reduce the
   // visible count so that we don't pop out a previously-hidden action.
   if (IsActionVisible(action_id) && !all_icons_visible())
@@ -669,8 +679,7 @@ void ToolbarActionsModel::IncognitoPopulate() {
   visible_icon_count_ = 0;
 
   std::set<std::string> component_ids =
-      ComponentToolbarActionsFactory::GetInstance()->GetInitialComponentIds(
-          profile_);
+      component_actions_factory_->GetInitialComponentIds();
   for (std::vector<ToolbarItem>::const_iterator iter =
            original_model->toolbar_items_.begin();
        iter != original_model->toolbar_items_.end(); ++iter) {