Update search chip icon for MD, vectorize.

chrome/browser/ui/views/location_bar/selected_keyword_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/selected_keyword_view.h"
 
 #include "base/logging.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/ui/location_bar/location_bar_util.h"
 #include "chrome/browser/ui/views/location_bar/keyword_hint_view.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/search_engines/template_url_service.h"
+#include "grit/components_scaled_resources.h"
 #include "grit/theme_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/material_design/material_design_controller.h"
+#include "ui/base/theme_provider.h"
+#include "ui/gfx/color_palette.h"
+#include "ui/gfx/paint_vector_icon.h"
+#include "ui/gfx/vector_icons_public.h"
+#include "ui/native_theme/native_theme.h"
+#include "ui/views/controls/image_view.h"

[Peter Kasting, 2015/10/23 23:44:09]

Are all these needed?  It kinda surprises me that we need image_view.h, for
example.

[Evan Stade, 2015/10/26 19:29:19]

a couple were not necessary

 #include "ui/views/painter.h"
 
 SelectedKeywordView::SelectedKeywordView(const gfx::FontList& font_list,
                                          SkColor text_color,
                                          SkColor parent_background_color,
                                          Profile* profile)
-    : IconLabelBubbleView(IDR_KEYWORD_SEARCH_MAGNIFIER,

[Evan Stade, 2015/10/23 21:49:40]

this was actually never visible because it got overridden in
location_bar_view.cc

+    : IconLabelBubbleView(0,
                           font_list,
                           text_color,
                           parent_background_color,
                           false),
       profile_(profile) {
   if (ui::MaterialDesignController::IsModeMaterial()) {
     // The inset for IDR_OMNIBOX_SELECTED_KEYWORD_BUBBLE for which to perfom
     // nine-slicing.
     static const int kImageInset = 4;
     gfx::Insets insets(kImageInset, kImageInset, kImageInset, kImageInset);
     SetBackgroundImageWithInsets(IDR_OMNIBOX_SELECTED_KEYWORD_BUBBLE, insets);
   } else {
     static const int kBackgroundImages[] =
         IMAGE_GRID(IDR_OMNIBOX_SELECTED_KEYWORD_BUBBLE);
     SetBackgroundImageGrid(kBackgroundImages);
   }
   full_label_.SetFontList(font_list);
   full_label_.SetVisible(false);
   partial_label_.SetFontList(font_list);
   partial_label_.SetVisible(false);
 }
 
 SelectedKeywordView::~SelectedKeywordView() {
 }
 
+void SelectedKeywordView::ResetImage() {
+  if (ui::MaterialDesignController::IsModeMaterial()) {
+    SkColor link_color =
+        GetNativeTheme()->GetSystemColor(ui::NativeTheme::kColorId_LinkEnabled);

[Peter Kasting, 2015/10/23 23:44:09]

Huh.  Are we sure we want the magnifier to be the system "enabled link" color? 
That seems odd considering this isn't a link or anything.

I would think we'd want some starting color (blue or white or something) run
through GetReadableColor() over the background here, or the like.

[Evan Stade, 2015/10/26 19:29:19]

The magnifier is set in a chip and the border of the chip is also the link color
(at least, it's the same blue as we're supposed to use for MD links).
Presumably, the link color should always be readable relative to a text entry bg
color? Sebastien did say it was intentionally the same blue as links, and not
the same blue as the "call to action" color used for active omnibox icons.

[Peter Kasting, 2015/10/26 19:57:41]

GetSystemColor() probably isn't right, then, since it's supposed to mean "get
the native OS color for this" rather than just "get the color for this" (e.g. if
we hardcoded links to a particular blue in MD we would want to basically ignore
the system colors, rather than have the system color accessors return that
blue).  We'd want to make something like ThemeProvider::GetColor() return the
relevant blue here instead.
What background color does this chip have?

If we were using system link and window BG colors, I might assume readability,
but if we're hardcoding a link color it's likely we'll need to force
readability.  But I'd know better if I knew what background this has.

[Evan Stade, 2015/10/26 20:19:36]

NativeTheme does not necessarily draw from system colors except where it makes
sense to. ThemeProvider::GetColor only handles browser-specific colors (which
links are not). Link colors have to live in NativeTheme because stuff like
views::Link or other UI outside browser (maybe cros system UI?) needs access to
them. For Windows, I don't know or particularly care if we will choose MD link
colors or system link colors for links, but if we do hardcode the link color we
should at least hardcode it to something that looks good on the theme's bg color
(e.g. by returning a different color for links in inverted color schemes).
Same as link color with greatly reduced opacity (5%). So essentially, the
omnibox bg color.

[Peter Kasting, 2015/10/26 20:34:01]

At least on Windows, where there is a concept of an OS link color,
NativeTheme::GetSystemColor() should definitely return that.
To me the MD colors are browser-specific.  If we were to write non-browser code
that had UI with links, it should probably be using the system link color,
whereas the browser should use the MD colors.

But if you don't want that, then we need some sort of layer that can be accessed
outside the browser that isn't NativeTheme.  NativeTheme should always be
returning the OS native colors when they exist, and should only hardcode other
colors in cases where there's no OS color.

Luckily, ThemeProvider is a concept that lives in ui/base/, and the ui/
implementation of it is DefaultThemeProvider.  So you could put your color there
and have ui/-level stuff call Widget::GetThemeProvider().  For widgets living in
the browser world this would return the browser-scope theme provider, and for
things living outside that it would return the DefaultThemeProvider.  Then I
think everything works automatically.
Yes; generally we do this by running things through GetReadableColor() at a low
level (inside the ThemeProvider or similar).
OK; as long as we ensure _that_ color follows the theme/system BG color, then if
we make the link color getter readable over that color at a low level, we should
be good to avoid doing another similar check here.

[Evan Stade, 2015/10/26 21:29:42]

Perhaps that was the original intent, but right now that simply isn't how
NativeTheme is used. For example, on Linux we use either NativeThemeAura or
NativeThemeGtk but only the latter actually cares about system colors. If we
move this to ThemeProvider, Linux has no way of making the link color match the
system link color. DefaultThemeProvider's concrete methods are all no-ops so I
don't think there's precedent for putting any kind of real implementation there.

This discussion is starting to reach outside the scope of this CL. If you would
like to campaign to change how ThemeProvider and NativeTheme are used, that
discussion is probably better off on a broader email list. I think we have
agreed that this icon's color should match the link color and currently this is
how you get the link color used throughout Chrome, so in isolation I believe
this CL to be correct.
In my experience that doesn't always produce a satisfying color so it makes more
sense to me to handpick an inverted link color. But sure, we can take care of
this within whatever class is providing link colors.
I believe the omnibox bg color does follow the theme/system. At least it does on
linux.

[Peter Kasting, 2015/10/26 22:21:19]

I haven't studied Linux in detail, but last time I looked (when Dave implemented
AiS) there were a lot of things about its theming code that seemed broken.  I
would base your assumptions around whatever Windows does instead, which is also
not guaranteed to be correct, but is at least less likely to be incorrect.
Why?  Why can't the ThemeProvider ask the NativeThemeProvider as needed?
Until recently the only colors we've needed outside chrome/ have been ones where
we can either use the system color, or there isn't a system color (or we choose
to ignore it entirely) and we can use a hardcoded color.  So there hasn't been a
need for any kind of above-chrome/-but-not-native-theme functionality.

