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

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 <algorithm>
 #include <string>
 
 #include "base/message_loop/message_loop.h"
@@ skipping 376 matching lines @@
       if (highlighted_items_.empty())
         StopHighlighting();
     }
   } else {
     FOR_EACH_OBSERVER(Observer, observers_, ToolbarExtensionRemoved(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:
 // 1. Create a vector of extensions sorted by their pref values. This vector may
 // have holes.
 // 2. Create a vector of extensions that did not have a pref value.
 // 3. Remove holes from the sorted vector and append the unsorted vector.
 void ExtensionToolbarModel::InitializeExtensionList() {
   DCHECK(toolbar_items_.empty());  // We shouldn't have any items yet.
 
@@ skipping 151 matching lines @@
   ExtensionIdList pref_positions = extension_prefs_->GetToolbarOrder();
   size_t pref_position_size = pref_positions.size();
   for (size_t i = 0; i < last_known_positions_.size(); ++i) {
     if (std::find(pref_positions.begin(), pref_positions.end(),
                   last_known_positions_[i]) == pref_positions.end()) {
       pref_positions.push_back(last_known_positions_[i]);
     }
   }
   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.
     base::MessageLoop::current()->PostTask(
         FROM_HERE,
         base::Bind(&ExtensionToolbarModel::UpdatePrefs,
                    weak_ptr_factory_.GetWeakPtr()));
   }
 }
 
@@ skipping 122 matching lines @@
   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_, ToolbarHighlightModeChanged(false));
   }
 }
 
 }  // namespace extensions