Change toolbar height for material design on Ash

Change toolbar height for material design on Ash

Render the tabstrip separator line
programmatically as 1 device pixel
(regardless of device scale factor) for
material design. This results in a
toolbar height of 37px for 'material'
and 41px for 'material hybrid', which
is the distance between the bottom
toolbar/content separator (inclusive)
and the tabstrip separator (exclusive).

BUG=527969
TEST=manual
R=pkasting
TBR=pkotwicz

Committed: https://crrev.com/0ef75c98477f3eec8a48ee897a01ee4ad9437b22
Cr-Commit-Position: refs/heads/master@{#348235}

[tdanderson, 2015-09-08 18:30:04]

tdanderson@chromium.org changed reviewers:
+ pkasting@chromium.org

[tdanderson, 2015-09-08 18:30:04]

Peter, can you please take a look?

[Peter Kasting, 2015-09-08 19:38:34]

Comments in ash frame view file apply to other files also.

https://codereview.chromium.org/1322323006/diff/1/chrome/browser/themes/theme...
File chrome/browser/themes/theme_properties.h (right):

https://codereview.chromium.org/1322323006/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_properties.h:141:
PROPERTY_TOOLBAR_VIEW_TOP_VERTICAL_PADDING,
Nit: top then bottom, please (here and elsewhere)

https://codereview.chromium.org/1322323006/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/1322323006/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:671:
IDR_THEME_TOOLBAR : IDR_TOOLBAR_SHADE_TOP);
It's not clear to me why the previous material code was avoiding drawing these
but you changed it to draw them.  Is MD drawing the toolbar by just doing a
rectangular fill with the toolbar color?  If so, this code seems wrong; if not,
where was this previously happening?

Even if we wanted to draw these, we'd want to do separate material and
non-material blocks, since the material block would just draw once instead of
doing all these piecewise bits.  But reply to the above first.

https://codereview.chromium.org/1322323006/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:704: // Draw
the tabstrip separator.
Just avoid drawing this at all, I'll take care of it.

https://codereview.chromium.org/1322323006/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/toolbar_view.cc (right):

https://codereview.chromium.org/1322323006/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_view.cc:110: int child_y =
(parent_height - child_height + 1) / 2;
Nit: Why not just:

  int roundoff_amount = ui::MaterialDesignController::IsModeMaterial() ? 0 : 1;
  return (parent_height - child_height + roundoff_amount ) / 2;

https://codereview.chromium.org/1322323006/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_view.cc:570: // padding above the
buttons.
Nit: Delete the last sentence here

https://codereview.chromium.org/1322323006/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_view.cc:781: // ThemeProperties. For
non-material the size is based on the provided
This description seems somewhat inaccurate.  We don't encode the size as a
constant, we compute the size based on given padding amounts.

[tdanderson, 2015-09-09 17:23:06]

Peter, please take another look.

https://codereview.chromium.org/1322323006/diff/1/chrome/browser/themes/theme...
File chrome/browser/themes/theme_properties.h (right):

https://codereview.chromium.org/1322323006/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_properties.h:141:
PROPERTY_TOOLBAR_VIEW_TOP_VERTICAL_PADDING,
On 2015/09/08 19:38:33, Peter Kasting wrote:
> Nit: top then bottom, please (here and elsewhere)

Done.

https://codereview.chromium.org/1322323006/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/1322323006/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:671:
IDR_THEME_TOOLBAR : IDR_TOOLBAR_SHADE_TOP);
On 2015/09/08 19:38:33, Peter Kasting wrote:
> It's not clear to me why the previous material code was avoiding drawing these
> but you changed it to draw them.  Is MD drawing the toolbar by just doing a
> rectangular fill with the toolbar color?  If so, this code seems wrong; if
not,
> where was this previously happening?

The previous material code was just a quick hack to stop rendering the shade
assets along the edges of the toolbar, but this came at the cost of the toolbar
appearing shorter than it actually is.

We're rendering the toolbar background by tiling IDR_THEME_TOOLBAR. But as you
pointed out, for MD it's not necessary to split this up into pieces (see the
next patch set). I also don't think it's necessary for MD have the call to
FillRect() on line 648.

> 
> Even if we wanted to draw these, we'd want to do separate material and
> non-material blocks, since the material block would just draw once instead of
> doing all these piecewise bits.  But reply to the above first.

Agreed, this code is messy and a lot of it is not necessary for MD. In the next
patch set I have separated MD and non-MD.

https://codereview.chromium.org/1322323006/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:704: // Draw
the tabstrip separator.
On 2015/09/08 19:38:33, Peter Kasting wrote:
> Just avoid drawing this at all, I'll take care of it.

I included this for the purposes of screenshots / agreement over where the
separator should be. I've removed it for each frame.

https://codereview.chromium.org/1322323006/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/toolbar_view.cc (right):

https://codereview.chromium.org/1322323006/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_view.cc:110: int child_y =
(parent_height - child_height + 1) / 2;
On 2015/09/08 19:38:34, Peter Kasting wrote:
> Nit: Why not just:
> 
>   int roundoff_amount = ui::MaterialDesignController::IsModeMaterial() ? 0 :
1;
>   return (parent_height - child_height + roundoff_amount ) / 2;

Changed.

