From b878916a5b44cae6e959c214202a2a5cae20236f Mon Sep 17 00:00:00 2001
From: "finnur@chromium.org"
 <finnur@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Fri, 30 May 2014 07:00:45 +0000
Subject: [PATCH] Fix issue with browser action toolbar putting all extension
 icons in overflow once sync happens.

This is based on a patch sent by Jiang Kelvin, with some modifications.

BUG=369613

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@273739 0039d316-1c4b-4281-b951-d872f2087c98
---
 .../browser/extensions/extension_toolbar_model.cc  | 37 ++++++++++++++--------
 .../extension_toolbar_model_browsertest.cc         | 22 +++++++++++++
 2 files changed, 46 insertions(+), 13 deletions(-)

diff --git a/chrome/browser/extensions/extension_toolbar_model.cc b/chrome/browser/extensions/extension_toolbar_model.cc
index 62050ab606cd..7779514cb8cb 100644
--- a/chrome/browser/extensions/extension_toolbar_model.cc
+++ b/chrome/browser/extensions/extension_toolbar_model.cc
@@ -375,15 +375,21 @@ void ExtensionToolbarModel::Populate(const ExtensionIdList& positions,
       unsorted.push_back(make_scoped_refptr(extension));
   }
 
-  // Erase current icons.
-  for (size_t i = 0; i < toolbar_items_.size(); i++) {
+  size_t items_count = toolbar_items_.size();
+  for (size_t i = 0; i < items_count; i++) {
+    const Extension* extension = toolbar_items_.back();
+    // By popping the extension here (before calling BrowserActionRemoved),
+    // we will not shrink visible count by one after BrowserActionRemoved
+    // calls SetVisibleCount.
+    toolbar_items_.pop_back();
     FOR_EACH_OBSERVER(
-        Observer, observers_, BrowserActionRemoved(toolbar_items_[i].get()));
+        Observer, observers_, BrowserActionRemoved(extension));
   }
-  toolbar_items_.clear();
+  DCHECK(toolbar_items_.empty());
 
   // Merge the lists.
   toolbar_items_.reserve(sorted.size() + unsorted.size());
+
   for (ExtensionList::const_iterator iter = sorted.begin();
        iter != sorted.end(); ++iter) {
     // It's possible for the extension order to contain items that aren't
@@ -392,11 +398,22 @@ void ExtensionToolbarModel::Populate(const ExtensionIdList& positions,
     // syncing NPAPI-containing extensions, so if one of those is not actually
     // synced, we'll get a NULL in the list.  This sort of case can also happen
     // if some error prevents an extension from loading.
-    if (iter->get() != NULL)
+    if (iter->get() != NULL) {
+      toolbar_items_.push_back(*iter);
+      FOR_EACH_OBSERVER(
+          Observer, observers_, BrowserActionAdded(
+              *iter, toolbar_items_.size() - 1));
+    }
+  }
+  for (ExtensionList::const_iterator iter = unsorted.begin();
+       iter != unsorted.end(); ++iter) {
+    if (iter->get() != NULL) {
       toolbar_items_.push_back(*iter);
+      FOR_EACH_OBSERVER(
+          Observer, observers_, BrowserActionAdded(
+              *iter, toolbar_items_.size() - 1));
+    }
   }
-  toolbar_items_.insert(toolbar_items_.end(), unsorted.begin(),
-                        unsorted.end());
 
   UMA_HISTOGRAM_COUNTS_100(
       "ExtensionToolbarModel.BrowserActionsPermanentlyHidden", hidden);
@@ -412,12 +429,6 @@ void ExtensionToolbarModel::Populate(const ExtensionIdList& positions,
                                  base::HistogramBase::kSampleType_MAX :
                                  visible_icon_count_);
   }
-
-  // Inform observers.
-  for (size_t i = 0; i < toolbar_items_.size(); i++) {
-    FOR_EACH_OBSERVER(
-        Observer, observers_, BrowserActionAdded(toolbar_items_[i].get(), i));
-  }
 }
 
 void ExtensionToolbarModel::UpdatePrefs() {
diff --git a/chrome/browser/extensions/extension_toolbar_model_browsertest.cc b/chrome/browser/extensions/extension_toolbar_model_browsertest.cc
index e9d5a98fd5e0..d58f9fafcd14 100644
--- a/chrome/browser/extensions/extension_toolbar_model_browsertest.cc
+++ b/chrome/browser/extensions/extension_toolbar_model_browsertest.cc
@@ -584,4 +584,26 @@ IN_PROC_BROWSER_TEST_F(ExtensionToolbarModelTest, HighlightModeAdd) {
   EXPECT_EQ(id_c, ExtensionAt(2)->id());
 }
 
+IN_PROC_BROWSER_TEST_F(ExtensionToolbarModelTest, SizeAfterPrefChange) {
+  // Load two extensions with browser action.
+  base::FilePath extension_a_path(test_data_dir_.AppendASCII("api_test")
+                                                .AppendASCII("browser_action")
+                                                .AppendASCII("basics"));
+  ASSERT_TRUE(LoadExtension(extension_a_path));
+  base::FilePath extension_b_path(test_data_dir_.AppendASCII("api_test")
+                                                .AppendASCII("browser_action")
+                                                .AppendASCII("popup"));
+  ASSERT_TRUE(LoadExtension(extension_b_path));
+  std::string id_a = ExtensionAt(0)->id();
+  std::string id_b = ExtensionAt(1)->id();
+
+  // Should be at max size (-1).
+  EXPECT_EQ(-1, model_->GetVisibleIconCount());
+
+  model_->OnExtensionToolbarPrefChange();
+
+  // Should still be at max size.
+  EXPECT_EQ(-1, model_->GetVisibleIconCount());
+}
+
 }  // namespace extensions
-- 
2.11.4.GIT