Update shelf spacing in Chrome OS according to the MD specs

Update shelf spacing in Chrome OS according to the MD specs:
- Update the height of the shelf.
- Update the spacing between icons in the shelf.
- Update the spacing around status tray area, notification center, app list button and overview button.
- Update the spacing between the status tray area and notification center.
- Update the spacing around the app list button.

TEST=manual
1. Uses GIMP to measure Shelf Spacing requirements when Shelf is positioned to Left, Right and Bottom in Chrome OS .

BUG=595005
Committed: https://crrev.com/20e7d3f385060e0c2de2a86d5dcf81c4c0ca8c8f
Cr-Commit-Position: refs/heads/master@{#399792}

[yiyix, 2016-05-20 05:19:27]

Description was changed from

==========
Update shelf spacing according to the MD specs:
- Update the height of the shelf.
- Update the spacing between icons in the shelf.
- Update the spacing around status tray area, notification center, app list
button and overview button.
- Update the spacing between the status tray area and notification center.
- Update the spacing around the app list button.

BUG=595005
==========

to

==========
Update shelf spacing in Chrome OS according to the MD specs:
- Update the height of the shelf.
- Update the spacing between icons in the shelf.
- Update the spacing around status tray area, notification center, app list
button and overview button.
- Update the spacing between the status tray area and notification center.
- Update the spacing around the app list button.

BUG=595005
==========

[yiyix, 2016-05-20 05:19:52]

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

[commit-bot: I haz the power, 2016-05-20 05:22:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1998933002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1998933002/1

[commit-bot: I haz the power, 2016-05-20 05:59:50]

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

[commit-bot: I haz the power, 2016-05-20 05:59:51]

Dry run: This issue passed the CQ dry run.

[yiyix, 2016-05-20 14:48:57]

yiyix@chromium.org changed reviewers:
+ bruthig@chromium.org, tdanderson@chromium.org, varkha@chromium.org

[yiyix, 2016-05-20 18:54:49]

Description was changed from

==========
Update shelf spacing in Chrome OS according to the MD specs:
- Update the height of the shelf.
- Update the spacing between icons in the shelf.
- Update the spacing around status tray area, notification center, app list
button and overview button.
- Update the spacing between the status tray area and notification center.
- Update the spacing around the app list button.

BUG=595005
==========

to

==========
Update shelf spacing in Chrome OS according to the MD specs:
- Update the height of the shelf.
- Update the spacing between icons in the shelf.
- Update the spacing around status tray area, notification center, app list
button and overview button.
- Update the spacing between the status tray area and notification center.
- Update the spacing around the app list button.

TEST=manual
1. Uses GIMP to measure Shelf Spacing requirements when Shelf is positioned to
Left, Right and Bottom in Chrome OS .

BUG=595005
==========

[bruthig, 2016-05-25 20:24:53]

I'll let you update the CL so that the AppLauncher button and
StatusTray/Overview target-able bounds extend to the left/right edges and then
take another look.

https://codereview.chromium.org/1998933002/diff/1/ash/root_window_controller_...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/1998933002/diff/1/ash/root_window_controller_...
ash/root_window_controller_unittest.cc:11: #include
"ash/shelf/shelf_constants.h"
Is this needed?  This file doesn't appear to actually reference anything from
shelf_constants.h other than the comment.

https://codereview.chromium.org/1998933002/diff/1/ash/shelf/shelf_constants.cc
File ash/shelf/shelf_constants.cc (right):

https://codereview.chromium.org/1998933002/diff/1/ash/shelf/shelf_constants.c...
ash/shelf/shelf_constants.cc:21: int mode =
MaterialDesignController::Mode::NON_MATERIAL;
Alternatively replace lines 21-23 using the ternary operator like so:

int mode = MaterialDesignController::IsShelfMaterial() ?
MaterialDesignController::Mode::MATERIAL_NORMAL :
MaterialDesignController::Mode::NON_MATERIAL;

https://codereview.chromium.org/1998933002/diff/1/ash/shelf/shelf_constants.h
File ash/shelf/shelf_constants.h (right):

https://codereview.chromium.org/1998933002/diff/1/ash/shelf/shelf_constants.h...
ash/shelf/shelf_constants.h:13: // Size of the shelf when visible (height when
the shelf is horizontal).
nit: Can you add 'and width when the shelf is vertical'

https://codereview.chromium.org/1998933002/diff/1/ash/shelf/shelf_constants.h...
ash/shelf/shelf_constants.h:30: extern const int kWorkspaceAreaVisibleInset;
Please wrap these three constants in a class.  'kAutoHideSize' is too general of
a variable name to be in the ash namespace.

It might make sense to move the GetShelfLayoutConstants() function to this class
too, i.e.  ShelfLayoutConstants::Get(ShelfLayoutConstant).

https://codereview.chromium.org/1998933002/diff/1/ash/shelf/shelf_layout_mana...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/1998933002/diff/1/ash/shelf/shelf_layout_mana...
ash/shelf/shelf_layout_manager.h:73: SHELF_ITEM_INSET,
Can this be moved to the ShelfLayoutConstants enum instead?

https://codereview.chromium.org/1998933002/diff/1/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/1998933002/diff/1/ash/shell.cc#newcode39
ash/shell.cc:39: #include "ash/material_design/material_design_controller.h"
Is this needed/used?

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_backgr...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_backgr...
ash/system/tray/tray_background_view.cc:199:
SetBorder(views::Border::CreateEmptyBorder(
There appears to be quite a bit of duplicated code here.  From what I can tell
the only difference between the 'if' and the 'else' is the first parameter
passed to BoxLayout() c'tor.

Feel free to simplify this code :)

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_consta...
File ash/system/tray/tray_constants.h (right):

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_consta...
ash/system/tray/tray_constants.h:25: extern const int kEmptyBoarderPadding;
sp: Boarder->Border

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_consta...
ash/system/tray/tray_constants.h:34: PADDING_FROM_EDGE_TO_OVERVIEW_BUTTON,
Remove this value and use PADDING_FROM_EDGE_OF_SHELF.

https://codereview.chromium.org/1998933002/diff/1/chrome/test/base/view_event...
File chrome/test/base/view_event_test_platform_part_chromeos.cc (right):

https://codereview.chromium.org/1998933002/diff/1/chrome/test/base/view_event...
chrome/test/base/view_event_test_platform_part_chromeos.cc:61:
ash::MaterialDesignController::Initialize();
This test fixture should clean up after itself by calling
MaterialDesignControllerTestAPI::Uninitialize() from the destructor.

[tdanderson, 2016-05-26 22:55:57]

I have a few comments to add below:

https://codereview.chromium.org/1998933002/diff/1/ash/shelf/overflow_button.cc
File ash/shelf/overflow_button.cc (right):

https://codereview.chromium.org/1998933002/diff/1/ash/shelf/overflow_button.c...
ash/shelf/overflow_button.cc:98: // If Material Design for Chrome OS shelf is
enabled.
nit: remove comment. (similarly in app_list_button.cc)

https://codereview.chromium.org/1998933002/diff/1/ash/shelf/overflow_button.c...
ash/shelf/overflow_button.cc:101: alignment == wm::SHELF_ALIGNMENT_RIGHT) {
nit: can you instead use !wm::IsHorizontalAlignment(alignment) ? (similarly in
app_list_button.cc)

https://codereview.chromium.org/1998933002/diff/1/ash/shelf/shelf_constants.h
File ash/shelf/shelf_constants.h (right):

https://codereview.chromium.org/1998933002/diff/1/ash/shelf/shelf_constants.h...
ash/shelf/shelf_constants.h:30: extern const int kWorkspaceAreaVisibleInset;
On 2016/05/25 20:24:52, bruthig wrote:
> Please wrap these three constants in a class.  'kAutoHideSize' is too general
of
> a variable name to be in the ash namespace.

For the purposes of this CL, I would actually refrain from touching these three
constants at all; they have the same values in both pre-MD and MD.

> 
> It might make sense to move the GetShelfLayoutConstants() function to this
class
> too, i.e.  ShelfLayoutConstants::Get(ShelfLayoutConstant).

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_backgr...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_backgr...
ash/system/tray/tray_background_view.cc:424: ?
GetShelfStatusAreaInset(PADDING_FROM_EDGE_TO_OVERVIEW_BUTTON)
I think I have a better idea of what's happening here. Let's discuss in person
tomorrow.

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_consta...
File ash/system/tray/tray_constants.cc (right):

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_consta...
ash/system/tray/tray_constants.cc:26: // Padding used on empty border to Adjust
the size of status tray dark
nit: Adjust -> adjust

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_consta...
ash/system/tray/tray_constants.cc:27: // background.
Also, I am not a fan of how the comment is worded. Let's discuss in person.

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_consta...
ash/system/tray/tray_constants.cc:92: int
GetShelfStatusAreaInset(ShelfStatusAreaInset constant) {
I would just call this GetTrayConstant(). Similarly rename ShelfStatusAreaInset.

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_consta...
ash/system/tray/tray_constants.cc:106: case
PADDING_FROM_EDGE_TO_OVERVIEW_BUTTON:
nit: be consistent with the naming of the constants and the naming of the enum
items. (here and at line 102)

https://codereview.chromium.org/1998933002/diff/1/ash/system/web_notification...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/1998933002/diff/1/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:257: std::max(0, max_height
- kMessageCenterBubblePadding));
I could be mistaken but shouldn't this still be
GetShelfStatusAreaInset(TRAY_SPACING)?

https://codereview.chromium.org/1998933002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/launcher_favicon_loader.cc (right):

https://codereview.chromium.org/1998933002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/launcher_favicon_loader.cc:25: // link tag,
storing the one that best matches ash::SHELF_SIZE.
nit: ash::ShelfLayoutConstant::SHELF_SIZE.

[yiyix, 2016-06-02 03:54:54]

https://codereview.chromium.org/1998933002/diff/1/ash/root_window_controller_...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/1998933002/diff/1/ash/root_window_controller_...
ash/root_window_controller_unittest.cc:11: #include
"ash/shelf/shelf_constants.h"
On 2016/05/25 20:24:52, bruthig wrote:
> Is this needed?  This file doesn't appear to actually reference anything from
> shelf_constants.h other than the comment.

Done.

https://codereview.chromium.org/1998933002/diff/1/ash/shelf/overflow_button.cc
File ash/shelf/overflow_button.cc (right):

https://codereview.chromium.org/1998933002/diff/1/ash/shelf/overflow_button.c...
ash/shelf/overflow_button.cc:98: // If Material Design for Chrome OS shelf is
enabled.
On 2016/05/26 22:55:56, tdanderson wrote:
> nit: remove comment. (similarly in app_list_button.cc)

Done.

https://codereview.chromium.org/1998933002/diff/1/ash/shelf/overflow_button.c...
ash/shelf/overflow_button.cc:101: alignment == wm::SHELF_ALIGNMENT_RIGHT) {
On 2016/05/26 22:55:56, tdanderson wrote:
> nit: can you instead use !wm::IsHorizontalAlignment(alignment) ? (similarly in
> app_list_button.cc)

https://codereview.chromium.org/1998933002/diff/1/ash/shelf/shelf_constants.cc
File ash/shelf/shelf_constants.cc (right):

https://codereview.chromium.org/1998933002/diff/1/ash/shelf/shelf_constants.c...
ash/shelf/shelf_constants.cc:21: int mode =
MaterialDesignController::Mode::NON_MATERIAL;
On 2016/05/25 20:24:52, bruthig wrote:
> Alternatively replace lines 21-23 using the ternary operator like so:
> 
> int mode = MaterialDesignController::IsShelfMaterial() ?
> MaterialDesignController::Mode::MATERIAL_NORMAL :
> MaterialDesignController::Mode::NON_MATERIAL; 

Done.

https://codereview.chromium.org/1998933002/diff/1/ash/shelf/shelf_constants.h
File ash/shelf/shelf_constants.h (right):

https://codereview.chromium.org/1998933002/diff/1/ash/shelf/shelf_constants.h...
ash/shelf/shelf_constants.h:13: // Size of the shelf when visible (height when
the shelf is horizontal).
On 2016/05/25 20:24:52, bruthig wrote:
> nit: Can you add 'and width when the shelf is vertical'

