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

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

I suspect this is what's causing the listed bugs, as it would lead to the
observers' lists getting out of order.

BUG=433757
BUG=433764
Committed: https://crrev.com/ad3ae498cb192d7a78c8efdc053d5d22c8713a1d
Cr-Commit-Position: refs/heads/master@{#304519}

[Devlin, 2014-11-17 21:47:20]

Patchset #1 (id:1) has been deleted

[Devlin, 2014-11-17 21:47:29]

rdevlin.cronin@chromium.org changed reviewers:
+ mek@chromium.org

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

lgtm

https://codereview.chromium.org/731353002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/731353002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.cc:570: for (size_t i = 0; i <
items_count; ++i) {
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.

https://codereview.chromium.org/731353002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.cc:572:
toolbar_items_.pop_back();
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.

https://codereview.chromium.org/731353002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_toolbar_model.h (left):

https://codereview.chromium.org/731353002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.h:228: void ClearItems();
Just curious what changed that before you had this as a separate method, and now
you inlined it in OnExtensionToolbarPrefChange?

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

https://codereview.chromium.org/731353002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/731353002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.cc:570: for (size_t i = 0; i <
items_count; ++i) {
On 2014/11/17 22:18:02, Marijn Kruisselbrink wrote:
> 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.

Done.

https://codereview.chromium.org/731353002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.cc:572:
toolbar_items_.pop_back();
On 2014/11/17 22:18:02, Marijn Kruisselbrink wrote:
> 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.

Done (wrapped extension in a refptr)

https://codereview.chromium.org/731353002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_toolbar_model.h (left):

https://codereview.chromium.org/731353002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.h:228: void ClearItems();
On 2014/11/17 22:18:02, Marijn Kruisselbrink wrote:
> Just curious what changed that before you had this as a separate method, and
now
> you inlined it in OnExtensionToolbarPrefChange?

At one point, it was called multiple times; now that we only use it in the one
place, it seems kind of silly to have it separate (since it's only 4 lines). 
Also, I think it makes the add/remove in the pref update more clear.

[Devlin, 2014-11-17 22:44:16]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-11-17 22:45:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/731353002/40001

[commit-bot: I haz the power, 2014-11-17 23:37:39]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2014-11-17 23:38:18]

Patchset 2 (id:??) landed as
https://crrev.com/ad3ae498cb192d7a78c8efdc053d5d22c8713a1d
Cr-Commit-Position: refs/heads/master@{#304519}