[Extensions Toolbar] Protect against crazy bounds

[Extensions Toolbar] Protect against crazy bounds

Address two issues:
- The extensions toolbar can be bigger than the browser window.
- The user could, potentially, have so many extensions that the
overflow container makes the wrench menu expand ridiculously.

Address the former by automatically overflowing any actions
that won't fit on the main bar into the overflow menu (without
setting it as the preferred size, so resizing the window returns
to normal).

Address the latter by giving the overflow item a maximum height of
416 pixels (enough for around 104 overflowed actions), and make it
a scroll view on Views.  Mac users will see it truncated. Note that
this will affect <0.0002% of users (but we want to avoid a completely
broken experience for anyone).

BUG=530303
BUG=58915
Committed: https://crrev.com/ede728b9fff930b95b40e434f6f0c31244ef8b05
Cr-Commit-Position: refs/heads/master@{#349504}

[Devlin, 2015-09-10 20:48:23]

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

[Devlin, 2015-09-10 20:48:23]

Peter and Avi, mind taking a look?
Peter - c/b/ui/toolbar, c/b/ui/views
Avi - c/b/ui/cocoa

Cheers!

[Avi (use Gerrit), 2015-09-10 20:58:56]

lgtm

https://codereview.chromium.org/1330423003/diff/1/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

https://codereview.chromium.org/1330423003/diff/1/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:670: // Button
is fully in the container view, and should get full opacity.
*The* button?

https://codereview.chromium.org/1330423003/diff/1/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:677: // Button
is only partially in the container view. If the user is resizing
*The* button?

[Devlin, 2015-09-11 16:44:29]

Patchset #2 (id:20001) has been deleted

[Devlin, 2015-09-11 16:46:15]

https://codereview.chromium.org/1330423003/diff/1/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

https://codereview.chromium.org/1330423003/diff/1/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:670: // Button
is fully in the container view, and should get full opacity.
On 2015/09/10 20:58:56, Avi wrote:
> *The* button?

Done.

https://codereview.chromium.org/1330423003/diff/1/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:677: // Button
is only partially in the container view. If the user is resizing
On 2015/09/10 20:58:56, Avi wrote:
> *The* button?

Done.

[Devlin, 2015-09-14 18:41:03]

Peter, friendly ping for c/b/ui/views and c/b/ui/toolbar?

[Peter Kasting, 2015-09-15 00:13:39]

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:269:
main_bar_->GetEndIndexInBounds() : 0u;
Nit: Don't add 'u' here unless the code won't compile without it; if that's the
case we'd need to either static_cast to size_t or split the return to a separate
statement, as "0u" still isn't the same type as size_t on all architectures.

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:969: // |browser_->window()| can
be null during startup.
Nit: No || around a function call

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:971:
browser_->window()->GetToolbarActionsBar()->NeedsOverflow()) {
We now have ToolbarActionsBar::NeedsOverflow() and
ToolbarActionsdModel::all_icons_visible().  It's confusing to have both and to
know which should be used where.  Can we eliminate one?  If not can we somehow
document on these how they differ and why?

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_action_test_util_views.cc (right):

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_action_test_util_views.cc:38:
container_->SetBounds(0, 0, s.width(), s.height());
container_->SizeToPreferredSize();

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_action_test_util_views.cc:203:
container->SetSize(gfx::Size(width, container->size().height()));
container->SetSize(gfx::Size(width, container->height()));

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

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:630: // shrank to
give room to the omnibox, but to adjust the size, it needs to
Nit: shrunk (or remove "was" on previous line)

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc (right):

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:26: const int
kMaxOverflowContainerHeight = 416;
Nit: Prefer to set the desired number of overflow actions or rows, and then
compute the height based on that.  This way if we ever change the size of the
action icons the code will keep working (and you won't need the comment about
how this is 104 actions).

Also, prefer to move all this to the function that uses it, especially when your
explanation describes something the code should do ("cap the height") rather
than just describing what the constant is.

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:68:
s.set_width(s.width() + GetScrollBarWidth());
I assume GetScrollBarWidth() returns 0 when no scrollbar is showing?

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:86:
SetBounds(start_padding() + 1, 0, sz.width(), sz.height());
Nit: Maybe

  SetPosition(gfx::Point(start_padding() + 1, 0));
  SizeToPreferredSize();

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_view.cc (right):

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_view.cc:616: int
kMinimumLocationBarWidth = 100;
(1) You should probably BUG=58915 in your CL description
(2) Use GetMinimumSize() on various elements (e.g. location_bar_) rather than
hardcoding values here

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_view.cc:624: available_width -=
browser_actions_width;
This gives the browser actions area as much space as it wants up to the limit of
making the location bar really small.  Maybe if people haven't manually sized
this area we should instead limit the space this takes up to e.g. half the
toolbar width or something?  After all, even if you have a minimum width for the
location bar to avoid glitchy-looking UI, it's still not going to be much fun to
use.

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_view.cc:625: int location_bar_width =
kMinimumLocationBarWidth + available_width;
This statement seems wrong.  Why are we adding in kMinimumLocationBarWidth?

[Devlin, 2015-09-15 18:22:39]

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:269:
main_bar_->GetEndIndexInBounds() : 0u;
On 2015/09/15 00:13:38, Peter Kasting wrote:
> Nit: Don't add 'u' here unless the code won't compile without it; if that's
the
> case we'd need to either static_cast to size_t or split the return to a
separate
> statement, as "0u" still isn't the same type as size_t on all architectures.

Done.

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:969: // |browser_->window()| can
be null during startup.
On 2015/09/15 00:13:38, Peter Kasting wrote:
> Nit: No || around a function call

Wasn't sure about this one, because it was a simple accessor, so I was basically
referencing the variable.  But that is a little inconsistent.  Done.

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:971:
browser_->window()->GetToolbarActionsBar()->NeedsOverflow()) {
On 2015/09/15 00:13:38, Peter Kasting wrote:
> We now have ToolbarActionsBar::NeedsOverflow() and
> ToolbarActionsdModel::all_icons_visible().  It's confusing to have both and to
> know which should be used where.  Can we eliminate one?  If not can we somehow
> document on these how they differ and why?

Documented in toolbar_actions_bar.h

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_action_test_util_views.cc (right):

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_action_test_util_views.cc:38:
container_->SetBounds(0, 0, s.width(), s.height());
On 2015/09/15 00:13:38, Peter Kasting wrote:
> container_->SizeToPreferredSize();

ah, useful.  Done.

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_action_test_util_views.cc:203:
container->SetSize(gfx::Size(width, container->size().height()));
On 2015/09/15 00:13:38, Peter Kasting wrote:
> container->SetSize(gfx::Size(width, container->height()));

d'oh.  Done.

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

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:630: // shrank to
give room to the omnibox, but to adjust the size, it needs to
On 2015/09/15 00:13:38, Peter Kasting wrote:
> Nit: shrunk (or remove "was" on previous line)

Done.

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc (right):

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:26: const int
kMaxOverflowContainerHeight = 416;
On 2015/09/15 00:13:38, Peter Kasting wrote:
> Nit: Prefer to set the desired number of overflow actions or rows, and then
> compute the height based on that.  This way if we ever change the size of the
> action icons the code will keep working (and you won't need the comment about
> how this is 104 actions).
> 
> Also, prefer to move all this to the function that uses it, especially when
your
> explanation describes something the code should do ("cap the height") rather
> than just describing what the constant is.

Done.  I don't want to calculate it based on number of actions, because then it
doesn't really limit the height, if the number of icons per row is adjusted. 
But doing a number of rows sounds good.

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:68:
s.set_width(s.width() + GetScrollBarWidth());
On 2015/09/15 00:13:38, Peter Kasting wrote:
> I assume GetScrollBarWidth() returns 0 when no scrollbar is showing?

I would have thought so.  Apparently not.  Addressed.

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:86:
SetBounds(start_padding() + 1, 0, sz.width(), sz.height());
On 2015/09/15 00:13:38, Peter Kasting wrote:
> Nit: Maybe
> 
>   SetPosition(gfx::Point(start_padding() + 1, 0));
>   SizeToPreferredSize();

Sure, done.

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_view.cc (right):

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_view.cc:616: int
kMinimumLocationBarWidth = 100;
On 2015/09/15 00:13:38, Peter Kasting wrote:
> (1) You should probably BUG=58915 in your CL description
> (2) Use GetMinimumSize() on various elements (e.g. location_bar_) rather than
> hardcoding values here

1) Wow, that's an old one.  Good find.
2) Done.

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_view.cc:624: available_width -=
browser_actions_width;
On 2015/09/15 00:13:39, Peter Kasting wrote:
> This gives the browser actions area as much space as it wants up to the limit
of
> making the location bar really small.  Maybe if people haven't manually sized
> this area we should instead limit the space this takes up to e.g. half the
> toolbar width or something?  After all, even if you have a minimum width for
the
> location bar to avoid glitchy-looking UI, it's still not going to be much fun
to
> use.

