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

chrome/browser/ui/views/sad_tab_view.cc

Index: chrome/browser/ui/views/sad_tab_view.cc
diff --git a/chrome/browser/ui/views/sad_tab_view.cc b/chrome/browser/ui/views/sad_tab_view.cc
index 926d73a73fdcf2ec742a6bb48a4d64a54a7880ce..aeaa218893a6848b3db6169c8d8573266c9e9ed4 100644
--- a/chrome/browser/ui/views/sad_tab_view.cc
+++ b/chrome/browser/ui/views/sad_tab_view.cc
@@ -36,10 +36,8 @@ constexpr int kTitleBottomSpacing = 13;
 SadTabView::SadTabView(content::WebContents* web_contents,
                        chrome::SadTabKind kind)
     : SadTab(web_contents, kind) {
-  // Set the background color.
-  set_background(
-      views::Background::CreateSolidBackground(GetNativeTheme()->GetSystemColor(
-          ui::NativeTheme::kColorId_DialogBackground)));
+  set_background(views::Background::CreateThemedSolidBackground(
+      this, ui::NativeTheme::kColorId_DialogBackground));
 
   views::GridLayout* layout = new views::GridLayout(this);
   SetLayoutManager(layout);
@@ -61,7 +59,7 @@ SadTabView::SadTabView(content::WebContents* web_contents,
   layout->StartRow(0, column_set_id);
   layout->AddView(image, 2, 1);
 
-  title_ = CreateLabel(l10n_util::GetStringUTF16(GetTitle()));
+  title_ = new views::Label(l10n_util::GetStringUTF16(GetTitle()));

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

These labels and links no longer initialize the background color for
readability. Why shouldn't they call SetBackgroundColor any more?

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

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.

   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
   title_->SetFontList(rb.GetFontList(ui::ResourceBundle::LargeFont));
   title_->SetMultiLine(true);
@@ -70,13 +68,10 @@ SadTabView::SadTabView(content::WebContents* web_contents,
                               views::kPanelVerticalSpacing);
   layout->AddView(title_, 2, 1);
 
-  const SkColor text_color = GetNativeTheme()->GetSystemColor(
-      ui::NativeTheme::kColorId_LabelDisabledColor);
-
-  message_ = CreateLabel(l10n_util::GetStringUTF16(GetMessage()));
+  message_ = new views::Label(l10n_util::GetStringUTF16(GetMessage()));
 
   message_->SetMultiLine(true);
-  message_->SetEnabledColor(text_color);
+  message_->SetEnabled(false);
   message_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   message_->SetLineHeight(views::kPanelSubVerticalSpacing);
 
@@ -86,8 +81,8 @@ SadTabView::SadTabView(content::WebContents* web_contents,
 
   action_button_ = views::MdTextButton::CreateSecondaryUiBlueButton(
       this, l10n_util::GetStringUTF16(GetButtonTitle()));
-  help_link_ =
-      CreateLink(l10n_util::GetStringUTF16(GetHelpLinkTitle()), text_color);
+  help_link_ = new views::Link(l10n_util::GetStringUTF16(GetHelpLinkTitle()));

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

This link no longer gets the color of a disabled label; why change that?

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

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

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

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.

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

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

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

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'.
Yeah, I pointed out the issues I found just from reading code (looking for
colors used near your updated backgrounds).
Let's not introduce more :)

+  help_link_->set_listener(this);
   layout->StartRowWithPadding(0, column_set_id, 0,
                               views::kPanelVerticalSpacing);
   layout->AddView(help_link_, 1, 1, views::GridLayout::LEADING,
@@ -150,21 +145,6 @@ void SadTabView::OnPaint(gfx::Canvas* canvas) {
   View::OnPaint(canvas);
 }
 
-views::Label* SadTabView::CreateLabel(const base::string16& text) {
-  views::Label* label = new views::Label(text);
-  label->SetBackgroundColor(background()->get_color());
-  return label;
-}
-
-views::Link* SadTabView::CreateLink(const base::string16& text,
-                                    const SkColor& color) {
-  views::Link* link = new views::Link(text);
-  link->SetBackgroundColor(background()->get_color());
-  link->SetEnabledColor(color);
-  link->set_listener(this);
-  return link;
-}
-
 namespace chrome {
 
 SadTab* SadTab::Create(content::WebContents* web_contents,