Make hiding an extension action cause it to go to the overflow menu

chrome/browser/extensions/extension_toolbar_model.cc

Index: chrome/browser/extensions/extension_toolbar_model.cc
diff --git a/chrome/browser/extensions/extension_toolbar_model.cc b/chrome/browser/extensions/extension_toolbar_model.cc
index a87656b451b4153c6dbcd74dd8f3ab50e757120a..bb1988ca6409d600e089f8da6de5913eacc637f9 100644
--- a/chrome/browser/extensions/extension_toolbar_model.cc
+++ b/chrome/browser/extensions/extension_toolbar_model.cc
@@ -6,6 +6,7 @@
 
 #include <string>
 
+#include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/histogram_base.h"
 #include "base/prefs/pref_service.h"
@@ -115,7 +116,7 @@ void ExtensionToolbarModel::MoveExtensionIcon(const Extension* extension,
 
   FOR_EACH_OBSERVER(
       Observer, observers_, ToolbarExtensionMoved(extension, index));
-
+  MaybeUpdateVisibilityPref(extension, index);
   UpdatePrefs();
 }
 
@@ -156,11 +157,21 @@ ExtensionAction::ShowAction ExtensionToolbarModel::ExecuteBrowserAction(
 void ExtensionToolbarModel::SetVisibleIconCount(int count) {
   visible_icon_count_ =
       count == static_cast<int>(toolbar_items_.size()) ? -1 : count;
+
   // Only set the prefs if we're not in highlight mode. Highlight mode is
   // designed to be a transitory state, and should not persist across browser
   // restarts (though it may be re-entered).
-  if (!is_highlighting_)
+  if (!is_highlighting_) {
+    // Additionally, if we are using the new toolbar, any icons which are in the
+    // overflow menu are considered "hidden". But it so happens that the times
+    // we are likely to call SetVisibleIconCount() are also those when we are
+    // in flux. So wait for things to cool down before setting the prefs.
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(&ExtensionToolbarModel::MaybeUpdateVisibilityPrefs,
+                   weak_ptr_factory_.GetWeakPtr()));
     prefs_->SetInteger(pref_names::kToolbarSize, visible_icon_count_);
+  }
 }
 
 void ExtensionToolbarModel::OnExtensionLoaded(
@@ -204,18 +215,42 @@ void ExtensionToolbarModel::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
-  DCHECK_EQ(
-      extensions::NOTIFICATION_EXTENSION_BROWSER_ACTION_VISIBILITY_CHANGED,
-      type);
+  DCHECK_EQ(NOTIFICATION_EXTENSION_BROWSER_ACTION_VISIBILITY_CHANGED, type);
   const Extension* extension =
       ExtensionRegistry::Get(profile_)->GetExtensionById(
           *content::Details<const std::string>(details).ptr(),
           ExtensionRegistry::EVERYTHING);
-  if (ExtensionActionAPI::GetBrowserActionVisibility(extension_prefs_,
-                                                     extension->id()))
-    AddExtension(extension);
-  else
-    RemoveExtension(extension);
+
+  bool visible = ExtensionActionAPI::GetBrowserActionVisibility(
+                     extension_prefs_, extension->id());
+  // Hiding works differently with the new and old toolbars.
+  if (include_all_extensions_) {
+    int new_size = 0;
+    int new_index = 0;
+    if (visible) {
+      // If this action used to be hidden, we can't possible be showing all.
+      DCHECK_NE(-1, visible_icon_count_);
+      // Grow the bar by one and move the extension to the end of the visibles.
+      new_size = visible_icon_count_ + 1;
+      new_index = new_size - 1;
+    } else {
+      // If we're hiding one, we must be showing at least one.
+      DCHECK_NE(visible_icon_count_, 0);
+      // Shrink the bar by one and move the extension to the beginning of the
+      // overflow menu.
+      new_size = visible_icon_count_ == -1 ?
+                     toolbar_items_.size() - 1 : visible_icon_count_ - 1;
+      new_index = new_size;
+    }
+    SetVisibleIconCount(new_size);
+    MoveExtensionIcon(extension, new_index);
+    FOR_EACH_OBSERVER(Observer, observers_, ToolbarVisibleCountChanged());
+  } else {  // Don't include all extensions.
+    if (visible)
+      AddExtension(extension);
+    else
+      RemoveExtension(extension);
+  }
 }
 
 void ExtensionToolbarModel::OnReady() {
@@ -299,6 +334,8 @@ void ExtensionToolbarModel::AddExtension(const Extension* extension) {
     UpdatePrefs();
   }
 
+  MaybeUpdateVisibilityPref(extension, new_index);
+
   // If we're currently highlighting, then even though we add a browser action
   // to the full list (|toolbar_items_|, there won't be another *visible*
   // browser action, which was what the observers care about.
@@ -350,6 +387,7 @@ void ExtensionToolbarModel::InitializeExtensionList(
   Populate(last_known_positions_, extensions);
 
   extensions_initialized_ = true;
+  MaybeUpdateVisibilityPrefs();
   FOR_EACH_OBSERVER(Observer, observers_, ToolbarVisibleCountChanged());
 }
 
@@ -450,6 +488,33 @@ void ExtensionToolbarModel::UpdatePrefs() {
   pref_change_registrar_.Add(pref_names::kToolbar, pref_change_callback_);
 }
 
+void ExtensionToolbarModel::MaybeUpdateVisibilityPref(
+    const Extension* extension, int index) {
+  // We only update the visibility pref for hidden/not hidden based on the
+  // overflow menu with the new toolbar design.
+  if (include_all_extensions_) {
+    bool visible = index < visible_icon_count_ || visible_icon_count_ == -1;
+    if (visible != ExtensionActionAPI::GetBrowserActionVisibility(
+                       extension_prefs_, extension->id())) {
+      // Don't observe changes caused by ourselves.
+      bool registered = !registrar_.IsEmpty();
+      registrar_.RemoveAll();  // Cheat since we only have one notification.

[Finnur, 2014/08/19 10:18:04]

Seems like you aren't saving much with this. Why not explicitly remove it and
avoid potential bugs in the future?

[Devlin, 2014/08/19 16:54:39]

sigh... probably for the best.  But typing
NOTIFICATION_EXTENSION_BROWSER_ACTION_VISIBILITY three times in ten lines just
hurts my fingers. :P

Just more incentive to make it an observer method instead... but that's a CL for
another time.

Done.

[Finnur, 2014/08/20 12:06:16]

That's why they invented the clipboard! ;)

[Devlin, 2014/08/20 15:31:22]

:P

+      ExtensionActionAPI::SetBrowserActionVisibility(
+          extension_prefs_, extension->id(), visible);
+      if (registered) {
+        registrar_.Add(this,
+                       NOTIFICATION_EXTENSION_BROWSER_ACTION_VISIBILITY_CHANGED,
+                       content::Source<ExtensionPrefs>(extension_prefs_));
+      }
+    }
+  }
+}
+
+void ExtensionToolbarModel::MaybeUpdateVisibilityPrefs() {
+  for (size_t i = 0u; i < toolbar_items_.size(); ++i)
+    MaybeUpdateVisibilityPref(toolbar_items_[i], i);
+}
+
 int ExtensionToolbarModel::IncognitoIndexToOriginal(int incognito_index) {
   int original_index = 0, i = 0;
   for (ExtensionList::iterator iter = toolbar_items_.begin();