Use native theme colors for textfields; etc.

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/command_line.h"
@@ skipping 102 matching lines @@
 const int kBorderCornerRadius = 2;
 
 const int kDesktopItemPadding = 3;
 const int kDesktopEdgeItemPadding = kDesktopItemPadding;
 const int kDesktopScriptBadgeItemPadding = 9;
 const int kDesktopScriptBadgeEdgeItemPadding = kDesktopScriptBadgeItemPadding;
 
 const int kTouchItemPadding = 8;
 const int kTouchEdgeItemPadding = kTouchItemPadding;
 
-#if defined(OS_CHROMEOS)
-const SkColor kOmniboxBackgroundColor = SkColorSetARGB(0, 255, 255, 255);
-#else
-const SkColor kOmniboxBackgroundColor = SkColorSetARGB(255, 255, 255, 255);
-#endif
-
 }  // namespace
 
 // static
 const int LocationBarView::kNormalHorizontalEdgeThickness = 2;
 const int LocationBarView::kVerticalEdgeThickness = 3;
 const int LocationBarView::kIconInternalPadding = 2;
 const int LocationBarView::kBubbleHorizontalPadding = 1;
 const char LocationBarView::kViewClassName[] =
     "browser/ui/views/location_bar/LocationBarView";
 
@@ skipping 186 matching lines @@
 
   OnChanged();
 }
 
 bool LocationBarView::IsInitialized() const {
   return location_entry_view_ != NULL;
 }
 
 SkColor LocationBarView::GetColor(ToolbarModel::SecurityLevel security_level,
                                   ColorKind kind) const {
-#if defined(OS_WIN)
-  if (GetNativeTheme() == ui::NativeThemeWin::instance()) {
-    switch (kind) {
-      case BACKGROUND:
-        return color_utils::GetSysSkColor(COLOR_WINDOW);
-      case TEXT:
-        return color_utils::GetSysSkColor(COLOR_WINDOWTEXT);
-      case SELECTED_TEXT:
-        return color_utils::GetSysSkColor(COLOR_HIGHLIGHTTEXT);
-      default:
-        // Other cases are handled below.
-        break;
-    }
-  }
+  const ui::NativeTheme* native_theme = GetNativeTheme();
+  switch (kind) {
+    case BACKGROUND:
+#if defined(OS_CHROMEOS)
+      // Chrome OS requires a transparent omnibox background color.

[varunjain, 2012/12/06 18:36:58]

I agree that native_theme should not have a separate color for omnibox. I think
the omnibox popup (nothing else seems to be getting affected) should not be
using textfieldBackgroundColor. It should probably use the background color of a
popup. There is none defined in native_theme, closest seems to be
kColorId_MenuBackgroundColor

[msw, 2012/12/06 18:54:46]

That might be feasible for a follow-up change. It may make more sense to offer
control over the presence of the solid color background used in
NativeTextfieldViews.

A problem with either approach (and the current code) is that DEEMPHASIZED_TEXT
and SECURITY_TEXT calculate colors using the BACKGROUND color, which is actually
the unspecified color within the images, not the transparent white supplied
here.

[sky, 2012/12/06 20:43:57]

Doesn't the transparent background apply to OS_WIN when native theme ==
nativethemeaura?

[msw, 2012/12/07 02:04:01]

Hmm, not afaik; I think this is okay as-is. The old kOmniboxBackgroundColor
definition was transparent white for CrOS and opaque white for Win. My new code
preserves the local transparent white for CrOS and uses the equivalent
NativeThemeAura value for kColorId_TextfieldDefaultBackground
(kTextfieldDefaultBackground == SK_ColorWHITE) on Aura.

[sky, 2012/12/07 16:35:12]

I'm still confused. OS_CHROMEOS and OS_WIN when the theme == NativeThemeAura
should get the same colors. If you're saying native_theme->GetSystemColor for
!OS_CHROMEOS does the right thing, why do we need the ifdef at all?

[msw, 2012/12/07 18:22:28]

This change preserves the existing behavior, which contradicts your assertion.
Prior to my change, OS_CHROMEOS and OS_WIN when the theme == NativeThemeAura get
different colors. They'd both wind up outside the #if block and return
kOmniboxBackgroundColor, defined as transparent (0, 255, 255, 255) for CrOS and
normal (255, 255, 255, 255) for Win (see left side line 123). I can change/test
that, but I'm not sure that's our intent. Switching to transparent white here
breaks the non-Aura Win omnibox result dropdown, not sure if it would also break
Aura Win. Sorry for the trouble :(

[sky, 2012/12/07 18:58:02]

Ah, ok. I see it now.

+      return SkColorSetARGB(0, 255, 255, 255);
+#else
+      return native_theme->GetSystemColor(
+          ui::NativeTheme::kColorId_TextfieldDefaultBackground);
 #endif
-  switch (kind) {
-    // TODO(beng): source from theme provider.
-    case BACKGROUND:    return kOmniboxBackgroundColor;
-    case TEXT:          return SK_ColorBLACK;
-    case SELECTED_TEXT: return SK_ColorWHITE;
+
+    case TEXT:
+      return native_theme->GetSystemColor(
+          ui::NativeTheme::kColorId_TextfieldDefaultColor);
+
+    case SELECTED_TEXT:
+      return native_theme->GetSystemColor(
+          ui::NativeTheme::kColorId_TextfieldSelectionColor);
 
     case DEEMPHASIZED_TEXT:
       return color_utils::AlphaBlend(
           GetColor(security_level, TEXT),
           GetColor(security_level, BACKGROUND),
           128);
 
     case SECURITY_TEXT: {
       SkColor color;
       switch (security_level) {
@@ skipping 1206 matching lines @@
 int LocationBarView::GetInternalHeight(bool use_preferred_size) {
   int total_height =
       use_preferred_size ? GetPreferredSize().height() : height();
   return std::max(total_height - (kVerticalEdgeThickness * 2), 0);
 }
 
 bool LocationBarView::HasValidSuggestText() const {
   return suggested_text_view_ && !suggested_text_view_->size().IsEmpty() &&
       !suggested_text_view_->text().empty();
 }