Hmm... I'd like to avoid this for now.  For one thing, it's more consistent with
current behavior, and I'd be worried that it'd be hard to find the right balance
for a minimum without a lot of experimentation, and would probably require a
little UI review, which would slow this down a lot.

More importantly, though, most of our users have relatively few extensions (over
90% have 3 or fewer), so the number of users for whom this would have an effect
is pretty small, and is also (probably?) more likely to be the power users that
already know they can resize it.  I'd like to avoid any "implicit" resizing,
unless absolutely necessary for usability (as in the case of the omnibox
shrinking to nothingness).

https://codereview.chromium.org/1330423003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_view.cc:625: int location_bar_width =
kMinimumLocationBarWidth + available_width;
On 2015/09/15 00:13:39, Peter Kasting wrote:
> This statement seems wrong.  Why are we adding in kMinimumLocationBarWidth?

Above, we subtract out the minimum location bar width from the available width,
so the "leftover" width is the available width + the minimum location bar width.
 I've added a comment to explain this a bit more.

[Peter Kasting, 2015-09-17 00:18:04]

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar.h (right):

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.h:117: // represents the
underlying model of *all* windows) is set to, such as in
So does this mean that if all windows are equally narrow, the two will be in
agreement, i.e. the model describes the visibility state of the widest window? 
It's still a bit unclear to me; see next comment also.

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.h:118: // the case of a very
narrow window.
This comment helps, but it might be necessary to instead/additionally use
class-scope comments or more function-scope comments not only here but in
ToolbarActionsModel as well to make clear the different concepts of visibility. 
Bonus points if you invent terminology to distinguish these so the classes
aren't both talking about "visible" but meaning different things by it.

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_action_test_util_views.cc (right):

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_action_test_util_views.cc:76: // Give
the container plenty of room to play.
Nit: Prefer a comment that's a bit more specific about what the container needs
room to do (or, if this number is completely arbitrary, drop the comment
entirely).

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

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:416: if (end_index
!= toolbar_action_views_.size() && !suppress_chevron_ &&
Nit: Is this first condition the same as toolbar_actions_bar_->NeedsOverflow()? 
That might be clearer, I'm not sure.

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc (right):

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:50: max_height_ =
ToolbarActionsBar::IconHeight() * 1;
I'm confused.  Does this mean one row of icons?  How do we fit 104 icons on one
row?

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:65: // not the
scrollbar width. Add it in if necessary.
I wonder if we should fix this in ScrollView.

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:67:
s.set_width(s.width() + GetScrollBarWidth());
Nit: s.Enlarge(GetScrollbarWidth(), 0));

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:80: return
views::ScrollView::GetHeightForWidth(width);
(Does this include the scrollbar width in the calculation at the right times?)

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:84:
SetPosition(gfx::Point(start_padding() + 1, 0));
Why +1?

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_view.cc (right):

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_view.cc:635: // aside" above.
This helps, but I think instead of subtracting the location bar min width from
available_width above, I'd subtract it during the calculation of
|browser_actions_width|, then give the location bar just the available width,
instead of adding the minimum back there.

