[ash-md] Applied Material Design layout to TrayItemMore system menu rows.

Applied Material Design layout to TrayItemMore system menu rows.

This change applies the new common system menu layout to all rows that
inherit from the TrayItemMore class.  This includes the following rows:

- SMS default row
- Bluetooth default row
- Cast default row
- Accessibility default row
- IME default row
- Network default row
- Display rotation lock default row
- VPN default row

TBR=jamescook@chromium.org
BUG=657669
Committed: https://crrev.com/7964cb1f73d2ca30deadfb8a0cd0f2e11a76e8ce
Cr-Commit-Position: refs/heads/master@{#429171}

[bruthig, 2016-10-31 17:38:49]

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

[bruthig, 2016-10-31 17:38:59]

Description was changed from

==========
Applied Material Design layout to TrayItemMore system menu rows.

This change applies the new common system menu layout to all rows that
inherit from the TrayItemMore class.  This includes the following rows:

- SMS default row
- Bluetooth default row
- Cast default row
- Accessibility default row
- IME default row
- Network default row
- Display rotation lock default row
- VPN default row

BUG=657669
==========

to

==========
Applied Material Design layout to TrayItemMore system menu rows.

This change applies the new common system menu layout to all rows that
inherit from the TrayItemMore class.  This includes the following rows:

- SMS default row
- Bluetooth default row
- Cast default row
- Accessibility default row
- IME default row
- Network default row
- Display rotation lock default row
- VPN default row

BUG=657669
==========

[bruthig, 2016-10-31 17:40:40]

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

[bruthig, 2016-10-31 17:40:41]

Terry, can you PTAL?

[bruthig, 2016-10-31 17:48:27]

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

[commit-bot: I haz the power, 2016-10-31 17:48:54]

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-31 18:14:34]

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

[commit-bot: I haz the power, 2016-10-31 18:14:34]

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

[tdanderson, 2016-11-01 18:59:44]

Sorry for the delay. LGTM with comments below.

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

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_constants.cc:56: const int
kTrayPopupLabelHorizontalPadding = 4;
I think this may be a duplicate of kTrayPopupTextSpacingVertical in this file.

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_constants.cc:106: const int kTrayPopupItemHeight[] =
{46, 48, 48};
Note that since these arrays have the form {a, b, b} (the material normal value
is the same as the material experimental value) make sure
https://chromiumcodereview.appspot.com/2454393002 has landed before this CL goes
into the commit queue.

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

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_constants.h:54: // The padding used on the left and
right of labels.
nit: please specify in the comment if the constant should be used only for
labels in default view rows, or if it applies to all labels throughout the
system menu. Ditto for the values on lines 136 and 139.

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/tray_item_more.cc (left):

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_item_more.cc:115: bounds.set_width(more_->x() -
kTrayPopupPaddingBetweenItems - label_->x());
Double-checking since it could be easy to forget when only testing with short
strings: will TriView handle this case nicely?

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

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_item_more.cc:35: SetLayoutManager(new
views::FillLayout);
optional: Consider adding something to the class-level documentation of TriView
to mention that a FillLayout should be used as the layout manager of TriView's
parent view.

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_item_more.cc:41:
label_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
Would it be appropriate to move line 41 into CreateDefaultLabel() ?

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_item_more.cc:46:
more_->EnableCanvasFlippingForRTLUI(true);
Similar to above comment, should this be moved into CreateMoreImageView() ?

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_item_more.cc:55:
tri_view->SetContainerVisible(TriView::Container::END, false);
Interesting, I didn't realize this would have been necessary; presumably this
empty container affects the appearance of the layout? Have you considered
modifying TriView to make containers invisible whenever they are empty? (OK to
discuss / do as follow up to this CL).

I think manually setting empty containers to invisible would be an easy thing
for someone to forget.

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

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_popup_utils.cc:15: #include
"ui/views/controls/slider.h"
Is this include needed?

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_popup_utils.cc:43:
views::BoxLayout::CROSS_AXIS_ALIGNMENT_STRETCH);
It seems CENTER is what you want here instead of STRETCH?

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/tray_popup_utils.h (right):

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_popup_utils.h:5: #ifndef
ASH_COMMON_SYSTEM_TRAY_TRAY_POPUP_UTIL_H_
nit: UTILS_H_ rather than UTIL_H_

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_popup_utils.h:45: // Returns a label that has been
configured for system menu layout.
Similar to the comments I made in the constants file, can you please clarify in
the documentation for CreateDefaultLabel(), CreateMainImageView(), and Create
MoreImageView() if they are to be used only by default menu rows. Specifically,
it would be good to make it clear for everyone whether or not these can be used
for the rows in the detailed views (e.g., the rows for each available Bluetooth
device).

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/tri_view.cc (left):

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tri_view.cc:116:
GetLayoutManager(container)->Installed(GetContainer(container));
What's the reason for removing this?

