[Extensions Menu] Forget pin state on uninstallation

When an extension is uninstalled, the prefs related to that extension
should be erased. Do so for the pinned state of the extension, and add
a unittest to cover the same.

Bug: 1071634
Change-Id: Ic08da5ee19ad618ecddc44a85a8372412962a223
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2153702Reviewed-by: Caroline Rising <corising@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#760743}

chrome/browser/ui/toolbar/toolbar_actions_model.cc

@@ -250,6 +250,19 @@ void ToolbarActionsModel::RemovePref(const ActionId& action_id) {
     last_known_positions_.erase(pos);
     UpdatePrefs();
   }
+
+  if (base::FeatureList::IsEnabled(features::kExtensionsToolbarMenu)) {
+    // The extension is already unloaded at this point, and so shouldn't be in
+    // the active pinned set.
+    DCHECK(!IsActionPinned(action_id));
+    auto stored_pinned_actions = extension_prefs_->GetPinnedExtensions();
+    auto iter = std::find(stored_pinned_actions.begin(),
+                          stored_pinned_actions.end(), action_id);
+    if (iter != stored_pinned_actions.end()) {
+      stored_pinned_actions.erase(iter);
+      extension_prefs_->SetPinnedExtensions(stored_pinned_actions);
+    }
+  }
 }
 
 void ToolbarActionsModel::OnReady() {
@@ -400,9 +413,6 @@ void ToolbarActionsModel::RemoveAction(const ActionId& action_id) {
 
   action_ids_.erase(pos);
 
-  // TODO(pbos): Remove previously-pinned actions from ExtensionPrefs if this is
-  // an uninstall and not a disable. This might need to be handled in another
-  // place (ExtensionPrefs?) as we don't know the reason for RemoveAction here.
   UpdatePinnedActionIds();
 
   // If we're in highlight mode, we also have to remove the action from

chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc

@@ -1533,3 +1533,40 @@ TEST_F(ToolbarActionsModelUnitTest,
       extension_prefs->GetPinnedExtensions(),
       testing::ElementsAre(browser_action_a()->id(), browser_action_b()->id()));
 }
+
+TEST_F(ToolbarActionsModelUnitTest, PinStateErasedOnUninstallation) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kExtensionsToolbarMenu);
+
+  Init();
+
+  scoped_refptr<const extensions::Extension> extension =
+      extensions::ExtensionBuilder("extension")
+          .SetAction(ActionType::BROWSER_ACTION)
+          .SetLocation(extensions::Manifest::INTERNAL)
+          .Build();
+
+  // Add and pin an extension.
+  EXPECT_TRUE(AddExtension(extension));
+  EXPECT_FALSE(toolbar_model()->IsActionPinned(extension->id()));
+  extensions::ExtensionPrefs* const prefs =
+      extensions::ExtensionPrefs::Get(profile());
+  EXPECT_THAT(prefs->GetPinnedExtensions(), testing::IsEmpty());
+
+  toolbar_model()->SetActionVisibility(extension->id(), true);
+  EXPECT_TRUE(toolbar_model()->IsActionPinned(extension->id()));
+  EXPECT_THAT(prefs->GetPinnedExtensions(),
+              testing::ElementsAre(extension->id()));
+
+  // Uninstall the extension. The pin state should be forgotten.
+  service()->UninstallExtension(
+      extension->id(), extensions::UNINSTALL_REASON_FOR_TESTING, nullptr);
+
+  EXPECT_FALSE(toolbar_model()->IsActionPinned(extension->id()));
+  EXPECT_THAT(prefs->GetPinnedExtensions(), testing::IsEmpty());
+
+  // Re-add the extension. It should be in the default (unpinned) state.
+  EXPECT_TRUE(AddExtension(extension));
+  EXPECT_FALSE(toolbar_model()->IsActionPinned(extension->id()));
+  EXPECT_THAT(prefs->GetPinnedExtensions(), testing::IsEmpty());
+}