Introduce a type of View background that stays in sync with its host

Introduce a type of View background that stays in sync with its host
View's native theme.

This fixes a few bugs where we weren't using the right native theme,
whether by not updating after the NativeTheme changes or by trying to
access the NativeTheme before the View is added to a hierarchy (which
yields the default NativeTheme --- for most platforms, this didn't
effectively create a bug as there's only one NativeTheme).

Best example is that now the sad tab respects the GTK native theme.

Get rid of Ash's tray_constants::kBackgroundColor in favor of using the
bubble background color from the NativeTheme.

BUG=711183, 693282
TBR=stevenjb@chromium.org

Review-Url: https://codereview.chromium.org/2816193002
Cr-Commit-Position: refs/heads/master@{#466847}
Committed: https://chromium.googlesource.com/chromium/src/+/097f9cde453ea57eb4aa037f44add782391c5eb9

[Evan Stade, 2017-04-14 18:55:48]

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

[commit-bot: I haz the power, 2017-04-14 18:56:04]

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

[Evan Stade, 2017-04-14 19:07:51]

estade@chromium.org changed reviewers:
+ msw@chromium.org, tdanderson@chromium.org

[Evan Stade, 2017-04-14 19:07:51]

+msw for ui/views
+tdanderson for ash/system

I'll find an OWNER for c/b/ui/views after you two review.

[commit-bot: I haz the power, 2017-04-14 19:47:31]

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

[commit-bot: I haz the power, 2017-04-14 19:47:32]

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_...)

[msw, 2017-04-14 20:42:17]

ui/views lgtm with FIXME fixed

https://codereview.chromium.org/2816193002/diff/20001/ui/views/view_observer.h
File ui/views/view_observer.h (right):

https://codereview.chromium.org/2816193002/diff/20001/ui/views/view_observer....
ui/views/view_observer.h:38: // FIXME
FIXME

[Evan Stade, 2017-04-17 15:19:59]

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

[Evan Stade, 2017-04-17 15:20:03]

https://codereview.chromium.org/2816193002/diff/20001/ui/views/view_observer.h
File ui/views/view_observer.h (right):

https://codereview.chromium.org/2816193002/diff/20001/ui/views/view_observer....
ui/views/view_observer.h:38: // FIXME
On 2017/04/14 20:42:17, msw wrote:
> FIXME

Done.

[commit-bot: I haz the power, 2017-04-17 15:20:10]

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, 2017-04-17 16:08:43]

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

[commit-bot: I haz the power, 2017-04-17 16:08:44]

Dry run: This issue passed the CQ dry run.

[msw, 2017-04-17 18:04:53]

https://codereview.chromium.org/2816193002/diff/80001/ash/system/user/user_vi...
File ash/system/user/user_view.cc (left):

