Add extension pinning to extensions menu

Adds a syncable extension preference for pinned extensions to keep them
in sync. This is used in place of the previous "visible extensions"
paradigm.

Notably extensions are not pinned by default, whereas before they were
initially visible on the toolbar.

Bug: chromium:959932
Change-Id: I23fa0d5314317c079841540e18449646f4da9076
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1598818
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#665682}

chrome/browser/ui/toolbar/toolbar_actions_bar.cc

@@ -794,6 +794,10 @@ void ToolbarActionsBar::OnToolbarModelInitialized() {
   ResizeDelegate(gfx::Tween::EASE_OUT);
 }
 
+void ToolbarActionsBar::OnToolbarPinnedActionsChanged() {
+  NOTREACHED();
+}
+
 void ToolbarActionsBar::OnTabStripModelChanged(
     TabStripModel* tab_strip_model,
     const TabStripModelChange& change,

chrome/browser/ui/toolbar/toolbar_actions_bar.h

@@ -270,6 +270,7 @@ class ToolbarActionsBar : public ExtensionsContainer,
   void OnToolbarVisibleCountChanged() override;
   void OnToolbarHighlightModeChanged(bool is_highlighting) override;
   void OnToolbarModelInitialized() override;
+  void OnToolbarPinnedActionsChanged() override;
 
   // TabStripModelObserver:
   void OnTabStripModelChanged(

chrome/browser/ui/toolbar/toolbar_actions_model.cc

@@ -65,14 +65,25 @@ ToolbarActionsModel::ToolbarActionsModel(
   visible_icon_count_ =
       prefs_->GetInteger(extensions::pref_names::kToolbarSize);
 
-  // We only care about watching the prefs if not in incognito mode.
-  if (!profile_->IsOffTheRecord()) {
+  // We only care about watching toolbar-order prefs if not in incognito mode.
+  const bool watch_toolbar_order = !profile_->IsOffTheRecord();
+  const bool watch_pinned_extensions =
+      base::FeatureList::IsEnabled(features::kExtensionsToolbarMenu);
+  if (watch_toolbar_order || watch_pinned_extensions) {
     pref_change_registrar_.Init(prefs_);
     pref_change_callback_ =
         base::Bind(&ToolbarActionsModel::OnActionToolbarPrefChange,
                    base::Unretained(this));
-    pref_change_registrar_.Add(extensions::pref_names::kToolbar,
-                               pref_change_callback_);
+
+    if (watch_toolbar_order) {
+      pref_change_registrar_.Add(extensions::pref_names::kToolbar,
+                                 pref_change_callback_);
+    }
+
+    if (watch_pinned_extensions) {
+      pref_change_registrar_.Add(extensions::pref_names::kPinnedExtensions,
+                                 pref_change_callback_);
+    }
   }
 }
 
@@ -424,6 +435,11 @@ ToolbarActionsModel::GetExtensionMessageBubbleController(Browser* browser) {
   return controller;
 }
 
+bool ToolbarActionsModel::IsActionPinned(const ActionId& action_id) const {
+  DCHECK(base::FeatureList::IsEnabled(features::kExtensionsToolbarMenu));
+  return base::ContainsValue(pinned_action_ids_, action_id);
+}
+
 void ToolbarActionsModel::RemoveExtension(
     const extensions::Extension* extension) {
   RemoveAction(extension->id());
@@ -440,6 +456,9 @@ void ToolbarActionsModel::InitializeActionList() {
   CHECK(action_ids_.empty());  // We shouldn't have any actions yet.
 
   last_known_positions_ = extension_prefs_->GetToolbarOrder();
+  if (base::FeatureList::IsEnabled(features::kExtensionsToolbarMenu))
+    pinned_action_ids_ = extension_prefs_->GetPinnedExtensions();
+
   if (profile_->IsOffTheRecord())
     IncognitoPopulate();
   else
@@ -614,7 +633,17 @@ void ToolbarActionsModel::UpdatePrefs() {
 void ToolbarActionsModel::SetActionVisibility(const ActionId& action_id,
                                               bool is_now_visible) {
   if (base::FeatureList::IsEnabled(features::kExtensionsToolbarMenu)) {
-    // TODO(pbos): Add extension pinning using a vector of pinned action IDs.
+    DCHECK_NE(is_now_visible, IsActionPinned(action_id));
+    auto new_pinned_action_ids = pinned_action_ids_;
+    if (is_now_visible) {
+      new_pinned_action_ids.push_back(action_id);
+    } else {
+      base::Erase(new_pinned_action_ids, action_id);
+    }
+    extension_prefs_->SetPinnedExtensions(new_pinned_action_ids);
+    // The |pinned_action_ids_| should be updated as a result of updating the
+    // preference.
+    DCHECK(pinned_action_ids_ == new_pinned_action_ids);
     return;
   }
 
@@ -645,6 +674,16 @@ void ToolbarActionsModel::OnActionToolbarPrefChange() {
   if (!actions_initialized_)
     return;
 
+  if (base::FeatureList::IsEnabled(features::kExtensionsToolbarMenu)) {
+    std::vector<ActionId> pinned_extensions =
+        extension_prefs_->GetPinnedExtensions();
+    if (pinned_extensions != pinned_action_ids_) {
+      pinned_action_ids_ = pinned_extensions;
+      for (Observer& observer : observers_)
+        observer.OnToolbarPinnedActionsChanged();
+    }
+  }
+
   // Recalculate |last_known_positions_| to be |pref_positions| followed by
   // ones that are only in |last_known_positions_|.
   std::vector<ActionId> pref_positions = extension_prefs_->GetToolbarOrder();

chrome/browser/ui/toolbar/toolbar_actions_model.h

@@ -101,6 +101,9 @@ class ToolbarActionsModel : public extensions::ExtensionActionAPI::Observer,
     // can catch up.
     virtual void OnToolbarModelInitialized() = 0;
 
+    // Called whenever the pinned actions change.
+    virtual void OnToolbarPinnedActionsChanged() = 0;
+
    protected:
     virtual ~Observer() {}
   };
@@ -181,6 +184,14 @@ class ToolbarActionsModel : public extensions::ExtensionActionAPI::Observer,
   std::unique_ptr<extensions::ExtensionMessageBubbleController>
   GetExtensionMessageBubbleController(Browser* browser);
 
+  // Returns true if the action is pinned to the toolbar.
+  bool IsActionPinned(const ActionId& action_id) const;
+
+  // Returns the ordered list of ids of pinned actions.
+  const std::vector<ActionId>& pinned_action_ids() const {
+    return pinned_action_ids_;
+  }
+
  private:
   // Callback when actions are ready.
   void OnReady();
@@ -279,6 +290,9 @@ class ToolbarActionsModel : public extensions::ExtensionActionAPI::Observer,
   // List of browser action IDs which should be highlighted.
   std::vector<ActionId> highlighted_action_ids_;
 
+  // Set of pinned action IDs.
+  std::vector<ActionId> pinned_action_ids_;
+
   // The current type of highlight (with HIGHLIGHT_NONE indicating no current
   // highlight).
   HighlightType highlight_type_;

chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc

@@ -16,6 +16,7 @@
 #include "base/stl_util.h"
 #include "base/strings/string16.h"
 #include "base/strings/stringprintf.h"
+#include "base/test/scoped_feature_list.h"
 #include "chrome/browser/extensions/api/extension_action/extension_action_api.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/extensions/extension_action_test_util.h"
@@ -27,6 +28,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/session_tab_helper.h"
 #include "chrome/browser/ui/toolbar/test_toolbar_action_view_controller.h"
+#include "chrome/browser/ui/ui_features.h"
 #include "chrome/common/extensions/api/extension_action/action_info.h"
 #include "chrome/common/pref_names.h"
 #include "components/crx_file/id_util.h"
@@ -44,6 +46,7 @@
 #include "extensions/common/manifest.h"
 #include "extensions/common/value_builder.h"
 #include "extensions/test/test_extension_dir.h"
+#include "testing/gmock/include/gmock/gmock.h"
 
 namespace {
 
@@ -61,6 +64,11 @@ class ToolbarActionsModelTestObserver : public ToolbarActionsModel::Observer {
   int highlight_mode_count() const { return highlight_mode_count_; }
   size_t initialized_count() const { return initialized_count_; }
 
+  const std::vector<ToolbarActionsModel::ActionId>& last_pinned_action_ids()
+      const {
+    return last_pinned_action_ids_;
+  }
+
  private:
   // ToolbarActionsModel::Observer:
   void OnToolbarActionAdded(const ToolbarActionsModel::ActionId& action_id,
@@ -92,7 +100,11 @@ class ToolbarActionsModelTestObserver : public ToolbarActionsModel::Observer {
 
   void OnToolbarModelInitialized() override { ++initialized_count_; }
 
-  ToolbarActionsModel* model_;
+  void OnToolbarPinnedActionsChanged() override {
+    last_pinned_action_ids_ = model_->pinned_action_ids();
+  }
+
+  ToolbarActionsModel* const model_;
 
   size_t inserted_count_;
   size_t removed_count_;
@@ -101,6 +113,8 @@ class ToolbarActionsModelTestObserver : public ToolbarActionsModel::Observer {
   int highlight_mode_count_;
   size_t initialized_count_;
 
+  std::vector<ToolbarActionsModel::ActionId> last_pinned_action_ids_;
+
   DISALLOW_COPY_AND_ASSIGN(ToolbarActionsModelTestObserver);
 };
 
@@ -1208,3 +1222,142 @@ TEST_F(ToolbarActionsModelUnitTest, AddUserScriptExtension) {
   EXPECT_EQ(1u, toolbar_model()->visible_icon_count());
   EXPECT_EQ(extension.get()->id(), GetActionIdAtIndex(0u));
 }
+
+TEST_F(ToolbarActionsModelUnitTest, IsActionPinnedCorrespondsToPinningState) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kExtensionsToolbarMenu);
+
+  Init();
+  ASSERT_TRUE(AddBrowserActionExtensions());
+
+  // The actions should initially not be pinned.
+  EXPECT_FALSE(toolbar_model()->IsActionPinned(browser_action_a()->id()));
+
+  // Pinning is reflected in |IsActionPinned|.
+  toolbar_model()->SetActionVisibility(browser_action_a()->id(), true);
+  EXPECT_TRUE(toolbar_model()->IsActionPinned(browser_action_a()->id()));
+
+  // Removing pinning should also be reflected in |IsActionPinned|.
+  toolbar_model()->SetActionVisibility(browser_action_a()->id(), false);
+  EXPECT_FALSE(toolbar_model()->IsActionPinned(browser_action_a()->id()));
+}
+
+TEST_F(ToolbarActionsModelUnitTest,
+       TogglingVisibilityAppendsToPinnedExtensions) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kExtensionsToolbarMenu);
+
+  Init();
+  ASSERT_TRUE(AddBrowserActionExtensions());
+
+  EXPECT_THAT(toolbar_model()->pinned_action_ids(), testing::IsEmpty());
+
+  toolbar_model()->SetActionVisibility(browser_action_a()->id(), true);
+  EXPECT_THAT(toolbar_model()->pinned_action_ids(),
+              testing::ElementsAre(browser_action_a()->id()));
+
+  // Pin the remaining two extensions.
+  toolbar_model()->SetActionVisibility(browser_action_b()->id(), true);
+  toolbar_model()->SetActionVisibility(browser_action_c()->id(), true);
+
+  // Verify that they are added in order.
+  EXPECT_THAT(
+      toolbar_model()->pinned_action_ids(),
+      testing::ElementsAre(browser_action_a()->id(), browser_action_b()->id(),
+                           browser_action_c()->id()));
+
+  // Verify that unpinning an extension updates the list of pinned ids.
+  toolbar_model()->SetActionVisibility(browser_action_b()->id(), false);
+  EXPECT_THAT(
+      toolbar_model()->pinned_action_ids(),
+      testing::ElementsAre(browser_action_a()->id(), browser_action_c()->id()));
+
+  // Verify that re-pinning an extension adds it back to the end of the list.
+  toolbar_model()->SetActionVisibility(browser_action_b()->id(), true);
+  EXPECT_THAT(
+      toolbar_model()->pinned_action_ids(),
+      testing::ElementsAre(browser_action_a()->id(), browser_action_c()->id(),
+                           browser_action_b()->id()));
+}
+
+TEST_F(ToolbarActionsModelUnitTest, ChangesToPinningNotifiesObserver) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kExtensionsToolbarMenu);
+
+  Init();
+  ASSERT_TRUE(AddBrowserActionExtensions());
+
+  // The observer should not think that any extensions are initially pinned.
+  EXPECT_THAT(observer()->last_pinned_action_ids(), testing::IsEmpty());
+
+  // Verify that pinning the action notifies the observer.
+  toolbar_model()->SetActionVisibility(browser_action_a()->id(), true);
+  EXPECT_THAT(observer()->last_pinned_action_ids(),
+              testing::ElementsAre(browser_action_a()->id()));
+
+  // Verify that un-pinning an action also notifies the observer.
+  toolbar_model()->SetActionVisibility(browser_action_a()->id(), false);
+  EXPECT_THAT(observer()->last_pinned_action_ids(), testing::IsEmpty());
+}
+
+TEST_F(ToolbarActionsModelUnitTest, ChangesToPinningSavedInExtensionPrefs) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kExtensionsToolbarMenu);
+
+  Init();
+  ASSERT_TRUE(AddBrowserActionExtensions());
+
+  extensions::ExtensionPrefs* const extension_prefs =
+      extensions::ExtensionPrefs::Get(profile());
+
+  // The preferences shouldn't have any extensions initially pinned.
+  EXPECT_THAT(extension_prefs->GetPinnedExtensions(), testing::IsEmpty());
+
+  // Verify that pinned extensions are reflected in preferences.
+  toolbar_model()->SetActionVisibility(browser_action_a()->id(), true);
+  toolbar_model()->SetActionVisibility(browser_action_b()->id(), true);
+  toolbar_model()->SetActionVisibility(browser_action_c()->id(), true);
+  EXPECT_THAT(
+      extension_prefs->GetPinnedExtensions(),
+      testing::ElementsAre(browser_action_a()->id(), browser_action_b()->id(),
+                           browser_action_c()->id()));
+
+  // Verify that un-pinning an action is also reflected in preferences.
+  toolbar_model()->SetActionVisibility(browser_action_b()->id(), false);
+  EXPECT_THAT(
+      extension_prefs->GetPinnedExtensions(),
+      testing::ElementsAre(browser_action_a()->id(), browser_action_c()->id()));
+
+  // Verify that re-pinning is added last.
+  toolbar_model()->SetActionVisibility(browser_action_b()->id(), true);
+  EXPECT_THAT(
+      extension_prefs->GetPinnedExtensions(),
+      testing::ElementsAre(browser_action_a()->id(), browser_action_c()->id(),
+                           browser_action_b()->id()));
+}
+
+TEST_F(ToolbarActionsModelUnitTest, ChangesToExtensionPrefsReflectedInModel) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kExtensionsToolbarMenu);
+
+  Init();
+  ASSERT_TRUE(AddBrowserActionExtensions());
+
+  extensions::ExtensionPrefs* const extension_prefs =
+      extensions::ExtensionPrefs::Get(profile());
+
+  // No actions should be initially pinned.
+  EXPECT_THAT(toolbar_model()->pinned_action_ids(), testing::IsEmpty());
+
+  // Update preferences to indicate that extensions A and C are pinned.
+  extensions::ExtensionIdList pinned_extension_list = {
+      browser_action_a()->id(), browser_action_c()->id()};
+
+  // Verify that setting the extension preferences updates the model.
+  extension_prefs->SetPinnedExtensions(pinned_extension_list);
+  EXPECT_EQ(pinned_extension_list, extension_prefs->GetPinnedExtensions());
+  EXPECT_EQ(pinned_extension_list, toolbar_model()->pinned_action_ids());
+
+  // Verify that the observer is notified as well.
+  EXPECT_EQ(pinned_extension_list, observer()->last_pinned_action_ids());
+}

chrome/browser/ui/views/extensions/extensions_menu_view.cc

@@ -199,32 +199,43 @@ void ExtensionsMenuView::OnToolbarActionAdded(
     int index) {
   Repopulate();
 }
+
 void ExtensionsMenuView::OnToolbarActionRemoved(
     const ToolbarActionsModel::ActionId& action_id) {
   Repopulate();
 }
+
 void ExtensionsMenuView::OnToolbarActionMoved(
     const ToolbarActionsModel::ActionId& action_id,
     int index) {
   Repopulate();
 }
+
 void ExtensionsMenuView::OnToolbarActionLoadFailed() {
   Repopulate();
 }
+
 void ExtensionsMenuView::OnToolbarActionUpdated(
     const ToolbarActionsModel::ActionId& action_id) {
   Repopulate();
 }
+
 void ExtensionsMenuView::OnToolbarVisibleCountChanged() {
   Repopulate();
 }
+
 void ExtensionsMenuView::OnToolbarHighlightModeChanged(bool is_highlighting) {
   Repopulate();
 }
+
 void ExtensionsMenuView::OnToolbarModelInitialized() {
   Repopulate();
 }
 
+void ExtensionsMenuView::OnToolbarPinnedActionsChanged() {
+  Repopulate();
+}
+
 // static
 void ExtensionsMenuView::ShowBubble(views::View* anchor_view,
                                     Browser* browser,

chrome/browser/ui/views/extensions/extensions_menu_view.h

@@ -60,6 +60,7 @@ class ExtensionsMenuView : public views::ButtonListener,
   void OnToolbarVisibleCountChanged() override;
   void OnToolbarHighlightModeChanged(bool is_highlighting) override;
   void OnToolbarModelInitialized() override;
+  void OnToolbarPinnedActionsChanged() override;
 
   views::View* extension_menu_button_container_for_testing() {
     return extension_menu_button_container_for_testing_;

chrome/browser/ui/views/extensions/extensions_toolbar_container.cc

@@ -43,7 +43,8 @@ ToolbarActionViewController* ExtensionsToolbarContainer::GetPoppedOutAction()
 
 bool ExtensionsToolbarContainer::IsActionVisibleOnToolbar(
     const ToolbarActionViewController* action) const {
-  return false;
+  return model_->IsActionPinned(action->GetId()) ||
+         action == popped_out_action_;
 }
 
 void ExtensionsToolbarContainer::UndoPopOut() {
@@ -135,8 +136,17 @@ void ExtensionsToolbarContainer::OnToolbarModelInitialized() {
   CreateActions();
 }
 
+void ExtensionsToolbarContainer::OnToolbarPinnedActionsChanged() {
+  for (auto& it : icons_)
+    it.second->SetVisible(IsActionVisibleOnToolbar(GetActionForId(it.first)));
+  ReorderViews();
+}
+
 void ExtensionsToolbarContainer::ReorderViews() {
   // TODO(pbos): Reorder pinned actions here once they exist.
+  const auto& pinned_action_ids = model_->pinned_action_ids();
+  for (size_t i = 0; i < pinned_action_ids.size(); ++i)
+    ReorderChildView(icons_[pinned_action_ids[i]].get(), i);
 
   // Popped out actions should be at the end.
   if (popped_out_action_)

chrome/browser/ui/views/extensions/extensions_toolbar_container.h

@@ -81,6 +81,7 @@ class ExtensionsToolbarContainer : public ToolbarIconContainerView,
   void OnToolbarVisibleCountChanged() override;
   void OnToolbarHighlightModeChanged(bool is_highlighting) override;
   void OnToolbarModelInitialized() override;
+  void OnToolbarPinnedActionsChanged() override;
 
   // ToolbarActionView::Delegate:
   content::WebContents* GetCurrentWebContents() override;

extensions/browser/extension_prefs.cc

@@ -1130,6 +1130,17 @@ void ExtensionPrefs::SetToolbarOrder(const ExtensionIdList& extension_ids) {
   SetExtensionPrefFromContainer(pref_names::kToolbar, extension_ids);
 }
 
+ExtensionIdList ExtensionPrefs::GetPinnedExtensions() const {
+  ExtensionIdList id_list_out;
+  GetUserExtensionPrefIntoContainer(pref_names::kPinnedExtensions,
+                                    &id_list_out);
+  return id_list_out;
+}
+
+void ExtensionPrefs::SetPinnedExtensions(const ExtensionIdList& extension_ids) {
+  SetExtensionPrefFromContainer(pref_names::kPinnedExtensions, extension_ids);
+}
+
 void ExtensionPrefs::OnExtensionInstalled(
     const Extension* extension,
     Extension::State initial_state,
@@ -1858,6 +1869,8 @@ void ExtensionPrefs::RegisterProfilePrefs(
   registry->RegisterDictionaryPref(pref_names::kExtensions);
   registry->RegisterListPref(pref_names::kToolbar,
                              user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterListPref(pref_names::kPinnedExtensions,
+                             user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
   registry->RegisterIntegerPref(pref_names::kToolbarSize, -1);
   registry->RegisterDictionaryPref(kExtensionsBlacklistUpdate);
   registry->RegisterListPref(pref_names::kInstallAllowList);

extensions/browser/extension_prefs.h

@@ -186,6 +186,13 @@ class ExtensionPrefs : public KeyedService {
   ExtensionIdList GetToolbarOrder() const;
   void SetToolbarOrder(const ExtensionIdList& extension_ids);
 
+  // Get/Set the set of extensions that are pinned to the toolbar. Only used
+  // when the experiment ExtensionsMenu is active."
+  // TODO(crbug.com/943702): Remove reference to experiment when it launches or
+  // remove code if it does not.
+  ExtensionIdList GetPinnedExtensions() const;
+  void SetPinnedExtensions(const ExtensionIdList& extension_ids);
+
   // Called when an extension is installed, so that prefs get created.
   // If |page_ordinal| is invalid then a page will be found for the App.
   // |install_flags| are a bitmask of extension::InstallFlags.

extensions/browser/pref_names.cc

@@ -45,6 +45,7 @@ const char kNativeMessagingBlacklist[] = "native_messaging.blacklist";
 const char kNativeMessagingWhitelist[] = "native_messaging.whitelist";
 const char kNativeMessagingUserLevelHosts[] =
     "native_messaging.user_level_hosts";
+const char kPinnedExtensions[] = "extensions.pinned_extensions";
 const char kStorageGarbageCollect[] = "extensions.storage.garbagecollect";
 const char kToolbar[] = "extensions.toolbar";
 const char kToolbarSize[] = "extensions.toolbarsize";

extensions/browser/pref_names.h

@@ -88,6 +88,10 @@ extern const char kNativeMessagingUserLevelHosts[];
 // Time of the next scheduled extensions auto-update checks.
 extern const char kNextUpdateCheck[];
 
+// A preference that tracks extensions pinned to the toolbar. This is a list
+// object stored in the Preferences file. The extensions are stored by ID.
+extern const char kPinnedExtensions[];
+
 // Indicates on-disk data might have skeletal data that needs to be cleaned
 // on the next start of the browser.
 extern const char kStorageGarbageCollect[];