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

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
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273739

[Finnur, 2014-05-23 16:30:43]

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

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

Mostly lg, just a few small comments.

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

https://codereview.chromium.org/296983014/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.cc:393:
toolbar_items_.pop_back();
On 2014/05/23 16:30:44, Finnur wrote:
> By popping the extension here (before calling BrowserActionRemoved), we'll be
> judged to be at max visible size after BrowserActionRemoved finishes calling
> SetVisibleCount.

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

https://codereview.chromium.org/296983014/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.cc:402: int count = 0;
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...

https://codereview.chromium.org/296983014/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.cc:404: iter != sorted.end();
++iter) {
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.)

https://codereview.chromium.org/296983014/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.cc:415: count++;
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.)

[Finnur, 2014-05-29 20:00:21]

Thanks. All addressed. 

Also CC-ing Jiang, who provided the initial patch this is based on.

[Finnur, 2014-05-29 20:00:26]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2014-05-29 20:01:12]

Hmmm, I suspect LG is not enough for a green light for the CQ.
Can you spell it out? :)

[commit-bot: I haz the power, 2014-05-29 20:02:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/finnur@chromium.org/296983014/50001

[commit-bot: I haz the power, 2014-05-29 20:02:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-29 20:02:55]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Devlin, 2014-05-29 20:03:18]

lgtm

[Finnur, 2014-05-29 20:03:57]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2014-05-29 20:05:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/finnur@chromium.org/296983014/50001

[leiyi.jp, 2014-05-30 01:35:43]

Glad to see the suggestion was adopted!

[commit-bot: I haz the power, 2014-05-30 07:00:47]

Change committed as 273739

[Finnur, 2014-05-30 16:18:41]

Thank you for providing the fix, Jiang!