Reuse NativeThemeAuraDark for cros tray.

Reuse NativeThemeAuraDark for cros tray.

This means we'll stay in sync with other light-on-dark surfaces
(e.g. incognito) and we won't need to manually set label colors.

BUG=638694
Committed: https://crrev.com/b35763c08fbb775ce4748ac6167c83ea05e0c25e
Cr-Commit-Position: refs/heads/master@{#413361}

[Evan Stade, 2016-08-17 18:09:15]

estade@chromium.org changed reviewers:
+ sadrul@chromium.org, sky@chromium.org, tdanderson@chromium.org

[Evan Stade, 2016-08-17 18:09:16]

+sky for ui/views
+tdanderson for ash/common/system/ (can we make you an owner of that?)
+sadrul for ash/common/system/ (already an owner)

Also, Terry, is there a relevant bug number to attach here?

[Evan Stade, 2016-08-17 18:09:24]

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

[commit-bot: I haz the power, 2016-08-17 18:09:51]

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-08-17 18:57:43]

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

[commit-bot: I haz the power, 2016-08-17 18:57:44]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2016-08-17 19:35:01]

LGTM with one suggestion. Please reference BUG=638694 in the CL description.

Regarding your suggestion of adding me to ash/common/system OWNERS, I would be
in favor of that provided that sadrul@, sky@, and jamescook@ have no objections
(+cc jamescook@).

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

https://codereview.chromium.org/2254933002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_background_view.cc:325: return
ui::NativeThemeDarkAura::instance();
I would prefer for this were MD-only, i.e.,

if (MaterialDesignController::IsShelfMaterial())
  return ui::NativeThemeDarkAura::instance();
return ActionableView::GetNativeTheme();

This guards against the possibility we accidentally regress something in non-MD
(not just for the system tray, but also for the other derived classes of
TrayBackgroundView).

[sky, 2016-08-17 20:06:12]

Can't the tray override this at the Widget level rather than introducing a
virtual method to View?

[Evan Stade, 2016-08-17 20:16:07]

On 2016/08/17 20:06:12, sky wrote:
> Can't the tray override this at the Widget level rather than introducing a
> virtual method to View?

Good question, I was told the trays (i.e. TrayBackgroundView subclasses) share a
widget with other, non-tray Views, so I didn't investigate that, but it's
possible that's not correct and we can just use StatusAreaWidget.

[sky, 2016-08-17 20:19:03]

Description was changed from

==========
Reuse NativeThemeAuraDark for cros tray.

This means we'll stay in sync with other light-on-dark surfaces
(e.g. incognito) and we won't need to manually set label colors.

BUG=
==========

to

==========
Reuse NativeThemeAuraDark for cros tray.

This means we'll stay in sync with other light-on-dark surfaces
(e.g. incognito) and we won't need to manually set label colors.

BUG=
==========

[sky, 2016-08-17 20:19:03]

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

[sky, 2016-08-17 20:19:50]

On 2016/08/17 20:16:07, Evan Stade wrote:
> On 2016/08/17 20:06:12, sky wrote:
> > Can't the tray override this at the Widget level rather than introducing a
> > virtual method to View?
> 
> Good question, I was told the trays (i.e. TrayBackgroundView subclasses) share
a
> widget with other, non-tray Views, so I didn't investigate that, but it's
> possible that's not correct and we can just use StatusAreaWidget.

+jamescook for question on sharing widget with non-tray views.

[James Cook, 2016-08-17 21:05:50]

On 2016/08/17 20:19:50, sky wrote:
> On 2016/08/17 20:16:07, Evan Stade wrote:
> > On 2016/08/17 20:06:12, sky wrote:
> > > Can't the tray override this at the Widget level rather than introducing a
> > > virtual method to View?
> > 
> > Good question, I was told the trays (i.e. TrayBackgroundView subclasses)
share
> a
> > widget with other, non-tray Views, so I didn't investigate that, but it's
> > possible that's not correct and we can just use StatusAreaWidget.
> 
> +jamescook for question on sharing widget with non-tray views.

