[Extensions Toolbar] Notify observers of extensions re-added after a pref change

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 cc3f817e686d92bca05ed88a08d09dd4b0f3e363..863c793fe267bf86839dd2085a9290b1a5195af0 100644
--- a/chrome/browser/extensions/extension_toolbar_model.cc
+++ b/chrome/browser/extensions/extension_toolbar_model.cc
@@ -394,16 +394,6 @@ void ExtensionToolbarModel::RemoveExtension(const Extension* extension) {
   UpdatePrefs();
 }
 
-void ExtensionToolbarModel::ClearItems() {
-  size_t items_count = toolbar_items_.size();
-  for (size_t i = 0; i < items_count; ++i) {
-    const Extension* extension = toolbar_items_.back().get();
-    toolbar_items_.pop_back();
-    FOR_EACH_OBSERVER(Observer, observers_, ToolbarExtensionRemoved(extension));
-  }
-  DCHECK(toolbar_items_.empty());
-}
-
 // Combine the currently enabled extensions that have browser actions (which
 // we get from the ExtensionRegistry) with the ordering we get from the
 // pref service. For robustness we use a somewhat inefficient process:
@@ -575,11 +565,25 @@ void ExtensionToolbarModel::OnExtensionToolbarPrefChange() {
   }
   last_known_positions_.swap(pref_positions);
 
-  ClearItems();
+  // Clear out the old...
+  size_t items_count = toolbar_items_.size();
+  for (size_t i = 0; i < items_count; ++i) {

[Marijn Kruisselbrink, 2014/11/17 22:18:02]

Since your not using i, couldn't you just do a |while (!items_.empty())| ?
The code already would break if some observer would cause items to be added to
toolbar_items_ while this loop runs, so it wouldn't be any more fragile, and I
think it makes the intent of this loop clearer.

[Devlin, 2014/11/17 22:44:13]

Done.

+    const Extension* extension = toolbar_items_.back().get();
+    toolbar_items_.pop_back();

[Marijn Kruisselbrink, 2014/11/17 22:18:02]

It seems a safer thing would be to move the pop_back to be after the use of
|extension|? Of course in reality it should/will never happen that the only
reference to an Extension is in toolbar_items_ so in practice this pop_back will
never cause the Extension to be deleted, but it still seems better to not rely
on such things.
Although thinking more, I guess the pop_back has to be called before calling the
observers since that is how ToolbarExtensionRemoved is specified. So in that
case you should probably store the extension as a scoped_refptr, or at least add
a comment indicating why this code is safe.

[Devlin, 2014/11/17 22:44:13]

Done (wrapped extension in a refptr)

+    FOR_EACH_OBSERVER(Observer, observers_, ToolbarExtensionRemoved(extension));
+  }
+  DCHECK(toolbar_items_.empty());
 
-  // Re-populate.
+  // ...Add the new...
   Populate(last_known_positions_);
 
+  // ...And notify.
+  for (size_t i = 0; i < toolbar_items().size(); ++i) {
+    FOR_EACH_OBSERVER(Observer,
+                      observers_,
+                      ToolbarExtensionAdded(toolbar_items()[i].get(), i));
+  }
+
   if (last_known_positions_.size() > pref_position_size) {
     // Need to update pref because we have extra icons. But can't call
     // UpdatePrefs() directly within observation closure.