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 2a318efd44470ad6b4f3b3c84f29429997376db1..76d5e5339ea01845816b8b3c9ac972480316c2ac 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,7 +259,7 @@ 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()) {
+      model_->actions_initialized()) {
     size_t num_extension_actions = 0u;
     for (ToolbarActionViewController* action : toolbar_actions_) {
       // No component action should ever have a valid extension id, so we can
@@ -273,7 +274,7 @@ size_t ToolbarActionsBar::GetIconCount() const {
     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_EQ(model_->toolbar_items().size(), num_total_actions);
   }
 #endif
 
@@ -307,7 +308,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;
 
   {
@@ -318,37 +319,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_->GetActions(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()) {
@@ -438,7 +417,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);
@@ -523,8 +502,7 @@ void ToolbarActionsBar::MaybeShowExtensionBubble(
     // so wait until animation stops.
     pending_extension_bubble_controller_ = controller.Pass();
   } else {
-    const extensions::ExtensionIdList& affected_extensions =
-        controller->GetExtensionIdList();
+    const ActionIdList& affected_extensions = controller->GetExtensionIdList();
     ToolbarActionViewController* anchor_action = nullptr;
     for (const std::string& id : affected_extensions) {
       anchor_action = GetActionForId(id);
@@ -535,12 +513,22 @@ 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";
 
+  // Check that |action_id| is not a component action since

[Devlin, 2015/08/06 22:04:55]

Sounds like a DCHECK to me :)

[apacible, 2015/08/10 22:38:09]

Removed per below comment.

+  // OnToolbarActionAdded is only called after the toolbar model has been
+  // initialized.
+  if (model_->IsKnownActionWithType(action_id,
+      ToolbarActionsModel::ACTION_COMPONENT))
+    return;
+
+  const extensions::Extension* extension =

[Devlin, 2015/08/06 22:04:55]

Can you put a TODO (feel free to put it as me) saying that this is a minor
layering violation, and that the model should pass in an action directly?

[apacible, 2015/08/10 22:38:09]

Done.

+      extensions::ExtensionRegistry::Get(browser_->profile())->
+          enabled_extensions().GetByID(action_id);
+
   toolbar_actions_.insert(
       toolbar_actions_.begin() + index,
       new ExtensionActionViewController(
@@ -553,7 +541,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
@@ -567,15 +555,19 @@ 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())
     return;
 
+  // Component actions cannot be removed.

[Devlin, 2015/08/06 22:04:55]

Again, DCHECK?  Though actually I'm not *too* worried about this and the check
above, since there's a pretty clear flow from the model to here only for
extensions.  If removing them means you can also make IsKnownActionWIthType
private, I think I'd probably prefer that.

[apacible, 2015/08/10 22:38:09]

Removed both DCHECKs, kept IsKnownActionWithType as public for usage in
GetIconCount DCHECKs.

+  if (model_->IsKnownActionWithType(action_id,
+      ToolbarActionsModel::ACTION_COMPONENT))
+    return;
+
   // The action should outlive the UI element (which is owned by the delegate),
   // so we can't delete it just yet. But we should remove it from the list of
   // actions so that any width calculations are correct.
@@ -589,9 +581,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
@@ -610,9 +602,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
@@ -620,9 +611,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) {
@@ -631,14 +621,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);
 }
 
@@ -668,7 +657,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
@@ -704,8 +693,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 ext) {

[Devlin, 2015/08/06 22:04:55]

const &, not just const.  Also, this should probably be more than just "ext" now
(e.g., id)

[apacible, 2015/08/10 22:38:09]

Done.

+    return action->GetId() == ext;
   };
   SortContainer(&toolbar_actions_.get(), model_->toolbar_items(), compare);
 
@@ -740,9 +729,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;