Use correct fonts and add ripples to palette tray help/settings icon.

Use correct fonts and add ripples to palette tray help/settings icon.

BUG=630464
Committed: https://crrev.com/07ede9c168f25c7eccfefeb76e703139d28c249f
Cr-Commit-Position: refs/heads/master@{#434701}

[jdufault, 2016-11-15 23:42:00]

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

[commit-bot: I haz the power, 2016-11-15 23:42:43]

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-16 02:14:50]

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

[commit-bot: I haz the power, 2016-11-16 02:14:51]

Dry run: This issue passed the CQ dry run.

[jdufault, 2016-11-16 19:21:58]

Description was changed from

==========
Use correct fonts and add ripples to palette tray help/settings icon.

BUG=630464
==========

to

==========
Use correct fonts and add ripples to palette tray help/settings icon.

BUG=630464
==========

[jdufault, 2016-11-16 19:21:58]

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

[jdufault, 2016-11-16 19:22:08]

tdanderson@ PTAL. Thanks!

[tdanderson, 2016-11-16 20:44:16]

Thanks for coming back to this. LGTM with the comments addressed.

https://codereview.chromium.org/2502153002/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/palette/common_palette_tool.cc (right):

https://codereview.chromium.org/2502153002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/palette/common_palette_tool.cc:103:
style.SetupLabel(highlight_view_->text_label());
Please move lines 101-103 into a helper called UpdateStyle():

void CommonPaletteTool::UpdateStyle() {
  if (!highlight_view_->text_label())
    return;
  TrayPopupItemStyle style(...);
  style.SetupLabel(...);
}

Then override OnNativeThemeChanged() to call UpdateStyle():

void CommonPaletteTool::OnNativeThemeChanged(...) override {
  UpdateStyle();
}

