Modify Ash MD system menu layout for title rows

Modify Ash MD system menu layout for title rows

Correct the layout for the detailed view title rows
in the Ash material design system menu. This is
done by making use of TriView (as is used in
many other places in the system menu for uniformity)
and making SpecialPopupRow only used in non-MD.

Furthermore, this CL corrects the base spoken feedback
name used for all toggles in the system menu
(Bluetooth, WiFi/cellular, and IME VK).

BUG=663050, 652677, 661319
TEST=ash_unittests

Committed: https://crrev.com/f0cd6680fbb8a4562fdb047618233b2482f6225c
Cr-Commit-Position: refs/heads/master@{#432569}

[tdanderson, 2016-11-14 03:16:15]

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

[commit-bot: I haz the power, 2016-11-14 03:16:31]

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

[tdanderson, 2016-11-14 03:16:43]

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

[tdanderson, 2016-11-14 03:16:43]

Ben, can you please take a look?

[commit-bot: I haz the power, 2016-11-14 03:51:55]

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

[commit-bot: I haz the power, 2016-11-14 03:51:56]

Dry run: This issue passed the CQ dry run.

[bruthig, 2016-11-14 04:39:12]

lgtm with nuts

High level question though, what is the spoken feedback for the Help and
Settings buttons?  Does it included the type of detailed view that is open? e.g.
"Bluetooth", "Accessibility", etc.

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:486: DCHECK(tri_view());
nit: I think convention says not to add the DCHECK if it's simply going to crash
on the next line.

CHECK(!toggle_ && !settings_) might be helpful here.

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/network/network_state_list_detailed_view.cc
(right):

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:481:
DCHECK(tri_view());
nit: not needed.

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:668:
Layout();
nit: I don't think this is needed since NetworkStateListDetailedView::Update()
calls Layout().

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/ime/t...
File ash/common/system/ime/tray_ime_chromeos.cc (right):

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/ime/t...
ash/common/system/ime/tray_ime_chromeos.cc:162: DCHECK(tri_view());
nit: not needed.

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

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.cc:312: TrayPopupItemStyle
style(GetNativeTheme(),
You should be reapplying the style to |label_| in OnNativeThemeChanged().

And you shouldn't be applying it here if GetNativeTheme() is null. Once
TrayPopupItemStyle is fixed to use the theme, this would crash if the theme is
not available here, e.g. |this| hasn't been added to a View yet.

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.cc:330: DCHECK(!title_row_);
If you move this DCHECK and the one on line 302 above the if statement it will
also guard against this method being called once when UseMd() returns true, and
once when it returns false.

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.cc:397: views::CustomButton*
TrayDetailsView::CreateSettingsButton(LoginStatus status) {
nit: Might benefit from a DCHECK(UseMd());
Same applies to CreateHelpButton() and CreateBackButton().

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

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.h:10: #include
"ash/common/system/tray/system_menu_button.h"
nit: Is this needed?

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.h:11: #include
"ash/common/system/tray/tri_view.h"
nit: This could be a forward declaration.

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.h:79: views::CustomButton*
CreateSettingsButton(LoginStatus status);
Can you mention that the caller assumes ownership?
Same applies to CreateBackButton().

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/tray_...
File ash/common/system/tray_accessibility.cc (right):

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/tray_...
ash/common/system/tray_accessibility.cc:340: DCHECK(tri_view());
nit: not needed.

[tdanderson, 2016-11-14 19:24:59]

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:486: DCHECK(tri_view());
On 2016/11/14 04:39:11, bruthig wrote:
> nit: I think convention says not to add the DCHECK if it's simply going to
crash
> on the next line.
> 
> CHECK(!toggle_ && !settings_) might be helpful here.

Done, here and changed similarly for the other overrides of
CreateExtraTitleRowButtons() in the CL.

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/network/network_state_list_detailed_view.cc
(right):

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:481:
DCHECK(tri_view());
On 2016/11/14 04:39:11, bruthig wrote:
> nit: not needed.

Done.

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:668:
Layout();
On 2016/11/14 04:39:11, bruthig wrote:
> nit: I don't think this is needed since NetworkStateListDetailedView::Update()
> calls Layout().

Done.

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/ime/t...
File ash/common/system/ime/tray_ime_chromeos.cc (right):

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/ime/t...
ash/common/system/ime/tray_ime_chromeos.cc:162: DCHECK(tri_view());
On 2016/11/14 04:39:11, bruthig wrote:
> nit: not needed.

Done.

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

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.cc:312: TrayPopupItemStyle
style(GetNativeTheme(),
On 2016/11/14 04:39:11, bruthig wrote:
> You should be reapplying the style to |label_| in OnNativeThemeChanged().
> 
> And you shouldn't be applying it here if GetNativeTheme() is null. Once
> TrayPopupItemStyle is fixed to use the theme, this would crash if the theme is
> not available here, e.g. |this| hasn't been added to a View yet.

Done.

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.cc:330: DCHECK(!title_row_);
On 2016/11/14 04:39:11, bruthig wrote:
> If you move this DCHECK and the one on line 302 above the if statement it will
> also guard against this method being called once when UseMd() returns true,
and
> once when it returns false.

Done.

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.cc:397: views::CustomButton*
TrayDetailsView::CreateSettingsButton(LoginStatus status) {
On 2016/11/14 04:39:11, bruthig wrote:
> nit: Might benefit from a DCHECK(UseMd());
> Same applies to CreateHelpButton() and CreateBackButton().

Done.

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

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.h:10: #include
"ash/common/system/tray/system_menu_button.h"
On 2016/11/14 04:39:11, bruthig wrote:
> nit: Is this needed?

Done.

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.h:11: #include
"ash/common/system/tray/tri_view.h"
On 2016/11/14 04:39:11, bruthig wrote:
> nit: This could be a forward declaration.

Done.

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.h:79: views::CustomButton*
CreateSettingsButton(LoginStatus status);
On 2016/11/14 04:39:11, bruthig wrote:
> Can you mention that the caller assumes ownership?
> Same applies to CreateBackButton().

Done.

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/tray_...
File ash/common/system/tray_accessibility.cc (right):

https://codereview.chromium.org/2497813002/diff/20001/ash/common/system/tray_...
ash/common/system/tray_accessibility.cc:340: DCHECK(tri_view());
On 2016/11/14 04:39:12, bruthig wrote:
> nit: not needed.

Done.

[tdanderson, 2016-11-14 19:25:50]

The CQ bit was checked by tdanderson@chromium.org

[tdanderson, 2016-11-14 19:25:51]

The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org
Link to the patchset: https://codereview.chromium.org/2497813002/#ps40001
(title: "Ben comments")

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

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-14 20:27:09]

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

[commit-bot: I haz the power, 2016-11-14 20:27:10]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[bruthig, 2016-11-16 14:58:32]

https://codereview.chromium.org/2497813002/diff/40001/ash/common/system/tray/...
File ash/common/system/tray/tray_details_view.cc (right):

https://codereview.chromium.org/2497813002/diff/40001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.cc:422: if (!GetNativeTheme())
I have to apologize, I was wrong.  GetNativeTheme() should never return null. 
So you can remove this check.

[tdanderson, 2016-11-16 18:05:55]

The CQ bit was checked by tdanderson@chromium.org

[tdanderson, 2016-11-16 18:05:55]

The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org
Link to the patchset: https://codereview.chromium.org/2497813002/#ps60001
(title: "trybots")

[commit-bot: I haz the power, 2016-11-16 18:06:40]

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-16 18:40:43]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-16 18:52:49]

Description was changed from

==========
Modify Ash MD system menu layout for title rows

Correct the layout for the detailed view title rows
in the Ash material design system menu. This is
done by making use of TriView (as is used in
many other places in the system menu for uniformity)
and making SpecialPopupRow only used in non-MD.

Furthermore, this CL corrects the base spoken feedback
name used for all toggles in the system menu
(Bluetooth, WiFi/cellular, and IME VK).

BUG=663050,652677,661319
TEST=ash_unittests
==========

to

==========
Modify Ash MD system menu layout for title rows

Correct the layout for the detailed view title rows
in the Ash material design system menu. This is
done by making use of TriView (as is used in
many other places in the system menu for uniformity)
and making SpecialPopupRow only used in non-MD.

Furthermore, this CL corrects the base spoken feedback
name used for all toggles in the system menu
(Bluetooth, WiFi/cellular, and IME VK).

BUG=663050,652677,661319
TEST=ash_unittests

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

[commit-bot: I haz the power, 2016-11-16 18:52:50]

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