Done.

https://codereview.chromium.org/1998933002/diff/1/ash/shelf/shelf_constants.h...
ash/shelf/shelf_constants.h:30: extern const int kWorkspaceAreaVisibleInset;
On 2016/05/26 22:55:56, tdanderson wrote:
> On 2016/05/25 20:24:52, bruthig wrote:
> > Please wrap these three constants in a class.  'kAutoHideSize' is too
general
> of
> > a variable name to be in the ash namespace.
> 
> For the purposes of this CL, I would actually refrain from touching these
three
> constants at all; they have the same values in both pre-MD and MD.
> 
> > 
> > It might make sense to move the GetShelfLayoutConstants() function to this
> class
> > too, i.e.  ShelfLayoutConstants::Get(ShelfLayoutConstant).
> 

I wanted to move most of the shelf property related constants into this
shelf_constansts.h file. So I moved them here.

https://codereview.chromium.org/1998933002/diff/1/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/1998933002/diff/1/ash/shell.cc#newcode39
ash/shell.cc:39: #include "ash/material_design/material_design_controller.h"
On 2016/05/25 20:24:52, bruthig wrote:
> Is this needed/used?

Done.

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_backgr...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_backgr...
ash/system/tray/tray_background_view.cc:199:
SetBorder(views::Border::CreateEmptyBorder(
On 2016/05/25 20:24:52, bruthig wrote:
> There appears to be quite a bit of duplicated code here.  From what I can tell
> the only difference between the 'if' and the 'else' is the first parameter
> passed to BoxLayout() c'tor.
> 
> Feel free to simplify this code :)

Done.

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_backgr...
ash/system/tray/tray_background_view.cc:424: ?
GetShelfStatusAreaInset(PADDING_FROM_EDGE_TO_OVERVIEW_BUTTON)
On 2016/05/26 22:55:56, tdanderson wrote:
> I think I have a better idea of what's happening here. Let's discuss in person
> tomorrow.

We have discussed it in person earlier this week.

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_consta...
File ash/system/tray/tray_constants.cc (right):

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_consta...
ash/system/tray/tray_constants.cc:26: // Padding used on empty border to Adjust
the size of status tray dark
On 2016/05/26 22:55:56, tdanderson wrote:
> nit: Adjust -> adjust

Done.

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_consta...
ash/system/tray/tray_constants.cc:27: // background.
On 2016/05/26 22:55:56, tdanderson wrote:
> Also, I am not a fan of how the comment is worded. Let's discuss in person.

Done. I update the name as we have discussed in person.

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_consta...
ash/system/tray/tray_constants.cc:92: int
GetShelfStatusAreaInset(ShelfStatusAreaInset constant) {
On 2016/05/26 22:55:56, tdanderson wrote:
> I would just call this GetTrayConstant(). Similarly rename
ShelfStatusAreaInset.

Done.

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_consta...
ash/system/tray/tray_constants.cc:106: case
PADDING_FROM_EDGE_TO_OVERVIEW_BUTTON:
On 2016/05/26 22:55:56, tdanderson wrote:
> nit: be consistent with the naming of the constants and the naming of the enum
> items. (here and at line 102)

Done.

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_consta...
File ash/system/tray/tray_constants.h (right):

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_consta...
ash/system/tray/tray_constants.h:25: extern const int kEmptyBoarderPadding;
On 2016/05/25 20:24:53, bruthig wrote:
> sp: Boarder->Border

Done.

https://codereview.chromium.org/1998933002/diff/1/ash/system/tray/tray_consta...
ash/system/tray/tray_constants.h:34: PADDING_FROM_EDGE_TO_OVERVIEW_BUTTON,
On 2016/05/25 20:24:53, bruthig wrote:
> Remove this value and use PADDING_FROM_EDGE_OF_SHELF.

Done.

https://codereview.chromium.org/1998933002/diff/1/ash/system/web_notification...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/1998933002/diff/1/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:257: std::max(0, max_height
- kMessageCenterBubblePadding));
On 2016/05/26 22:55:56, tdanderson wrote:
> I could be mistaken but shouldn't this still be
> GetShelfStatusAreaInset(TRAY_SPACING)?

