Support Component Actions in the toolbar.

chrome/browser/ui/toolbar/toolbar_actions_bar.cc

Index: chrome/browser/ui/toolbar/toolbar_actions_bar.cc
diff --git a/chrome/browser/ui/toolbar/toolbar_actions_bar.cc b/chrome/browser/ui/toolbar/toolbar_actions_bar.cc
index 98ef0f627be99f56b3b54f65a01250f64ee8de09..de6b8a870e39257f356a33925717384d841d957d 100644
--- a/chrome/browser/ui/toolbar/toolbar_actions_bar.cc
+++ b/chrome/browser/ui/toolbar/toolbar_actions_bar.cc
@@ -25,6 +25,7 @@
 #include "chrome/common/pref_names.h"
 #include "components/crx_file/id_util.h"
 #include "components/pref_registry/pref_registry_syncable.h"
+#include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/runtime_data.h"
 #include "extensions/common/extension.h"
@@ -119,7 +120,7 @@ ToolbarActionsBar::ToolbarActionsBar(ToolbarActionsBarDelegate* delegate,
                                      ToolbarActionsBar* main_bar)
     : delegate_(delegate),
       browser_(browser),
-      model_(extensions::ExtensionToolbarModel::Get(browser_->profile())),
+      model_(ToolbarActionsModel::Get(browser_->profile())),
       main_bar_(main_bar),
       platform_settings_(main_bar != nullptr),
       popup_owner_(nullptr),
