[ash-md] Updated layout of Brightness and Audio system menu rows to the material design spec.

[ash-md] Updated layout of Brightness and Audio system menu rows to the material design spec.

The new material design look has a different layout.  This CL updates
the layout of the brightness and audio rows of the system menu to
conform to the material layout while keeping a reasonable layout for
non-material.

This change also adds the ink drop to the audio row's mute and more
buttons but does not size it correctly.

BUG=663009, 662951, 662947
Committed: https://crrev.com/2e8f8286016a001848e03b530131fb1592be4ef5
Cr-Commit-Position: refs/heads/master@{#431361}

[bruthig, 2016-11-08 04:10:53]

Description was changed from

==========
Updated layout of Brightness and Audio system menu rows to the material design
spec.

The new material design look has a different layout.  This CL updates
the layout of the brightness and audio rows of the system menu to
conform to the material layout while keeping a reasonable layout for
non-material.

This change also adds the ink drop to the audio row's mute and more
buttons.

BUG=663009, 662951, 662947
==========

to

==========
[ash-md] Updated layout of Brightness and Audio system menu rows to the material
design spec.

The new material design look has a different layout.  This CL updates
the layout of the brightness and audio rows of the system menu to
conform to the material layout while keeping a reasonable layout for
non-material.

This change also adds the ink drop to the audio row's mute and more
buttons.

BUG=663009, 662951, 662947
==========

[bruthig, 2016-11-08 22:59:50]

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

[bruthig, 2016-11-08 23:00:12]

Description was changed from

==========
[ash-md] Updated layout of Brightness and Audio system menu rows to the material
design spec.

The new material design look has a different layout.  This CL updates
the layout of the brightness and audio rows of the system menu to
conform to the material layout while keeping a reasonable layout for
non-material.

This change also adds the ink drop to the audio row's mute and more
buttons.

BUG=663009, 662951, 662947
==========

to

==========
[ash-md] Updated layout of Brightness and Audio system menu rows to the material
design spec.

The new material design look has a different layout.  This CL updates
the layout of the brightness and audio rows of the system menu to
conform to the material layout while keeping a reasonable layout for
non-material.

This change also adds the ink drop to the audio row's mute and more
buttons but does not size it correctly.

BUG=663009, 662951, 662947
==========

[commit-bot: I haz the power, 2016-11-08 23:00:30]

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

[bruthig, 2016-11-08 23:00:34]

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

[bruthig, 2016-11-08 23:00:34]

tdanderson@, can you PTAL?

[commit-bot: I haz the power, 2016-11-08 23:49:49]

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

[commit-bot: I haz the power, 2016-11-08 23:49:50]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2016-11-09 01:38:34]

A few questions and comments, otherwise LGTM

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
File ash/common/system/audio/volume_view.cc (left):

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
ash/common/system/audio/volume_view.cc:70:
SetFocusPainter(views::Painter::CreateSolidFocusPainter(
Looks like you're inheriting the focus painter from ActionableView, right? (did
you double check tabbing through this row in non-MD and MD?)

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
File ash/common/system/audio/volume_view.cc (right):

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
ash/common/system/audio/volume_view.cc:180:
gfx::CreateVectorIcon(kSystemMenuArrowRightIcon, kMenuIconColor));
The arrow color may possibly change if the row is disabled (e.g., Bluetooth,
Network).

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
ash/common/system/audio/volume_view.cc:199: TrayPopupItemContainer*
more_container_ =
If you mean for this to be local, remove the trailing _

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
ash/common/system/audio/volume_view.cc:207: if (!is_default_view_)
Consider moving this earlier (before line 171) as an early return to avoid doing
all of the creation of the more button and icons if they're not even going to be
visible. i.e.,

set_background(views::Background::CreateSolidBackground(kBackgroundColor));
if (!is_default_view_) {
  tri_view_->SetContainerVisible(END, false);
  Update();
  return;
}

// ... lines 170-205

Update();

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
ash/common/system/audio/volume_view.cc:294: if (!more_button_->visible())
Consider restructuring as 'else if (sender == more && more button visible) - or
dropping the visibility check entirely since I don't see how that could ever be
applicable

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
ash/common/system/audio/volume_view.cc:297: }
optional nit: else NOTREACHED() ?

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
File ash/common/system/audio/volume_view.h (right):

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
ash/common/system/audio/volume_view.h:65: // Container view used to layout the
children.
Maybe add to this comment to clarify that |tri_view_| is the only child of
|this|, and all other views elements in this row are parented to the tri view.

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
ash/common/system/audio/volume_view.h:73: bool is_default_view_;
nit: can you please add a "// not used in material design" or "// only used in
material design" after these members where applicable?

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/brightness/tray_brightness.cc (right):

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/brightness/tray_brightness.cc:101: // that's what the
audio row uses and we want the two rows to layout the same.
I interpret this as "the sliders should be the same width and x-offset" but
that's not always true (volume slider is shorter if there is a device type
icon). Consider re-wording this comment.

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

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/actionable_view.cc:133: : ActionableView(owner),
listener_(listener), destroyed_(nullptr) {}
did you run git cl format ash? This looks a bit off...

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/actionable_view.cc:144: if (destroyed)
I'm not sure what this gains you, since the only place you potentially access a
member in the destroyed case is on line 146, and there you're just nulling
something out.

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

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/actionable_view.h:97: // Used by ButtonPressed() to
determine whether |this| has been destroyed as a
Copy-paste from the above class?

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

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_constants.cc:108: const int
kTrayPopupItemMoreImageSize[] = {25, 20, 20};
Consider using kMenuIconSize in place of the literal '20's here.

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_constants.cc:110: const int
kTrayPopupItemLeftInset[] = {0, 4, 4};
Good catch

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

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_popup_utils.h:87: // configure on the returned
TriView for the given |container|. To be used
wording: "... just like CreateDefaultRowView() would configure  |container| on
its returned TriView."