You are right.

https://codereview.chromium.org/1998933002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/launcher_favicon_loader.cc (right):

https://codereview.chromium.org/1998933002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/launcher_favicon_loader.cc:25: // link tag,
storing the one that best matches ash::SHELF_SIZE.
On 2016/05/26 22:55:56, tdanderson wrote:
> nit: ash::ShelfLayoutConstant::SHELF_SIZE.

Done.

https://codereview.chromium.org/1998933002/diff/1/chrome/test/base/view_event...
File chrome/test/base/view_event_test_platform_part_chromeos.cc (right):

https://codereview.chromium.org/1998933002/diff/1/chrome/test/base/view_event...
chrome/test/base/view_event_test_platform_part_chromeos.cc:61:
ash::MaterialDesignController::Initialize();
On 2016/05/25 20:24:53, bruthig wrote:
> This test fixture should clean up after itself by calling
> MaterialDesignControllerTestAPI::Uninitialize() from the destructor.

The Uninitialize() function simply set the mode to uninitialized. It does not
free any memory. It is unnecessary to call
MaterialDesignControllerTestAPI::Uninitialize(). The memory will be freed after
the test case finished.

[yiyix, 2016-06-02 15:39:12]

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

https://codereview.chromium.org/1998933002/diff/20001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:5: #include <iostream>
I will delete this line.

[yiyix, 2016-06-02 17:24:16]

Patchset #2 (id:20001) has been deleted

[tdanderson, 2016-06-03 19:25:24]

Hi Yi, please see my next round of comments below. Ben is writing up the details
of the new approach we discussed today.

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

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/app_list_butt...
ash/shelf/app_list_button.cc:133: // SHELF_ALIGHTMENT_RIGHT AND
SHELF_ALIGHMENT_LEFT
nit: comment not needed

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/app_list_butt...
ash/shelf/app_list_button.cc:134: if (!wm::IsHorizontalAlignment(alignment)) {
super nit: swap if/else to structure this as:

if (wm::IsHorizontalAlignment(alignment)) {
  ...
} else {
  ...
}

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/app_list_butt...
ash/shelf/app_list_button.cc:140: } else {  // SHELF_ALIGNMENT_BOTTOM
nit: remove comment

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

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/overflow_butt...
ash/shelf/overflow_button.cc:99: // SHELF_ALIGNMENT_LEFT and
SHELF_ALIGNMENT_RIGHT
Same comments as in AppListButton: remove comments at line 99 and 105, and put
the horizontal case first:

if (horizontal) {
 ...
} else {
 ...
}

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:274: gfx::Rect(rect.x(), rect.bottom() -
GetShelfLayoutConstant(SHELF_SIZE),
nit: use const int shelf_size = GetShelfLayoutConstant(SHELF_SIZE) for better
readability.

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:900: GetShelfLayoutConstant(SHELF_SIZE));
I know you didn't add this if/else, but since the body of the else is more than
one line can you please add in {}, i.e.,

if (...) {
  ...
} else {
  ...
}

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/shelf_view.cc...
ash/shelf/shelf_view.cc:1471: 
nit: declare a const int shelf_size = GetShelfLayoutConstant(SHELF_SIZE) as a
local variable to avoid calling GetShelfLayoutConstant() multiple times within
this function.

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/shelf_view.cc...
ash/shelf/shelf_view.cc:1494: GetShelfLayoutConstant(SHELF_SIZE));
nit: enclose in {} since it is more than one line.

https://codereview.chromium.org/1998933002/diff/40001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/1998933002/diff/40001/ash/system/overview/ove...
ash/system/overview/overview_button_tray.cc:130: // TODO(yiyix): Remove the 3dp
border around the button.
Move to TrayContainer, reword to be something like "TODO(tdanderson|yiyix):
Allow different borders for different buttons, e.g., Overview button should not
have a left border in MD" (as discussed in person). In general TODOs should be
placed nearby the code that eventually needs to be changed.

https://codereview.chromium.org/1998933002/diff/40001/ash/system/status_area_...
File ash/system/status_area_widget_delegate.cc (left):

https://codereview.chromium.org/1998933002/diff/40001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:4: 
nit: replace this blank line

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

https://codereview.chromium.org/1998933002/diff/40001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:128: columns->AddPaddingColumn(0,
GetTrayConstant(TRAY_SPACING));
nit: use {}

https://codereview.chromium.org/1998933002/diff/40001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:141: 
nit: remove blank line

https://codereview.chromium.org/1998933002/diff/40001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:154: layout->AddPaddingRow(0,
GetTrayConstant(TRAY_SPACING));
nit: use {} since the if + boolean condition takes up more than one line.

https://codereview.chromium.org/1998933002/diff/40001/ash/system/tray/tray_co...
File ash/system/tray/tray_constants.cc (right):

https://codereview.chromium.org/1998933002/diff/40001/ash/system/tray/tray_co...
ash/system/tray/tray_constants.cc:91: // static
nit: remove this if the function isn't static

[bruthig, 2016-06-03 20:18:17]

Discussed offline between yiyix@, tdanderson@, and bruthig@:

We are going to try a different approach such that:

- All TrackBackgroundView's will have the correct paddings and the StatusTray
and OverviewButtonTray will not be special cased.
- The StatusAreaWidgetDelegate::AddTray() |view| parameter will be more strongly
typed to a TrayBackgroundView
- StatusAreaWidgetDelegate will be responsible for adding additional padding to
the right most child via a new function added to the TrayBackgroundView.  The
StatusAreaWidgetDelegate::UpdateLayout() function will have to cast it's child
Views to the TrayBackgroundView type. (TrayBackgroundView* background_view =
static_cast<TrayBackgroundView*>(child_at(0));)

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/shelf_constan...
File ash/shelf/shelf_constants.h (right):

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/shelf_constan...
ash/shelf/shelf_constants.h:38: ASH_EXPORT extern const int kAutoHideSize;
|kAutoHideSize| is too general of a variable name to be in the ash namespace
scope.  You could add it to a class or at least make the name more specific.
i.e. kShelfAutoHideSize.  My preference is the former.

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