https://codereview.chromium.org/1322323006/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_view.cc:570: // padding above the
buttons.
On 2015/09/08 19:38:34, Peter Kasting wrote:
> Nit: Delete the last sentence here

Done.

https://codereview.chromium.org/1322323006/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_view.cc:781: // ThemeProperties. For
non-material the size is based on the provided
On 2015/09/08 19:38:33, Peter Kasting wrote:
> This description seems somewhat inaccurate.  We don't encode the size as a
> constant, we compute the size based on given padding amounts.

Re-worded.

[Peter Kasting, 2015-09-09 22:41:53]

https://codereview.chromium.org/1322323006/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_properties.h (right):

https://codereview.chromium.org/1322323006/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_properties.h:141:
PROPERTY_TOOLBAR_VIEW_BOTTOM_VERTICAL_PADDING,
Now that I look at this list I see that all the other items are alphabetical.

So you probably should actually list this as the first PROPERTY_TOOLBAR_VIEW_
item.  Sorry :/.  But, see comments in toolbar_view.cc.

https://codereview.chromium.org/1322323006/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

https://codereview.chromium.org/1322323006/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:363: const bool
mode_material = ui::MaterialDesignController::IsModeMaterial();
When I asked for the opaque and glass frames to be updated, I intended for you
to make a much more substantial change, like the one you've made in the ash
frame, to actually bypass all the existing code in MD and instead draw exactly
what we need where in a simple fashion.

That would still be helpful, so if you can do that, please go ahead.

Otherwise, just revert all the changes in this file and the opaque frame.  The
only functional changes you're making here won't have any effect in my client
anyway since I'm entirely bypassing the code you're modifying, since it's
clearly not what we're going to want in MD.  It will mean more work for me as
I'll need to do the changes I refer to above myself based on your work in the
ash frame, but I'd rather that than add the hacks here, which to some degree
obscure what the correct implementation should be.

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

https://codereview.chromium.org/1322323006/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_view.cc:784:
ThemeProperties::PROPERTY_TOOLBAR_VIEW_BOTTOM_VERTICAL_PADDING);
If this is the only place you use this constant you might as well just define
the value locally here instead of in the theme provider, unless you'll need this
elsewhere later.

https://codereview.chromium.org/1322323006/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_view.cc:786: gfx::Size(0, content_height
+ (top_padding + bottom_padding)));
Nit: No need for parens around final addition

[tdanderson, 2015-09-10 19:48:26]

Peter, please take a look at the next patch set.

https://codereview.chromium.org/1322323006/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_properties.h (right):

https://codereview.chromium.org/1322323006/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_properties.h:141:
PROPERTY_TOOLBAR_VIEW_BOTTOM_VERTICAL_PADDING,
On 2015/09/09 22:41:53, Peter Kasting wrote:
> Now that I look at this list I see that all the other items are alphabetical.
> 
> So you probably should actually list this as the first PROPERTY_TOOLBAR_VIEW_
> item.  Sorry :/.  But, see comments in toolbar_view.cc.

Done.

https://codereview.chromium.org/1322323006/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

https://codereview.chromium.org/1322323006/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:363: const bool
mode_material = ui::MaterialDesignController::IsModeMaterial();
On 2015/09/09 22:41:53, Peter Kasting wrote:
> When I asked for the opaque and glass frames to be updated, I intended for you
> to make a much more substantial change, like the one you've made in the ash
> frame, to actually bypass all the existing code in MD and instead draw exactly
> what we need where in a simple fashion.
> 
> That would still be helpful, so if you can do that, please go ahead.
> 
> Otherwise, just revert all the changes in this file and the opaque frame.  The
> only functional changes you're making here won't have any effect in my client
> anyway since I'm entirely bypassing the code you're modifying, since it's
> clearly not what we're going to want in MD.  It will mean more work for me as
> I'll need to do the changes I refer to above myself based on your work in the
> ash frame, but I'd rather that than add the hacks here, which to some degree
> obscure what the correct implementation should be.

Reverted these changes for now, though I'd like to follow up with you offline
about this.

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

https://codereview.chromium.org/1322323006/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_view.cc:784:
ThemeProperties::PROPERTY_TOOLBAR_VIEW_BOTTOM_VERTICAL_PADDING);
On 2015/09/09 22:41:53, Peter Kasting wrote:
> If this is the only place you use this constant you might as well just define
> the value locally here instead of in the theme provider, unless you'll need
this
> elsewhere later.

I'm not sure whether or not I'd need it later, but regardless I'd prefer to keep
it in the theme provider so that the top padding and bottom padding are defined
in the same place. But I can define it locally if you have strong feelings
otherwise.

https://codereview.chromium.org/1322323006/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_view.cc:786: gfx::Size(0, content_height
+ (top_padding + bottom_padding)));
On 2015/09/09 22:41:53, Peter Kasting wrote:
> Nit: No need for parens around final addition

Done.

[Peter Kasting, 2015-09-10 20:21:29]

LGTM

[tdanderson, 2015-09-10 20:26:11]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2015-09-10 20:26:25]

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

[commit-bot: I haz the power, 2015-09-10 21:11:49]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-10 21:12:38]

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

[commit-bot: I haz the power, 2015-09-23 12:14:15]

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