Do not use assets for Ash window control button backgrounds

ash/frame/caption_buttons/frame_caption_button.cc

Index: ash/frame/caption_buttons/frame_caption_button.cc
diff --git a/ash/frame/caption_buttons/frame_caption_button.cc b/ash/frame/caption_buttons/frame_caption_button.cc
index 2b166436a2498ea7ec13bca7ef6266ad55753127..c487d93b3f07d25c4f377569b87ceb80de5399c0 100644
--- a/ash/frame/caption_buttons/frame_caption_button.cc
+++ b/ash/frame/caption_buttons/frame_caption_button.cc
@@ -23,6 +23,11 @@ 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
@@ -32,11 +37,10 @@ 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);
 
@@ -48,18 +52,14 @@ FrameCaptionButton::FrameCaptionButton(views::ButtonListener* listener,
 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;
   }
 
@@ -68,15 +68,9 @@ void FrameCaptionButton::SetImages(CaptionButtonIcon icon,
 
   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);
@@ -101,8 +95,7 @@ void FrameCaptionButton::SetAlpha(int alpha) {
 }
 
 gfx::Size FrameCaptionButton::GetPreferredSize() const {
-  return hovered_background_image_.isNull() ?
-      gfx::Size() : hovered_background_image_.size();
+  return size_;
 }
 
 const char* FrameCaptionButton::GetClassName() const {
@@ -111,13 +104,15 @@ const char* FrameCaptionButton::GetClassName() const {
 
 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);