https://codereview.chromium.org/1998933002/diff/40001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:116: bool is_first_visible_child =
true;
I think keeping this variable in the tightest scope makes it a lot easier to
understand.  I understand there was a similarly named variable in the 'else'
branch below but pulling in to the outer scope here makes me see if it's used
beyond the if/else block.

https://codereview.chromium.org/1998933002/diff/40001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:123: // If the overview Button is
visible, then do not add the additional
I agree this needs a comment, but the 'why' is more valuable than the 'what'. 
Suggestion:

'No padding is added between the OverviewButton and the StatusTray because the
StatusTray includes an extra wide inset on the right side ..."

(This may become irrelevant with the new approach).

https://codereview.chromium.org/1998933002/diff/40001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:126: if
(!((child_at(kOverviewButtonIndex)->visible()) &&
I think you have an set of extra brackets around
'child_at(kOverviewButtonIndex)->visible()'

https://codereview.chromium.org/1998933002/diff/40001/ash/system/tray/tray_ba...
File ash/system/tray/tray_background_view.h (right):

https://codereview.chromium.org/1998933002/diff/40001/ash/system/tray/tray_ba...
ash/system/tray/tray_background_view.h:155: virtual void
GetHitRegionInsets(gfx::Insets& insets);
Google style guide says: "All parameters passed by reference must be labeled
const."

In otherwords, use a pointer for output parameters.

https://codereview.chromium.org/1998933002/diff/40001/chrome/test/base/view_e...
File chrome/test/base/view_event_test_platform_part_chromeos.cc (right):

https://codereview.chromium.org/1998933002/diff/40001/chrome/test/base/view_e...
chrome/test/base/view_event_test_platform_part_chromeos.cc:61:
ash::MaterialDesignController::Initialize();
Don't forget to Uninitialize() in tear down.

[tdanderson, 2016-06-05 21:57:08]

https://chromiumcodereview.appspot.com/1998933002/diff/40001/ash/shelf/shelf_...
File ash/shelf/shelf_constants.cc (right):

https://chromiumcodereview.appspot.com/1998933002/diff/40001/ash/shelf/shelf_...
ash/shelf/shelf_constants.cc:5: #include
"ash/material_design/material_design_controller.h"
nit: the corresponding .h should always be first in a .cc's include order. So
this should look like:

#include "ash/shelf/shelf_constants.h"
<blank line>
#include "ash/material_design/material_design_controller.h"
#include "base/logging.h"

[yiyix, 2016-06-10 19:26:05]

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

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/app_list_butt...
ash/shelf/app_list_button.cc:133: // SHELF_ALIGHTMENT_RIGHT AND
SHELF_ALIGHMENT_LEFT
On 2016/06/03 19:25:23, tdanderson wrote:
> nit: comment not needed

Done.

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/app_list_butt...
ash/shelf/app_list_button.cc:134: if (!wm::IsHorizontalAlignment(alignment)) {
On 2016/06/03 19:25:23, tdanderson wrote:
> super nit: swap if/else to structure this as:
> 
> if (wm::IsHorizontalAlignment(alignment)) {
>   ...
> } else {
>   ...
> }

Done.

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/app_list_butt...
ash/shelf/app_list_button.cc:140: } else {  // SHELF_ALIGNMENT_BOTTOM
On 2016/06/03 19:25:23, tdanderson wrote:
> nit: remove comment

Done.

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

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/overflow_butt...
ash/shelf/overflow_button.cc:99: // SHELF_ALIGNMENT_LEFT and
SHELF_ALIGNMENT_RIGHT
On 2016/06/03 19:25:23, tdanderson wrote:
> Same comments as in AppListButton: remove comments at line 99 and 105, and put
> the horizontal case first:
> 
> if (horizontal) {
>  ...
> } else {
>  ...
> }

Done.

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/shelf_constan...
File ash/shelf/shelf_constants.cc (right):

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/shelf_constan...
ash/shelf/shelf_constants.cc:5: #include
"ash/material_design/material_design_controller.h"
On 2016/06/05 21:57:07, tdanderson wrote:
> nit: the corresponding .h should always be first in a .cc's include order. So
> this should look like:
> 
> #include "ash/shelf/shelf_constants.h"
> <blank line>
> #include "ash/material_design/material_design_controller.h"
> #include "base/logging.h"

Done.

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/shelf_constan...
File ash/shelf/shelf_constants.h (right):

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/shelf_constan...
ash/shelf/shelf_constants.h:38: ASH_EXPORT extern const int kAutoHideSize;
On 2016/06/03 20:18:17, bruthig wrote:
> |kAutoHideSize| is too general of a variable name to be in the ash namespace
> scope.  You could add it to a class or at least make the name more specific.
> i.e. kShelfAutoHideSize.  My preference is the former.

This constant is used in both shelf_layout_manager class and
ash_popup_alignment_delegate class. I update the name to kShelfAutoHideSize to
make it more specific.

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:274: gfx::Rect(rect.x(), rect.bottom() -
GetShelfLayoutConstant(SHELF_SIZE),
On 2016/06/03 19:25:23, tdanderson wrote:
> nit: use const int shelf_size = GetShelfLayoutConstant(SHELF_SIZE) for better
> readability.

Done.

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:900: GetShelfLayoutConstant(SHELF_SIZE));
On 2016/06/03 19:25:23, tdanderson wrote:
> I know you didn't add this if/else, but since the body of the else is more
than
> one line can you please add in {}, i.e.,
> 
> if (...) {
>   ...
> } else {
>   ...
> }

Done.

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/shelf_view.cc...
ash/shelf/shelf_view.cc:1471: 
On 2016/06/03 19:25:23, tdanderson wrote:
> nit: declare a const int shelf_size = GetShelfLayoutConstant(SHELF_SIZE) as a
> local variable to avoid calling GetShelfLayoutConstant() multiple times within
> this function.

Done.

https://codereview.chromium.org/1998933002/diff/40001/ash/shelf/shelf_view.cc...
ash/shelf/shelf_view.cc:1494: GetShelfLayoutConstant(SHELF_SIZE));
On 2016/06/03 19:25:23, tdanderson wrote:
> nit: enclose in {} since it is more than one line.

Done.

https://codereview.chromium.org/1998933002/diff/40001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/1998933002/diff/40001/ash/system/overview/ove...
ash/system/overview/overview_button_tray.cc:130: // TODO(yiyix): Remove the 3dp
border around the button.
On 2016/06/03 19:25:23, tdanderson wrote:
> Move to TrayContainer, reword to be something like "TODO(tdanderson|yiyix):
> Allow different borders for different buttons, e.g., Overview button should
not
> have a left border in MD" (as discussed in person). In general TODOs should be
> placed nearby the code that eventually needs to be changed.

Done.

https://codereview.chromium.org/1998933002/diff/40001/ash/system/status_area_...
File ash/system/status_area_widget_delegate.cc (left):

https://codereview.chromium.org/1998933002/diff/40001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:4: 
On 2016/06/03 19:25:23, tdanderson wrote:
> nit: replace this blank line

Done.

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

https://codereview.chromium.org/1998933002/diff/40001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:116: bool is_first_visible_child =
true;
On 2016/06/03 20:18:17, bruthig wrote:
> I think keeping this variable in the tightest scope makes it a lot easier to
> understand.  I understand there was a similarly named variable in the 'else'
> branch below but pulling in to the outer scope here makes me see if it's used
> beyond the if/else block.

Done.

https://codereview.chromium.org/1998933002/diff/40001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:123: // If the overview Button is
visible, then do not add the additional
On 2016/06/03 20:18:17, bruthig wrote:
> I agree this needs a comment, but the 'why' is more valuable than the 'what'. 
> Suggestion:
> 
> 'No padding is added between the OverviewButton and the StatusTray because the
> StatusTray includes an extra wide inset on the right side ..."
> 
> (This may become irrelevant with the new approach).   
> 
> 
> 
> 
>      

I will add some explanation for the new approach too.

https://codereview.chromium.org/1998933002/diff/40001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:126: if
(!((child_at(kOverviewButtonIndex)->visible()) &&
On 2016/06/03 20:18:17, bruthig wrote:
> I think you have an set of extra brackets around
> 'child_at(kOverviewButtonIndex)->visible()'

You are right! I didn't notice it!

https://codereview.chromium.org/1998933002/diff/40001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:128: columns->AddPaddingColumn(0,
GetTrayConstant(TRAY_SPACING));
On 2016/06/03 19:25:23, tdanderson wrote:
> nit: use {}

Done.

https://codereview.chromium.org/1998933002/diff/40001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:141: 
On 2016/06/03 19:25:23, tdanderson wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/1998933002/diff/40001/ash/system/tray/tray_co...
File ash/system/tray/tray_constants.cc (right):

https://codereview.chromium.org/1998933002/diff/40001/ash/system/tray/tray_co...
ash/system/tray/tray_constants.cc:91: // static
On 2016/06/03 19:25:24, tdanderson wrote:
> nit: remove this if the function isn't static

Sorry, I copied this over from another function

https://codereview.chromium.org/1998933002/diff/40001/chrome/test/base/view_e...
File chrome/test/base/view_event_test_platform_part_chromeos.cc (right):

https://codereview.chromium.org/1998933002/diff/40001/chrome/test/base/view_e...
chrome/test/base/view_event_test_platform_part_chromeos.cc:61:
ash::MaterialDesignController::Initialize();
On 2016/06/03 20:18:17, bruthig wrote:
> Don't forget to Uninitialize() in tear down.

Done.

[tdanderson, 2016-06-10 22:54:43]

Hey Yi, it's looking good, but I do have some more comments below (most of them
are nits). Please see below.

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

https://codereview.chromium.org/1998933002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/tray_constants.h:27: // Padding between icons in the
status tray area.
nit: icons -> items

https://codereview.chromium.org/1998933002/diff/60001/ash/root_window_control...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/1998933002/diff/60001/ash/root_window_control...
ash/root_window_controller_unittest.cc:15: #include
"ash/shelf/shelf_layout_manager.h"
are these needed?

https://codereview.chromium.org/1998933002/diff/60001/ash/root_window_control...
ash/root_window_controller_unittest.cc:204: // Maximized area on primary display
has 48px (ash::SHELF_SIZE) inset at the
nit: please include the old comment since we haven't deprecated non-MD code yet
(i.e., say area is 47 for non-md and 48 for md).

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

https://codereview.chromium.org/1998933002/diff/60001/ash/shelf/overflow_butt...
ash/shelf/overflow_button.cc:148: gfx::Rect(bounds.right() - background->width()
- kShelfItemSizeMD,
Is this a typo (lines 148, 153, and 159)? You're including an MD constant in
non-MD code.

https://codereview.chromium.org/1998933002/diff/60001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/1998933002/diff/60001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:271: int shelf_size =
GetShelfConstant(SHELF_SIZE);
nit: const

https://codereview.chromium.org/1998933002/diff/60001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:953: }
thanks for adding the {} here!

https://codereview.chromium.org/1998933002/diff/60001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/1998933002/diff/60001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager_unittest.cc:18: #include
"ash/shelf/shelf_constants.h"
This file moved to ash/common. (If you don't run tests locally before uploading
a patchset, it's always a good idea to run git cl try from the command line so
that the trybots can catch things like this for you.)

https://codereview.chromium.org/1998933002/diff/60001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/1998933002/diff/60001/ash/shelf/shelf_view.cc...
ash/shelf/shelf_view.cc:1477: int shelf_size = GetShelfConstant(SHELF_SIZE);
nit: const

https://codereview.chromium.org/1998933002/diff/60001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/1998933002/diff/60001/ash/system/overview/ove...
ash/system/overview/overview_button_tray.cc:13: #include
"ash/common/system/tray/tray_constants.h"
Is this #include needed given that you haven't made any other changes to this
file?

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

https://codereview.chromium.org/1998933002/diff/60001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:136: for (int c = child_count() - 1; c
>= 0; --c) {
Are you sure you still want to reverse the order of the iteration here?

https://codereview.chromium.org/1998933002/diff/60001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:164: c == kOverviewButtonIndex)) {
Unless I am missing something, I think you can remove lines 163-164 since
they're no longer necessary. Be sure to remove the constant
|kOverviewButtonIndex| as well.

https://codereview.chromium.org/1998933002/diff/60001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:212: bool on_edge = (child ==
child_at(0));
Now that you're checking which child is on the edge in lines 118-130, I don't
think this variable is needed anymore.

To expand on my earlier suggestion I think 196-277 can be condensed into:

void SetBorderOnChild(View* child, bool extend_border_to_edge) {
  if (material) {
    ...
  } else {
    ... // Use |extend_border_to_edge| instead of |on_edge|
  }
}

https://codereview.chromium.org/1998933002/diff/60001/ash/system/status_area_...
File ash/system/status_area_widget_delegate.h (right):

https://codereview.chromium.org/1998933002/diff/60001/ash/system/status_area_...
ash/system/status_area_widget_delegate.h:16: namespace ash {
you don't need/want both line 16 and line 20

https://codereview.chromium.org/1998933002/diff/60001/ash/system/status_area_...
ash/system/status_area_widget_delegate.h:30: void AddTray(TrayBackgroundView*
tray);
I was looking over the chat you, me, and Ben had last week (in the CL comments)
and he suggested using a static_cast<TrayBackgroundView> in
StatusAreaWidgetDelegate::UpdateLayout(). You haven't done that, but I think I
actually prefer what you've done instead (move the border-setting code out of
TrayBackgroundView and into StatusAreaWidgetDelegate).

So with that said, since you're not doing the static cast, there's really no
reason to change this type anymore. I'd just keep it as a View*. (Also you can
remove the class declaration on line 17)

https://codereview.chromium.org/1998933002/diff/60001/ash/system/status_area_...
ash/system/status_area_widget_delegate.h:62: // Tray shows on the right most
edge requires an extra wide inset, so edge is
nit on wording, change to something like: "Sets a border on |child|. If
|extend_border_to_edge| is true, then an extra wide border is added to extend
the view's hit region to the edge of the screen."

https://codereview.chromium.org/1998933002/diff/60001/ash/system/status_area_...
ash/system/status_area_widget_delegate.h:65: void
SetBorderOnNonEdgeTray(views::View* child);
Combine into a single function, such as:

void SetBorderOnChild(View* child, bool extend_border_to_edge)

https://codereview.chromium.org/1998933002/diff/60001/ash/system/toast/toast_...
File ash/system/toast/toast_manager_unittest.cc (right):

https://codereview.chromium.org/1998933002/diff/60001/ash/system/toast/toast_...
ash/system/toast/toast_manager_unittest.cc:8: #include
"ash/shelf/shelf_constants.h"
file moved to ash/common

https://codereview.chromium.org/1998933002/diff/60001/ash/system/tray/system_...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/1998933002/diff/60001/ash/system/tray/system_...
ash/system/tray/system_tray.cc:8: #include
"ash/common/material_design/material_design_controller.h"
is this #include needed?

https://codereview.chromium.org/1998933002/diff/60001/ash/system/tray/tray_ba...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/1998933002/diff/60001/ash/system/tray/tray_ba...
ash/system/tray/tray_background_view.cc:216: // Overview button should not have
a left border in MD
Change this to just "TODO(tdanderson): Adjust TrayContainer borders according to
the material design specs. See crbug.com/617295."

[yiyix, 2016-06-13 18:43:55]

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

https://codereview.chromium.org/1998933002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/tray_constants.h:27: // Padding between icons in the
status tray area.
On 2016/06/10 22:54:42, tdanderson wrote:
> nit: icons -> items

Done.

https://codereview.chromium.org/1998933002/diff/60001/ash/root_window_control...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/1998933002/diff/60001/ash/root_window_control...
ash/root_window_controller_unittest.cc:15: #include
"ash/shelf/shelf_layout_manager.h"
On 2016/06/10 22:54:42, tdanderson wrote:
> are these needed?

sorry, these are from other patches.

https://codereview.chromium.org/1998933002/diff/60001/ash/root_window_control...
ash/root_window_controller_unittest.cc:204: // Maximized area on primary display
has 48px (ash::SHELF_SIZE) inset at the
On 2016/06/10 22:54:42, tdanderson wrote:
> nit: please include the old comment since we haven't deprecated non-MD code
yet
> (i.e., say area is 47 for non-md and 48 for md).

Done.

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

https://codereview.chromium.org/1998933002/diff/60001/ash/shelf/overflow_butt...
ash/shelf/overflow_button.cc:148: gfx::Rect(bounds.right() - background->width()
- kShelfItemSizeMD,
On 2016/06/10 22:54:43, tdanderson wrote:
> Is this a typo (lines 148, 153, and 159)? You're including an MD constant in
> non-MD code.

It is a typo. Sorry.

https://codereview.chromium.org/1998933002/diff/60001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/1998933002/diff/60001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:271: int shelf_size =
GetShelfConstant(SHELF_SIZE);
On 2016/06/10 22:54:43, tdanderson wrote:
> nit: const

Done.

https://codereview.chromium.org/1998933002/diff/60001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:953: }
On 2016/06/10 22:54:43, tdanderson wrote:
> thanks for adding the {} here!

:) from the previous reviews, I learnt that we need {} around if and else.

https://codereview.chromium.org/1998933002/diff/60001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/1998933002/diff/60001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager_unittest.cc:18: #include
"ash/shelf/shelf_constants.h"
On 2016/06/10 22:54:43, tdanderson wrote:
> This file moved to ash/common. (If you don't run tests locally before
uploading
> a patchset, it's always a good idea to run git cl try from the command line so
> that the trybots can catch things like this for you.)

I only build the chrome target... thanks, I will do that for future.

https://codereview.chromium.org/1998933002/diff/60001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/1998933002/diff/60001/ash/shelf/shelf_view.cc...
ash/shelf/shelf_view.cc:1477: int shelf_size = GetShelfConstant(SHELF_SIZE);
On 2016/06/10 22:54:43, tdanderson wrote:
> nit: const

Done.

https://codereview.chromium.org/1998933002/diff/60001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/1998933002/diff/60001/ash/system/overview/ove...
ash/system/overview/overview_button_tray.cc:13: #include
"ash/common/system/tray/tray_constants.h"
On 2016/06/10 22:54:43, tdanderson wrote:
> Is this #include needed given that you haven't made any other changes to this
> file?

Done.

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

https://codereview.chromium.org/1998933002/diff/60001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:136: for (int c = child_count() - 1; c
>= 0; --c) {
On 2016/06/10 22:54:43, tdanderson wrote:
> Are you sure you still want to reverse the order of the iteration here?

I think reverse the order makes more sense. With the loop ordering (int c = 0; c
< child_count(); ++c), we allocate the space on shelf for system trays with one
order, then we use a different order (int c = child_count() - 1; c >= 0; --c) in
the second for loop to use the space for another different tray.

https://codereview.chromium.org/1998933002/diff/60001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:164: c == kOverviewButtonIndex)) {
On 2016/06/10 22:54:43, tdanderson wrote:
> Unless I am missing something, I think you can remove lines 163-164 since
> they're no longer necessary. Be sure to remove the constant
> |kOverviewButtonIndex| as well.

Done.

https://codereview.chromium.org/1998933002/diff/60001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:212: bool on_edge = (child ==
child_at(0));
On 2016/06/10 22:54:43, tdanderson wrote:
> Now that you're checking which child is on the edge in lines 118-130, I don't
> think this variable is needed anymore.
> 
> To expand on my earlier suggestion I think 196-277 can be condensed into:
> 
> void SetBorderOnChild(View* child, bool extend_border_to_edge) {
>   if (material) {
>     ...
>   } else {
>     ... // Use |extend_border_to_edge| instead of |on_edge|
>   }
> }

