Render the tab close button as a vector-based icon.

chrome/browser/ui/views/tabs/tab.cc

Index: chrome/browser/ui/views/tabs/tab.cc
diff --git a/chrome/browser/ui/views/tabs/tab.cc b/chrome/browser/ui/views/tabs/tab.cc
index 6c179d1075d2fa4fea81fe6c460a589563fb1456..749a708b5f127b13f760328b828387d73cc14243 100644
--- a/chrome/browser/ui/views/tabs/tab.cc
+++ b/chrome/browser/ui/views/tabs/tab.cc
@@ -37,8 +37,12 @@
 #include "ui/gfx/favicon_size.h"
 #include "ui/gfx/geometry/rect_conversions.h"
 #include "ui/gfx/image/image_skia_operations.h"
+#include "ui/gfx/paint_vector_icon.h"
 #include "ui/gfx/path.h"
 #include "ui/gfx/skia_util.h"
+#include "ui/gfx/vector_icons_public.h"
+#include "ui/native_theme/common_theme.h"
+#include "ui/native_theme/native_theme.h"
 #include "ui/resources/grit/ui_resources.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/button/image_button.h"
@@ -75,8 +79,8 @@ const int kTouchWidth = 120;
 const int kToolbarOverlap = 1;
 const int kExtraLeftPaddingToBalanceCloseButtonPadding = 2;
 const int kFaviconTitleSpacing = 4;
-const int kAfterTitleSpacing = 3;
-const int kCloseButtonRightPaddingOverlap = 3;
+const int kAfterTitleSpacing = 4;
+const int kCloseButtonRightPaddingOverlap = 2;
 const int kStandardTitleWidth = 175;
 
 // Width of the content inside a pinned tab.
@@ -144,6 +148,7 @@ const double kImmersiveTabMinThrobOpacity = 0.66;
 const int kImmersiveLoadingStepCount = 32;
 
 const char kTabCloseButtonName[] = "TabCloseButton";