Finally, make a call to UpdateStyle() after line 100 (effectively replacing
lines 101-103. See the documentation in the TrayPopupItemStyle header for
further details.

https://codereview.chromium.org/2502153002/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/palette/palette_tray.cc (right):

https://codereview.chromium.org/2502153002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/palette/palette_tray.cc:99: TrayPopupItemStyle
style(title_label->GetNativeTheme(),
Ditto to my previous comment, please introduce UpdateStyle() and override
OnNativeThemeChanged.

https://codereview.chromium.org/2502153002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/palette/palette_tray.cc:103:
box_layout->SetFlexForView(title_label, 1);
Please add a "TODO(tdanderson|jdufault): Use TriView to handle the layout of the
title. See crbug.com/614453." TriView was introduced to unify all of the layout
logic in the system tray rows so you don't have to manually set up layout
details; feel free to address it in this CL if you wish, but I'm ok leaving it
as-is and adding the TODO.

https://codereview.chromium.org/2502153002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/palette/palette_tray.cc:106: new
SystemMenuButton(this, SystemMenuButton::InkDropStyle::FLOOD_FILL,
Since the help and settings button don't have a vertical separator between them,
the ink drop style used for both should be
SystemMenuButton::InkDropStyle::SQUARE instead of FLOOD_FILL.

[tdanderson, 2016-11-16 20:51:03]

https://codereview.chromium.org/2502153002/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/palette/common_palette_tool.cc (right):

https://codereview.chromium.org/2502153002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/palette/common_palette_tool.cc:103:
style.SetupLabel(highlight_view_->text_label());
On 2016/11/16 20:44:16, tdanderson wrote:
> Please move lines 101-103 into a helper called UpdateStyle():
> 
> void CommonPaletteTool::UpdateStyle() {
>   if (!highlight_view_->text_label())
>     return;
>   TrayPopupItemStyle style(...);
>   style.SetupLabel(...);
> }
> 
> Then override OnNativeThemeChanged() to call UpdateStyle():
> 
> void CommonPaletteTool::OnNativeThemeChanged(...) override {
>   UpdateStyle();
> }
> 
> Finally, make a call to UpdateStyle() after line 100 (effectively replacing
> lines 101-103. See the documentation in the TrayPopupItemStyle header for
> further details.

Also, would be worth double-checking that after these changes are made that
things look as you expect with and without Ash MD enabled. It's currently
enabled by default on ToT and you can disable it by setting #ash-md to "Non
material" in about:settings. It's possible you may want to gate the call(s) to
UpdateStyle() on if (MaterialDesignController::IsSystemTrayMenuMaterial()).

[jdufault, 2016-11-17 00:00:26]

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

[commit-bot: I haz the power, 2016-11-17 00:01:35]

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-17 03:37:18]

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

[commit-bot: I haz the power, 2016-11-17 03:37:18]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2016-11-22 04:01:14]

On 2016/11/16 20:51:03, tdanderson wrote:
>
https://codereview.chromium.org/2502153002/diff/1/ash/common/system/chromeos/...
> File ash/common/system/chromeos/palette/common_palette_tool.cc (right):
> 
>
https://codereview.chromium.org/2502153002/diff/1/ash/common/system/chromeos/...
> ash/common/system/chromeos/palette/common_palette_tool.cc:103:
> style.SetupLabel(highlight_view_->text_label());
> On 2016/11/16 20:44:16, tdanderson wrote:
> > Please move lines 101-103 into a helper called UpdateStyle():
> > 
> > void CommonPaletteTool::UpdateStyle() {
> >   if (!highlight_view_->text_label())
> >     return;
> >   TrayPopupItemStyle style(...);
> >   style.SetupLabel(...);
> > }
> > 
> > Then override OnNativeThemeChanged() to call UpdateStyle():
> > 
> > void CommonPaletteTool::OnNativeThemeChanged(...) override {
> >   UpdateStyle();
> > }
> > 
> > Finally, make a call to UpdateStyle() after line 100 (effectively replacing
> > lines 101-103. See the documentation in the TrayPopupItemStyle header for
> > further details.
> 
> Also, would be worth double-checking that after these changes are made that
> things look as you expect with and without Ash MD enabled. It's currently
> enabled by default on ToT and you can disable it by setting #ash-md to "Non
> material" in about:settings. It's possible you may want to gate the call(s) to
> UpdateStyle() on if (MaterialDesignController::IsSystemTrayMenuMaterial()).

Hi, friendly ping - are you still planning to land this (and ideally have it
merged back to 56 for consistency with the system menu?)

[tdanderson, 2016-11-23 02:12:26]

https://codereview.chromium.org/2502153002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/palette/palette_tray.cc (right):

https://codereview.chromium.org/2502153002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/palette/palette_tray.cc:109:
IDS_ASH_STATUS_TRAY_SETTINGS);
Note that Evan's CL https://chromiumcodereview.appspot.com/2527513002/ will turn
these into SystemMenuButtons. (But the rest of this CL would still be good to
land for 56, though)

[tdanderson, 2016-11-23 02:15:35]

On 2016/11/23 02:12:26, tdanderson wrote:
>
https://codereview.chromium.org/2502153002/diff/20001/ash/common/system/chrom...
> File ash/common/system/chromeos/palette/palette_tray.cc (right):
> 
>
https://codereview.chromium.org/2502153002/diff/20001/ash/common/system/chrom...
> ash/common/system/chromeos/palette/palette_tray.cc:109:
> IDS_ASH_STATUS_TRAY_SETTINGS);
> Note that Evan's CL https://chromiumcodereview.appspot.com/2527513002/ will
turn
> these into SystemMenuButtons. (But the rest of this CL would still be good to
> land for 56, though)

Cancel the above comment, we can't merge Evan's CL back into 56 since
it touches focus rects, so I have asked him to leave out the changes
to the palette for now. It would be really nice to have this CL land
as-is for 56 so that it can get merged back to 56, giving the palette
and system menu a consistent appearance.

[jdufault, 2016-11-28 17:58:15]

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

[commit-bot: I haz the power, 2016-11-28 17:58:58]

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

[jdufault, 2016-11-28 17:59:05]

The CQ bit was unchecked by jdufault@chromium.org

[jdufault, 2016-11-28 17:59:15]

https://codereview.chromium.org/2502153002/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/palette/common_palette_tool.cc (right):

https://codereview.chromium.org/2502153002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/palette/common_palette_tool.cc:103:
style.SetupLabel(highlight_view_->text_label());
On 2016/11/16 20:51:03, tdanderson wrote:
> On 2016/11/16 20:44:16, tdanderson wrote:
> > Please move lines 101-103 into a helper called UpdateStyle():
> > 
> > void CommonPaletteTool::UpdateStyle() {
> >   if (!highlight_view_->text_label())
> >     return;
> >   TrayPopupItemStyle style(...);
> >   style.SetupLabel(...);
> > }
> > 
> > Then override OnNativeThemeChanged() to call UpdateStyle():
> > 
> > void CommonPaletteTool::OnNativeThemeChanged(...) override {
> >   UpdateStyle();
> > }
> > 
> > Finally, make a call to UpdateStyle() after line 100 (effectively replacing
> > lines 101-103. See the documentation in the TrayPopupItemStyle header for
> > further details.
> 
> Also, would be worth double-checking that after these changes are made that
> things look as you expect with and without Ash MD enabled. It's currently
> enabled by default on ToT and you can disable it by setting #ash-md to "Non
> material" in about:settings. It's possible you may want to gate the call(s) to
> UpdateStyle() on if (MaterialDesignController::IsSystemTrayMenuMaterial()).

Removed this code, I think HoverHighlightView will do the right thing by
default.

https://codereview.chromium.org/2502153002/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/palette/palette_tray.cc (right):

https://codereview.chromium.org/2502153002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/palette/palette_tray.cc:99: TrayPopupItemStyle
style(title_label->GetNativeTheme(),
On 2016/11/16 20:44:16, tdanderson wrote:
> Ditto to my previous comment, please introduce UpdateStyle() and override
> OnNativeThemeChanged.

Done.

https://codereview.chromium.org/2502153002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/palette/palette_tray.cc:103:
box_layout->SetFlexForView(title_label, 1);
On 2016/11/16 20:44:16, tdanderson wrote:
> Please add a "TODO(tdanderson|jdufault): Use TriView to handle the layout of
the
> title. See crbug.com/614453." TriView was introduced to unify all of the
layout
> logic in the system tray rows so you don't have to manually set up layout
> details; feel free to address it in this CL if you wish, but I'm ok leaving it
> as-is and adding the TODO.

Added TODO; I'll try to come back to this after non-md has been
deprecated/removed.

https://codereview.chromium.org/2502153002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/palette/palette_tray.cc:106: new
SystemMenuButton(this, SystemMenuButton::InkDropStyle::FLOOD_FILL,
On 2016/11/16 20:44:16, tdanderson wrote:
> Since the help and settings button don't have a vertical separator between
them,
> the ink drop style used for both should be
> SystemMenuButton::InkDropStyle::SQUARE instead of FLOOD_FILL.

Done.

[jdufault, 2016-11-28 17:59:19]

The CQ bit was checked by jdufault@chromium.org

[jdufault, 2016-11-28 17:59:19]

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/2502153002/#ps40001
(title: "Rebase")

[jdufault, 2016-11-28 17:59:42]

On 2016/11/23 02:15:35, tdanderson wrote:
> Cancel the above comment, we can't merge Evan's CL back into 56 since
> it touches focus rects, so I have asked him to leave out the changes
> to the palette for now. It would be really nice to have this CL land
> as-is for 56 so that it can get merged back to 56, giving the palette
> and system menu a consistent appearance.

Submitting now, sorry about the delay.

[commit-bot: I haz the power, 2016-11-28 17:59: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-28 18:31:37]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480355959210900,
"parent_rev": "83d99f7d19b087758494d75c73c154fa35285bad", "commit_rev":
"5bd2051a21d4e68bbe05667f50af22d5f20a21e7"}

[commit-bot: I haz the power, 2016-11-28 18:33:45]

Description was changed from

==========
Use correct fonts and add ripples to palette tray help/settings icon.

BUG=630464
==========

to

==========
Use correct fonts and add ripples to palette tray help/settings icon.

BUG=630464
==========

[commit-bot: I haz the power, 2016-11-28 18:33:50]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-28 18:36:15]

Description was changed from

==========
Use correct fonts and add ripples to palette tray help/settings icon.

BUG=630464
==========

to

==========
Use correct fonts and add ripples to palette tray help/settings icon.

BUG=630464

Committed: https://crrev.com/07ede9c168f25c7eccfefeb76e703139d28c249f
Cr-Commit-Position: refs/heads/master@{#434701}
==========

[commit-bot: I haz the power, 2016-11-28 18:36:16]

Patchset 3 (id:??) landed as
https://crrev.com/07ede9c168f25c7eccfefeb76e703139d28c249f
Cr-Commit-Position: refs/heads/master@{#434701}