[bruthig, 2016-11-01 22:59:45]

Description was changed from

==========
Applied Material Design layout to TrayItemMore system menu rows.

This change applies the new common system menu layout to all rows that
inherit from the TrayItemMore class.  This includes the following rows:

- SMS default row
- Bluetooth default row
- Cast default row
- Accessibility default row
- IME default row
- Network default row
- Display rotation lock default row
- VPN default row

BUG=657669
==========

to

==========
Applied Material Design layout to TrayItemMore system menu rows.

This change applies the new common system menu layout to all rows that
inherit from the TrayItemMore class.  This includes the following rows:

- SMS default row
- Bluetooth default row
- Cast default row
- Accessibility default row
- IME default row
- Network default row
- Display rotation lock default row
- VPN default row

TBR=jamescook@chromium.org
BUG=657669
==========

[bruthig, 2016-11-01 23:00:42]

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

[bruthig, 2016-11-01 23:00:43]

Terry, take a look if you wish but not required.

James, I've added you as a TBR for ash/BUILD.gn.  Can you take a look when you
have a chance?

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

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_constants.cc:56: const int
kTrayPopupLabelHorizontalPadding = 4;
On 2016/11/01 18:59:43, tdanderson wrote:
> I think this may be a duplicate of kTrayPopupTextSpacingVertical in this file.

kTrayPopupTextSpacingVertical doesn't appear to be used so I removed it.

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_constants.cc:106: const int kTrayPopupItemHeight[] =
{46, 48, 48};
On 2016/11/01 18:59:44, tdanderson wrote:
> Note that since these arrays have the form {a, b, b} (the material normal
value
> is the same as the material experimental value) make sure
> https://chromiumcodereview.appspot.com/2454393002 has landed before this CL
goes
> into the commit queue.

Acknowledged.

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

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_constants.h:54: // The padding used on the left and
right of labels.
On 2016/11/01 18:59:44, tdanderson wrote:
> nit: please specify in the comment if the constant should be used only for
> labels in default view rows, or if it applies to all labels throughout the
> system menu. Ditto for the values on lines 136 and 139.

Done.

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/tray_item_more.cc (left):

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_item_more.cc:115: bounds.set_width(more_->x() -
kTrayPopupPaddingBetweenItems - label_->x());
On 2016/11/01 18:59:44, tdanderson wrote:
> Double-checking since it could be easy to forget when only testing with short
> strings: will TriView handle this case nicely?

Yup it will. +1 for the TriView.

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

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_item_more.cc:35: SetLayoutManager(new
views::FillLayout);
On 2016/11/01 18:59:44, tdanderson wrote:
> optional: Consider adding something to the class-level documentation of
TriView
> to mention that a FillLayout should be used as the layout manager of TriView's
> parent view.

I'm torn on whether this should be documented or not.  In order for a child view
to be visible it must be allocated space via a layout.  I think it falls under
general domain knowledge within the View/LayoutManager framework.  I think the
requested docs are to specific to be added to the more general to TriView class.

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_item_more.cc:41:
label_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
On 2016/11/01 18:59:44, tdanderson wrote:
> Would it be appropriate to move line 41 into CreateDefaultLabel() ?

Good call, done.

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_item_more.cc:46:
more_->EnableCanvasFlippingForRTLUI(true);
On 2016/11/01 18:59:44, tdanderson wrote:
> Similar to above comment, should this be moved into CreateMoreImageView() ?

Done.

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_item_more.cc:55:
tri_view->SetContainerVisible(TriView::Container::END, false);
On 2016/11/01 18:59:44, tdanderson wrote:
> Interesting, I didn't realize this would have been necessary; presumably this
> empty container affects the appearance of the layout? Have you considered
> modifying TriView to make containers invisible whenever they are empty? (OK to
> discuss / do as follow up to this CL).
> 
> I think manually setting empty containers to invisible would be an easy thing
> for someone to forget.

Setting the END container to hidden would allocate more space to the CENTER
container.  This may end up being desired for the END container but we don't
want to do this for the START container because that is how we ensure CENTER
container labels are left-aligned.  I am going to defer this change until a more
obvious pattern emerges.

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

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_popup_utils.cc:15: #include
"ui/views/controls/slider.h"
On 2016/11/01 18:59:44, tdanderson wrote:
> Is this include needed?

