Browser actions container layout changes for M6 theme.

Browser actions container layout changes for M6 theme.

In short, this:
* Eliminates the divider on the right (the new chevron has a built-in divider)
* Changes the padding between items (see updated comments in header for precise layout details)
* Updates the imagery for the new theme
* Makes the chevron respond to theme changes

BUG=50107
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=54174

[Finnur, 2010-07-28 23:15:19]

General comment, while I read through this...

The changelist needs a much better description. I have no idea what the specific
intent of this CL is...

[Peter Kasting, 2010-07-28 23:23:03]

On 2010/07/28 23:15:19, Finnur wrote:
> General comment, while I read through this...
> 
> The changelist needs a much better description. I have no idea what the
specific
> intent of this CL is...

Are the comment updates in browser_actions_container.h not clear?  They describe
exactly what the new layout is.

[Finnur, 2010-07-28 23:54:34]

I was referring to the overall CL description. All it says is that you are
making layout changes, but you have to dig into the CL to find out the details
of what you are actually doing. 


On 2010/07/28 23:23:03, Peter Kasting wrote:
> On 2010/07/28 23:15:19, Finnur wrote:
> > General comment, while I read through this...
> > 
> > The changelist needs a much better description. I have no idea what the
> specific
> > intent of this CL is...
> 
> Are the comment updates in browser_actions_container.h not clear?  They
describe
> exactly what the new layout is.

[Peter Kasting, 2010-07-29 00:03:20]

On 2010/07/28 23:54:34, Finnur wrote:
> I was referring to the overall CL description. All it says is that you are
> making layout changes, but you have to dig into the CL to find out the details
> of what you are actually doing.

Right.  I normally assume code comments are what matters when reading the code
and I shouldn't duplicate them in the change description.  I'll add a summary to
this one though.

[Peter Kasting, 2010-07-29 01:14:35]

I made a small change to the TestSetIconVisibilityCount() function to keep the
model in sync with test-requested changes.  This fixes a break in a browser_test
for this container, where adding an action to a non-max-size container
erroneously enlarged it (because the model said it was at max size, and we
didn't double-check that).  However, I don't understand why the old code worked
-- I looked through my diffs and it seems like it should have suffered from the
same issue.

[Finnur, 2010-07-29 10:11:03]

Thanks for the CL description. Looks much better.

> (the new chevron has a built-in divider) 
> Updates the imagery for the new theme 

I presume those are already in since this CL doesn't include those?

I'd still like to play with a test build a bit, to see if I can find any corner
cases...

http://codereview.chromium.org/3057016/diff/1/2
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/3057016/diff/1/2#newcode526
chrome/browser/views/browser_actions_container.cc:526: return
gfx::Size(clamped_width, 0);
Seems a bit weird to return a 0 height, when you know it is clearly false...

http://codereview.chromium.org/3057016/diff/10001/11001#newcode48
chrome/browser/views/browser_actions_container.cc:48: static const int
kItemSpacing = ToolbarView::kStandardSpacing;
Document? Something like:
Horizontal spacing between icons in the browser action container.
?

http://codereview.chromium.org/3057016/diff/10001/11001#newcode49
chrome/browser/views/browser_actions_container.cc:49: static const int
kChevronSpacing = kItemSpacing - 2;
Document? Maybe: Spacing left of chevron (if chevron is visible)?

http://codereview.chromium.org/3057016/diff/10001/11001#newcode394
chrome/browser/views/browser_actions_container.cc:394: // We wait to set the
container width until now so that the chevron images
nit: space above this line?

http://codereview.chromium.org/3057016/diff/10001/11001#newcode537
chrome/browser/views/browser_actions_container.cc:537: IconHeight());
Don't you want the resize area to be just a little bit higher?  (I guess I need
the test build to see how it works).

http://codereview.chromium.org/3057016/diff/10001/11001#newcode639
chrome/browser/views/browser_actions_container.cc:639: // Next, we determine
which icon to place the indicator in front of.  We want
nit: space above this line?

http://codereview.chromium.org/3057016/diff/10001/11001#newcode650
chrome/browser/views/browser_actions_container.cc:650: // netween a and b, we
want to return "1".
typo: netween

http://codereview.chromium.org/3057016/diff/10001/11001#newcode673
chrome/browser/views/browser_actions_container.cc:673: return
DragDropTypes::DRAG_MOVE;
nit: linebreak above?

http://codereview.chromium.org/3057016/diff/10001/11001#newcode796
chrome/browser/views/browser_actions_container.cc:796: int max_size =
IconCountToWidth(-1, false);
Can't you just use max_width from above?

http://codereview.chromium.org/3057016/diff/10001/11001#newcode955
chrome/browser/views/browser_actions_container.cc:955: // the container size
(since we either removed an icon from the main
nit: be changing -> change?

http://codereview.chromium.org/3057016/diff/10001/11001#newcode1046
chrome/browser/views/browser_actions_container.cc:1046: int chevron_size =
display_chevron ?
nit: Don't need the outer braces at the end.

[Peter Kasting, 2010-07-29 18:38:30]

New snap up.  Comments addressed except as noted.

http://codereview.chromium.org/3057016/diff/10001/11001
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/3057016/diff/10001/11001#newcode537
chrome/browser/views/browser_actions_container.cc:537: IconHeight());
On 2010/07/29 10:11:03, Finnur wrote:
> Don't you want the resize area to be just a little bit higher?  (I guess I
need
> the test build to see how it works).

This value seems OK to me.  It will make the resize cursor appear when you're
"between" the omnibox and first icon (+1 pixel above and below due to built-in
padding/shadows in the images).  Having the resize cursor appear when higher or
lower than that, when there's no visible gripper, seems glitchy-looking.

http://codereview.chromium.org/3057016/diff/10001/11001#newcode796
chrome/browser/views/browser_actions_container.cc:796: int max_size =
IconCountToWidth(-1, false);
On 2010/07/29 10:11:03, Finnur wrote:
> Can't you just use max_width from above?

This whole function was a bit confusing.  Rewrote in a way that tries to make it
more clear when animation really needs to happen.

http://codereview.chromium.org/3057016/diff/10001/11001#newcode955
chrome/browser/views/browser_actions_container.cc:955: // If we have more icons
than we can show, then we must not be changing
On 2010/07/29 10:11:03, Finnur wrote:
> nit: be changing -> change?

No, "must not be changing" is what I meant.

http://codereview.chromium.org/3057016/diff/10001/11001#newcode1046
chrome/browser/views/browser_actions_container.cc:1046: (icons == 0) ? 0 :
((icons * IconWidth(true)) - kItemSpacing);
On 2010/07/29 10:11:03, Finnur wrote:
> nit: Don't need the outer braces at the end.

I'd rather keep them, since I've been bitten by weird precedence order issues
when using ?: before.

[Finnur, 2010-07-29 20:09:23]

LGTM. Thanks for the cleanup and for providing the test build. I played with it
and it looked solid. The only nit I could find was that the drop indicator
looked a bit too tall for my taste. I would shave 1px off the top and the
bottom, but I'll leave it up to you and the UX team. :)