[Chrome OS MD] Update spacing between material design icons in the system tray

Update spacing between material design icons in the system tray

Updated spacing between material design icons to 6dp; adjusted the
spacing between the hours and minutes when the clock is aligned
vertically.

TEST=MANUAL
- Measured the distance between tray items using gimp while setting
  the shelf alignment to bottom, right and left.

BUG=630296, 646975
Committed: https://crrev.com/d73de42d6da01d9c7c24c0b6573e89ce6bab3a3f
Cr-Commit-Position: refs/heads/master@{#423366}

[tdanderson, 2016-10-03 20:58:07]

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

[yiyix, 2016-10-05 19:03:03]

Patchset #1 (id:1) has been deleted

[yiyix, 2016-10-05 19:03:09]

Patchset #1 (id:20001) has been deleted

[yiyix, 2016-10-05 19:03:18]

Patchset #1 (id:40001) has been deleted

[yiyix, 2016-10-05 19:15:11]

Description was changed from

==========
rough WIP for adjusting the tray spacing

BUG=
==========

to

==========
Update spacing between material design icons in the system tray

Updated spacing between material design icons to 6dp; adjusted the 
spacing between the hours and minutes when the clock is aligned 
vertically.  

TEST=MANUAL
- Measured the distance between tray items using gimp while setting 
  the shelf alignment to bottom, right and left.

BUG=630296, 646975
==========

[yiyix, 2016-10-05 19:15:39]

@tdanderson, Could you please take a look?

[yiyix, 2016-10-05 19:16:04]

The CQ bit was checked by yiyix@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 19:16:31]

Dry run: 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-10-05 19:19:24]

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

[commit-bot: I haz the power, 2016-10-05 19:19:25]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tdanderson, 2016-10-05 21:34:29]

LGTM with the comments below addressed.

https://chromiumcodereview.appspot.com/2390003002/diff/60001/ash/common/syste...
File ash/common/system/date/date_view.cc (right):

https://chromiumcodereview.appspot.com/2390003002/diff/60001/ash/common/syste...
ash/common/system/date/date_view.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
As discussed in person, can you please file a bug along the lines of "take
internal icon padding into account when adjusting tray icon spacing" so that it
can be looked at as part of follow-up work?

https://chromiumcodereview.appspot.com/2390003002/diff/60001/ash/common/syste...
ash/common/system/date/date_view.cc:45: const int kVerticalClockMinutesTopOffset
= -2;
This constant is used on line 346 which can be executed in non-MD. Does changing
this constant to always be -4 fix a bug in non-MD? If not, you should keep this
as -4 for non-MD and only use the -2 value for MD.

https://chromiumcodereview.appspot.com/2390003002/diff/60001/ash/common/syste...
ash/common/system/date/date_view.cc:48: const int kClockOnTrayBackgroundPadding
= 8;
Since this is only used for a vertical shelf, can you rename the variable to
something like kClockVerticalShelfPadding (and update the comment accordingly)

https://chromiumcodereview.appspot.com/2390003002/diff/60001/ash/common/syste...
ash/common/system/date/date_view.cc:294: bool is_material_design =
MaterialDesignController::IsShelfMaterial();
nit: const

https://chromiumcodereview.appspot.com/2390003002/diff/60001/ash/common/syste...
ash/common/system/date/date_view.cc:330: } else {
Thanks for adding the {} !

https://chromiumcodereview.appspot.com/2390003002/diff/60001/ash/common/syste...
File ash/common/system/tray/tray_constants.h (right):

https://chromiumcodereview.appspot.com/2390003002/diff/60001/ash/common/syste...
ash/common/system/tray/tray_constants.h:121: // The padding needed between 2
items in the tray area in the shelf.
The spec says we need a padding of 6 between two items. It looks like you're
using this to specify the padding on one side of a single icon (which would give
a total between-icon padding of 3+3=6). That's fine, but can you reword the
comment to something like "extra padding beside a single icon" instead of
"padding between two items"?

also, nit: 2 -> "two"

https://chromiumcodereview.appspot.com/2390003002/diff/60001/ash/common/syste...
File ash/common/system/user/tray_user.cc (right):

https://chromiumcodereview.appspot.com/2390003002/diff/60001/ash/common/syste...
ash/common/system/user/tray_user.cc:183: bool is_horizontal =
nit: const

https://chromiumcodereview.appspot.com/2390003002/diff/60001/ash/common/syste...
ash/common/system/user/tray_user.cc:187: is_horizontal ? distance_to_avatar : 0,
0, 0));
nice use of ternary

[yiyix, 2016-10-05 22:46:47]

https://codereview.chromium.org/2390003002/diff/60001/ash/common/system/date/...
File ash/common/system/date/date_view.cc (right):

https://codereview.chromium.org/2390003002/diff/60001/ash/common/system/date/...
ash/common/system/date/date_view.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2016/10/05 21:34:28, tdanderson wrote:
> As discussed in person, can you please file a bug along the lines of "take
> internal icon padding into account when adjusting tray icon spacing" so that
it
> can be looked at as part of follow-up work?