For one I think this is a little clearer and wouldn't need this comment anymore;
for another, if the window is actually so small that the location bar min width
won't fit, this would shrink the location bar down, which is probably better. 
(For an example of this, try shrinking a single-tab window to the minimum width,
then changing the settings from not showing a home button to showing a home
button.  You'll see the location bar shrink smaller than its min width, which is
probably better than pushing the settings button off the side of the window.)

[Devlin, 2015-09-17 17:00:28]

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar.h (right):

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.h:117: // represents the
underlying model of *all* windows) is set to, such as in
On 2015/09/17 00:18:04, Peter Kasting wrote:
> So does this mean that if all windows are equally narrow, the two will be in
> agreement, i.e. the model describes the visibility state of the widest window?

> It's still a bit unclear to me; see next comment also.

I've cleared up this comment, and added some better class comments both here and
in the ToolbarActionsModel.  Lemme know if it's a bit better. :)

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.h:118: // the case of a very
narrow window.
On 2015/09/17 00:18:04, Peter Kasting wrote:
> This comment helps, but it might be necessary to instead/additionally use
> class-scope comments or more function-scope comments not only here but in
> ToolbarActionsModel as well to make clear the different concepts of
visibility. 
> Bonus points if you invent terminology to distinguish these so the classes
> aren't both talking about "visible" but meaning different things by it.

