Make ToolbarActionsModel own ComponentToolbarActionsFactory

chrome/browser/ui/toolbar/toolbar_actions_model.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_UI_TOOLBAR_TOOLBAR_ACTIONS_MODEL_H_
 #define CHROME_BROWSER_UI_TOOLBAR_TOOLBAR_ACTIONS_MODEL_H_
 
 #include <stddef.h>
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/observer_list.h"
 #include "base/scoped_observer.h"
 #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"
 #include "extensions/browser/extension_registry_observer.h"
 #include "extensions/common/extension.h"
 
 class Browser;
 class PrefService;
 class Profile;
 class ToolbarActionsBar;
@@ skipping 99 matching lines @@
   void RemoveObserver(Observer* observer);
 
   // Moves the given action with |id|'s icon to the given |index|.
   void MoveActionIcon(const std::string& id, size_t index);
 
   // Sets the number of action icons that should be visible.
   // If count == size(), this will set the visible icon count to -1, meaning
   // "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.
   size_t visible_icon_count() const {
     // We have guards around this because |visible_icon_count_| can be set by
     // prefs/sync, and we want to ensure that the icon count returned is within
     // bounds.
     return visible_icon_count_ == -1
                ? toolbar_items().size()
                : std::min(static_cast<size_t>(visible_icon_count_),
@@ skipping 10 matching lines @@
       ToolbarActionsBar* bar);
   std::unique_ptr<ToolbarActionViewController> CreateActionForItem(
       Browser* browser,
       ToolbarActionsBar* bar,
       const ToolbarItem& item);
 
   const std::vector<ToolbarItem>& toolbar_items() const {
     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_; }
 
   bool has_active_bubble() const { return has_active_bubble_; }
   void set_has_active_bubble(bool has_active_bubble) {
     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:
   // AddComponentAction() is a no-op if |actions_initialized_| is false.
   void AddComponentAction(const std::string& action_id) override;
   void RemoveComponentAction(const std::string& action_id) override;
   bool HasComponentAction(const std::string& action_id) const override;
 
   void OnActionToolbarPrefChange();
 
   // Highlights the actions specified by |action_ids|. This will cause
   // the ToolbarModel to only display those actions.
   // Highlighting mode is only entered if there is at least one action to be
   // shown.
   // Returns true if highlighting mode is entered, false otherwise.
   bool HighlightActions(const std::vector<std::string>& action_ids,
                         HighlightType type);
 
   // Stop highlighting actions. All actions can be shown again, and the
   // number of visible icons will be reset to what it was before highlighting.
   void StopHighlighting();
 
   // Gets the ExtensionMessageBubbleController that should be shown for this
   // profile, if any.
   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();
 
   // ExtensionRegistryObserver:
   void OnExtensionLoaded(content::BrowserContext* browser_context,
                          const extensions::Extension* extension) override;
   void OnExtensionUnloaded(
       content::BrowserContext* browser_context,
       const extensions::Extension* extension,
@@ skipping 69 matching lines @@
 
   // The ExtensionActionAPI object, cached for convenience.
   extensions::ExtensionActionAPI* extension_action_api_;
 
   // The ExtensionRegistry object, cached for convenience.
   extensions::ExtensionRegistry* extension_registry_;
 
   // 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_;
 
   // If true, we include all actions in the toolbar model.
   bool use_redesign_;
 
   // Ordered list of browser actions.
   std::vector<ToolbarItem> toolbar_items_;
 
@@ skipping 31 matching lines @@
   // For observing change of toolbar order preference by external entity (sync).
   PrefChangeRegistrar pref_change_registrar_;
   base::Closure pref_change_callback_;
 
   base::WeakPtrFactory<ToolbarActionsModel> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ToolbarActionsModel);
 };
 
 #endif  // CHROME_BROWSER_UI_TOOLBAR_TOOLBAR_ACTIONS_MODEL_H_