The only child with index 0 is overview button. In the non-md design, there is a
larger inset between overview and the edge of the screen.

https://codereview.chromium.org/1998933002/diff/60001/ash/system/status_area_...
File ash/system/status_area_widget_delegate.h (right):

https://codereview.chromium.org/1998933002/diff/60001/ash/system/status_area_...
ash/system/status_area_widget_delegate.h:16: namespace ash {
On 2016/06/10 22:54:43, tdanderson wrote:
> you don't need/want both line 16 and line 20

Done.

https://codereview.chromium.org/1998933002/diff/60001/ash/system/status_area_...
ash/system/status_area_widget_delegate.h:30: void AddTray(TrayBackgroundView*
tray);
On 2016/06/10 22:54:43, tdanderson wrote:
> I was looking over the chat you, me, and Ben had last week (in the CL
comments)
> and he suggested using a static_cast<TrayBackgroundView> in
> StatusAreaWidgetDelegate::UpdateLayout(). You haven't done that, but I think I
> actually prefer what you've done instead (move the border-setting code out of
> TrayBackgroundView and into StatusAreaWidgetDelegate).
> 
> So with that said, since you're not doing the static cast, there's really no
> reason to change this type anymore. I'd just keep it as a View*. (Also you can
> remove the class declaration on line 17)