+const int kTabCloseButtonSize = 16;
 
 void DrawIconAtLocation(gfx::Canvas* canvas,
                         const gfx::ImageSkia& image,
@@ -428,7 +433,7 @@ Tab::Tab(TabController* controller)
       showing_icon_(false),
       showing_media_indicator_(false),
       showing_close_button_(false),
-      close_button_color_(0) {
+      close_button_color_(SK_ColorTRANSPARENT) {
   DCHECK(controller);
   InitTabResources();
 
@@ -448,20 +453,25 @@ Tab::Tab(TabController* controller)
   SetEventTargeter(
       scoped_ptr<views::ViewTargeter>(new views::ViewTargeter(this)));
 
-  // Add the Close Button.
   close_button_ = new TabCloseButton(this);
-  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-  close_button_->SetImage(views::CustomButton::STATE_NORMAL,
-                          rb.GetImageSkiaNamed(IDR_CLOSE_1));
-  close_button_->SetImage(views::CustomButton::STATE_HOVERED,
-                          rb.GetImageSkiaNamed(IDR_CLOSE_1_H));
-  close_button_->SetImage(views::CustomButton::STATE_PRESSED,
-                          rb.GetImageSkiaNamed(IDR_CLOSE_1_P));
   close_button_->SetAccessibleName(
       l10n_util::GetStringUTF16(IDS_ACCNAME_CLOSE));
+  // The normal image is set in various other places because it depends on the
+  // current theme and active state.  The hovered and pressed images don't
+  // depend on the these, so we can set them here.
+  const gfx::ImageSkia& hovered = gfx::CreateVectorIcon(
+      gfx::VectorIconId::TAB_CLOSE_HOVERED_PRESSED, kTabCloseButtonSize,
+      SkColorSetARGB(0xFF, 0xDB, 0x44, 0x37));
+  const gfx::ImageSkia& pressed = gfx::CreateVectorIcon(
+      gfx::VectorIconId::TAB_CLOSE_HOVERED_PRESSED, kTabCloseButtonSize,
+      SkColorSetARGB(0xFF, 0xA8, 0x35, 0x2A));
+  close_button_->SetImage(views::CustomButton::STATE_HOVERED, &hovered);
+  close_button_->SetImage(views::CustomButton::STATE_PRESSED, &pressed);
+
   // Disable animation so that the red danger sign shows up immediately
   // to help avoid mis-clicks.
   close_button_->SetAnimationDuration(0);
+
   AddChildView(close_button_);
 
   set_context_menu_controller(this);
@@ -481,6 +491,7 @@ bool Tab::IsActive() const {
 
 void Tab::ActiveStateChanged() {
   GetMediaIndicatorButton()->UpdateEnabledForMuteToggle();
+  SetCloseButtonNormalStateImage();
 }
 
 bool Tab::IsSelected() const {
@@ -735,6 +746,10 @@ bool Tab::GetHitTestMask(gfx::Path* mask) const {
 ////////////////////////////////////////////////////////////////////////////////
 // Tab, views::View overrides:
 
+void Tab::ViewHierarchyChanged(const ViewHierarchyChangedDetails& details) {
+  SetCloseButtonNormalStateImage();

[Evan Stade, 2015/09/18 21:51:18]

maybe guard with:

if (details.is_add && details.child == this)

[Peter Kasting, 2015/09/19 01:20:56]

I intentionally didn't do that.  Let's say a parent View creates us and calls
AddChildView() but isn't in a widget hierarchy yet.  Then we'll get called here,
but GetThemeProvider() will still return null, so we can't set the correct
image.  Once the parent gets put in a hierarchy that has a widget, we'll get
another call here, and then we can do the right thing.

If necessary, I could add comments about this.  I could also try to do something
more complex like checking whether we could now get a theme provider and we
hadn't previously set a color.  I didn't bother with this because we already
early-out before calling CreateVectorIcon() when the color is the same as
before.

[Evan Stade, 2015/09/21 18:49:10]

OK, although it seems like if this is a problem it should also apply in many
other places we use that guard. You can probably still add the is_add check
although I'm not sure of its value.

[Peter Kasting, 2015/09/23 00:12:34]

Most places we guard with this we're not so much worried about waiting for a
default theme provider as we're just waiting to create objects until we're part
of a hierarchy.

And, in fact, most of _those_ places are wrong.  I'm the source of much of this:
for years I mistakenly believed that if a view was destroyed while not in a view
hierarchy its children weren't deleted, so I wrote code to not add children
until the parent was put in a view hierarchy, and directed others to do the
same.  Then sky corrected my mistake.  What would be best would be to take all
those places and just init stuff in the constructor or similar, but since
there's no real "bug" this causes and the simplification is minimal, it hasn't
seemed worth it.

So anyway, no one who's doing something like that needs to loosen their guard,
only if there are other places wanting to wait for a theme provider.  There
might be a few of those but I'm not sure where offhand.
Seems like a good idea.  Added.

+}
+
 void Tab::OnPaint(gfx::Canvas* canvas) {
   // Don't paint if we're narrower than we can render correctly. (This should
   // only happen during animations).
@@ -744,6 +759,11 @@ void Tab::OnPaint(gfx::Canvas* canvas) {
   gfx::Rect clip;
   if (!controller_->ShouldPaintTab(this, &clip))
     return;
+
+  // If we haven't set the correct close button normal image, we can do so now.
+  if (close_button_color_ == SK_ColorTRANSPARENT)
+    OnThemeChanged();
+
   if (!clip.IsEmpty()) {
     canvas->Save();
     canvas->ClipRect(clip);
@@ -847,6 +867,7 @@ void Tab::Layout() {
 
 void Tab::OnThemeChanged() {
   LoadTabImages();
+  SetCloseButtonNormalStateImage();
 }
 
 const char* Tab::GetClassName() const {
@@ -1048,29 +1069,18 @@ void Tab::PaintTab(gfx::Canvas* canvas) {
   const bool show_close_button = ShouldShowCloseBox();
   if (show_icon != showing_icon_ ||
       show_media_indicator != showing_media_indicator_ ||
-      show_close_button != showing_close_button_) {
+      show_close_button != showing_close_button_)
     Layout();
-  }
 
   PaintTabBackground(canvas);
 
   const SkColor title_color = GetThemeProvider()->GetColor(IsSelected() ?
       ThemeProperties::COLOR_TAB_TEXT :
       ThemeProperties::COLOR_BACKGROUND_TAB_TEXT);
-  title_->SetVisible(ShouldRenderAsNormalTab());
   title_->SetEnabledColor(title_color);
 
   if (show_icon)
     PaintIcon(canvas);
-
-  // If the close button color has changed, generate a new one.
-  if (!close_button_color_ || title_color != close_button_color_) {
-    close_button_color_ = title_color;
-    ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-    close_button_->SetBackground(close_button_color_,
-        rb.GetImageSkiaNamed(IDR_CLOSE_1),
-        rb.GetImageSkiaNamed(IDR_CLOSE_1_MASK));
-  }
 }
 
 void Tab::PaintImmersiveTab(gfx::Canvas* canvas) {
@@ -1513,6 +1523,25 @@ bool Tab::IsPerformingCrashAnimation() const {
   return crash_icon_animation_.get() && data_.IsCrashed();
 }
 
+void Tab::SetCloseButtonNormalStateImage() {
+  // The theme provider may be null if we're not currently in a widget
+  // hierarchy.
+  ui::ThemeProvider* theme_provider = GetThemeProvider();
+  if (!theme_provider)
+    return;
+
+  const SkColor new_close_button_color = SkColorSetA(theme_provider->GetColor(

[Evan Stade, 2015/09/18 21:51:18]

why not use COLOR_TAB_ICON?

[Peter Kasting, 2015/09/19 01:20:56]

Thanks for pointing me at that.  Looks like only the media indicator uses this
today.

As you noted, this also causes an issue (even before my change) where the close
button can look like a very different color than the media indicator.  This also
doesn't handle themes correctly which have foreground and background tabs with
wildly different colors from each other, since the button tint was designed for
the toolbar buttons, which always render against the toolbar background, which
may be different than the background tab background.

I think the right behavior is to make the media indicator match the close button
rather than the other way around.  This deals with background tabs better.  And
since the close button is much more common than the media indicator today, this
is less of a behavioral change than the alternative.

Now, how to implement all this.  In email, Sebastien asked me to change the
close button to the equivalent of rendering the chrome grey with no alpha.  That
means that in the default theme, the close button should be exactly the same
color the media indicator already is.  So I think we can just use one piece of
code to compute one color and use it for both icons.  I think the best solution
is to use code like what I'm doing now.

Another option would be to replace this with something like
COLOR_ACTIVE_TAB_ICON and COLOR_INACTIVE_TAB_ICON and then move this logic
there.  The problem then is that the current implementation, which doesn't need
a specific theme to be loaded, is more safely callable from earlier points than
this logic is.  While we could move this logic up, it would be somewhat risky,
because callers who called it "too early" would get a value back but not
necessarily know they needed to call again later.

Since this is all tab-specific anyway, it seems OK to confine the implementation
to this file.  I'll work on this for the next patch iteration.

[Evan Stade, 2015/09/21 18:49:10]

sure.
Moving logic into ThemeService would be my first choice because that's where
much of the theme-related color generation code lives. You are checking nullness
and making calls to GetThemeProvider() either way in this function, so I'm not
sure I follow your point about the safety of calling.

[Peter Kasting, 2015/09/23 00:12:33]

I was worrying about cases like we had in the omnibox code, where we got the
default theme provider, but it was the wrong theme provider, because we weren't
in the right widget, and once we were in the right widget we had a different
theme provider.  That's part of why having layout_constants.h is better than
getting those off the theme provider.

However, I think the code I'm doing here would address this problem anyway,
because we'd re-get the right color after setting a new theme provider due to
the ViewHierarchyChanged() override.  So this probably isn't a problem.

So let's ignore that and just back up and look at the latest implementation to
see what's changed and decide what makes the most sense.

First, I moved setting the title color into this function as well.  I made a
slight behavioral change: non-active multi-selected tabs will now use the
background title color instead of the foreground title color.  I think this is
more correct given how we draw the tab backgrounds.

Second, basically the difference now comes down to whether I want to replace one
line in tab.cc:

  const SkColor new_button_color = SkColorSetA(title_color, 0xA0);

And the existence of the button_color() getter, which is called once in the
media indicator code, with a call like:

  const SkColor new_button_color = theme_provider->GetColor(IsActive() ?
      ThemeProperties::COLOR_TAB_ICON :
      ThemeProperties::COLOR_BACKGROUND_TAB_ICON);

...in both places, add these two constants to the theme properties, and add
implementations to the theme provider to construct them by returning
COLOR_TAB_TEXT with a different alpha (or the like).

I don't think this is really a win.  It's longer and spreads this knowledge
across more files.  It makes it less clear to the reader that we want the tab
icon colors to be shades of the title color.

It's not an enormous difference, and the code would work either way.  It just
seems a bit better the way I have it now.

[Evan Stade, 2015/09/23 18:16:35]

fair enough

+      IsActive() ? ThemeProperties::COLOR_TAB_TEXT
+                 : ThemeProperties::COLOR_BACKGROUND_TAB_TEXT), 0x70);
+  if (close_button_color_ != new_close_button_color) {
+    close_button_color_ = new_close_button_color;
+    const gfx::ImageSkia& normal = gfx::CreateVectorIcon(
+        gfx::VectorIconId::TAB_CLOSE_NORMAL, kTabCloseButtonSize,
+        close_button_color_);
+    close_button_->SetImage(views::CustomButton::STATE_NORMAL, &normal);
+  }
+}
+
 void Tab::ScheduleIconPaint() {
   gfx::Rect bounds = favicon_bounds_;
   if (bounds.IsEmpty())