@@ -258,22 +259,19 @@ size_t ToolbarActionsBar::GetIconCount() const {
   // icons than we have, and we should always have a view per item in the model.
   // (The only exception is if this is in initialization.)
   if (!toolbar_actions_.empty() && !suppress_layout_ &&
-      model_->extensions_initialized()) {
-    size_t num_extension_actions = 0u;
+      model_->actions_initialized()) {
+    size_t num_actions = 0u;
     for (ToolbarActionViewController* action : toolbar_actions_) {
       // No component action should ever have a valid extension id, so we can
       // use this to check the extension amount.
-      if (crx_file::id_util::IdIsValid(action->GetId()))
-        ++num_extension_actions;
+      if (crx_file::id_util::IdIsValid(action->GetId()) ||

[Devlin, 2015/08/13 21:21:11]

In updating these, it looks like the checks here are pretty trivial (to the
point of basically being "make sure that this the action is some kind of
possible action"), so they're not really helpful.  I'd just do the basic DCHECKs
with toolbar_actions_.

That is,
DCHECK_LE(visible_icons, toolbar_actions_.size());
DCHECK_EQ(model_->toolbar_items().size(), num_actions);

[apacible, 2015/08/15 08:46:41]

Done.

+          model_->IsKnownActionWithType(action->GetId(),
+                                        ToolbarActionsModel::COMPONENT_ACTION))
+        ++num_actions;
     }
 
-    int num_component_actions =
-        ComponentToolbarActionsFactory::GetInstance()->
-            GetNumComponentActions(browser_);
-    size_t num_total_actions = num_extension_actions + num_component_actions;
-
-    DCHECK_LE(visible_icons, num_total_actions);
-    DCHECK_EQ(model_->toolbar_items().size(), num_extension_actions);
+    DCHECK_LE(visible_icons, num_actions);
+    DCHECK_EQ(model_->toolbar_items().size(), num_actions);
   }
 #endif
 
@@ -331,7 +329,7 @@ void ToolbarActionsBar::CreateActions() {
   DCHECK(toolbar_actions_.empty());
   // We wait for the extension system to be initialized before we add any
   // actions, as they rely on the extension system to function.
-  if (!model_ || !model_->extensions_initialized())
+  if (!model_ || !model_->actions_initialized())
     return;
 
   {
@@ -342,37 +340,15 @@ void ToolbarActionsBar::CreateActions() {
     // We don't redraw the view while creating actions.
     base::AutoReset<bool> layout_resetter(&suppress_layout_, true);
 
-    // Extension actions come first.
-    extensions::ExtensionActionManager* action_manager =
-        extensions::ExtensionActionManager::Get(browser_->profile());
-    const extensions::ExtensionList& toolbar_items = model_->toolbar_items();
-    for (const scoped_refptr<const extensions::Extension>& extension :
-             toolbar_items) {
-      toolbar_actions_.push_back(new ExtensionActionViewController(
-          extension.get(),
-          browser_,
-          action_manager->GetExtensionAction(*extension),
-          this));
-    }
+    // Get the toolbar actions.
+    toolbar_actions_ = model_->CreateActions(browser_, this);
 
-    // Component actions come second, and are suppressed if the extension
-    // actions are being highlighted.
     if (!model_->is_highlighting()) {
       // TODO(robliao): Remove ScopedTracker below once https://crbug.com/463337
       // is fixed.
       tracked_objects::ScopedTracker tracking_profile2(
           FROM_HERE_WITH_EXPLICIT_FUNCTION(
               "ToolbarActionsBar::CreateActions2"));
-
-      ScopedVector<ToolbarActionViewController> component_actions =
-          ComponentToolbarActionsFactory::GetInstance()->
-              GetComponentToolbarActions(browser_);
-      DCHECK(component_actions.empty() ||
-          extensions::FeatureSwitch::extension_action_redesign()->IsEnabled());
-      toolbar_actions_.insert(toolbar_actions_.end(),
-                              component_actions.begin(),
-                              component_actions.end());
-      component_actions.weak_clear();
     }
 
     if (!toolbar_actions_.empty()) {
@@ -462,7 +438,7 @@ void ToolbarActionsBar::OnDragDrop(int dragged_index,
     delta = -1;
   else if (drag_type == DRAG_TO_MAIN)
     delta = 1;
-  model_->MoveExtensionIcon(toolbar_actions_[dragged_index]->GetId(),
+  model_->MoveActionIcon(toolbar_actions_[dragged_index]->GetId(),
                             dropped_index);
   if (delta)
     model_->SetVisibleIconCount(model_->visible_icon_count() + delta);
@@ -552,8 +528,7 @@ void ToolbarActionsBar::MaybeShowExtensionBubble(
     // so wait until animation stops.
     pending_extension_bubble_controller_ = controller.Pass();
   } else {
-    const extensions::ExtensionIdList& affected_extensions =
-        controller->GetExtensionIdList();
+    const ActionIds& affected_extensions = controller->GetExtensionIdList();
     ToolbarActionViewController* anchor_action = nullptr;
     for (const std::string& id : affected_extensions) {
       anchor_action = GetActionForId(id);
@@ -564,12 +539,17 @@ void ToolbarActionsBar::MaybeShowExtensionBubble(
   }
 }
 
-void ToolbarActionsBar::OnToolbarExtensionAdded(
-    const extensions::Extension* extension,
-    int index) {
-  DCHECK(GetActionForId(extension->id()) == nullptr) <<
+void ToolbarActionsBar::OnToolbarActionAdded(const std::string& action_id,
+                                             int index) {
+  DCHECK(GetActionForId(action_id) == nullptr) <<
       "Asked to add a toolbar action view for an extension that already exists";
 
+  // TODO(rdevlin.cronin): This is a minor layering violation and the model

[Devlin, 2015/08/13 21:21:11]

even though it's not the norm, my TODOs are (mostly) TODO(devlin).

[apacible, 2015/08/15 08:46:41]

Done.

+  // should pass in an action directly.
+  const extensions::Extension* extension =
+      extensions::ExtensionRegistry::Get(browser_->profile())->
+          enabled_extensions().GetByID(action_id);

[Devlin, 2015/08/13 21:21:11]

// Only extensions should be added after initialization.
DCHECK(extension)

[apacible, 2015/08/15 08:46:41]

Done.

+
   toolbar_actions_.insert(
       toolbar_actions_.begin() + index,
       new ExtensionActionViewController(
@@ -582,7 +562,7 @@ void ToolbarActionsBar::OnToolbarExtensionAdded(
   delegate_->AddViewForAction(toolbar_actions_[index], index);
 
   // If we are still initializing the container, don't bother animating.
-  if (!model_->extensions_initialized())
+  if (!model_->actions_initialized())
     return;
 
   // We may need to resize (e.g. to show the new icon, or the chevron). We don't
@@ -596,10 +576,9 @@ void ToolbarActionsBar::OnToolbarExtensionAdded(
   ResizeDelegate(gfx::Tween::LINEAR, true);
 }
 
-void ToolbarActionsBar::OnToolbarExtensionRemoved(
-    const extensions::Extension* extension) {
+void ToolbarActionsBar::OnToolbarActionRemoved(const std::string& action_id) {
   ToolbarActions::iterator iter = toolbar_actions_.begin();
-  while (iter != toolbar_actions_.end() && (*iter)->GetId() != extension->id())
+  while (iter != toolbar_actions_.end() && (*iter)->GetId() != action_id)
     ++iter;
 
   if (iter == toolbar_actions_.end())
@@ -618,9 +597,9 @@ void ToolbarActionsBar::OnToolbarExtensionRemoved(
   // There is an exception if this is an off-the-record profile, and the
   // extension is no longer incognito-enabled.
   if (!extensions::ExtensionSystem::Get(browser_->profile())->runtime_data()->
-            IsBeingUpgraded(extension->id()) ||
+            IsBeingUpgraded(action_id) ||
       (browser_->profile()->IsOffTheRecord() &&
-       !extensions::util::IsIncognitoEnabled(extension->id(),
+       !extensions::util::IsIncognitoEnabled(action_id,
                                              browser_->profile()))) {
     if (toolbar_actions_.size() > model_->visible_icon_count()) {
       // If we have more icons than we can show, then we must not be changing
@@ -639,9 +618,8 @@ void ToolbarActionsBar::OnToolbarExtensionRemoved(
   SetOverflowedActionWantsToRun();
 }
 
-void ToolbarActionsBar::OnToolbarExtensionMoved(
-    const extensions::Extension* extension,
-    int index) {
+void ToolbarActionsBar::OnToolbarActionMoved(const std::string& action_id,
+                                             int index) {
   DCHECK(index >= 0 && index < static_cast<int>(toolbar_actions_.size()));
   // Unfortunately, |index| doesn't really mean a lot to us, because this
   // window's toolbar could be different (if actions are popped out). Just
@@ -649,9 +627,8 @@ void ToolbarActionsBar::OnToolbarExtensionMoved(
   ReorderActions();
 }
 
-void ToolbarActionsBar::OnToolbarExtensionUpdated(
-    const extensions::Extension* extension) {
-  ToolbarActionViewController* action = GetActionForId(extension->id());
+void ToolbarActionsBar::OnToolbarActionUpdated(const std::string& action_id) {
+  ToolbarActionViewController* action = GetActionForId(action_id);
   // There might not be a view in cases where we are highlighting or if we
   // haven't fully initialized the actions.
   if (action) {
@@ -660,14 +637,13 @@ void ToolbarActionsBar::OnToolbarExtensionUpdated(
   }
 }
 
-bool ToolbarActionsBar::ShowExtensionActionPopup(
-    const extensions::Extension* extension,
-    bool grant_active_tab) {
+bool ToolbarActionsBar::ShowToolbarActionPopup(const std::string& action_id,
+                                               bool grant_active_tab) {
   // Don't override another popup, and only show in the active window.
   if (popup_owner() || !browser_->window()->IsActive())
     return false;
 
-  ToolbarActionViewController* action = GetActionForId(extension->id());
+  ToolbarActionViewController* action = GetActionForId(action_id);
   return action && action->ExecuteAction(grant_active_tab);
 }
 
@@ -697,7 +673,7 @@ void ToolbarActionsBar::ResizeDelegate(gfx::Tween::Type tween_type,
 }
 
 void ToolbarActionsBar::OnToolbarHighlightModeChanged(bool is_highlighting) {
-  if (!model_->extensions_initialized())
+  if (!model_->actions_initialized())
     return;
   // It's a bit of a pain that we delete and recreate everything here, but given
   // everything else going on (the lack of highlight, [n] more extensions
@@ -733,8 +709,8 @@ void ToolbarActionsBar::ReorderActions() {
 
   // First, reset the order to that of the model.
   auto compare = [](ToolbarActionViewController* const& action,
-                    const scoped_refptr<const extensions::Extension>& ext) {
-    return action->GetId() == ext->id();
+                    const std::string& id) {
+    return action->GetId() == id;
   };
   SortContainer(&toolbar_actions_.get(), model_->toolbar_items(), compare);
 
@@ -769,9 +745,9 @@ void ToolbarActionsBar::SetOverflowedActionWantsToRun() {
 }
 
 ToolbarActionViewController* ToolbarActionsBar::GetActionForId(
-    const std::string& id) {
+    const std::string& action_id) {
   for (ToolbarActionViewController* action : toolbar_actions_) {
-    if (action->GetId() == id)
+    if (action->GetId() == action_id)
       return action;
   }
   return nullptr;