See above.

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_action_test_util_views.cc (right):

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_action_test_util_views.cc:76: // Give
the container plenty of room to play.
On 2015/09/17 00:18:04, Peter Kasting wrote:
> Nit: Prefer a comment that's a bit more specific about what the container
needs
> room to do (or, if this number is completely arbitrary, drop the comment
> entirely).

Dropped.

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

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:416: if (end_index
!= toolbar_action_views_.size() && !suppress_chevron_ &&
On 2015/09/17 00:18:04, Peter Kasting wrote:
> Nit: Is this first condition the same as
toolbar_actions_bar_->NeedsOverflow()? 
> That might be clearer, I'm not sure.

Agreed, might be clearer.  Done.

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc (right):

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:50: max_height_ =
ToolbarActionsBar::IconHeight() * 1;
On 2015/09/17 00:18:04, Peter Kasting wrote:
> I'm confused.  Does this mean one row of icons?  How do we fit 104 icons on
one
> row?

D'oh.  Was testing to make sure it worked (and didn't want to install 104
extensions) ;)

s/1/13.

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:65: // not the
scrollbar width. Add it in if necessary.
On 2015/09/17 00:18:04, Peter Kasting wrote:
> I wonder if we should fix this in ScrollView.

That would make me concerned about breaking anything else that uses a
ScrollView...

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:67:
s.set_width(s.width() + GetScrollBarWidth());
On 2015/09/17 00:18:04, Peter Kasting wrote:
> Nit: s.Enlarge(GetScrollbarWidth(), 0));

Done.

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:80: return
views::ScrollView::GetHeightForWidth(width);
On 2015/09/17 00:18:04, Peter Kasting wrote:
> (Does this include the scrollbar width in the calculation at the right times?)

Yes.

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:84:
SetPosition(gfx::Point(start_padding() + 1, 0));
On 2015/09/17 00:18:04, Peter Kasting wrote:
> Why +1?

It was because the start_padding() was returning the range of the padding,
rather than the width of the padding (hence the -1 in that method).  Removed
both.

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_view.cc (right):

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_view.cc:635: // aside" above.
On 2015/09/17 00:18:04, Peter Kasting wrote:
> This helps, but I think instead of subtracting the location bar min width from
> available_width above, I'd subtract it during the calculation of
> |browser_actions_width|, then give the location bar just the available width,
> instead of adding the minimum back there.
> 
> For one I think this is a little clearer and wouldn't need this comment
anymore;
> for another, if the window is actually so small that the location bar min
width
> won't fit, this would shrink the location bar down, which is probably better. 
> (For an example of this, try shrinking a single-tab window to the minimum
width,
> then changing the settings from not showing a home button to showing a home
> button.  You'll see the location bar shrink smaller than its min width, which
is
> probably better than pushing the settings button off the side of the window.)

Done.

[Peter Kasting, 2015-09-17 19:59:53]

LGTM

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar.h (right):

https://codereview.chromium.org/1330423003/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.h:117: // represents the
underlying model of *all* windows) is set to, such as in
On 2015/09/17 17:00:27, Devlin wrote:
> On 2015/09/17 00:18:04, Peter Kasting wrote:
> > So does this mean that if all windows are equally narrow, the two will be in
> > agreement, i.e. the model describes the visibility state of the widest
window?
> 
> > It's still a bit unclear to me; see next comment also.
> 
> I've cleared up this comment, and added some better class comments both here
and
> in the ToolbarActionsModel.  Lemme know if it's a bit better. :)

Yeah, these help.

https://codereview.chromium.org/1330423003/diff/80001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_model.h (right):

https://codereview.chromium.org/1330423003/diff/80001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_model.h:34: // ToolbarActionsBar (see
also that class for details).
Nit: Maybe instead of the sentence about "special circumstances" you could more
specifically say "when the window is narrow" or the like?  Something like:

Each browser window will attempt to show browser actions as specified by this
model, but if the window is too narrow, actions may end up pushed into the
overflow menu on a per-window basis.  Callers interested in the arrangement of
actions in a particular window should check that window's instance of
ToolbarActionsBar, which is responsible for this per-window overflow.