Done, I also included a todo in the file.

https://codereview.chromium.org/2390003002/diff/60001/ash/common/system/date/...
ash/common/system/date/date_view.cc:45: const int kVerticalClockMinutesTopOffset
= -2;
On 2016/10/05 21:34:29, tdanderson wrote:
> This constant is used on line 346 which can be executed in non-MD. Does
changing
> this constant to always be -4 fix a bug in non-MD? If not, you should keep
this
> as -4 for non-MD and only use the -2 value for MD.

You are right. I forget about the non-md path.

https://codereview.chromium.org/2390003002/diff/60001/ash/common/system/date/...
ash/common/system/date/date_view.cc:48: const int kClockOnTrayBackgroundPadding
= 8;
It is used for both horizontal and vertical aligned shelf, in line 304 and line
323, respectively.

https://codereview.chromium.org/2390003002/diff/60001/ash/common/system/date/...
ash/common/system/date/date_view.cc:294: bool is_material_design =
MaterialDesignController::IsShelfMaterial();
On 2016/10/05 21:34:28, tdanderson wrote:
> nit: const

Done.

https://codereview.chromium.org/2390003002/diff/60001/ash/common/system/date/...
ash/common/system/date/date_view.cc:330: } else {
On 2016/10/05 21:34:28, tdanderson wrote:
> Thanks for adding the {} !

:)

https://codereview.chromium.org/2390003002/diff/60001/ash/common/system/tray/...
File ash/common/system/tray/tray_constants.h (right):

https://codereview.chromium.org/2390003002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/tray_constants.h:121: // The padding needed between 2
items in the tray area in the shelf.
On 2016/10/05 21:34:29, tdanderson wrote:
> The spec says we need a padding of 6 between two items. It looks like you're
> using this to specify the padding on one side of a single icon (which would
give
> a total between-icon padding of 3+3=6). That's fine, but can you reword the
> comment to something like "extra padding beside a single icon" instead of
> "padding between two items"?
> 
> also, nit: 2 -> "two"

Done.

https://codereview.chromium.org/2390003002/diff/60001/ash/common/system/user/...
File ash/common/system/user/tray_user.cc (right):

https://codereview.chromium.org/2390003002/diff/60001/ash/common/system/user/...
ash/common/system/user/tray_user.cc:183: bool is_horizontal =
On 2016/10/05 21:34:29, tdanderson wrote:
> nit: const

Done.

https://codereview.chromium.org/2390003002/diff/60001/ash/common/system/user/...
ash/common/system/user/tray_user.cc:187: is_horizontal ? distance_to_avatar : 0,
0, 0));
On 2016/10/05 21:34:29, tdanderson wrote:
> nice use of ternary

thanks.

[yiyix, 2016-10-05 22:49:51]

The CQ bit was checked by yiyix@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 22:50:17]

Dry run: 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-10-06 00:06:33]

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

[commit-bot: I haz the power, 2016-10-06 00:06:34]

Dry run: This issue passed the CQ dry run.

[yiyix, 2016-10-06 00:52:46]

The CQ bit was checked by yiyix@chromium.org

[yiyix, 2016-10-06 00:52:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2390003002/#ps80001
(title: "address nits")

[commit-bot: I haz the power, 2016-10-06 00:53:49]

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-10-06 01:00:44]

Description was changed from

==========
Update spacing between material design icons in the system tray

Updated spacing between material design icons to 6dp; adjusted the 
spacing between the hours and minutes when the clock is aligned 
vertically.  

TEST=MANUAL
- Measured the distance between tray items using gimp while setting 
  the shelf alignment to bottom, right and left.

BUG=630296, 646975
==========

to

==========
Update spacing between material design icons in the system tray

Updated spacing between material design icons to 6dp; adjusted the 
spacing between the hours and minutes when the clock is aligned 
vertically.  

TEST=MANUAL
- Measured the distance between tray items using gimp while setting 
  the shelf alignment to bottom, right and left.

BUG=630296, 646975
==========

[commit-bot: I haz the power, 2016-10-06 01:00:45]

Committed patchset #2 (id:80001)

[commit-bot: I haz the power, 2016-10-06 01:03:22]

Description was changed from

==========
Update spacing between material design icons in the system tray

Updated spacing between material design icons to 6dp; adjusted the 
spacing between the hours and minutes when the clock is aligned 
vertically.  

TEST=MANUAL
- Measured the distance between tray items using gimp while setting 
  the shelf alignment to bottom, right and left.

BUG=630296, 646975
==========

to

==========
Update spacing between material design icons in the system tray

Updated spacing between material design icons to 6dp; adjusted the
spacing between the hours and minutes when the clock is aligned
vertically.

TEST=MANUAL
- Measured the distance between tray items using gimp while setting
  the shelf alignment to bottom, right and left.

BUG=630296, 646975

Committed: https://crrev.com/d73de42d6da01d9c7c24c0b6573e89ce6bab3a3f
Cr-Commit-Position: refs/heads/master@{#423366}
==========

[commit-bot: I haz the power, 2016-10-06 01:03:23]

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