Give Ash material design tray items the correct size and layout

Give Ash material design tray items the correct size and layout

Modify the size and layout of the items that appear
in the material design tray so that they conform
to the MD specs (notifications button, system tray, etc.)
This CL should only have user-visible changes for
material design, which is currently off by default behind
the --ash-md flag.

This CL also cleans up many constants specified in
tray_constants.h.

BUG=617295
TEST=manual

Committed: https://crrev.com/5ae209e5c9a10d9896b2b9767056f88c4cf93430
Cr-Commit-Position: refs/heads/master@{#402587}

[tdanderson, 2016-06-27 00:56:37]

tdanderson@chromium.org changed reviewers:
+ jamescook@chromium.org, yiyix@chromium.org

[tdanderson, 2016-06-27 00:56:38]

James and Yi, can you please take a look?

[James Cook, 2016-06-27 18:04:12]

https://codereview.chromium.org/2099103002/diff/20001/ash/shelf/app_list_butt...
File ash/shelf/app_list_button.cc (right):

https://codereview.chromium.org/2099103002/diff/20001/ash/shelf/app_list_butt...
ash/shelf/app_list_button.cc:144: const int kRadius = kTrayItemSize / 2;
nit: elsewhere you use the style "const int radius =". Either is fine, but be
consistent.

https://codereview.chromium.org/2099103002/diff/20001/ash/shelf/overflow_butt...
File ash/shelf/overflow_button.cc (right):

https://codereview.chromium.org/2099103002/diff/20001/ash/shelf/overflow_butt...
ash/shelf/overflow_button.cc:146: bounds = gfx::Rect((bounds.x() +
(bounds.width() - kTrayItemSize) / 2),
It's odd that you're using a constant named "tray" in code that is sizing a
shelf item that isn't related to the system tray. Maybe continue to use separate
constants? Or come up with a name that implies both?

[yiyix, 2016-06-27 18:39:43]

https://codereview.chromium.org/2099103002/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/tray_constants.cc (right):

https://codereview.chromium.org/2099103002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_constants.cc:94: return kTrayItemHeightLegacy[mode];
Is it the idea that we will use variable kTrayItemSize instead when we remove
no-md code?

https://codereview.chromium.org/2099103002/diff/20001/ash/shelf/overflow_butt...
File ash/shelf/overflow_button.cc (right):

https://codereview.chromium.org/2099103002/diff/20001/ash/shelf/overflow_butt...
ash/shelf/overflow_button.cc:150: bounds = gfx::Rect(bounds.x() +
((bounds.height() - kTrayItemSize) / 2),
May create a constant for (bounds.height() - kTrayItemSize) / 2 and
(bounds.width() - kTrayItemSize) / 2)?

https://codereview.chromium.org/2099103002/diff/20001/ash/system/chromeos/vir...
File ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right):

https://codereview.chromium.org/2099103002/diff/20001/ash/system/chromeos/vir...
ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:70: const int
bottom_padding = size - image.height() - top_padding;
size - image.height() - top_padding = (size - image.height()) / 2
may be use the later one for less computation?

https://codereview.chromium.org/2099103002/diff/20001/ash/system/status_area_...
File ash/system/status_area_widget_delegate.cc (right):

https://codereview.chromium.org/2099103002/diff/20001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:204: : padding;
Nice simplification!

[tdanderson, 2016-06-27 21:29:43]

Thanks for the comments. Can you please take another look?

https://codereview.chromium.org/2099103002/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/tray_constants.cc (right):

https://codereview.chromium.org/2099103002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_constants.cc:94: return kTrayItemHeightLegacy[mode];
On 2016/06/27 18:39:43, yiyix wrote:
> Is it the idea that we will use variable kTrayItemSize instead when we remove
> no-md code?

Yes, once we delete the non-MD code then |kTrayItemSize| is the only one that
will remain.

https://codereview.chromium.org/2099103002/diff/20001/ash/shelf/app_list_butt...
File ash/shelf/app_list_button.cc (right):

https://codereview.chromium.org/2099103002/diff/20001/ash/shelf/app_list_butt...
ash/shelf/app_list_button.cc:144: const int kRadius = kTrayItemSize / 2;
On 2016/06/27 18:04:12, James Cook wrote:
> nit: elsewhere you use the style "const int radius =". Either is fine, but be
> consistent.

Done.

https://codereview.chromium.org/2099103002/diff/20001/ash/shelf/overflow_butt...
File ash/shelf/overflow_button.cc (right):

https://codereview.chromium.org/2099103002/diff/20001/ash/shelf/overflow_butt...
ash/shelf/overflow_button.cc:146: bounds = gfx::Rect((bounds.x() +
(bounds.width() - kTrayItemSize) / 2),
On 2016/06/27 18:04:12, James Cook wrote:
> It's odd that you're using a constant named "tray" in code that is sizing a
> shelf item that isn't related to the system tray. Maybe continue to use
separate
> constants? Or come up with a name that implies both?

This has been in the back of my mind for a while now. I realize "the shelf" and
"the tray [area]" are two different surfaces, but I definitely want to share the
same constant to reinforce the fact that 32dp is a standard size used in MD.
Unfortunately I can't think of any (good) names that don't contain either
"shelf" or "tray". 

Then there is the question of where the constant should live; there is a handful
of constants common to both (button size, icon color, button background when
shelf is transparent, etc) that may benefit from being moved to a new constants
file.

WDYT?

https://codereview.chromium.org/2099103002/diff/20001/ash/shelf/overflow_butt...
ash/shelf/overflow_button.cc:150: bounds = gfx::Rect(bounds.x() +
((bounds.height() - kTrayItemSize) / 2),
On 2016/06/27 18:39:43, yiyix wrote:
> May create a constant for (bounds.height() - kTrayItemSize) / 2 and
> (bounds.width() - kTrayItemSize) / 2)?

Done.

https://codereview.chromium.org/2099103002/diff/20001/ash/system/chromeos/vir...
File ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right):

https://codereview.chromium.org/2099103002/diff/20001/ash/system/chromeos/vir...
ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:70: const int
bottom_padding = size - image.height() - top_padding;
On 2016/06/27 18:39:43, yiyix wrote:
> size - image.height() - top_padding = (size - image.height()) / 2
> may be use the later one for less computation?

Thanks for the suggestion - I can actually do one better than this since it
works out that bottom_padding = top_padding :) This looks a lot nicer in the
next patch set.