https://codereview.chromium.org/1330423003/diff/80001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_model.h:139: bool all_icons_visible()
const {
Nit: Do you think you should put a warning on here about this being about the
global default, and being inappropriate if you want to know what icons are
actually visible to the user in any particular window?

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

https://codereview.chromium.org/1330423003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:47: // overflow
won't make the menu larger than the height of the screen.
Now that I read this comment I'm a bit worried.  If the window is near the
bottom of the screen or the screen is short, how do we know 13 rows won't still
go off the bottom?  Likewise if the screen is some huge portrait mode setup, we
might have space for more rows?  If the core desire is to stay on the screen it
seems like you'd need to check the screen bounds.  If the core desires is just
"don't make the menu excessively tall" irrespective of screen size, maybe the
comment should be updated.

https://codereview.chromium.org/1330423003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:49: // less than
0.0002% of our users will be affected.
Nit: I might just drop this part of the comment and leave that in the bug or the
CL description if future spelunkers really care; I don't think the code needs
this justification, and I don't like putting time-sensitive comments in the
code, because a future reader won't know whether the comment is still accurate
or not anyway.

https://codereview.chromium.org/1330423003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:50: max_height_ =
ToolbarActionsBar::IconHeight() * 13;
Tiny nit: Consider adding a named kMaxOverflowRows constant defined right here.

https://codereview.chromium.org/1330423003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:109: return
views::MenuItemView::label_start() - 1 -
You didn't actually drop the -1 here.

[Devlin, 2015-09-17 20:29:42]

Patchset #5 (id:100001) has been deleted

[Devlin, 2015-09-17 21:38:38]

https://codereview.chromium.org/1330423003/diff/80001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_model.h (right):

https://codereview.chromium.org/1330423003/diff/80001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_model.h:34: // ToolbarActionsBar (see
also that class for details).
On 2015/09/17 19:59:53, Peter Kasting wrote:
> Nit: Maybe instead of the sentence about "special circumstances" you could
more
> specifically say "when the window is narrow" or the like?  Something like:
> 
> Each browser window will attempt to show browser actions as specified by this
> model, but if the window is too narrow, actions may end up pushed into the
> overflow menu on a per-window basis.  Callers interested in the arrangement of
> actions in a particular window should check that window's instance of
> ToolbarActionsBar, which is responsible for this per-window overflow.

Done.

https://codereview.chromium.org/1330423003/diff/80001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_model.h:139: bool all_icons_visible()
const {
On 2015/09/17 19:59:53, Peter Kasting wrote:
> Nit: Do you think you should put a warning on here about this being about the
> global default, and being inappropriate if you want to know what icons are
> actually visible to the user in any particular window?

Done.

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

https://codereview.chromium.org/1330423003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:47: // overflow
won't make the menu larger than the height of the screen.
On 2015/09/17 19:59:53, Peter Kasting wrote:
> Now that I read this comment I'm a bit worried.  If the window is near the
> bottom of the screen or the screen is short, how do we know 13 rows won't
still
> go off the bottom?

Views will actually make the menu go upwards if there's not enough room going
downwards, so this is only a concern on a screen that has too few pixels overall
- which should be *very* unlikely with this pixel size.

> Likewise if the screen is some huge portrait mode setup, we
> might have space for more rows?  If the core desire is to stay on the screen
it
> seems like you'd need to check the screen bounds.  If the core desires is just
> "don't make the menu excessively tall" irrespective of screen size, maybe the
> comment should be updated.

Comment updated.

https://codereview.chromium.org/1330423003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:49: // less than
0.0002% of our users will be affected.
On 2015/09/17 19:59:53, Peter Kasting wrote:
> Nit: I might just drop this part of the comment and leave that in the bug or
the
> CL description if future spelunkers really care; I don't think the code needs
> this justification, and I don't like putting time-sensitive comments in the
> code, because a future reader won't know whether the comment is still accurate
> or not anyway.

Done.

https://codereview.chromium.org/1330423003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:50: max_height_ =
ToolbarActionsBar::IconHeight() * 13;
On 2015/09/17 19:59:53, Peter Kasting wrote:
> Tiny nit: Consider adding a named kMaxOverflowRows constant defined right
here.

Done.

https://codereview.chromium.org/1330423003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:109: return
views::MenuItemView::label_start() - 1 -
On 2015/09/17 19:59:53, Peter Kasting wrote:
> You didn't actually drop the -1 here.

Could have sworn I did... thanks.

[Devlin, 2015-09-17 21:39:04]

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

[Devlin, 2015-09-17 21:39:04]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1330423003/#ps120001
(title: "Final nits")

[commit-bot: I haz the power, 2015-09-17 21:39:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1330423003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1330423003/120001

[commit-bot: I haz the power, 2015-09-17 21:47:39]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-09-17 21:48:37]

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