Do not use assets for Ash window control button backgrounds

ash/frame/caption_buttons/frame_caption_button.cc

 // Copyright 2013 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 "ash/frame/caption_buttons/frame_caption_button.h"
 
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/animation/slide_animation.h"
 #include "ui/gfx/animation/throb_animation.h"
 #include "ui/gfx/canvas.h"
 
 namespace ash {
 
 namespace {
 
 // The duration of the crossfade animation when swapping the button's images.
 const int kSwapImagesAnimationDurationMs = 200;
 
 // The duration of the fade out animation of the old icon during a crossfade
 // animation as a ratio of |kSwapImagesAnimationDurationMs|.
 const float kFadeOutRatio = 0.5f;
 
 // The alpha to draw inactive icons with.
 const float kInactiveIconAlpha = 0.2f;
 
+// The background colors used for the button hovered and pressed states.
+// TODO(tdanderson|estade): Request these colors from ThemeProvider.
+const SkColor kBackgroundHoveredColor = SkColorSetARGB(0x14, 0x0, 0x0, 0x0);

[Peter Kasting, 2015/12/17 20:56:08]

Nit: Use SkColorSetA(SK_ColorBLACK, 0x14)

[tdanderson, 2015/12/18 17:42:53]

Done.

+const SkColor kBackgroundPressedColor = SkColorSetARGB(0x24, 0x0, 0x0, 0x0);

[tdanderson, 2015/12/17 18:00:54]

Evan, are we in a position now where I can move these colors out of here and use
ThemeProvider to access them?

+
 }  // namespace
 
 // static
 const char FrameCaptionButton::kViewClassName[] = "FrameCaptionButton";
 
 FrameCaptionButton::FrameCaptionButton(views::ButtonListener* listener,
                                        CaptionButtonIcon icon)
     : CustomButton(listener),
       icon_(icon),
+      size_(gfx::Size()),
       paint_as_active_(false),
       alpha_(255),
       icon_image_id_(-1),
-      hovered_background_image_id_(-1),
-      pressed_background_image_id_(-1),
       swap_images_animation_(new gfx::SlideAnimation(this)) {
   swap_images_animation_->Reset(1);
 
   // Do not flip the gfx::Canvas passed to the OnPaint() method. The snap left
   // and snap right button icons should not be flipped. The other icons are
   // horizontally symmetrical.
 }
 
 FrameCaptionButton::~FrameCaptionButton() {
 }
 
-void FrameCaptionButton::SetImages(CaptionButtonIcon icon,
-                                   Animate animate,
-                                   int icon_image_id,
-                                   int hovered_background_image_id,
-                                   int pressed_background_image_id) {
-  // The early return is dependant on |animate| because callers use SetImages()
+void FrameCaptionButton::SetImage(CaptionButtonIcon icon,
+                                  Animate animate,
+                                  int icon_image_id) {
+  // The early return is dependant on |animate| because callers use SetImage()
   // with ANIMATE_NO to progress the crossfade animation to the end.
   if (icon == icon_ &&
       (animate == ANIMATE_YES || !swap_images_animation_->is_animating()) &&
-      icon_image_id == icon_image_id_ &&
-      hovered_background_image_id == hovered_background_image_id_ &&
-      pressed_background_image_id == pressed_background_image_id_) {
+      icon_image_id == icon_image_id_) {
     return;
   }
 
   if (animate == ANIMATE_YES)
     crossfade_icon_image_ = icon_image_;
 
   icon_ = icon;
   icon_image_id_ = icon_image_id;
-  hovered_background_image_id_ = hovered_background_image_id;
-  pressed_background_image_id_ = pressed_background_image_id;
 
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
   icon_image_ = *rb.GetImageSkiaNamed(icon_image_id);
-  hovered_background_image_ = *rb.GetImageSkiaNamed(
-      hovered_background_image_id);
-  pressed_background_image_ = *rb.GetImageSkiaNamed(
-      pressed_background_image_id);
 
   if (animate == ANIMATE_YES) {
     swap_images_animation_->Reset(0);
     swap_images_animation_->SetSlideDuration(kSwapImagesAnimationDurationMs);
     swap_images_animation_->Show();
   } else {
     swap_images_animation_->Reset(1);
   }
   PreferredSizeChanged();
   SchedulePaint();
 }
 
 bool FrameCaptionButton::IsAnimatingImageSwap() const {
   return swap_images_animation_->is_animating();
 }
 
 void FrameCaptionButton::SetAlpha(int alpha) {
   if (alpha_ != alpha) {
     alpha_ = alpha;
     SchedulePaint();
   }
 }
 
 gfx::Size FrameCaptionButton::GetPreferredSize() const {
-  return hovered_background_image_.isNull() ?
-      gfx::Size() : hovered_background_image_.size();
+  return size_;
 }
 
 const char* FrameCaptionButton::GetClassName() const {
   return kViewClassName;
 }
 
 void FrameCaptionButton::OnPaint(gfx::Canvas* canvas) {
   if (hover_animation_->is_animating() || state() == STATE_HOVERED) {
-    int hovered_background_alpha = hover_animation_->is_animating() ?
-        hover_animation_->CurrentValueBetween(0, 255) : 255;
-    SkPaint paint;
-    paint.setAlpha(hovered_background_alpha);
-    canvas->DrawImageInt(hovered_background_image_, 0, 0, paint);
+    SkColor color = kBackgroundHoveredColor;
+    int hovered_background_alpha =
+        hover_animation_->is_animating()
+            ? hover_animation_->CurrentValueBetween(0, SkColorGetA(color))
+            : SkColorGetA(color);
+    color = SkColorSetA(color, hovered_background_alpha);

[Evan Stade, 2015/12/17 21:52:37]

hmm, it seems like you have two distinct values here:
- HoveredColor (black)
- HoveredAlpha (0x14)

You can combine them into one constant, but IMO this code is a bit easier to
follow as

int hovered_bg_alpha = hover_animation_->is_animating() ?
hover_animation_->CurrentValueBetween(0, HoveredAlpha) : HoveredAlpha;
canvas->DrawColor(SkColorSetA(HoveredColor, hovered_bg_alpha));

p.s. Seems like the ternary shouldn't be necessary. If 1.0 * value != value,
CurrentValueBetween should do this checking instead.

[tdanderson, 2015/12/18 17:42:53]

Done.
To clarify this is a comment on how CurrentValueBetween() should ideally work,
and not a suggestion to change my implementation here?

[Peter Kasting, 2015/12/18 18:59:15]

No, he's suggesting changing your implementation.

That suggestion is only correct if when the hover animation isn't animating its
value is 1, or else in the cases where its value is 0 you actually want fully
transparent and not the normal alpha from the constant.  That said, if that
isn't the case today, then things are confusing, because the animation value can
be out of sync with your desired alpha, so I'd fix things so that the comment
applies.

[Evan Stade, 2015/12/18 19:12:04]

Assuming I'm understanding how this all works (and the motivation for the
ternary[1]), I was suggesting changing CurrentValueBetween() which would then
allow you to change this line as well. I do believe hover_animation_'s current
value will always be the intuitive value, based on [1], but I guess it's
possible that's not the case so it should be checked.

[1] I assumed the motivation was concern about floating point arithmetic
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...

[Peter Kasting, 2015/12/18 19:36:38]

Oh.  I assumed the motivation was that it's possible for the hover animation to
have a value other than 1 when not animating, but that we still wanted those
cases to use the final opaque alpha value.

If whoever wrote the original code was worried about floating-point arithmetic
they need not be -- 1.0 * x is always equal to x as long as x isn't NaN or some
similar thing.  I don't think any change in CurrentValueBetween() is necessary,
because its current functionality will always do the right thing.

+    canvas->DrawColor(color);
   } else if (state() == STATE_PRESSED) {
-    canvas->DrawImageInt(pressed_background_image_, 0, 0);
+    canvas->DrawColor(kBackgroundPressedColor);
   }
 
   int icon_alpha = swap_images_animation_->CurrentValueBetween(0, 255);
   int crossfade_icon_alpha = 0;
   if (icon_alpha < static_cast<int>(kFadeOutRatio * 255))
      crossfade_icon_alpha = static_cast<int>(255 - icon_alpha / kFadeOutRatio);
 
   if (crossfade_icon_alpha > 0 && !crossfade_icon_image_.isNull()) {
     gfx::Canvas icon_canvas(icon_image_.size(), canvas->image_scale(), false);
     SkPaint paint;
@@ skipping 53 matching lines @@
 
   SkPaint paint;
   paint.setAlpha(alpha);
   canvas->DrawImageInt(to_center,
                        (width() - to_center.width()) / 2,
                        (height() - to_center.height()) / 2,
                        paint);
 }
 
 }  // namespace ash