Changed here and in overview_button_tray

https://codereview.chromium.org/2099103002/diff/20001/ash/system/status_area_...
File ash/system/status_area_widget_delegate.cc (right):

https://codereview.chromium.org/2099103002/diff/20001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:204: : padding;
On 2016/06/27 18:39:43, yiyix wrote:
> Nice simplification!

Thanks!

[James Cook, 2016-06-27 22:53:21]

https://codereview.chromium.org/2099103002/diff/20001/ash/shelf/overflow_butt...
File ash/shelf/overflow_button.cc (right):

https://codereview.chromium.org/2099103002/diff/20001/ash/shelf/overflow_butt...
ash/shelf/overflow_button.cc:146: bounds = gfx::Rect((bounds.x() +
(bounds.width() - kTrayItemSize) / 2),
On 2016/06/27 21:29:42, tdanderson wrote:
> On 2016/06/27 18:04:12, James Cook wrote:
> > It's odd that you're using a constant named "tray" in code that is sizing a
> > shelf item that isn't related to the system tray. Maybe continue to use
> separate
> > constants? Or come up with a name that implies both?
> 
> This has been in the back of my mind for a while now. I realize "the shelf"
and
> "the tray [area]" are two different surfaces, but I definitely want to share
the
> same constant to reinforce the fact that 32dp is a standard size used in MD.
> Unfortunately I can't think of any (good) names that don't contain either
> "shelf" or "tray". 
> 
> Then there is the question of where the constant should live; there is a
handful
> of constants common to both (button size, icon color, button background when
> shelf is transparent, etc) that may benefit from being moved to a new
constants
> file.
> 
> WDYT?

Yeah, I can't think of a good shared name either.

Visually the "shelf" is a superset of shelf+tray, so if there are shared
constants I would name them "shelf" and stick them somewhere shelf-y.

[yiyix, 2016-06-28 15:56:31]

LGTM

[tdanderson, 2016-06-28 16:32:44]

James, please take a look

https://codereview.chromium.org/2099103002/diff/20001/ash/shelf/overflow_butt...
File ash/shelf/overflow_button.cc (right):

https://codereview.chromium.org/2099103002/diff/20001/ash/shelf/overflow_butt...
ash/shelf/overflow_button.cc:146: bounds = gfx::Rect((bounds.x() +
(bounds.width() - kTrayItemSize) / 2),
On 2016/06/27 22:53:21, James Cook wrote:
> On 2016/06/27 21:29:42, tdanderson wrote:
> > On 2016/06/27 18:04:12, James Cook wrote:
> > > It's odd that you're using a constant named "tray" in code that is sizing
a
> > > shelf item that isn't related to the system tray. Maybe continue to use
> > separate
> > > constants? Or come up with a name that implies both?
> > 
> > This has been in the back of my mind for a while now. I realize "the shelf"
> and
> > "the tray [area]" are two different surfaces, but I definitely want to share
> the
> > same constant to reinforce the fact that 32dp is a standard size used in MD.
> > Unfortunately I can't think of any (good) names that don't contain either
> > "shelf" or "tray". 
> > 
> > Then there is the question of where the constant should live; there is a
> handful
> > of constants common to both (button size, icon color, button background when
> > shelf is transparent, etc) that may benefit from being moved to a new
> constants
> > file.
> > 
> > WDYT?
> 
> Yeah, I can't think of a good shared name either.
> 
> Visually the "shelf" is a superset of shelf+tray, so if there are shared
> constants I would name them "shelf" and stick them somewhere shelf-y.

As discussed, in this CL I will un-blur the lines between the shelf using tray
constants and vice versa. However the longer term plan is to refactor all common
constants to a separate place. Filed crbug.com/623987 to track that work.

[James Cook, 2016-06-28 17:25:17]

LGTM

[tdanderson, 2016-06-28 17:30:44]

The CQ bit was checked by tdanderson@chromium.org

[tdanderson, 2016-06-28 17:30:45]

The patchset sent to the CQ was uploaded after l-g-t-m from yiyix@chromium.org
Link to the patchset: https://codereview.chromium.org/2099103002/#ps60001
(title: "split shelf and tray constants")

[commit-bot: I haz the power, 2016-06-28 17:32:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 19:44:19]

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

[commit-bot: I haz the power, 2016-06-28 19:44:20]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tdanderson, 2016-06-28 19:48:11]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2016-06-28 19:49:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 20:04:30]

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

[commit-bot: I haz the power, 2016-06-28 20:04:31]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tdanderson, 2016-06-28 20:05:40]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2016-06-28 20:06:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 23:14:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-28 23:15:54]

Description was changed from

==========
Give Ash material design tray items the correct size and layout

Modify the size and layout of the items that appear
in the material design tray so that they conform
to the MD specs (notifications button, system tray, etc.)
This CL should only have user-visible changes for
material design, which is currently off by default behind
the --ash-md flag.

This CL also cleans up many constants specified in
tray_constants.h.

BUG=617295
TEST=manual
==========

to

==========
Give Ash material design tray items the correct size and layout

Modify the size and layout of the items that appear
in the material design tray so that they conform
to the MD specs (notifications button, system tray, etc.)
This CL should only have user-visible changes for
material design, which is currently off by default behind
the --ash-md flag.

This CL also cleans up many constants specified in
tray_constants.h.

BUG=617295
TEST=manual

Committed: https://crrev.com/5ae209e5c9a10d9896b2b9767056f88c4cf93430
Cr-Commit-Position: refs/heads/master@{#402587}
==========

[commit-bot: I haz the power, 2016-06-28 23:15:56]

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