Revert of [Extensions Toolbar] Don't destroy actions when changing highlight mode (patchset #2 id:60001 of https://codereview.chromium.org/1391843003/ )

Reason for revert:
Broke ActiveScriptController tests - crbug.com/543329

Original issue's description:
> [Extensions Toolbar] Don't destroy actions when changing highlight mode
>
> Instead of completely destroying/recreating actions when entering or exiting
> highlight mode, keep any actions possible, remove any unneeded ones, and add
> any missing ones. This should reduce visible noise.
>
> BUG=538802
> TBR=sky@chromium.org (micro page action changes)
>
> Committed: https://crrev.com/a31943c767686003094044df85597f3e84ff2e31
> Cr-Commit-Position: refs/heads/master@{#354090}

TBR=finnur@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=538802

Review URL: https://codereview.chromium.org/1399533006

Cr-Commit-Position: refs/heads/master@{#354143}

14 files changed, 129 insertions, 200 deletions

diff --git a/chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm b/chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm
index 3b34961..a911f4a 100644
--- a/chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm
+++ b/chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm
@@ -40,7 +40,7 @@ PageActionDecoration::PageActionDecoration(
   DCHECK(extension);
 
   viewController_.reset(new ExtensionActionViewController(
-      extension, browser, nullptr));
+      extension, browser, page_action, nullptr));
   viewController_->SetDelegate(this);
 
   // We set the owner last of all so that we can determine whether we are in
diff --git a/chrome/browser/ui/extensions/extension_action_view_controller.cc b/chrome/browser/ui/extensions/extension_action_view_controller.cc
index d37b43b..22decd3 100644
--- a/chrome/browser/ui/extensions/extension_action_view_controller.cc
+++ b/chrome/browser/ui/extensions/extension_action_view_controller.cc
@@ -9,7 +9,6 @@
 #include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/api/extension_action/extension_action_api.h"
 #include "chrome/browser/extensions/extension_action.h"
-#include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/extensions/extension_view.h"
 #include "chrome/browser/extensions/extension_view_host.h"
 #include "chrome/browser/extensions/extension_view_host_factory.h"
@@ -37,25 +36,25 @@ using extensions::CommandService;
 ExtensionActionViewController::ExtensionActionViewController(
     const extensions::Extension* extension,
     Browser* browser,
+    ExtensionAction* extension_action,
     ToolbarActionsBar* toolbar_actions_bar)
     : extension_(extension),
       browser_(browser),
-      extension_action_(
-          extensions::ExtensionActionManager::Get(browser_->profile())->
-              GetExtensionAction(*extension)),
+      extension_action_(extension_action),
       toolbar_actions_bar_(toolbar_actions_bar),
       popup_host_(nullptr),
       view_delegate_(nullptr),
       platform_delegate_(ExtensionActionPlatformDelegate::Create(this)),
-      icon_factory_(browser->profile(), extension, extension_action_, this),
+      icon_factory_(browser->profile(), extension, extension_action, this),
       icon_observer_(nullptr),
       extension_registry_(
           extensions::ExtensionRegistry::Get(browser_->profile())),
       popup_host_observer_(this),
       weak_factory_(this) {
-  DCHECK(extension_action_);
-  DCHECK(extension_action_->action_type() == ActionInfo::TYPE_PAGE ||
-         extension_action_->action_type() == ActionInfo::TYPE_BROWSER);
+  DCHECK(extension_action);
+  DCHECK(extension_action->action_type() == ActionInfo::TYPE_PAGE ||
+         extension_action->action_type() == ActionInfo::TYPE_BROWSER);
+  DCHECK(extension);
 }
 
 ExtensionActionViewController::~ExtensionActionViewController() {
diff --git a/chrome/browser/ui/extensions/extension_action_view_controller.h b/chrome/browser/ui/extensions/extension_action_view_controller.h
index 5996c59..793ce0e 100644
--- a/chrome/browser/ui/extensions/extension_action_view_controller.h
+++ b/chrome/browser/ui/extensions/extension_action_view_controller.h
@@ -43,6 +43,7 @@ class ExtensionActionViewController
 
   ExtensionActionViewController(const extensions::Extension* extension,
                                 Browser* browser,
+                                ExtensionAction* extension_action,
                                 ToolbarActionsBar* toolbar_actions_bar);
   ~ExtensionActionViewController() override;
 
diff --git a/chrome/browser/ui/toolbar/component_toolbar_actions_browsertest.cc b/chrome/browser/ui/toolbar/component_toolbar_actions_browsertest.cc
index 838cd80..51d882b 100644
--- a/chrome/browser/ui/toolbar/component_toolbar_actions_browsertest.cc
+++ b/chrome/browser/ui/toolbar/component_toolbar_actions_browsertest.cc
@@ -49,13 +49,12 @@ IN_PROC_BROWSER_TEST_F(ComponentToolbarActionsBrowserTest,
 
   // Even though the method says "ExtensionId", this actually refers to any id
   // for the action.
-  EXPECT_EQ(MockComponentToolbarActionsFactory::kActionIdForTesting,
+  EXPECT_EQ(ComponentToolbarActionsFactory::kActionIdForTesting,
             browser_actions_bar.GetExtensionId(0));
 
   // There should only have been one created component action.
-  EXPECT_EQ(1u,
-            ComponentToolbarActionsFactory::GetInstance()->GetComponentIds(
-                browser()->profile()).size());
+  const std::vector<std::string>& action_ids = mock_factory()->action_ids();
+  ASSERT_EQ(1u, action_ids.size());
 
   const std::vector<ToolbarActionViewController*>& actions =
       browser_actions_bar.GetToolbarActionsBar()->GetActions();
diff --git a/chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc b/chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc
index cdde653..45720cc 100644
--- a/chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc
+++ b/chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc
@@ -25,6 +25,8 @@ base::LazyInstance<ComponentToolbarActionsFactory> lazy_factory =
 // static
 const char ComponentToolbarActionsFactory::kMediaRouterActionId[] =
     "media_router_action";
+const char ComponentToolbarActionsFactory::kActionIdForTesting[] =
+    "mock_action";
 
 ComponentToolbarActionsFactory::ComponentToolbarActionsFactory() {}
 ComponentToolbarActionsFactory::~ComponentToolbarActionsFactory() {}
@@ -34,29 +36,40 @@ ComponentToolbarActionsFactory* ComponentToolbarActionsFactory::GetInstance() {
   return testing_factory_ ? testing_factory_ : &lazy_factory.Get();
 }
 
-std::set<std::string> ComponentToolbarActionsFactory::GetComponentIds(
-    Profile* profile) {
-  std::set<std::string> component_ids;
+// static
+std::vector<std::string> ComponentToolbarActionsFactory::GetComponentIds() {
+  std::vector<std::string> component_ids;
 
   // This is currently behind the extension-action-redesign flag, as it is
   // designed for the new toolbar.
   if (!extensions::FeatureSwitch::extension_action_redesign()->IsEnabled())
     return component_ids;
 
-  if (switches::MediaRouterEnabled() && !profile->IsOffTheRecord())
-    component_ids.insert(kMediaRouterActionId);
+  if (testing_factory_) {
+    component_ids.push_back(
+        ComponentToolbarActionsFactory::kActionIdForTesting);
+  } else if (switches::MediaRouterEnabled()) {
+    component_ids.push_back(
+        ComponentToolbarActionsFactory::kMediaRouterActionId);
+  }
 
   return component_ids;
 }
 
-scoped_ptr<ToolbarActionViewController>
-ComponentToolbarActionsFactory::GetComponentToolbarActionForId(
-    const std::string& id,
-    Browser* browser) {
+// static
+bool ComponentToolbarActionsFactory::EnabledIncognito(
+    const std::string& action_id) {
+  return action_id != kMediaRouterActionId;
+}
+
+ScopedVector<ToolbarActionViewController>
+ComponentToolbarActionsFactory::GetComponentToolbarActions(Browser* browser) {
+  ScopedVector<ToolbarActionViewController> component_actions;
+
   // This is currently behind the extension-action-redesign flag, as it is
   // designed for the new toolbar.
-  DCHECK(extensions::FeatureSwitch::extension_action_redesign()->IsEnabled());
-  DCHECK(GetComponentIds(browser->profile()).count(id));
+  if (!extensions::FeatureSwitch::extension_action_redesign()->IsEnabled())
+    return component_actions.Pass();
 
   // Add component toolbar actions here.
   // This current design means that the ComponentToolbarActionsFactory is aware
@@ -64,12 +77,11 @@ ComponentToolbarActionsFactory::GetComponentToolbarActionForId(
   // (since each will have an action in the toolbar or overflow menu), this
   // should be okay. If this changes, we should rethink this design to have,
   // e.g., RegisterChromeAction().
-  if (id == kMediaRouterActionId)
-    return scoped_ptr<ToolbarActionViewController>(
-        new MediaRouterAction(browser));
 
-  NOTREACHED();
-  return scoped_ptr<ToolbarActionViewController>();
+  if (switches::MediaRouterEnabled() && !browser->profile()->IsOffTheRecord())
+    component_actions.push_back(new MediaRouterAction(browser));
+
+  return component_actions.Pass();
 }
 
 // static
diff --git a/chrome/browser/ui/toolbar/component_toolbar_actions_factory.h b/chrome/browser/ui/toolbar/component_toolbar_actions_factory.h
index 6eaedc7..0171388 100644
--- a/chrome/browser/ui/toolbar/component_toolbar_actions_factory.h
+++ b/chrome/browser/ui/toolbar/component_toolbar_actions_factory.h
@@ -5,11 +5,8 @@
 #ifndef CHROME_BROWSER_UI_TOOLBAR_COMPONENT_TOOLBAR_ACTIONS_FACTORY_H_
 #define CHROME_BROWSER_UI_TOOLBAR_COMPONENT_TOOLBAR_ACTIONS_FACTORY_H_
 
-#include <set>
-#include <string>
-
 #include "base/macros.h"
-#include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_vector.h"
 
 class Browser;
 class Profile;
@@ -22,6 +19,7 @@ class ComponentToolbarActionsFactory {
  public:
   // Component action IDs.
   static const char kMediaRouterActionId[];
+  static const char kActionIdForTesting[];  // Only used for testing.
 
   ComponentToolbarActionsFactory();
   virtual ~ComponentToolbarActionsFactory();
@@ -29,12 +27,16 @@ class ComponentToolbarActionsFactory {
   static ComponentToolbarActionsFactory* GetInstance();
 
   // Returns a vector of IDs of the component actions.
-  virtual std::set<std::string> GetComponentIds(Profile* profile);
+  static std::vector<std::string> GetComponentIds();
+
+  // Returns true if the component action with |action_id| should be added
+  // in incognito mode.
+  static bool EnabledIncognito(const std::string& action_id);
 
   // Returns a collection of controllers for component actions. Declared
   // virtual for testing.
-  virtual scoped_ptr<ToolbarActionViewController>
-      GetComponentToolbarActionForId(const std::string& id, Browser* browser);
+  virtual ScopedVector<ToolbarActionViewController>
+      GetComponentToolbarActions(Browser* browser);
 
   // Sets the factory to use for testing purposes.
   // Ownership remains with the caller.
diff --git a/chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc b/chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc
index 05ecd87..9694d90 100644
--- a/chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc
+++ b/chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc
@@ -8,32 +8,25 @@
 #include "chrome/browser/ui/toolbar/test_toolbar_action_view_controller.h"
 #include "chrome/browser/ui/toolbar/toolbar_action_view_controller.h"
 
-// static
-const char MockComponentToolbarActionsFactory::kActionIdForTesting[] =
-    "mock_action";
-
 MockComponentToolbarActionsFactory::MockComponentToolbarActionsFactory(
     Browser* browser) {
   ComponentToolbarActionsFactory::SetTestingFactory(this);
+
+  ScopedVector<ToolbarActionViewController> actions =
+      GetComponentToolbarActions(browser);
+  for (const ToolbarActionViewController* action : actions)
+    action_ids_.push_back(action->GetId());
 }
 
 MockComponentToolbarActionsFactory::~MockComponentToolbarActionsFactory() {
   ComponentToolbarActionsFactory::SetTestingFactory(nullptr);
 }
 
-std::set<std::string> MockComponentToolbarActionsFactory::GetComponentIds(
-    Profile* profile) {
-  std::set<std::string> ids;
-  ids.insert(kActionIdForTesting);
-  return ids;
-}
-
-scoped_ptr<ToolbarActionViewController>
-MockComponentToolbarActionsFactory::GetComponentToolbarActionForId(
-    const std::string& id,
+ScopedVector<ToolbarActionViewController>
+MockComponentToolbarActionsFactory::GetComponentToolbarActions(
     Browser* browser) {
-  DCHECK_EQ(kActionIdForTesting, id);
-  return scoped_ptr<ToolbarActionViewController>(
-      new TestToolbarActionViewController(
-          MockComponentToolbarActionsFactory::kActionIdForTesting));
+  ScopedVector<ToolbarActionViewController> component_actions;
+  component_actions.push_back(new TestToolbarActionViewController(
+      ComponentToolbarActionsFactory::kActionIdForTesting));
+  return component_actions.Pass();
 }
diff --git a/chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h b/chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h
index 6770013..abf9d1b 100644
--- a/chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h
+++ b/chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h
@@ -14,18 +14,18 @@ class Browser;
 class MockComponentToolbarActionsFactory
     : public ComponentToolbarActionsFactory {
  public:
-  static const char kActionIdForTesting[];
-
   explicit MockComponentToolbarActionsFactory(Browser* browser);
   ~MockComponentToolbarActionsFactory() override;
 
   // ComponentToolbarActionsFactory:
-  std::set<std::string> GetComponentIds(Profile* profile) override;
-  scoped_ptr<ToolbarActionViewController> GetComponentToolbarActionForId(
-      const std::string& id,
+  ScopedVector<ToolbarActionViewController> GetComponentToolbarActions(
       Browser* browser) override;
 
+  const std::vector<std::string>& action_ids() const { return action_ids_; }
+
  private:
+  std::vector<std::string> action_ids_;
+
   DISALLOW_COPY_AND_ASSIGN(MockComponentToolbarActionsFactory);
 };
 
diff --git a/chrome/browser/ui/toolbar/toolbar_actions_bar.cc b/chrome/browser/ui/toolbar/toolbar_actions_bar.cc
index 6f39b9f..e0c7e08 100644
--- a/chrome/browser/ui/toolbar/toolbar_actions_bar.cc
+++ b/chrome/browser/ui/toolbar/toolbar_actions_bar.cc
@@ -593,6 +593,8 @@ void ToolbarActionsBar::OnToolbarActionAdded(const std::string& action_id,
       new ExtensionActionViewController(
           extension,
           browser_,
+          extensions::ExtensionActionManager::Get(browser_->profile())->
+              GetExtensionAction(*extension),
           this));
 
   delegate_->AddViewForAction(toolbar_actions_[index], index);
@@ -697,40 +699,15 @@ void ToolbarActionsBar::ResizeDelegate(gfx::Tween::Type tween_type,
 void ToolbarActionsBar::OnToolbarHighlightModeChanged(bool is_highlighting) {
   if (!model_->actions_initialized())
     return;
-
-  {
-    base::AutoReset<bool> layout_resetter(&suppress_layout_, true);
-    base::AutoReset<bool> animation_resetter(&suppress_animation_, true);
-    std::set<std::string> seen;
-    for (const ToolbarActionsModel::ToolbarItem item :
-         model_->toolbar_items()) {
-      auto current_pos = std::find_if(
-          toolbar_actions_.begin(),
-          toolbar_actions_.end(),
-          [&item](const ToolbarActionViewController* action) {
-        return action->GetId() == item.id;
-      });
-      if (current_pos == toolbar_actions_.end()) {
-        toolbar_actions_.push_back(
-            model_->CreateActionForItem(browser_, this, item).release());
-        delegate_->AddViewForAction(toolbar_actions_.back(),
-                                    toolbar_actions_.size() - 1);
-      }
-      seen.insert(item.id);
-    }
-
-    for (ToolbarActions::iterator iter = toolbar_actions_.begin();
-         iter != toolbar_actions_.end();) {
-      if (seen.count((*iter)->GetId()) == 0) {
-        delegate_->RemoveViewForAction(*iter);
-        iter = toolbar_actions_.erase(iter);
-      } else {
-        ++iter;
-      }
-    }
-  }
-
-  ReorderActions();
+  // 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
+  // appearing, etc), it's not worth the extra complexity to create and insert
+  // only the new actions.
+  DeleteActions();
+  CreateActions();
+  // Resize the delegate. We suppress the chevron so that we don't risk showing
+  // it only for the duration of the animation.
+  ResizeDelegate(gfx::Tween::LINEAR, true);
 }
 
 void ToolbarActionsBar::OnToolbarModelInitialized() {
diff --git a/chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc b/chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc
index 7568248..0f1907d 100644
--- a/chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc
+++ b/chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc
@@ -333,73 +333,6 @@ TEST_F(ToolbarActionsBarUnitTest, ToolbarActionsReorderOnPrefChange) {
   }
 }
 
-TEST_F(ToolbarActionsBarUnitTest, TestHighlightMode) {
-  std::vector<std::string> ids;
-  for (int i = 0; i < 3; ++i) {
-    ids.push_back(CreateAndAddExtension(
-        base::StringPrintf("extension %d", i),
-        extensions::extension_action_test_util::BROWSER_ACTION)->id());
-  }
-  EXPECT_EQ(3u, toolbar_actions_bar()->GetIconCount());
-  const char kExtension0[] = "extension 0";
-  const char kExtension1[] = "extension 1";
-  const char kExtension2[] = "extension 2";
-
-  {
-    // The order should start as 0, 1, 2.
-    const char* expected_names[] = { kExtension0, kExtension1, kExtension2 };
-    EXPECT_TRUE(VerifyToolbarOrder(expected_names, 3u, 3u));
-  }
-
-  std::vector<std::string> ids_to_highlight;
-  ids_to_highlight.push_back(ids[0]);
-  ids_to_highlight.push_back(ids[2]);
-  toolbar_model()->HighlightActions(ids_to_highlight,
-                                    ToolbarActionsModel::HIGHLIGHT_WARNING);
-
-  {
-    // The order should now be 0, 2, since 1 is not being highlighted.
-    const char* expected_names[] = { kExtension0, kExtension2 };
-    EXPECT_TRUE(VerifyToolbarOrder(expected_names, 2u, 2u));
-  }
-
-  toolbar_model()->StopHighlighting();
-
-  {
-    // The order should go back to normal.
-    const char* expected_names[] = { kExtension0, kExtension1, kExtension2 };
-    EXPECT_TRUE(VerifyToolbarOrder(expected_names, 3u, 3u));
-  }
-
-  ids_to_highlight.push_back(ids[1]);
-  toolbar_model()->HighlightActions(ids_to_highlight,
-                                    ToolbarActionsModel::HIGHLIGHT_WARNING);
-  {
-    // All actions should be highlighted (in the order of the vector passed in,
-    // so with '1' at the end).
-    const char* expected_names[] = { kExtension0, kExtension2, kExtension1 };
-    EXPECT_TRUE(VerifyToolbarOrder(expected_names, 3u, 3u));
-  }
-
-  ids_to_highlight.clear();
-  ids_to_highlight.push_back(ids[1]);
-  toolbar_model()->HighlightActions(ids_to_highlight,
-                                    ToolbarActionsModel::HIGHLIGHT_WARNING);
-
-  {
-    // Only extension 1 should be visible.
-    const char* expected_names[] = { kExtension1 };
-    EXPECT_TRUE(VerifyToolbarOrder(expected_names, 1u, 1u));
-  }
-
-  toolbar_model()->StopHighlighting();
-  {
-    // And, again, back to normal.
-    const char* expected_names[] = { kExtension0, kExtension1, kExtension2 };
-    EXPECT_TRUE(VerifyToolbarOrder(expected_names, 3u, 3u));
-  }
-}
-
 TEST_F(ToolbarActionsBarRedesignUnitTest, IconSurfacingBubbleAppearance) {
   // Without showing anything new, we shouldn't show the bubble, and should
   // auto-acknowledge it.
diff --git a/chrome/browser/ui/toolbar/toolbar_actions_model.cc b/chrome/browser/ui/toolbar/toolbar_actions_model.cc
index 2adcd25..e45fcb3 100644
--- a/chrome/browser/ui/toolbar/toolbar_actions_model.cc
+++ b/chrome/browser/ui/toolbar/toolbar_actions_model.cc
@@ -158,40 +158,58 @@ ScopedVector<ToolbarActionViewController> ToolbarActionsModel::CreateActions(
   DCHECK(bar);
   ScopedVector<ToolbarActionViewController> action_list;
 
-  // toolbar_items() might not equate to toolbar_items_ in the case where a
-  // subset is highlighted.
-  for (const ToolbarItem& item : toolbar_items())
-    action_list.push_back(CreateActionForItem(browser, bar, item).release());
+  // Get the component action list.
+  ScopedVector<ToolbarActionViewController> component_actions =
+      ComponentToolbarActionsFactory::GetInstance()->GetComponentToolbarActions(
+          browser);
 
-  return action_list.Pass();
-}
+  extensions::ExtensionActionManager* action_manager =
+      extensions::ExtensionActionManager::Get(profile_);
 
-scoped_ptr<ToolbarActionViewController>
-ToolbarActionsModel::CreateActionForItem(Browser* browser,
-                                         ToolbarActionsBar* bar,
-                                         const ToolbarItem& item) {
-  scoped_ptr<ToolbarActionViewController> result;
-  switch (item.type) {
-    case EXTENSION_ACTION: {
-      // Get the extension.
-      const extensions::Extension* extension = GetExtensionById(item.id);
-      DCHECK(extension);
-
-      // Create and add an ExtensionActionViewController for the extension.
-      result.reset(new ExtensionActionViewController(extension, browser, bar));
-      break;
-    }
-    case COMPONENT_ACTION: {
-      DCHECK(use_redesign_);
-      result = ComponentToolbarActionsFactory::GetInstance()->
-          GetComponentToolbarActionForId(item.id, browser).Pass();
-      break;
+  // toolbar_items() might not equate to toolbar_items_ in the case where a
+  // subset are highlighted.
+  for (const ToolbarItem& item : toolbar_items()) {
+    switch (item.type) {
+      case EXTENSION_ACTION: {
+        // Get the extension.
+        const extensions::Extension* extension = GetExtensionById(item.id);
+        DCHECK(extension);
+
+        // Create and add an ExtensionActionViewController for the extension.
+        action_list.push_back(new ExtensionActionViewController(
+            extension, browser, action_manager->GetExtensionAction(*extension),
+            bar));
+        break;
+      }
+      case COMPONENT_ACTION: {
+        DCHECK(use_redesign_);
+        // Find the index of the component action with the id.
+        auto iter = std::find_if(
+            component_actions.begin(), component_actions.end(),
+            [&item](const ToolbarActionViewController* action) {
+          return action->GetId() == item.id;
+        });
+        // We should always find a corresponding action.
+        DCHECK(iter != component_actions.end());
+        action_list.push_back(*iter);
+
+        // We have moved ownership of the action from |component_actions| to
+        // |action_list|.
+        component_actions.weak_erase(iter);
+        break;
+      }
+      case UNKNOWN_ACTION:
+        NOTREACHED();  // Should never have an UNKNOWN_ACTION in toolbar_items.
+        break;
     }
-    case UNKNOWN_ACTION:
-      NOTREACHED();  // Should never have an UNKNOWN_ACTION in toolbar_items.
-      break;
   }
-  return result.Pass();
+
+  // We've moved ownership of the subset of the component actions that we
+  // kept track of via toolbar_items() from |component_actions| to
+  // |action_list|. The rest will be deleted when |component_actions| goes out
+  // of scope.
+
+  return action_list.Pass();
 }
 
 void ToolbarActionsModel::OnExtensionActionVisibilityChanged(
@@ -469,8 +487,8 @@ void ToolbarActionsModel::Populate() {
   }
 
   // Next, add the component action ids.
-  std::set<std::string> component_ids =
-      ComponentToolbarActionsFactory::GetInstance()->GetComponentIds(profile_);
+  std::vector<std::string> component_ids =
+      ComponentToolbarActionsFactory::GetComponentIds();
   for (const std::string& id : component_ids)
     all_actions.push_back(ToolbarItem(id, COMPONENT_ACTION));
 
@@ -573,8 +591,6 @@ void ToolbarActionsModel::IncognitoPopulate() {
   // overflowed. Order is the same as in regular mode.
   visible_icon_count_ = 0;
 
-  std::set<std::string> component_ids =
-      ComponentToolbarActionsFactory::GetInstance()->GetComponentIds(profile_);
   for (std::vector<ToolbarItem>::const_iterator iter =
            original_model->toolbar_items_.begin();
        iter != original_model->toolbar_items_.end(); ++iter) {
@@ -586,9 +602,7 @@ void ToolbarActionsModel::IncognitoPopulate() {
         should_add = ShouldAddExtension(GetExtensionById(iter->id));
         break;
       case COMPONENT_ACTION:
-        // The component action factory only returns actions that should be
-        // added.
-        should_add = component_ids.count(iter->id) != 0;
+        should_add = ComponentToolbarActionsFactory::EnabledIncognito(iter->id);
         break;
       case UNKNOWN_ACTION:
         // We should never have an uninitialized action in the model.
diff --git a/chrome/browser/ui/toolbar/toolbar_actions_model.h b/chrome/browser/ui/toolbar/toolbar_actions_model.h
index 690c67a..3facd79 100644
--- a/chrome/browser/ui/toolbar/toolbar_actions_model.h
+++ b/chrome/browser/ui/toolbar/toolbar_actions_model.h
@@ -150,8 +150,6 @@ class ToolbarActionsModel : public extensions::ExtensionActionAPI::Observer,
   ScopedVector<ToolbarActionViewController> CreateActions(
       Browser* browser,
       ToolbarActionsBar* bar);
-  scoped_ptr<ToolbarActionViewController> CreateActionForItem(
-      Browser* browser, ToolbarActionsBar* bar, const ToolbarItem& item);
 
   const std::vector<ToolbarItem>& toolbar_items() const {
     return is_highlighting() ? highlighted_items_ : toolbar_items_;
diff --git a/chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc b/chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc
index 787e5da..3c46222 100644
--- a/chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc
+++ b/chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc
@@ -172,7 +172,7 @@ class ToolbarActionsModelUnitTest
 
   // The mock component action will be referred to as "MCA" below.
   const char* component_action_id() {
-    return MockComponentToolbarActionsFactory::kActionIdForTesting;
+    return ComponentToolbarActionsFactory::kActionIdForTesting;
   }
 
  private:
diff --git a/chrome/browser/ui/views/location_bar/page_action_image_view.cc b/chrome/browser/ui/views/location_bar/page_action_image_view.cc
index 0d4efe9..1ba65d4 100644
--- a/chrome/browser/ui/views/location_bar/page_action_image_view.cc
+++ b/chrome/browser/ui/views/location_bar/page_action_image_view.cc
@@ -30,6 +30,7 @@ PageActionImageView::PageActionImageView(LocationBarView* owner,
           extensions::ExtensionRegistry::Get(browser->profile())->
               enabled_extensions().GetByID(page_action->extension_id()),
           browser,
+          page_action,
           nullptr)),
       owner_(owner),
       preview_enabled_(false) {