CustomButton cleanup: make protected members private, create accessors

chrome/browser/ui/views/tabs/tab_strip.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_strip.h"
 
 #include <algorithm>
 #include <iterator>
 #include <string>
 #include <vector>
@@ skipping 290 matching lines @@
   // extended vertically to y = 0.  The caller uses this for Fitts' Law purposes
   // in maximized/fullscreen mode.
   void GetBorderPath(float button_y,
                      float scale,
                      bool extend_to_top,
                      SkPath* path) const;
 
   // Paints the fill region of the button into |canvas|, according to the
   // supplied values from GetImage() and the given |fill| path.
   void PaintFill(bool pressed,
-                 double hover_value,
                  float scale,
                  const SkPath& fill,
                  gfx::Canvas* canvas) const;
 
   // Tab strip that contains this button.
   TabStrip* tab_strip_;
 
   // The offset used to paint the background image.
   gfx::Point background_offset_;
 
@@ skipping 43 matching lines @@
   // start consuming gestures.
   views::ImageButton::OnGestureEvent(event);
   event->SetHandled();
 }
 
 void NewTabButton::OnPaint(gfx::Canvas* canvas) {
   gfx::ScopedCanvas scoped_canvas(canvas);
   canvas->Translate(gfx::Vector2d(0, height() - kNewTabButtonHeight));
 
   const bool pressed = state() == views::CustomButton::STATE_PRESSED;
-  double hover_value =
-      (state() == views::CustomButton::STATE_HOVERED) ? 1 : 0;
-  if (hover_animation_->is_animating())
-    hover_value = hover_animation_->GetCurrentValue();

[Peter Kasting, 2015/12/19 01:22:27]

I _think_ your changes to always trust the animation state are correct now that
I've made custom_button.cc always reset the hover animation on state
transitions, but I'm nervous enough that you should make sure you try all the
different possible state transitions.

[Evan Stade, 2015/12/21 20:19:28]

I've tried every combination of hovering, clicking, unhovering, etc. that I can
think of. Seems to work.

   const float scale = canvas->image_scale();
 
   SkPath fill;
   if (ui::MaterialDesignController::IsModeMaterial()) {
     // Fill.
     fill.moveTo(9.75 * scale, 16 * scale);
     fill.rCubicTo(-0.75 * scale, 0, -1.625 * scale, -0.5 * scale, -2 * scale,
                   -1.5 * scale);
     fill.rLineTo(-5.75 * scale, -12.5 * scale);
     fill.rCubicTo(0, -0.5 * scale, 0.25 * scale, -scale, scale, -scale);
     fill.rLineTo(23.25 * scale, 0);
     fill.rCubicTo(0.75 * scale, 0, 1.625 * scale, 0.5 * scale, 2 * scale,
                   1.5 * scale);
     fill.rLineTo(5.75 * scale, 12.5 * scale);
     fill.rCubicTo(0, 0.5 * scale, -0.25 * scale, scale, -scale, scale);
     fill.close();
-    PaintFill(pressed, hover_value, scale, fill, canvas);
+    PaintFill(pressed, scale, fill, canvas);
 
     // Stroke.
     gfx::ScopedCanvas scoped_canvas(canvas);
     canvas->UndoDeviceScaleFactor();
     SkPath stroke;
     GetBorderPath(0, scale, false, &stroke);
     // We want to draw a drop shadow either inside or outside the stroke,
     // depending on whether we're pressed; so, either clip out what's outside
     // the stroke, or clip out the fill inside it.
     if (pressed)
@@ skipping 10 matching lines @@
     paint.setColor(SkColorSetA(SK_ColorBLACK, pressed ? 0x38 : 0x27));
     canvas->DrawPath(stroke, paint);
   } else {
     // Fill.
     gfx::ImageSkia* mask =
         GetThemeProvider()->GetImageSkiaNamed(IDR_NEWTAB_BUTTON_MASK);
     // The canvas and mask have to use the same scale factor.
     const float fill_canvas_scale = mask->HasRepresentation(scale) ?
         scale : ui::GetScaleForScaleFactor(ui::SCALE_FACTOR_100P);
     gfx::Canvas fill_canvas(GetNewTabButtonSize(), fill_canvas_scale, false);
-    PaintFill(pressed, hover_value, fill_canvas_scale, fill, &fill_canvas);
+    PaintFill(pressed, fill_canvas_scale, fill, &fill_canvas);
     gfx::ImageSkia image(fill_canvas.ExtractImageRep());
     canvas->DrawImageInt(
         gfx::ImageSkiaOperations::CreateMaskedImage(image, *mask), 0, 0);
 
     // Stroke.  Draw the button border with a slight alpha.
     static const SkAlpha kGlassFrameOverlayAlpha = 178;
     static const SkAlpha kOpaqueFrameOverlayAlpha = 230;
     const SkAlpha alpha = GetWidget()->ShouldWindowContentsBeTransparent() ?
         kGlassFrameOverlayAlpha : kOpaqueFrameOverlayAlpha;
     const int overlay_id = pressed ? IDR_NEWTAB_BUTTON_P : IDR_NEWTAB_BUTTON;
@@ skipping 67 matching lines @@
     path->rCubicTo(0.75 * scale, 0, 1.625 * scale, 0.5 * scale, 2 * scale,
                    1.5 * scale);
     path->rLineTo(5.75 * scale, 12.5 * scale);
     path->rCubicTo(0.5 * scale, 1.125 * scale, -0.5 * scale, scale + 2, -scale,
                    scale + 2);
   }
   path->close();
 }
 
 void NewTabButton::PaintFill(bool pressed,
-                             double hover_value,
                              float scale,
                              const SkPath& fill,
                              gfx::Canvas* canvas) const {
   bool custom_image;
   const int bg_id = tab_strip_->GetBackgroundResourceId(&custom_image);
 
   gfx::ScopedCanvas scoped_canvas(canvas);
 
   const bool md = ui::MaterialDesignController::IsModeMaterial();
   if (md) {
@@ skipping 59 matching lines @@
     if (alpha != 255 && !md) {
       SkPaint paint;
       paint.setAlpha(alpha);
       paint.setXfermodeMode(SkXfermode::kDstIn_Mode);
       paint.setStyle(SkPaint::kFill_Style);
       canvas->DrawRect(gfx::Rect(size), paint);
     }
   }
 
   // White highlight on hover.
-  if (hover_value) {
-    const int alpha =
-        gfx::Tween::LinearIntValueBetween(hover_value, 0x00, md ? 0x4D : 0x40);
+  if (hover_animation().GetCurrentValue()) {

[Peter Kasting, 2015/12/19 01:22:27]

Nit: Store this in a temp to avoid multiple calls

[Evan Stade, 2015/12/21 20:19:28]

question: you authored this code recently. Why does it use
gfx::Tween::LinearIntValueBetween instead of Animation::CurrentValueBetween? Is
the "Linear" distinction important?

I've changed the code to a version that is slightly more concise, ptal.

[Peter Kasting, 2015/12/21 20:34:18]

Using LinearIntValueBetween() matched the previous behavior (I was just
refactoring it).  The difference between this and IntValueBetween() (which is
what CurrentValueBetween() will use) should not be very perceptible to the user,
so it's fine to use the other for simplicity.

+    const int alpha = gfx::Tween::LinearIntValueBetween(
+        hover_animation().GetCurrentValue(), 0x00, md ? 0x4D : 0x40);
     canvas->FillRect(GetLocalBounds(),
                      SkColorSetA(SK_ColorWHITE, static_cast<SkAlpha>(alpha)));
   }
 
   // For MD, most states' opacities are adjusted using an opacity recorder in
   // TabStrip::PaintChildren(), but the pressed state is excluded there and
   // instead rendered using a dark overlay here.  This produces a different
   // effect than for non-MD, and avoiding the use of the opacity recorder keeps
   // the stroke more visible in this state.
   if (md && pressed)
@@ skipping 2245 matching lines @@
           ConvertPointToViewAndGetEventHandler(this, newtab_button_, point);
       if (view)
         return view;
     }
     Tab* tab = FindTabForEvent(point);
     if (tab)
       return ConvertPointToViewAndGetEventHandler(this, tab, point);
   }
   return this;
 }