Fix issue with browser action toolbar putting all extension icons in overflow once sync happens.

chrome/browser/extensions/extension_toolbar_model.cc

 // 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.
 
 #include "chrome/browser/extensions/extension_toolbar_model.h"
 
 #include <string>
 
 #include "base/metrics/histogram.h"
 #include "base/metrics/histogram_base.h"
@@ skipping 369 matching lines @@
     }
 
     ExtensionIdList::const_iterator pos =
         std::find(positions.begin(), positions.end(), extension->id());
     if (pos != positions.end())
       sorted[pos - positions.begin()] = extension;
     else
       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();
+    toolbar_items_.pop_back();

[Finnur, 2014/05/23 16:30:44]

By popping the extension here (before calling BrowserActionRemoved), we'll be
judged to be at max visible size after BrowserActionRemoved finishes calling
SetVisibleCount.

[Devlin, 2014/05/23 17:18:26]

Good comment.  Such a good comment, it should probably go in the code. :)

     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());
+
+  int count = 0;

[Devlin, 2014/05/23 17:18:26]

I wonder if this is worth keeping, even.  Isn't count always equal to
toolbar_items_.size() - 1?  It seems that just using size() would be a bit more
clear and have less potential to break if someone makes a change at the wrong
spot.  And it shouldn't be any slower...

   for (ExtensionList::const_iterator iter = sorted.begin();
-       iter != sorted.end(); ++iter) {
+      iter != sorted.end(); ++iter) {

[Devlin, 2014/05/23 17:18:26]

nit: I think the "four-space line continuation" rule has an exception for
for-loops, where we indent to after the parenthesis.  At least, I overwhelmingly
see more that way, and probably best to stay consistent in the file. (Also line
419.)

     // It's possible for the extension order to contain items that aren't
     // actually loaded on this machine.  For example, when extension sync is on,
     // we sync the extension order as-is but double-check with the user before
     // 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, count));
+      count++;

[Devlin, 2014/05/23 17:18:26]

nit: ++count in chromium, unless there's a specific reason for count++ (also
line 424).  (This might be moot depending what you do with count.)

+    }
   }
-  toolbar_items_.insert(toolbar_items_.end(), unsorted.begin(),
-                        unsorted.end());
+  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, count));
+      count++;
+    }
+  }
 
   UMA_HISTOGRAM_COUNTS_100(
       "ExtensionToolbarModel.BrowserActionsPermanentlyHidden", hidden);
   UMA_HISTOGRAM_COUNTS_100("ExtensionToolbarModel.BrowserActionsCount",
                            toolbar_items_.size());
 
   if (!toolbar_items_.empty()) {
     // Visible count can be -1, meaning: 'show all'. Since UMA converts negative
     // values to 0, this would be counted as 'show none' unless we convert it to
     // max.
     UMA_HISTOGRAM_COUNTS_100("ExtensionToolbarModel.BrowserActionsVisible",
                              visible_icon_count_ == -1 ?
                                  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() {
   if (!extension_prefs_)
     return;
 
   // Don't observe change caused by self.
   pref_change_registrar_.Remove(pref_names::kToolbar);
   extension_prefs_->SetToolbarOrder(last_known_positions_);
   pref_change_registrar_.Add(pref_names::kToolbar, pref_change_callback_);
@@ skipping 139 matching lines @@
 void ExtensionToolbarModel::StopHighlighting() {
   if (is_highlighting_) {
     highlighted_items_.clear();
     is_highlighting_ = false;
     if (old_visible_icon_count_ != visible_icon_count_) {
       SetVisibleIconCount(old_visible_icon_count_);
       FOR_EACH_OBSERVER(Observer, observers_, VisibleCountChanged());
     }
     FOR_EACH_OBSERVER(Observer, observers_, HighlightModeChanged(false));
   }
-};
+}
 
 }  // namespace extensions