views: Fix linux omnibox colors by refactoring omnibox_result_view.cc.

chrome/browser/ui/views/omnibox/omnibox_result_view.cc

Index: chrome/browser/ui/views/omnibox/omnibox_result_view.cc
diff --git a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
index ebfd9f677939d558934d15700722dc32b2c45a3d..3898666dcc4bea2d8e5321bfbdfca2da00de6bc4 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
@@ -41,6 +41,8 @@
 #include "ui/native_theme/native_theme_aura.h"
 #endif
 
+using ui::NativeTheme;
+
 namespace {
 
 // The minimum distance between the top and bottom of the {icon|text} and the
@@ -125,42 +127,44 @@ OmniboxResultView::~OmniboxResultView() {
 SkColor OmniboxResultView::GetColor(
     ResultViewState state,
     ColorKind kind) const {
-  const ui::NativeTheme* theme = GetNativeTheme();
-#if defined(OS_WIN)
-  if (theme == ui::NativeThemeWin::instance()) {
-    static bool win_initialized = false;
-    static SkColor win_colors[NUM_STATES][NUM_KINDS];
-    if (!win_initialized) {
-      win_colors[NORMAL][BACKGROUND] = color_utils::GetSysSkColor(COLOR_WINDOW);
-      win_colors[SELECTED][BACKGROUND] =
-          color_utils::GetSysSkColor(COLOR_HIGHLIGHT);
-      win_colors[NORMAL][TEXT] = color_utils::GetSysSkColor(COLOR_WINDOWTEXT);
-      win_colors[SELECTED][TEXT] =
-          color_utils::GetSysSkColor(COLOR_HIGHLIGHTTEXT);
-      CommonInitColors(theme, win_colors);
-      win_initialized = true;
+  // A mapping from OmniboxResultView's ResultViewState/ColorKind types to
+  // NativeTheme colors.
+  struct TranslationTable {
+    OmniboxResultView::ResultViewState state;
+    OmniboxResultView::ColorKind kind;
+    ui::NativeTheme::ColorId id;
+  } static const kTranslationTable[] = {
+    { NORMAL, BACKGROUND, NativeTheme::kColorId_ResultsTableNormalBackground },

[msw, 2014/04/03 18:53:53]

nit: swapping the order of state and kind would make this more readable.

[Elliot Glaysher, 2014/04/03 20:25:27]

Done.

[msw, 2014/04/03 20:30:25]

You didn't actually do what I asked, but I guess the latest patch set is still
an improvement.

+    { HOVERED, BACKGROUND,
+      NativeTheme::kColorId_ResultsTableHoveredBackground },
+    { SELECTED, BACKGROUND,
+      NativeTheme::kColorId_ResultsTableSelectedBackground },
+    { NORMAL, TEXT, NativeTheme::kColorId_ResultsTableNormalText },
+    { HOVERED, TEXT, NativeTheme::kColorId_ResultsTableHoveredText },
+    { SELECTED, TEXT, NativeTheme::kColorId_ResultsTableSelectedText },
+    { NORMAL, DIMMED_TEXT,
+      NativeTheme::kColorId_ResultsTableNormalDimmedText },
+    { HOVERED, DIMMED_TEXT,
+      NativeTheme::kColorId_ResultsTableHoveredDimmedText },
+    { SELECTED, DIMMED_TEXT,
+      NativeTheme::kColorId_ResultsTableSelectedDimmedText },
+    { NORMAL, URL, NativeTheme::kColorId_ResultsTableNormalUrl },
+    { HOVERED, URL, NativeTheme::kColorId_ResultsTableHoveredUrl },
+    { SELECTED, URL, NativeTheme::kColorId_ResultsTableSelectedUrl },
+    { NORMAL, DIVIDER, NativeTheme::kColorId_ResultsTableNormalDivider },
+    { HOVERED, DIVIDER, NativeTheme::kColorId_ResultsTableHoveredDivider },
+    { SELECTED, DIVIDER, NativeTheme::kColorId_ResultsTableSelectedDivider },
+  };
+
+  for (size_t i = 0; i < arraysize(kTranslationTable); ++i) {
+    if (kTranslationTable[i].state == state &&
+        kTranslationTable[i].kind == kind) {
+      return GetNativeTheme()->GetSystemColor(kTranslationTable[i].id);
     }
-    return win_colors[state][kind];
-  }
-#endif
-  static bool initialized = false;
-  static SkColor colors[NUM_STATES][NUM_KINDS];

[Elliot Glaysher, 2014/04/03 18:14:58]

It's wrong that the colors here are stored in a local static. They'll get set
once, and will never be updated if the NativeTheme changes (or returns different
colors from the same instance).

I fiddled with making this a member variable, but instances of this class are
treated as disposable. I also don't see any evidence that we need this caching.

-  if (!initialized) {
-    colors[NORMAL][BACKGROUND] = theme->GetSystemColor(
-        ui::NativeTheme::kColorId_TextfieldDefaultBackground);
-    colors[NORMAL][TEXT] = theme->GetSystemColor(
-        ui::NativeTheme::kColorId_TextfieldDefaultColor);
-    colors[NORMAL][URL] = SkColorSetARGB(0xff, 0x00, 0x99, 0x33);
-    colors[SELECTED][BACKGROUND] = theme->GetSystemColor(
-        ui::NativeTheme::kColorId_TextfieldSelectionBackgroundFocused);
-    colors[SELECTED][TEXT] = theme->GetSystemColor(
-        ui::NativeTheme::kColorId_TextfieldSelectionColor);
-    colors[SELECTED][URL] = SkColorSetARGB(0xff, 0x00, 0x66, 0x22);
-    colors[HOVERED][URL] = SkColorSetARGB(0xff, 0x00, 0x66, 0x22);
-    CommonInitColors(theme, colors);
-    initialized = true;
   }
-  return colors[state][kind];
+
+  NOTREACHED();
+  return SK_ColorRED;
 }
 
 void OmniboxResultView::SetMatch(const AutocompleteMatch& match) {
@@ -415,39 +419,6 @@ int OmniboxResultView::GetDisplayOffset(
 }
 
 // static
-void OmniboxResultView::CommonInitColors(const ui::NativeTheme* theme,
-                                         SkColor colors[][NUM_KINDS]) {
-  colors[HOVERED][BACKGROUND] =
-      color_utils::AlphaBlend(colors[SELECTED][BACKGROUND],
-                              colors[NORMAL][BACKGROUND], 64);
-  colors[HOVERED][TEXT] = colors[NORMAL][TEXT];
-#if defined(USE_AURA)
-  const bool is_aura = theme == ui::NativeThemeAura::instance();
-#else
-  const bool is_aura = false;
-#endif
-  for (int i = 0; i < NUM_STATES; ++i) {
-    if (is_aura) {
-      colors[i][TEXT] =
-          color_utils::AlphaBlend(SK_ColorBLACK, colors[i][BACKGROUND], 0xdd);
-      colors[i][DIMMED_TEXT] =
-          color_utils::AlphaBlend(SK_ColorBLACK, colors[i][BACKGROUND], 0xbb);
-    } else {
-      colors[i][DIMMED_TEXT] =
-          color_utils::AlphaBlend(colors[i][TEXT], colors[i][BACKGROUND], 128);
-      colors[i][URL] = color_utils::GetReadableColor(SkColorSetRGB(0, 128, 0),
-                                                     colors[i][BACKGROUND]);
-    }
-
-    // TODO(joi): Programmatically draw the dropdown border using
-    // this color as well. (Right now it's drawn as black with 25%
-    // alpha.)
-    colors[i][DIVIDER] =
-        color_utils::AlphaBlend(colors[i][TEXT], colors[i][BACKGROUND], 0x34);
-  }
-}
-
-// static
 int OmniboxResultView::default_icon_size_ = 0;
 
 gfx::ImageSkia OmniboxResultView::GetIcon() const {