I did not really like the casting. I am glad that you prefer this way as well.

https://codereview.chromium.org/1998933002/diff/60001/ash/system/status_area_...
ash/system/status_area_widget_delegate.h:62: // Tray shows on the right most
edge requires an extra wide inset, so edge is
On 2016/06/10 22:54:43, tdanderson wrote:
> nit on wording, change to something like: "Sets a border on |child|. If
> |extend_border_to_edge| is true, then an extra wide border is added to extend
> the view's hit region to the edge of the screen."

Done.

https://codereview.chromium.org/1998933002/diff/60001/ash/system/status_area_...
ash/system/status_area_widget_delegate.h:65: void
SetBorderOnNonEdgeTray(views::View* child);
On 2016/06/10 22:54:43, tdanderson wrote:
> Combine into a single function, such as:
> 
> void SetBorderOnChild(View* child, bool extend_border_to_edge)

Done.

https://codereview.chromium.org/1998933002/diff/60001/ash/system/toast/toast_...
File ash/system/toast/toast_manager_unittest.cc (right):

https://codereview.chromium.org/1998933002/diff/60001/ash/system/toast/toast_...
ash/system/toast/toast_manager_unittest.cc:8: #include
"ash/shelf/shelf_constants.h"
On 2016/06/10 22:54:43, tdanderson wrote:
> file moved to ash/common

Done.