Sorry, what do you mean by "share a widget"? Do you mean the views are contained
in other widgets than StatusAreaWidget?

I think they are all in StatusAreaWidget.

There's a thing with the IME status area tray item where it contains a menu
detail view for IME that is shared with the main status tray menu, but that's
just a detail view.

(I'm gardener right now, so I can't look in detail. Let me know if you need more
info and I can try to dig in later.)

[Evan Stade, 2016-08-17 23:42:46]

On 2016/08/17 21:05:50, James Cook (slow reviews) wrote:
> On 2016/08/17 20:19:50, sky wrote:
> > On 2016/08/17 20:16:07, Evan Stade wrote:
> > > On 2016/08/17 20:06:12, sky wrote:
> > > > Can't the tray override this at the Widget level rather than introducing
a
> > > > virtual method to View?
> > > 
> > > Good question, I was told the trays (i.e. TrayBackgroundView subclasses)
> share
> > a
> > > widget with other, non-tray Views, so I didn't investigate that, but it's
> > > possible that's not correct and we can just use StatusAreaWidget.
> > 
> > +jamescook for question on sharing widget with non-tray views.
> 
> Sorry, what do you mean by "share a widget"? Do you mean the views are
contained
> in other widgets than StatusAreaWidget?
> 
> I think they are all in StatusAreaWidget.
> 
> There's a thing with the IME status area tray item where it contains a menu
> detail view for IME that is shared with the main status tray menu, but that's
> just a detail view.
> 
> (I'm gardener right now, so I can't look in detail. Let me know if you need
more
> info and I can try to dig in later.)

The question is whether StatusAreaWidget contains anything that is not a tray
(and would not want the light-on-dark color scheme that trays have). It doesn't
appear that it does. If the IME thing you're talking about is
ShowImeMenuBubble(), that seems to use a different widget.

[James Cook, 2016-08-17 23:50:39]

On 2016/08/17 23:42:46, Evan Stade wrote:
> On 2016/08/17 21:05:50, James Cook (slow reviews) wrote:
> > On 2016/08/17 20:19:50, sky wrote:
> > > On 2016/08/17 20:16:07, Evan Stade wrote:
> > > > On 2016/08/17 20:06:12, sky wrote:
> > > > > Can't the tray override this at the Widget level rather than
introducing
> a
> > > > > virtual method to View?
> > > > 
> > > > Good question, I was told the trays (i.e. TrayBackgroundView subclasses)
> > share
> > > a
> > > > widget with other, non-tray Views, so I didn't investigate that, but
it's
> > > > possible that's not correct and we can just use StatusAreaWidget.
> > > 
> > > +jamescook for question on sharing widget with non-tray views.
> > 
> > Sorry, what do you mean by "share a widget"? Do you mean the views are
> contained
> > in other widgets than StatusAreaWidget?
> > 
> > I think they are all in StatusAreaWidget.
> > 
> > There's a thing with the IME status area tray item where it contains a menu
> > detail view for IME that is shared with the main status tray menu, but
that's
> > just a detail view.
> > 
> > (I'm gardener right now, so I can't look in detail. Let me know if you need
> more
> > info and I can try to dig in later.)
> 
> The question is whether StatusAreaWidget contains anything that is not a tray
> (and would not want the light-on-dark color scheme that trays have). It
doesn't
> appear that it does. If the IME thing you're talking about is
> ShowImeMenuBubble(), that seems to use a different widget.

I think StatusAreaWidget only contains trays (and separators, but I think those
are drawn as part of the tray items).

The IME thing I was mentioning is ImeMenuTray, which is a TrayBackgroundView
like all the others.

https://cs.chromium.org/chromium/src/ash/common/system/chromeos/ime_menu/ime_...

[Evan Stade, 2016-08-18 00:38:02]

PTAL

sky, your review is no longer necessary as there are no changes to ui/views/,
but you can of course still weigh in if you like.

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