[bruthig, 2016-11-10 02:54:16]

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

[commit-bot: I haz the power, 2016-11-10 02:54:37]

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

[bruthig, 2016-11-10 02:54:54]

Terry, I've addressed your comments but the changes could use a once over from a
second pair of eyes.  Would you mind taking another look when you get a chance?

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
File ash/common/system/audio/volume_view.cc (left):

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
ash/common/system/audio/volume_view.cc:70:
SetFocusPainter(views::Painter::CreateSolidFocusPainter(
On 2016/11/09 01:38:33, tdanderson wrote:
> Looks like you're inheriting the focus painter from ActionableView, right?
(did
> you double check tabbing through this row in non-MD and MD?)

Yes the Mute button maintains the correct keyboard focus and ChromeVox feedback.

I did notice that with my change that the DeviceType/More button was now
focus-able when it was not before.  I have fixed this change to make it not
focus-able but created http://crbug.com/663767 assuming that it should be.

Good catch :)

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
File ash/common/system/audio/volume_view.cc (right):

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
ash/common/system/audio/volume_view.cc:180:
gfx::CreateVectorIcon(kSystemMenuArrowRightIcon, kMenuIconColor));
On 2016/11/09 01:38:33, tdanderson wrote:
> The arrow color may possibly change if the row is disabled (e.g., Bluetooth,
> Network).

I will defer that work but I have updated the spreadsheet accordingly.

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
ash/common/system/audio/volume_view.cc:199: TrayPopupItemContainer*
more_container_ =
On 2016/11/09 01:38:33, tdanderson wrote:
> If you mean for this to be local, remove the trailing _

Done.

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
ash/common/system/audio/volume_view.cc:207: if (!is_default_view_)
On 2016/11/09 01:38:33, tdanderson wrote:
> Consider moving this earlier (before line 171) as an early return to avoid
doing
> all of the creation of the more button and icons if they're not even going to
be
> visible. i.e.,
> 
> set_background(views::Background::CreateSolidBackground(kBackgroundColor));
> if (!is_default_view_) {
>   tri_view_->SetContainerVisible(END, false);
>   Update();
>   return;
> }
> 
> // ... lines 170-205
> 
> Update();

Done.

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
ash/common/system/audio/volume_view.cc:294: if (!more_button_->visible())
On 2016/11/09 01:38:33, tdanderson wrote:
> Consider restructuring as 'else if (sender == more && more button visible) -
or
> dropping the visibility check entirely since I don't see how that could ever
be
> applicable

Good point, dropped the visible() condition.

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
ash/common/system/audio/volume_view.cc:297: }
On 2016/11/09 01:38:33, tdanderson wrote:
> optional nit: else NOTREACHED() ?

Done.

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
File ash/common/system/audio/volume_view.h (right):

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
ash/common/system/audio/volume_view.h:65: // Container view used to layout the
children.
On 2016/11/09 01:38:33, tdanderson wrote:
> Maybe add to this comment to clarify that |tri_view_| is the only child of
> |this|, and all other views elements in this row are parented to the tri view.

Done.

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/audio...
ash/common/system/audio/volume_view.h:73: bool is_default_view_;
On 2016/11/09 01:38:33, tdanderson wrote:
> nit: can you please add a "// not used in material design" or "// only used in
> material design" after these members where applicable?

Done.

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/brightness/tray_brightness.cc (right):

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/brightness/tray_brightness.cc:101: // that's what the
audio row uses and we want the two rows to layout the same.
On 2016/11/09 01:38:33, tdanderson wrote:
> I interpret this as "the sliders should be the same width and x-offset" but
> that's not always true (volume slider is shorter if there is a device type
> icon). Consider re-wording this comment.

Done.

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

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/actionable_view.cc:133: : ActionableView(owner),
listener_(listener), destroyed_(nullptr) {}
On 2016/11/09 01:38:33, tdanderson wrote:
> did you run git cl format ash? This looks a bit off...