https://codereview.chromium.org/1998933002/diff/60001/ash/system/tray/system_...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/1998933002/diff/60001/ash/system/tray/system_...
ash/system/tray/system_tray.cc:8: #include
"ash/common/material_design/material_design_controller.h"
On 2016/06/10 22:54:43, tdanderson wrote:
> is this #include needed?

Done.

https://codereview.chromium.org/1998933002/diff/60001/ash/system/tray/tray_ba...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/1998933002/diff/60001/ash/system/tray/tray_ba...
ash/system/tray/tray_background_view.cc:216: // Overview button should not have
a left border in MD
On 2016/06/10 22:54:43, tdanderson wrote:
> Change this to just "TODO(tdanderson): Adjust TrayContainer borders according
to
> the material design specs. See crbug.com/617295."

Done.

[yiyix, 2016-06-13 18:53:15]

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

https://codereview.chromium.org/1998933002/diff/60001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:136: for (int c = child_count() - 1; c
>= 0; --c) {
On 2016/06/13 18:43:55, yiyix wrote:
> On 2016/06/10 22:54:43, tdanderson wrote:
> > Are you sure you still want to reverse the order of the iteration here?
> 
> I think reverse the order makes more sense. With the loop ordering (int c = 0;
c
> < child_count(); ++c), we allocate the space on shelf for system trays with
one
> order, then we use a different order (int c = child_count() - 1; c >= 0; --c)
in
> the second for loop to use the space for another different tray. 

Sorry, my explanation is not clear as I read it again. I want to say that with
the old approach, we use one order to allocate spacing in the layout, then we
use a different order to use the spacing. Ex: if child at index 0 is visible,
then we add the column c1 in the layout for this view. In the second loop, we
add logout button with index 4 (if logout button is visible) to the column c1. 

After changing the ordering in the first loop, we always add the view to the
column which is allocated for it.

[tdanderson, 2016-06-13 19:02:51]

lgtm with the comments below. Please send to jamescook@ for review when you're
ready.

Note to James: layout is only 90% up to spec with this CL; I need to make a few
more follow-on tweaks in crbug.com/617295. At that point we will be going
through the tests to make sure they will pass with and without the md flag
enabled.

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

https://codereview.chromium.org/1998933002/diff/80001/ash/common/system/tray/...
ash/common/system/tray/tray_constants.h:26: enum ShelfStatusAreaInset {
I would just call this TrayConstant (to be consistent with the naming we use in
shelf_constants.h)

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

https://codereview.chromium.org/1998933002/diff/80001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:105: void
StatusAreaWidgetDelegate::AddTray(TrayBackgroundView* tray) {
views::View*

https://codereview.chromium.org/1998933002/diff/80001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:128: }
replace lines 124-128 with just SetBorderOnChild(child, is_child_on_edge)

https://codereview.chromium.org/1998933002/diff/80001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:162: if (!is_first_visible_child) {
nit: remove {}

https://codereview.chromium.org/1998933002/diff/80001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:193: void
StatusAreaWidgetDelegate::SetBorderOnChild(views::View* child,
Can probably still be split up into helpers. Will defer to James on that to
avoid further churn.

[yiyix, 2016-06-13 19:13:55]

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

[yiyix, 2016-06-13 19:14:01]

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

https://codereview.chromium.org/1998933002/diff/80001/ash/common/system/tray/...
ash/common/system/tray/tray_constants.h:26: enum ShelfStatusAreaInset {
On 2016/06/13 19:02:51, tdanderson wrote:
> I would just call this TrayConstant (to be consistent with the naming we use
in
> shelf_constants.h)

Done.

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

https://codereview.chromium.org/1998933002/diff/80001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:105: void
StatusAreaWidgetDelegate::AddTray(TrayBackgroundView* tray) {
On 2016/06/13 19:02:51, tdanderson wrote:
> views::View*

Done.

https://codereview.chromium.org/1998933002/diff/80001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:128: }
On 2016/06/13 19:02:51, tdanderson wrote:
> replace lines 124-128 with just SetBorderOnChild(child, is_child_on_edge)

haha....lol... that's true, i not using is_child_on_edge properly.

https://codereview.chromium.org/1998933002/diff/80001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:162: if (!is_first_visible_child) {
On 2016/06/13 19:02:51, tdanderson wrote:
> nit: remove {}

Done.

[commit-bot: I haz the power, 2016-06-13 19:14:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1998933002/100001

[commit-bot: I haz the power, 2016-06-13 19:38:39]

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

[commit-bot: I haz the power, 2016-06-13 19:38:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yiyix, 2016-06-13 19:57:36]

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

[commit-bot: I haz the power, 2016-06-13 19:58:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1998933002/120001

[commit-bot: I haz the power, 2016-06-13 21:05:21]

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

[commit-bot: I haz the power, 2016-06-13 21:05:23]

Dry run: This issue passed the CQ dry run.

[yiyix, 2016-06-13 21:14:33]

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

[yiyix, 2016-06-13 21:14:34]

@jamescook, could you review the code? thank you very much

[James Cook, 2016-06-13 21:43:18]

LGTM with nits.

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

https://codereview.chromium.org/1998933002/diff/120001/ash/common/system/tray...
ash/common/system/tray/tray_constants.h:26: enum TrayConstant {
nit: blank line above

Also, I would move this down to just above GetTrayConstant() on line 91.

https://codereview.chromium.org/1998933002/diff/120001/ash/common/system/tray...
ash/common/system/tray/tray_constants.h:31: PADDING_FROM_EDGE_OF_SHELF,
If you're going to use a regular enum, please call this TRAY_PADDING_... so we
know it's for the tray. Or you could switch to "enum class TrayConstant" and use
TrayConstant::SPACING and TrayConstant::PADDING_FROM_EDGE_OF_SHELF.

https://codereview.chromium.org/1998933002/diff/120001/ash/common/system/tray...
ash/common/system/tray/tray_constants.h:89: extern const int kShelfItemSizeMD;
Is this the MD version of the line above? If so, I would either call both of
them Height or both Size.

If they are different, I would leave a comment explaining how/why.

https://codereview.chromium.org/1998933002/diff/120001/ash/system/status_area...
File ash/system/status_area_widget_delegate.cc (right):

https://codereview.chromium.org/1998933002/diff/120001/ash/system/status_area...
ash/system/status_area_widget_delegate.cc:17: #include
"ash/system/tray/tray_background_view.h"
Do you need this include?

https://codereview.chromium.org/1998933002/diff/120001/ash/system/status_area...
ash/system/status_area_widget_delegate.cc:150: bool is_first_visible_child =
true;
move this back down to where it was before

https://codereview.chromium.org/1998933002/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/launcher_favicon_loader.cc (right):

https://codereview.chromium.org/1998933002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/launcher_favicon_loader.cc:25: // link tag,
storing the one that best matches
rewrap with line above

https://codereview.chromium.org/1998933002/diff/120001/chrome/test/base/view_...
File chrome/test/base/view_event_test_platform_part_chromeos.cc (right):

https://codereview.chromium.org/1998933002/diff/120001/chrome/test/base/view_...
chrome/test/base/view_event_test_platform_part_chromeos.cc:90:
ash::test::MaterialDesignControllerTestAPI::Uninitialize();
nit: Move this to line 82, so you uninitialize things in the opposite order to
how you initialize them.

[yiyix, 2016-06-14 05:55:25]

yiyix@chromium.org changed reviewers:
+ msw@chromium.org

[yiyix, 2016-06-14 05:55:27]

msw@chromium.org: Please review changes in chrome? Thanks

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

https://codereview.chromium.org/1998933002/diff/120001/ash/common/system/tray...
ash/common/system/tray/tray_constants.h:26: enum TrayConstant {
On 2016/06/13 21:43:15, James Cook wrote:
> nit: blank line above
> 
> Also, I would move this down to just above GetTrayConstant() on line 91.

Done.

https://codereview.chromium.org/1998933002/diff/120001/ash/common/system/tray...
ash/common/system/tray/tray_constants.h:31: PADDING_FROM_EDGE_OF_SHELF,
On 2016/06/13 21:43:15, James Cook wrote:
> If you're going to use a regular enum, please call this TRAY_PADDING_... so we
> know it's for the tray. Or you could switch to "enum class TrayConstant" and
use
> TrayConstant::SPACING and TrayConstant::PADDING_FROM_EDGE_OF_SHELF.

I updated the variable name to TRAY_PADDING_FROM_EDGE_OF_SHELF

https://codereview.chromium.org/1998933002/diff/120001/ash/common/system/tray...
ash/common/system/tray/tray_constants.h:89: extern const int kShelfItemSizeMD;
On 2016/06/13 21:43:16, James Cook wrote:
> Is this the MD version of the line above? If so, I would either call both of
> them Height or both Size.
> 
> If they are different, I would leave a comment explaining how/why.

All tray background are sized 32 by 32 in Chrome OS MD. However, The backgrounds
are not sized properly now, so I cannot add this variable to the GetTrayConstant
function yet. I use this value when it makes sense in the code. I added a TODO
in the code.

https://codereview.chromium.org/1998933002/diff/120001/ash/system/status_area...
File ash/system/status_area_widget_delegate.cc (right):

https://codereview.chromium.org/1998933002/diff/120001/ash/system/status_area...
ash/system/status_area_widget_delegate.cc:17: #include
"ash/system/tray/tray_background_view.h"
On 2016/06/13 21:43:17, James Cook wrote:
> Do you need this include?

Done.

https://codereview.chromium.org/1998933002/diff/120001/ash/system/status_area...
ash/system/status_area_widget_delegate.cc:150: bool is_first_visible_child =
true;
On 2016/06/13 21:43:17, James Cook wrote:
> move this back down to where it was before

Done.

https://codereview.chromium.org/1998933002/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/launcher_favicon_loader.cc (right):

https://codereview.chromium.org/1998933002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/launcher_favicon_loader.cc:25: // link tag,
storing the one that best matches
On 2016/06/13 21:43:17, James Cook wrote:
> rewrap with line above

Done.

https://codereview.chromium.org/1998933002/diff/120001/chrome/test/base/view_...
File chrome/test/base/view_event_test_platform_part_chromeos.cc (right):

https://codereview.chromium.org/1998933002/diff/120001/chrome/test/base/view_...
chrome/test/base/view_event_test_platform_part_chromeos.cc:90:
ash::test::MaterialDesignControllerTestAPI::Uninitialize();
On 2016/06/13 21:43:17, James Cook wrote:
> nit: Move this to line 82, so you uninitialize things in the opposite order to
> how you initialize them.

Will commit this change in a different CL because this is a fix for test cases.

[yiyix, 2016-06-14 05:55:31]

msw@chromium.org: Please review changes in chrome? Thanks

[msw, 2016-06-14 17:03:15]

chrome/* lgtm with a nit

https://codereview.chromium.org/1998933002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/launcher_favicon_loader.cc (right):

https://codereview.chromium.org/1998933002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/launcher_favicon_loader.cc:25: // matches
ash::ShelfLayoutConstant::SHELF_SIZE.
nit: just "ash::SHELF_SIZE" (as elsewhere) and fix the line wrapping.

[yiyix, 2016-06-14 19:08:17]

The CQ bit was checked by yiyix@chromium.org

[yiyix, 2016-06-14 19:08:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org, jamescook@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1998933002/#ps160001
(title: "Fix Comments based on @msw comments")

[commit-bot: I haz the power, 2016-06-14 19:08:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1998933002/160001

[commit-bot: I haz the power, 2016-06-14 19:11:27]

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

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

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yiyix, 2016-06-14 19:32:55]

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

[commit-bot: I haz the power, 2016-06-14 19:33:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1998933002/180001

[yiyix, 2016-06-14 21:25:59]

The CQ bit was unchecked by yiyix@chromium.org

[yiyix, 2016-06-14 21:26:01]

The CQ bit was checked by yiyix@chromium.org

[yiyix, 2016-06-14 21:26:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org, jamescook@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1998933002/#ps180001
(title: "merge")

[yiyix, 2016-06-14 21:26:02]

The CQ bit was unchecked by yiyix@chromium.org

[yiyix, 2016-06-14 21:26:16]

The CQ bit was checked by yiyix@chromium.org

[yiyix, 2016-06-14 21:26:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org, jamescook@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1998933002/#ps180001
(title: "merge")

[commit-bot: I haz the power, 2016-06-14 21:27:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1998933002/180001

[commit-bot: I haz the power, 2016-06-14 21:49:51]

Description was changed from

==========
Update shelf spacing in Chrome OS according to the MD specs:
- Update the height of the shelf.
- Update the spacing between icons in the shelf.
- Update the spacing around status tray area, notification center, app list
button and overview button.
- Update the spacing between the status tray area and notification center.
- Update the spacing around the app list button.

TEST=manual
1. Uses GIMP to measure Shelf Spacing requirements when Shelf is positioned to
Left, Right and Bottom in Chrome OS .

BUG=595005
==========

to

==========
Update shelf spacing in Chrome OS according to the MD specs:
- Update the height of the shelf.
- Update the spacing between icons in the shelf.
- Update the spacing around status tray area, notification center, app list
button and overview button.
- Update the spacing between the status tray area and notification center.
- Update the spacing around the app list button.

TEST=manual
1. Uses GIMP to measure Shelf Spacing requirements when Shelf is positioned to
Left, Right and Bottom in Chrome OS .

BUG=595005
==========

[commit-bot: I haz the power, 2016-06-14 21:49:55]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2016-06-14 21:50:19]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-14 21:51:59]

Description was changed from

==========
Update shelf spacing in Chrome OS according to the MD specs:
- Update the height of the shelf.
- Update the spacing between icons in the shelf.
- Update the spacing around status tray area, notification center, app list
button and overview button.
- Update the spacing between the status tray area and notification center.
- Update the spacing around the app list button.

TEST=manual
1. Uses GIMP to measure Shelf Spacing requirements when Shelf is positioned to
Left, Right and Bottom in Chrome OS .

BUG=595005
==========

to

==========
Update shelf spacing in Chrome OS according to the MD specs:
- Update the height of the shelf.
- Update the spacing between icons in the shelf.
- Update the spacing around status tray area, notification center, app list
button and overview button.
- Update the spacing between the status tray area and notification center.
- Update the spacing around the app list button.

TEST=manual
1. Uses GIMP to measure Shelf Spacing requirements when Shelf is positioned to
Left, Right and Bottom in Chrome OS .

BUG=595005

Committed: https://crrev.com/20e7d3f385060e0c2de2a86d5dcf81c4c0ca8c8f
Cr-Commit-Position: refs/heads/master@{#399792}
==========

[commit-bot: I haz the power, 2016-06-14 21:52:01]

Patchset 9 (id:??) landed as
https://crrev.com/20e7d3f385060e0c2de2a86d5dcf81c4c0ca8c8f
Cr-Commit-Position: refs/heads/master@{#399792}