[Extensions Toolbar] Make the ExtensionToolbarModel icon count more stable

[Extensions Toolbar] Make the ExtensionToolbarModel icon count more stable

Make two changes to the ExtensionToolbarModel:
- Only create views for actions once the extension system is fully loaded;
- Make sure we guard the visible_icon_count() to the possible range.

BUG=432148
Committed: https://crrev.com/3ceefb6e7910127d12104c1ecbc6bddf49f0caea
Cr-Commit-Position: refs/heads/master@{#304295}

[Devlin, 2014-11-14 02:12:26]

Patchset #1 (id:1) has been deleted

[Devlin, 2014-11-14 02:12:31]

Patchset #1 (id:20001) has been deleted

[Devlin, 2014-11-14 02:12:37]

Patchset #1 (id:40001) has been deleted

[Devlin, 2014-11-14 02:12:43]

Patchset #1 (id:60001) has been deleted

[Devlin, 2014-11-14 02:18:28]

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

[Finnur, 2014-11-14 19:01:10]

LGTM

[Devlin, 2014-11-14 21:24:43]

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

[Devlin, 2014-11-14 21:24:43]

+Peter for ui/

[Peter Kasting, 2014-11-14 21:31:58]

https://codereview.chromium.org/726813002/diff/80001/chrome/browser/ui/toolba...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/726813002/diff/80001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:208: if (!model_ ||
!model_->extensions_initialized() || !delegate_->IsReady())
Hmm.  Is there code somewhere that triggers another call to this function when
each of these conditions changes, so we'll be sure to create actions as soon as
possible?

Is it possible to know for sure that one of these will happen after the others,
so we can just be gated on it?  (Then the change of that conditional could
trigger the call to here, removing the need to check anything here.)

https://codereview.chromium.org/726813002/diff/80001/chrome/browser/ui/toolba...
File chrome/browser/ui/toolbar/toolbar_actions_bar_delegate.h (right):

https://codereview.chromium.org/726813002/diff/80001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/toolbar_actions_bar_delegate.h:20: virtual bool
IsReady() const = 0;
Nit: Maybe IsReadyToAddViews()?

https://codereview.chromium.org/726813002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):

https://codereview.chromium.org/726813002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:223: return
initialized_;
So what happens if we allow additions at any time?  It seems like in theory that
ought to actually work.

https://codereview.chromium.org/726813002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:732: initialized_ =
true;
This seems suspicious.  Why do we have to set this both in Init() and here? 
Seems like we should only have one or the other.

Can we remove Init() entirely?

[Devlin, 2014-11-14 22:00:38]

https://codereview.chromium.org/726813002/diff/80001/chrome/browser/ui/toolba...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/726813002/diff/80001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:208: if (!model_ ||
!model_->extensions_initialized() || !delegate_->IsReady())
On 2014/11/14 21:31:58, Peter Kasting wrote:
> Hmm.  Is there code somewhere that triggers another call to this function when
> each of these conditions changes, so we'll be sure to create actions as soon
as
> possible?

Yep - the delegate (the view for the browser actions container) will call
CreateActions as soon as its ready (for mac, on construction, for views, in
ViewHierarchyChanged()), and this class calls it when the toolbar model is
initialized.
 
> Is it possible to know for sure that one of these will happen after the
others,
> so we can just be gated on it?  (Then the change of that conditional could
> trigger the call to here, removing the need to check anything here.)

Sadly, no - When first starting up the browser, the view will be ready before
the model is initialized; for subsequent windows, the model is already
initialized, so we have to wait on the view.

https://codereview.chromium.org/726813002/diff/80001/chrome/browser/ui/toolba...
File chrome/browser/ui/toolbar/toolbar_actions_bar_delegate.h (right):

https://codereview.chromium.org/726813002/diff/80001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/toolbar_actions_bar_delegate.h:20: virtual bool
IsReady() const = 0;
On 2014/11/14 21:31:58, Peter Kasting wrote:
> Nit: Maybe IsReadyToAddViews()?

Sure. Done.

https://codereview.chromium.org/726813002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):

https://codereview.chromium.org/726813002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:223: return
initialized_;
On 2014/11/14 21:31:58, Peter Kasting wrote:
> So what happens if we allow additions at any time?  It seems like in theory
that
> ought to actually work.

Hmm.  I think I misread the comment on line 734, and interpreted it to mean "We
cannot add views until ViewHierarchyChanged".  So yes, I think we should be able
to add views at any time (and this appears to work in testing).

Well, that cleans things up nicely. :)  Thanks for catching it!

https://codereview.chromium.org/726813002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:732: initialized_ =
true;
On 2014/11/14 21:31:58, Peter Kasting wrote:
> This seems suspicious.  Why do we have to set this both in Init() and here? 
> Seems like we should only have one or the other.
> 
> Can we remove Init() entirely?

We still need Init(), but we actually don't need initialized_, I think.  Cool.

[Peter Kasting, 2014-11-14 22:03:05]

Yay!  LGTM

[Devlin, 2014-11-14 22:45:02]

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

[commit-bot: I haz the power, 2014-11-14 22:46:40]

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

[commit-bot: I haz the power, 2014-11-14 23:38:47]

Committed patchset #2 (id:100001)

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

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