Yeah, this is the result of 'git cl format'.

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/actionable_view.cc:144: if (destroyed)
On 2016/11/09 01:38:33, tdanderson wrote:
> I'm not sure what this gains you, since the only place you potentially access
a
> member in the destroyed case is on line 146, and there you're just nulling
> something out.

Not sure why I had this here.  Removed.

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

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/actionable_view.h:97: // Used by ButtonPressed() to
determine whether |this| has been destroyed as a
On 2016/11/09 01:38:33, tdanderson wrote:
> Copy-paste from the above class?

Removed.

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

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_constants.cc:108: const int
kTrayPopupItemMoreImageSize[] = {25, 20, 20};
On 2016/11/09 01:38:33, tdanderson wrote:
> Consider using kMenuIconSize in place of the literal '20's here.

Done.

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

https://codereview.chromium.org/2482043002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_popup_utils.h:87: // configure on the returned
TriView for the given |container|. To be used
On 2016/11/09 01:38:33, tdanderson wrote:
> wording: "... just like CreateDefaultRowView() would configure  |container| on
> its returned TriView."

Done.

[commit-bot: I haz the power, 2016-11-10 02:57:58]

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

[commit-bot: I haz the power, 2016-11-10 02:57:59]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  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-10 03:46:19]

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

[commit-bot: I haz the power, 2016-11-10 03:46:33]

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-11-10 03:58:53]

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

[commit-bot: I haz the power, 2016-11-10 03:58:55]

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

[bruthig, 2016-11-10 14:38:01]

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

[commit-bot: I haz the power, 2016-11-10 14:38:16]

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

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

[commit-bot: I haz the power, 2016-11-10 15:09:03]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2016-11-10 18:32:53]

One comment, otherwise patch set 5 still LGTM

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

https://codereview.chromium.org/2482043002/diff/80001/ash/common/system/tray/...
ash/common/system/tray/actionable_view.h:89: // ~ButtonListenerActionableView()
override;
Uncomment? (ditto in the cc file)

[bruthig, 2016-11-10 18:38:47]

Thx for taking another look.  Sending it off to the CQ now.

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

https://codereview.chromium.org/2482043002/diff/80001/ash/common/system/tray/...
ash/common/system/tray/actionable_view.h:89: // ~ButtonListenerActionableView()
override;
On 2016/11/10 18:32:52, tdanderson wrote:
> Uncomment? (ditto in the cc file)

Opted to delete completely.

[bruthig, 2016-11-10 18:38:51]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2016-11-10 18:38:51]

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/2482043002/#ps100001
(title: "Removed ~ButtonListenerActionableView().")

[commit-bot: I haz the power, 2016-11-10 18:39:31]

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

[bruthig, 2016-11-10 18:49:57]

The CQ bit was unchecked by bruthig@chromium.org

[bruthig, 2016-11-10 20:28:10]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2016-11-10 20:28:11]

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/2482043002/#ps120001
(title: "Fixed the volume slider to transition to the detailed view on <return>
keypress.")

[commit-bot: I haz the power, 2016-11-10 20:28:39]

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-10 21:37:30]

Description was changed from

==========
[ash-md] Updated layout of Brightness and Audio system menu rows to the material
design spec.

The new material design look has a different layout.  This CL updates
the layout of the brightness and audio rows of the system menu to
conform to the material layout while keeping a reasonable layout for
non-material.

This change also adds the ink drop to the audio row's mute and more
buttons but does not size it correctly.

BUG=663009, 662951, 662947
==========

to

==========
[ash-md] Updated layout of Brightness and Audio system menu rows to the material
design spec.

The new material design look has a different layout.  This CL updates
the layout of the brightness and audio rows of the system menu to
conform to the material layout while keeping a reasonable layout for
non-material.

This change also adds the ink drop to the audio row's mute and more
buttons but does not size it correctly.

BUG=663009, 662951, 662947
==========

[commit-bot: I haz the power, 2016-11-10 21:37:31]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-10 22:34:33]

Description was changed from

==========
[ash-md] Updated layout of Brightness and Audio system menu rows to the material
design spec.

The new material design look has a different layout.  This CL updates
the layout of the brightness and audio rows of the system menu to
conform to the material layout while keeping a reasonable layout for
non-material.

This change also adds the ink drop to the audio row's mute and more
buttons but does not size it correctly.

BUG=663009, 662951, 662947
==========

to

==========
[ash-md] Updated layout of Brightness and Audio system menu rows to the material
design spec.

The new material design look has a different layout.  This CL updates
the layout of the brightness and audio rows of the system menu to
conform to the material layout while keeping a reasonable layout for
non-material.

This change also adds the ink drop to the audio row's mute and more
buttons but does not size it correctly.

BUG=663009, 662951, 662947

Committed: https://crrev.com/2e8f8286016a001848e03b530131fb1592be4ef5
Cr-Commit-Position: refs/heads/master@{#431361}
==========

[commit-bot: I haz the power, 2016-11-10 22:34:35]

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