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

chrome/browser/ui/views/location_bar/location_bar_view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 
 #include <algorithm>
 #include <map>
 
 #include "base/i18n/rtl.h"
@@ skipping 181 matching lines @@
   omnibox_view_->Init();
   AddChildView(omnibox_view_);
 
   // Initialize the inline autocomplete view which is visible only when IME is
   // turned on.  Use the same font with the omnibox and highlighted background.
   ime_inline_autocomplete_view_ =
       new views::Label(base::string16(), {font_list});
   ime_inline_autocomplete_view_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   ime_inline_autocomplete_view_->SetAutoColorReadabilityEnabled(false);
   ime_inline_autocomplete_view_->set_background(
-      views::Background::CreateSolidBackground(GetNativeTheme()->GetSystemColor(
-          ui::NativeTheme::kColorId_TextfieldSelectionBackgroundFocused)));
+      views::Background::CreateThemedSolidBackground(
+          ime_inline_autocomplete_view_,
+          ui::NativeTheme::kColorId_TextfieldSelectionBackgroundFocused));
   ime_inline_autocomplete_view_->SetEnabledColor(

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

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?

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

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

[msw, 2017/04/20 18:21:45]

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?

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

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

       GetNativeTheme()->GetSystemColor(
           ui::NativeTheme::kColorId_TextfieldSelectionColor));
   ime_inline_autocomplete_view_->SetVisible(false);
   AddChildView(ime_inline_autocomplete_view_);
 
   selected_keyword_view_ = new SelectedKeywordView(font_list, profile());
   AddChildView(selected_keyword_view_);
 
   gfx::FontList bubble_font_list =
       font_list.DeriveWithHeightUpperBound(bubble_height);
@@ skipping 825 matching lines @@
 // LocationBarView, private TemplateURLServiceObserver implementation:
 
 void LocationBarView::OnTemplateURLServiceChanged() {
   template_url_service_->RemoveObserver(this);
   template_url_service_ = nullptr;
   // If the browser is no longer active, let's not show the info bubble, as this
   // would make the browser the active window again.
   if (omnibox_view_ && omnibox_view_->GetWidget()->IsActive())
     ShowFirstRunBubble();
 }