The folks who started implementing MD initially started just putting overrides
into the NativeTheme code to return other colors, which wasn't how we should
have implemented this (but they didn't know and didn't ask, and also haven't yet
had any consideration for how system or custom themes interact with MD so as to
have encountered the reasons why this was the wrong implementation).  So they
didn't blaze a trail for you here, though they should have.

Anyway, it's not necessarily the case that you need to touch
DefaultThemeProvider.  That's only necessary if you want MD colors used in UI
objects that aren't used from chrome/ code.  As long as things are used from
chrome/ code, asking the widget for its theme provider should get you back the
non-native theme provider, which you can override to return the MD values.  Then
we can have any code that wants to respect those values ensure it's going that
route, and save the underlying NativeThemeProvider stuff for things that want to
be system-native regardless.

Or, if we want to change the meaning of what NativeThemeProvider is, so that
it's no longer related to "native" and is just the "default behavior" or
something, then we could probably combine a bunch of these classes, so all the
color selection code lived in one place in ui/.  It's not really obvious to me
why we need three or four classes here where one would do.  That would
_definitely_ be outside the scope of this CL, though.
I don't think this is really a change in how most code uses them.  Various parts
of Chrome's UI (like omnibox-related stuff) try to respect system colors (or
used to as of when they were implemented) and assume the
NativeTheme::GetSystemColor() machinery provides them.  Perhaps on GTK that
assumption is wrong, leading to non-system-respecting behavior of parts of this
UI, but that seems like a bug.  It seems like what you want is to change such
code to not be asking for the system color directly, but instead to just ask the
theme machinery for the right color and let it do fallback as appropriate.  I
don't think such a change is outside the scope of this CL -- that seems like the
whole point of this CL?

You say "campaign" as if there are lots of skeptical stakeholders to win over
here.  Who else would you want involved in the discussion?  At this point, I
think I'm the closest thing the theme color code has to an owner, but I'm not
opposed to getting more input if there are other good people.
Until now everyone has wanted the system-native link color, so it's no surprise
that code explicitly calls the NativeThemeProvider.  As I said above, it seems
like you want a different behavior than that.

+    SetImage(gfx::CreateVectorIcon(gfx::VectorIconId::SEARCH_CHIP_ICON, 16,
+                                   link_color));
+  } else {
+    SetImage(*GetThemeProvider()->GetImageSkiaNamed(IDR_OMNIBOX_SEARCH));
+  }
+}
+
 gfx::Size SelectedKeywordView::GetPreferredSize() const {
   // Height will be ignored by the LocationBarView.
   return GetSizeForLabelWidth(full_label_.GetPreferredSize().width());
 }
 
 gfx::Size SelectedKeywordView::GetMinimumSize() const {
   // Height will be ignored by the LocationBarView.
   return GetSizeForLabelWidth(partial_label_.GetMinimumSize().width());
 }
 
@@ skipping 26 matching lines @@
   const base::string16 partial_name = is_extension_keyword ?
       min_string :
       l10n_util::GetStringFUTF16(IDS_OMNIBOX_KEYWORD_TEXT, min_string);
   partial_label_.SetText(min_string.empty() ?
       full_label_.text() : partial_name);
 }
 
 const char* SelectedKeywordView::GetClassName() const {
   return "SelectedKeywordView";
 }