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

chrome/browser/ui/views/omnibox/omnibox_result_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.
 
 // For WinDDK ATL compatibility, these ATL headers must come first.
 #include "build/build_config.h"
 #if defined(OS_WIN)
 #include <atlbase.h>  // NOLINT
 #include <atlwin.h>  // NOLINT
 #endif
@@ skipping 23 matching lines @@
 #include "ui/native_theme/native_theme.h"
 
 #if defined(OS_WIN)
 #include "ui/native_theme/native_theme_win.h"
 #endif
 
 #if defined(USE_AURA)
 #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
 // top or bottom of the row.
 const int kMinimumIconVerticalPadding = 2;
 const int kMinimumTextVerticalPadding = 3;
 
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ skipping 64 matching lines @@
   keyword_icon_->SetImage(GetKeywordIcon());
   keyword_icon_->SizeToPreferredSize();
 }
 
 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) {
   match_ = match;
   ResetRenderTexts();
   animation_->Reset();
 
   AutocompleteMatch* associated_keyword_match = match_.associated_keyword.get();
   if (associated_keyword_match) {
     keyword_icon_->SetImage(GetKeywordIcon());
@@ skipping 234 matching lines @@
       input_render_text->GetGlyphBounds(contents_start_index);
   const int start_padding = is_match_contents_rtl ?
       std::max(glyph_bounds.start(), glyph_bounds.end()) :
       std::min(glyph_bounds.start(), glyph_bounds.end());
 
   return is_ui_rtl ?
       (input_render_text->GetContentWidth() - start_padding) : start_padding;
 }
 
 // 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 {
   const gfx::Image image = model_->GetIconIfExtensionMatch(model_index_);
   if (!image.IsEmpty())
     return image.AsImageSkia();
 
   int icon = match_.starred ?
       IDR_OMNIBOX_STAR : AutocompleteMatch::TypeToIcon(match_.type);
   if (GetState() == SELECTED) {
@@ skipping 121 matching lines @@
     }
     PaintMatch(*keyword_match, keyword_contents_rendertext_.get(),
                keyword_description_rendertext_.get(), canvas, x);
   }
 }
 
 void OmniboxResultView::AnimationProgressed(const gfx::Animation* animation) {
   Layout();
   SchedulePaint();
 }