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

chrome/browser/ui/views/tabs/tab.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/tabs/tab.h"
 
 #include <limits>
 
 #include "base/command_line.h"
 #include "base/debug/alias.h"
@@ skipping 19 matching lines @@
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/theme_provider.h"
 #include "ui/gfx/animation/animation_container.h"
 #include "ui/gfx/animation/multi_animation.h"
 #include "ui/gfx/animation/throb_animation.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/color_analysis.h"
 #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"
 #include "ui/views/controls/label.h"
 #include "ui/views/rect_based_targeting_utils.h"
 #include "ui/views/view_targeter.h"
 #include "ui/views/widget/tooltip_manager.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/window/non_client_view.h"
 
@@ skipping 16 matching lines @@
 
 // How long the pulse throb takes.
 const int kPulseDurationMs = 200;
 
 // Width of touch tabs.
 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.
 int kPinnedTabContentWidth = 25;
 
 // When a non-pinned tab becomes a pinned tab the width of the tab animates. If
 // the width of a pinned tab is at least kPinnedTabExtraWidthToRenderAsNormal
 // larger than the desired pinned tab width then the tab is rendered as a normal
 // tab. This is done to avoid having the title immediately disappear when
 // transitioning a tab from normal to pinned tab.
@@ skipping 47 matching lines @@
 const SkColor kImmersiveInactiveTabColor = SkColorSetRGB(190, 190, 190);
 
 // The minimum opacity (out of 1) when a tab (either active or inactive) is
 // throbbing in the immersive mode light strip.
 const double kImmersiveTabMinThrobOpacity = 0.66;
 
 // Number of steps in the immersive mode loading animation.
 const int kImmersiveLoadingStepCount = 32;
 
 const char kTabCloseButtonName[] = "TabCloseButton";
+const int kTabCloseButtonSize = 16;
 
 void DrawIconAtLocation(gfx::Canvas* canvas,
                         const gfx::ImageSkia& image,
                         int image_offset,
                         int dst_x,
                         int dst_y,
                         int icon_width,
                         int icon_height,
                         bool filter,
                         const SkPaint& paint) {
@@ skipping 264 matching lines @@
       immersive_loading_step_(0),
       should_display_crashed_favicon_(false),
       close_button_(NULL),
       media_indicator_button_(NULL),
       title_(new views::Label()),
       tab_activated_with_last_tap_down_(false),
       hover_controller_(this),
       showing_icon_(false),
       showing_media_indicator_(false),
       showing_close_button_(false),
-      close_button_color_(0) {
+      close_button_color_(SK_ColorTRANSPARENT) {
   DCHECK(controller);
   InitTabResources();
 
   // So we get don't get enter/exit on children and don't prematurely stop the
   // hover.
   set_notify_enter_exit_on_child(true);
 
   set_id(VIEW_ID_TAB);
 
   title_->SetHorizontalAlignment(gfx::ALIGN_TO_HEAD);
   title_->SetElideBehavior(gfx::FADE_TAIL);
   title_->SetHandlesTooltips(false);
   title_->SetAutoColorReadabilityEnabled(false);
   title_->SetText(CoreTabHelper::GetDefaultTitle());
   AddChildView(title_);
 
   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);
 }
 
 Tab::~Tab() {
 }
 
 void Tab::set_animation_container(gfx::AnimationContainer* container) {
   animation_container_ = container;
   hover_controller_.SetAnimationContainer(container);
 }
 
 bool Tab::IsActive() const {
   return controller_->IsActiveTab(this);
 }
 
 void Tab::ActiveStateChanged() {
   GetMediaIndicatorButton()->UpdateEnabledForMuteToggle();
+  SetCloseButtonNormalStateImage();
 }
 
 bool Tab::IsSelected() const {
   return controller_->IsTabSelected(this);
 }
 
 void Tab::SetData(const TabRendererData& data) {
   DCHECK(GetWidget());
 
   if (data_.Equals(data))
@@ skipping 234 matching lines @@
     if (!intersection.intersect(RectToSkRect(clip)))
        return false;
     mask->addRect(intersection);
   }
   return true;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // 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).
   if (width() < GetMinimumUnselectedSize().width() && !data().pinned)
     return;
 
   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);
   }
 
   if (controller_->IsImmersiveStyle())
     PaintImmersiveTab(canvas);
   else
     PaintTab(canvas);
 
@@ skipping 83 matching lines @@
       rect.set_y(lb.y() - (title_height - rect.height()) / 2);
       rect.set_height(title_height);
     }
     title_->SetBoundsRect(rect);
   }
   title_->SetVisible(show_title);
 }
 
 void Tab::OnThemeChanged() {
   LoadTabImages();
+  SetCloseButtonNormalStateImage();
 }
 
 const char* Tab::GetClassName() const {
   return kViewClassName;
 }
 
 bool Tab::GetTooltipText(const gfx::Point& p, base::string16* tooltip) const {
   // Note: Anything that affects the tooltip text should be accounted for when
   // calling TooltipTextChanged() from Tab::DataChanged().
   *tooltip = chrome::AssembleTabTooltipText(data_.title, data_.media_state);
@@ skipping 181 matching lines @@
     StopPulse();
 }
 
 void Tab::PaintTab(gfx::Canvas* canvas) {
   // See if the model changes whether the icons should be painted.
   const bool show_icon = ShouldShowIcon();
   const bool show_media_indicator = ShouldShowMediaIndicator();
   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) {
   // Use transparency for the draw-attention animation.
   int alpha = 255;
   if (pulse_animation_ && pulse_animation_->is_animating() && !data().pinned) {
     alpha = pulse_animation_->CurrentValueBetween(
         255, static_cast<int>(255 * kImmersiveTabMinThrobOpacity));
   }
 
@@ skipping 422 matching lines @@
 
 void Tab::StartCrashAnimation() {
   crash_icon_animation_.reset(new FaviconCrashAnimation(this));
   crash_icon_animation_->Start();
 }
 
 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())
     return;
 
   // Extends the area to the bottom when sad_favicon is animating.
   if (IsPerformingCrashAnimation())
     bounds.set_height(height() - bounds.y());
   bounds.set_x(GetMirroredXForRect(bounds));
   SchedulePaintInRect(bounds);
@@ skipping 143 matching lines @@
                          const gfx::ImageSkia& image) {
   DCHECK_NE(scale_factor, ui::SCALE_FACTOR_NONE);
   ImageCacheEntry entry;
   entry.resource_id = resource_id;
   entry.scale_factor = scale_factor;
   entry.image = image;
   image_cache_->push_front(entry);
   if (image_cache_->size() > kMaxImageCacheSize)
     image_cache_->pop_back();
 }