https://codereview.chromium.org/2254933002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_background_view.cc:325: return
ui::NativeThemeDarkAura::instance();
On 2016/08/17 19:35:00, tdanderson wrote:
> I would prefer for this were MD-only, i.e.,
> 
> if (MaterialDesignController::IsShelfMaterial())
>   return ui::NativeThemeDarkAura::instance();
> return ActionableView::GetNativeTheme();
> 
> This guards against the possibility we accidentally regress something in
non-MD
> (not just for the system tray, but also for the other derived classes of
> TrayBackgroundView).

I don't think this is necessary and it's a little bit gross, but I will agree it
is safer (less likely to cause regressions), so Done.

https://codereview.chromium.org/2254933002/diff/20001/ash/common/system/date/...
File ash/common/system/date/date_view.cc (left):

https://codereview.chromium.org/2254933002/diff/20001/ash/common/system/date/...
ash/common/system/date/date_view.cc:76:
label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
This alignment is not necessary for the time labels.

The background color is neither necessary nor correct for the time labels (they
don't have auto color readability, and the bg color is set in
SetupLabelForTray). The background color/auto readability adjustments is not
necessary for the date label either, which automatically has a light bg color
set. The 0 opacity disabled subpixel rendering, which is not necessary since the
date label is displayed on a solid bg.

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

https://codereview.chromium.org/2254933002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_utils.cc:20: if
(MaterialDesignController::IsShelfMaterial()) {
I went ahead and made changes here to match Sebastien's spec

[sky, 2016-08-18 13:07:06]

sky@chromium.org changed reviewers:
- sky@chromium.org

[sky, 2016-08-18 13:08:02]

Glad to hear we don't need the override in View. Thanks!
-me as I'm no longer needed.

[James Cook, 2016-08-18 18:19:51]

lgtm

[Evan Stade, 2016-08-18 18:20:41]

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

[commit-bot: I haz the power, 2016-08-18 18:21:05]

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

[Evan Stade, 2016-08-18 18:21:11]

ping Terry for optional re-review

[Evan Stade, 2016-08-18 18:22:26]

Description was changed from

==========
Reuse NativeThemeAuraDark for cros tray.

This means we'll stay in sync with other light-on-dark surfaces
(e.g. incognito) and we won't need to manually set label colors.

BUG=
==========

to

==========
Reuse NativeThemeAuraDark for cros tray.

This means we'll stay in sync with other light-on-dark surfaces
(e.g. incognito) and we won't need to manually set label colors.

BUG=638694
==========

[tdanderson, 2016-08-18 18:37:44]

On 2016/08/18 18:21:11, Evan Stade wrote:
> ping Terry for optional re-review

still lgtm

[Evan Stade, 2016-08-18 20:11:58]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2016-08-18 20:12:01]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-08-18 20:12:13]

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-08-18 20:39:55]

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

[commit-bot: I haz the power, 2016-08-18 20:39:56]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Evan Stade, 2016-08-21 00:10:00]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-08-21 00:10:16]

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-08-21 02:26:56]

Description was changed from

==========
Reuse NativeThemeAuraDark for cros tray.

This means we'll stay in sync with other light-on-dark surfaces
(e.g. incognito) and we won't need to manually set label colors.

BUG=638694
==========

to

==========
Reuse NativeThemeAuraDark for cros tray.

This means we'll stay in sync with other light-on-dark surfaces
(e.g. incognito) and we won't need to manually set label colors.

BUG=638694
==========

[commit-bot: I haz the power, 2016-08-21 02:26:57]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-21 02:28:09]

Description was changed from

==========
Reuse NativeThemeAuraDark for cros tray.

This means we'll stay in sync with other light-on-dark surfaces
(e.g. incognito) and we won't need to manually set label colors.

BUG=638694
==========

to

==========
Reuse NativeThemeAuraDark for cros tray.

This means we'll stay in sync with other light-on-dark surfaces
(e.g. incognito) and we won't need to manually set label colors.

BUG=638694

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

[commit-bot: I haz the power, 2016-08-21 02:28:10]

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