Removed.

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_popup_utils.cc:43:
views::BoxLayout::CROSS_AXIS_ALIGNMENT_STRETCH);
On 2016/11/01 18:59:44, tdanderson wrote:
> It seems CENTER is what you want here instead of STRETCH?

Actually this should be START if we are trying to account for multi-line rows. 
However, it's unclear if a one size fits all approach is going to work so I'm
going to defer those changes until we are applying the layout to a multi-line
row.

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/tray_popup_utils.h (right):

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_popup_utils.h:5: #ifndef
ASH_COMMON_SYSTEM_TRAY_TRAY_POPUP_UTIL_H_
On 2016/11/01 18:59:44, tdanderson wrote:
> nit: UTILS_H_ rather than UTIL_H_

Done.

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_popup_utils.h:45: // Returns a label that has been
configured for system menu layout.
On 2016/11/01 18:59:44, tdanderson wrote:
> Similar to the comments I made in the constants file, can you please clarify
in
> the documentation for CreateDefaultLabel(), CreateMainImageView(), and Create
> MoreImageView() if they are to be used only by default menu rows.
Specifically,
> it would be good to make it clear for everyone whether or not these can be
used
> for the rows in the detailed views (e.g., the rows for each available
Bluetooth
> device).

Done.

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/tri_view.cc (left):

https://codereview.chromium.org/2468533002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tri_view.cc:116:
GetLayoutManager(container)->Installed(GetContainer(container));
On 2016/11/01 18:59:44, tdanderson wrote:
> What's the reason for removing this?

It was incorrect in the first place.  The |containers| layout manager will be
notified of an install via SizeRangeLayout::SetLayoutManager() and
SizeRangeLayout::Installed().

[bruthig, 2016-11-01 23:00:48]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2016-11-01 23:00:48]

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/2468533002/#ps40001
(title: "Addressed review comments.")

[commit-bot: I haz the power, 2016-11-01 23:01:53]

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-11-01 23:05:11]

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

[commit-bot: I haz the power, 2016-11-01 23:05:12]

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

[bruthig, 2016-11-01 23:57:14]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2016-11-01 23:57:14]

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/2468533002/#ps60001
(title: "Merge branch 'master' into md_system_menu_tray_item_more_layout")

[commit-bot: I haz the power, 2016-11-01 23:57:44]

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-11-02 01:09:15]

Description was changed from

==========
Applied Material Design layout to TrayItemMore system menu rows.

This change applies the new common system menu layout to all rows that
inherit from the TrayItemMore class.  This includes the following rows:

- SMS default row
- Bluetooth default row
- Cast default row
- Accessibility default row
- IME default row
- Network default row
- Display rotation lock default row
- VPN default row

TBR=jamescook@chromium.org
BUG=657669
==========

to

==========
Applied Material Design layout to TrayItemMore system menu rows.

This change applies the new common system menu layout to all rows that
inherit from the TrayItemMore class.  This includes the following rows:

- SMS default row
- Bluetooth default row
- Cast default row
- Accessibility default row
- IME default row
- Network default row
- Display rotation lock default row
- VPN default row

TBR=jamescook@chromium.org
BUG=657669
==========

[commit-bot: I haz the power, 2016-11-02 01:09:16]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-02 01:10:31]

Description was changed from

==========
Applied Material Design layout to TrayItemMore system menu rows.

This change applies the new common system menu layout to all rows that
inherit from the TrayItemMore class.  This includes the following rows:

- SMS default row
- Bluetooth default row
- Cast default row
- Accessibility default row
- IME default row
- Network default row
- Display rotation lock default row
- VPN default row

TBR=jamescook@chromium.org
BUG=657669
==========

to

==========
Applied Material Design layout to TrayItemMore system menu rows.

This change applies the new common system menu layout to all rows that
inherit from the TrayItemMore class.  This includes the following rows:

- SMS default row
- Bluetooth default row
- Cast default row
- Accessibility default row
- IME default row
- Network default row
- Display rotation lock default row
- VPN default row

TBR=jamescook@chromium.org
BUG=657669

Committed: https://crrev.com/7964cb1f73d2ca30deadfb8a0cd0f2e11a76e8ce
Cr-Commit-Position: refs/heads/master@{#429171}
==========

[commit-bot: I haz the power, 2016-11-02 01:10:32]

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