https://codereview.chromium.org/2816193002/diff/80001/ash/system/user/user_vi...
ash/system/user/user_view.cc:126: if (policy == AddUserSessionPolicy::ALLOWED) {
What are these changes? They seem unrelated...

[Evan Stade, 2017-04-17 19:40:05]

https://codereview.chromium.org/2816193002/diff/80001/ash/system/user/user_vi...
File ash/system/user/user_view.cc (left):

https://codereview.chromium.org/2816193002/diff/80001/ash/system/user/user_vi...
ash/system/user/user_view.cc:126: if (policy == AddUserSessionPolicy::ALLOWED) {
On 2017/04/17 18:04:53, msw wrote:
> What are these changes? They seem unrelated...

I refactored the wrap-the-view-in-a-button bit to make it possible to use
view->background()->get_color() below.

[msw, 2017-04-17 20:05:40]

https://codereview.chromium.org/2816193002/diff/80001/ash/system/user/user_vi...
File ash/system/user/user_view.cc (left):

https://codereview.chromium.org/2816193002/diff/80001/ash/system/user/user_vi...
ash/system/user/user_view.cc:126: if (policy == AddUserSessionPolicy::ALLOWED) {
On 2017/04/17 19:40:05, Evan Stade wrote:
> On 2017/04/17 18:04:53, msw wrote:
> > What are these changes? They seem unrelated...
> 
> I refactored the wrap-the-view-in-a-button bit to make it possible to use
> view->background()->get_color() below.

Okay, I'll leave review here to tdanderson@

[tdanderson, 2017-04-18 19:14:46]

ash/ lgtm

https://codereview.chromium.org/2816193002/diff/80001/ash/system/audio/volume...
File ash/system/audio/volume_view.cc (right):

https://codereview.chromium.org/2816193002/diff/80001/ash/system/audio/volume...
ash/system/audio/volume_view.cc:28: #include "ui/views/layout/fill_layout.h"
#include native_theme.h here? Ditto for other files in this CL

https://codereview.chromium.org/2816193002/diff/80001/ash/system/user/user_vi...
File ash/system/user/user_view.cc (left):

https://codereview.chromium.org/2816193002/diff/80001/ash/system/user/user_vi...
ash/system/user/user_view.cc:126: if (policy == AddUserSessionPolicy::ALLOWED) {
On 2017/04/17 20:05:40, msw wrote:
> On 2017/04/17 19:40:05, Evan Stade wrote:
> > On 2017/04/17 18:04:53, msw wrote:
> > > What are these changes? They seem unrelated...
> > 
> > I refactored the wrap-the-view-in-a-button bit to make it possible to use
> > view->background()->get_color() below.
> 
> Okay, I'll leave review here to tdanderson@

This looks good to me.

https://codereview.chromium.org/2816193002/diff/80001/ash/system/user/user_vi...
File ash/system/user/user_view.cc (right):

https://codereview.chromium.org/2816193002/diff/80001/ash/system/user/user_vi...
ash/system/user/user_view.cc:362: add_user_view->background()->get_color()),
nit: optional: consider giving add_user_view->background()->get_color() a name
since it's used in two places.

[Evan Stade, 2017-04-19 15:58:30]

Description was changed from

==========
Introduce a type of View background that stays in sync with its host
View's native theme.

This fixes a few bugs where we weren't using the right native theme,
whether by not updating after the NativeTheme changes or by trying to
access the NativeTheme before the View is added to a hierarchy (which
yields the default NativeTheme --- for most platforms, this didn't
effectively create a bug as there's only one NativeTheme).

Best example is that now the sad tab respects the GTK native theme.

Get rid of Ash's tray_constants::kBackgroundColor in favor of using the
bubble background color from the NativeTheme.

BUG=711183,693282
==========

to

==========
Introduce a type of View background that stays in sync with its host
View's native theme.

This fixes a few bugs where we weren't using the right native theme,
whether by not updating after the NativeTheme changes or by trying to
access the NativeTheme before the View is added to a hierarchy (which
yields the default NativeTheme --- for most platforms, this didn't
effectively create a bug as there's only one NativeTheme).

Best example is that now the sad tab respects the GTK native theme.

Get rid of Ash's tray_constants::kBackgroundColor in favor of using the
bubble background color from the NativeTheme.

BUG=711183,693282
TBR=stevenjb@chromium.org
==========

[Evan Stade, 2017-04-19 15:58:31]

estade@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Evan Stade, 2017-04-19 15:59:01]

tbr stevenjb for deletions in message center

https://codereview.chromium.org/2816193002/diff/80001/ash/system/audio/volume...
File ash/system/audio/volume_view.cc (right):

https://codereview.chromium.org/2816193002/diff/80001/ash/system/audio/volume...
ash/system/audio/volume_view.cc:28: #include "ui/views/layout/fill_layout.h"
On 2017/04/18 19:14:45, tdanderson wrote:
> #include native_theme.h here? Ditto for other files in this CL

Done.

https://codereview.chromium.org/2816193002/diff/80001/ash/system/user/user_vi...
File ash/system/user/user_view.cc (right):

https://codereview.chromium.org/2816193002/diff/80001/ash/system/user/user_vi...
ash/system/user/user_view.cc:362: add_user_view->background()->get_color()),
On 2017/04/18 19:14:46, tdanderson wrote:
> nit: optional: consider giving add_user_view->background()->get_color() a name
> since it's used in two places.

Done.

[Evan Stade, 2017-04-19 15:59:10]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-04-19 15:59:10]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
tdanderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2816193002/#ps100001
(title: "tdanderson review + rebase")

[Evan Stade, 2017-04-19 15:59:24]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2017-04-19 16:00:01]

msw: forgot you are an owner of c/b/u/v. Could you take a look at that directory
too if you haven't already? thanks.

[Evan Stade, 2017-04-19 16:00:10]

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

[commit-bot: I haz the power, 2017-04-19 16:00:28]

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

[stevenjb, 2017-04-19 16:07:40]

ui/message_center lgtm

[commit-bot: I haz the power, 2017-04-19 17:10:02]

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

[commit-bot: I haz the power, 2017-04-19 17:10:03]

Dry run: This issue passed the CQ dry run.

[msw, 2017-04-19 17:47:36]

A couple questions reviewing c/b/u/v

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/sad_tab_view.cc (right):

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/sad_tab_view.cc:62: title_ = new
views::Label(l10n_util::GetStringUTF16(GetTitle()));
These labels and links no longer initialize the background color for
readability. Why shouldn't they call SetBackgroundColor any more?

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/sad_tab_view.cc:84: help_link_ = new
views::Link(l10n_util::GetStringUTF16(GetHelpLinkTitle()));
This link no longer gets the color of a disabled label; why change that?

[Evan Stade, 2017-04-19 18:25:53]

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/sad_tab_view.cc (right):

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/sad_tab_view.cc:62: title_ = new
views::Label(l10n_util::GetStringUTF16(GetTitle()));
On 2017/04/19 17:47:36, msw wrote:
> These labels and links no longer initialize the background color for
> readability. Why shouldn't they call SetBackgroundColor any more?

they shouldn't need to because any coherent NativeTheme ensures that default
text color for both labels and links render readably on the DialogBackground
color.

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/sad_tab_view.cc:84: help_link_ = new
views::Link(l10n_util::GetStringUTF16(GetHelpLinkTitle()));
On 2017/04/19 17:47:36, msw wrote:
> This link no longer gets the color of a disabled label; why change that?

I thought it better to match how all the other links in Chrome look, and this
also means that the link color will update when switching native themes. (And
since this is called before SadTabView is added to a widget hierarchy, that's
important.)

[Evan Stade, 2017-04-19 18:49:07]

https://codereview.chromium.org/2816193002/diff/100001/ash/system/user/user_v...
File ash/system/user/user_view.cc (right):

https://codereview.chromium.org/2816193002/diff/100001/ash/system/user/user_v...
ash/system/user/user_view.cc:373: add_user_padding->AddChildView(button);
oops, this will break the button pressed handler. note to self: fix before
landing.

[msw, 2017-04-19 19:00:54]

Hmm, this is less straightforward that it seemed to me at first.

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:205:
ime_inline_autocomplete_view_->SetEnabledColor(
This label's foreground color doesn't change with its dynamic background theme
color. What prevents the foreground and background text from
clashing/mismatching when the native theme changes here?

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/sad_tab_view.cc (right):

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/sad_tab_view.cc:84: help_link_ = new
views::Link(l10n_util::GetStringUTF16(GetHelpLinkTitle()));
On 2017/04/19 18:25:53, Evan Stade wrote:
> On 2017/04/19 17:47:36, msw wrote:
> > This link no longer gets the color of a disabled label; why change that?
> 
> I thought it better to match how all the other links in Chrome look, and this
> also means that the link color will update when switching native themes. (And
> since this is called before SadTabView is added to a widget hierarchy, that's
> important.)

Presumably a UX designer explicitly selected the disabled text theme color for
this sad tab link, shouldn't you ping them and/or ensure this matches
Cocoa/Android/iOS?

More generally, how do controls with custom (or pre-calculated themed) colors
handle backgrounds that change with the native theme? It seems like native theme
changes should also (for example) update a label's background color and either
update a themed foreground text color or recalculate a readable text color as
needed... That seems difficult to accomplish, so presumably any labels used on
top of CreateThemedSolidBackground views should never set non-default text
colors nor use SetBackgroundColor? It seems like this is a recipe for having
weird mismatching colors on native theme changes unless we carefully audit all
related views... I found a couple questionable examples by doing another quick
pass on c/b/u/v.

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/screen_capture_notification_ui_views.cc (right):

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/screen_capture_notification_ui_views.cc:282: SkColor
color = widget->GetNativeTheme()->GetSystemColor(
What updates this precalculated frame color to match the ThemedSolidBackground
changes that may occur?

[Evan Stade, 2017-04-20 04:44:22]

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:205:
ime_inline_autocomplete_view_->SetEnabledColor(
On 2017/04/19 19:00:54, msw wrote:
> This label's foreground color doesn't change with its dynamic background theme
> color. What prevents the foreground and background text from
> clashing/mismatching when the native theme changes here?

I didn't really intend to fix all native theming issues, just the background
ones. But this is easily addressed by moving this setter to
OnNativeThemeChanged.

(If you'd like me to I can back out some or all c/b/u/v changes which were
originally included mostly as proofs of the usefulness of the new background.)

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/sad_tab_view.cc (right):

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/sad_tab_view.cc:84: help_link_ = new
views::Link(l10n_util::GetStringUTF16(GetHelpLinkTitle()));
On 2017/04/19 19:00:54, msw wrote:
> On 2017/04/19 18:25:53, Evan Stade wrote:
> > On 2017/04/19 17:47:36, msw wrote:
> > > This link no longer gets the color of a disabled label; why change that?
> > 
> > I thought it better to match how all the other links in Chrome look, and
this
> > also means that the link color will update when switching native themes.
(And
> > since this is called before SadTabView is added to a widget hierarchy,
that's
> > important.)
> 
> Presumably a UX designer explicitly selected the disabled text theme color for
> this sad tab link, shouldn't you ping them and/or ensure this matches
> Cocoa/Android/iOS?

I would ping Hwi, but she is ooo. I see this as one of many places where in the
past we made disjointed UI decisions that we'd like to be more coherent now
(hence project name "harmony"). I think the only real use case for
SetEnabledColor on views::Link is for links affected by custom browser themes
(e.g. the link to import bookmarks).

> 
> More generally, how do controls with custom (or pre-calculated themed) colors
> handle backgrounds that change with the native theme? It seems like native
theme
> changes should also (for example) update a label's background color and either
> update a themed foreground text color or recalculate a readable text color as
> needed... That seems difficult to accomplish, so presumably any labels used on
> top of CreateThemedSolidBackground views should never set non-default text
> colors nor use SetBackgroundColor? 

yes

> It seems like this is a recipe for having
> weird mismatching colors on native theme changes unless we carefully audit all
> related views... I found a couple questionable examples by doing another quick
> pass on c/b/u/v.

Could you point them out? (Or are you referring to the ones you have already?)

In PasswordGenerationPopupViewViews I see many potential color/theme related
bugs, both with and without this change.

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/screen_capture_notification_ui_views.cc (right):

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/screen_capture_notification_ui_views.cc:282: SkColor
color = widget->GetNativeTheme()->GetSystemColor(
On 2017/04/19 19:00:54, msw wrote:
> What updates this precalculated frame color to match the ThemedSolidBackground
> changes that may occur?

I'll just leave this file alone. I think the correct fix is that
ScreenCaptureNotificationUIViews ought to be a BubbleDialogDelegate which would
update the bubble border on native theme changes and this function wouldn't
exist. But there's no need to chase down that rabbit hole at this moment.

[msw, 2017-04-20 18:21:46]

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:205:
ime_inline_autocomplete_view_->SetEnabledColor(
On 2017/04/20 04:44:22, Evan Stade wrote:
> On 2017/04/19 19:00:54, msw wrote:
> > This label's foreground color doesn't change with its dynamic background
theme
> > color. What prevents the foreground and background text from
> > clashing/mismatching when the native theme changes here?
> 
> I didn't really intend to fix all native theming issues, just the background
> ones. But this is easily addressed by moving this setter to
> OnNativeThemeChanged.
> 
> (If you'd like me to I can back out some or all c/b/u/v changes which were
> originally included mostly as proofs of the usefulness of the new background.)

I just think this isn't as simple as it seems; "fixing the background" might be
"making text illegible/invisible" if we're not careful. You should test these
various cases, especially around inverted color schemes, and seek out color
assignments used above these themed backgrounds.

Moving this assignment to OnNativeThemeChanged seems reasonable, but actually
makes me wonder if we should skip adding this smart class and just make views
set their background colors in OnNativeThemeChanged. That would keep more color
assignments together; rather than now setting the background in Init and the
foreground in OnNativeThemeChanged.... WDYT?

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/sad_tab_view.cc (right):

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/sad_tab_view.cc:84: help_link_ = new
views::Link(l10n_util::GetStringUTF16(GetHelpLinkTitle()));
On 2017/04/20 04:44:22, Evan Stade wrote:
> On 2017/04/19 19:00:54, msw wrote:
> > On 2017/04/19 18:25:53, Evan Stade wrote:
> > > On 2017/04/19 17:47:36, msw wrote:
> > > > This link no longer gets the color of a disabled label; why change that?
> > > 
> > > I thought it better to match how all the other links in Chrome look, and
> this
> > > also means that the link color will update when switching native themes.
> (And
> > > since this is called before SadTabView is added to a widget hierarchy,
> that's
> > > important.)
> > 
> > Presumably a UX designer explicitly selected the disabled text theme color
for
> > this sad tab link, shouldn't you ping them and/or ensure this matches
> > Cocoa/Android/iOS?
> 
> I would ping Hwi, but she is ooo. I see this as one of many places where in
the
> past we made disjointed UI decisions that we'd like to be more coherent now
> (hence project name "harmony"). I think the only real use case for
> SetEnabledColor on views::Link is for links affected by custom browser themes
> (e.g. the link to import bookmarks).

I'll go along with that if you plan on doing the same thing for other platforms'
Sad tabs... I presume we also care about cross-platform 'harmony'.

> > More generally, how do controls with custom (or pre-calculated themed)
colors
> > handle backgrounds that change with the native theme? It seems like native
> theme
> > changes should also (for example) update a label's background color and
either
> > update a themed foreground text color or recalculate a readable text color
as
> > needed... That seems difficult to accomplish, so presumably any labels used
on
> > top of CreateThemedSolidBackground views should never set non-default text
> > colors nor use SetBackgroundColor? 
> 
> yes
> 
> > It seems like this is a recipe for having
> > weird mismatching colors on native theme changes unless we carefully audit
all
> > related views... I found a couple questionable examples by doing another
quick
> > pass on c/b/u/v.
> 
> Could you point them out? (Or are you referring to the ones you have already?)

Yeah, I pointed out the issues I found just from reading code (looking for
colors used near your updated backgrounds).

> In PasswordGenerationPopupViewViews I see many potential color/theme related
> bugs, both with and without this change.

Let's not introduce more :)

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/screen_capture_notification_ui_views.cc (right):

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/screen_capture_notification_ui_views.cc:282: SkColor
color = widget->GetNativeTheme()->GetSystemColor(
On 2017/04/20 04:44:22, Evan Stade wrote:
> On 2017/04/19 19:00:54, msw wrote:
> > What updates this precalculated frame color to match the
ThemedSolidBackground
> > changes that may occur?
> 
> I'll just leave this file alone. I think the correct fix is that
> ScreenCaptureNotificationUIViews ought to be a BubbleDialogDelegate which
would
> update the bubble border on native theme changes and this function wouldn't
> exist. But there's no need to chase down that rabbit hole at this moment.

Acknowledged.

[Evan Stade, 2017-04-20 21:18:45]

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2816193002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:205:
ime_inline_autocomplete_view_->SetEnabledColor(
On 2017/04/20 18:21:45, msw wrote:
> On 2017/04/20 04:44:22, Evan Stade wrote:
> > On 2017/04/19 19:00:54, msw wrote:
> > > This label's foreground color doesn't change with its dynamic background
> theme
> > > color. What prevents the foreground and background text from
> > > clashing/mismatching when the native theme changes here?
> > 
> > I didn't really intend to fix all native theming issues, just the background
> > ones. But this is easily addressed by moving this setter to
> > OnNativeThemeChanged.
> > 
> > (If you'd like me to I can back out some or all c/b/u/v changes which were
> > originally included mostly as proofs of the usefulness of the new
background.)
> 
> I just think this isn't as simple as it seems; "fixing the background" might
be
> "making text illegible/invisible" if we're not careful. You should test these
> various cases, especially around inverted color schemes, and seek out color
> assignments used above these themed backgrounds.

It's also quite possible that bugs go the other direction. If you put a
defaultly colored label on top of another view and then statically set the
background of that view, the label updates according to the native theme but the
background won't. And of course, it's not like having two statically set colors
isn't a bug, even if it doesn't clash.

It's generally pretty hard to actually figure out how to show some bits of UI.
Since I /know/ the background is wrong in these cases, it's an easy fix, and if
it causes clashes it will flush out bugs in other colors that also need to be
updated according to the theme.

> 
> Moving this assignment to OnNativeThemeChanged seems reasonable, but actually
> makes me wonder if we should skip adding this smart class and just make views
> set their background colors in OnNativeThemeChanged. That would keep more
color
> assignments together; rather than now setting the background in Init and the
> foreground in OnNativeThemeChanged.... WDYT?

Many views do update colors in OnNativeThemeChanged. In this particular case,
it's convenient to move the color assignment there. But it's often not the case.
For example, if this view were created in a factory function, or weren't
otherwise being stored in a member variable, if |this| didn't override
OnNativeThemeChanged, or if |this| wasn't even a View, it would be much more
cumbersome. In the worst case, you'd have to create a special view subclass just
to override OnNativeThemeChanged. I think setting the background via native
theme ID is in line with the general desire to make Views more modular and to
use less inheritance. I see this as equivalent to SetBackground vs. overriding
OnPaintBackground. In most cases, the former is far more convenient and
flexible.

I do think that other, similar changes to Views, e.g. having some way to set the
native theme color id for label text instead of the color itself, would be
useful future improvements.

[msw, 2017-04-20 21:21:21]

lgtm

[Evan Stade, 2017-04-20 21:27:20]

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

[commit-bot: I haz the power, 2017-04-20 21:28:08]

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, 2017-04-20 21:33:14]

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

[commit-bot: I haz the power, 2017-04-20 21:33:15]

Dry run: 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...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Evan Stade, 2017-04-20 22:05:23]

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

[commit-bot: I haz the power, 2017-04-20 22:06:15]

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, 2017-04-20 22:49:56]

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

[commit-bot: I haz the power, 2017-04-20 22:49:57]

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_...)

[Evan Stade, 2017-04-25 00:07:30]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-04-25 00:07:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, tdanderson@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2816193002/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2017-04-25 00:08:00]

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, 2017-04-25 01:01:32]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1493078850759940,
"parent_rev": "6d3c612de88b0e825e6ecee338916cee1efcc1c5", "commit_rev":
"097f9cde453ea57eb4aa037f44add782391c5eb9"}

[commit-bot: I haz the power, 2017-04-25 01:03:58]

Description was changed from

==========
Introduce a type of View background that stays in sync with its host
View's native theme.

This fixes a few bugs where we weren't using the right native theme,
whether by not updating after the NativeTheme changes or by trying to
access the NativeTheme before the View is added to a hierarchy (which
yields the default NativeTheme --- for most platforms, this didn't
effectively create a bug as there's only one NativeTheme).

Best example is that now the sad tab respects the GTK native theme.

Get rid of Ash's tray_constants::kBackgroundColor in favor of using the
bubble background color from the NativeTheme.

BUG=711183,693282
TBR=stevenjb@chromium.org
==========

to

==========
Introduce a type of View background that stays in sync with its host
View's native theme.

This fixes a few bugs where we weren't using the right native theme,
whether by not updating after the NativeTheme changes or by trying to
access the NativeTheme before the View is added to a hierarchy (which
yields the default NativeTheme --- for most platforms, this didn't
effectively create a bug as there's only one NativeTheme).

Best example is that now the sad tab respects the GTK native theme.

Get rid of Ash's tray_constants::kBackgroundColor in favor of using the
bubble background color from the NativeTheme.

BUG=711183,693282
TBR=stevenjb@chromium.org

Review-Url: https://codereview.chromium.org/2816193002
Cr-Commit-Position: refs/heads/master@{#466847}
Committed:
https://chromium.googlesource.com/chromium/src/+/097f9cde453ea57eb4aa037f44ad...
==========

[commit-bot: I haz the power, 2017-04-25 01:04:01]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/097f9cde453ea57eb4aa037f44ad...

[pkalinnikov, 2017-04-25 12:30:22]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2835233005/ by pkalinnikov@chromium.org.

The reason for reverting is: Seems causing a lot of test crashes on "Linux
Chromium OS ASan" bots (ash_unittests, browser_tests, interactive_ui_tests,
unit_tests)..