Tweaks to ToggleButton details.

ui/views/controls/button/toggle_button.cc

Index: ui/views/controls/button/toggle_button.cc
diff --git a/ui/views/controls/button/toggle_button.cc b/ui/views/controls/button/toggle_button.cc
index e7c656b641bfaaeb7eee4fe06c47f89abc99c3ea..43809d4201b1be6917f96b948659b5dbeb8b1a46 100644
--- a/ui/views/controls/button/toggle_button.cc
+++ b/ui/views/controls/button/toggle_button.cc
@@ -20,38 +20,23 @@ namespace {
 const int kTrackHeight = 12;
 const int kTrackWidth = 28;
 // Margins from edge of track to edge of view.
-const int kTrackVerticalMargin = 5;
+const int kTrackVerticalMargin = 4;
 const int kTrackHorizontalMargin = 6;
-// Margin from edge of thumb to closest edge of view. Note that the thumb
-// margins must be sufficiently large to allow space for the shadow.
+// Margin from edge of thumb to closest edge of view.
 const int kThumbHorizontalMargin = 4;
 // Margin from top/bottom edge of thumb to top/bottom edge of view.
-const int kThumbVerticalMargin = 3;
-
-// TODO(estade): get the base color (black) from the theme?
-const SkColor kTrackOffColor =
-    SkColorSetA(SK_ColorBLACK, gfx::kDisabledControlAlpha);
+const int kThumbVerticalMargin = 2;
 
 }  // namespace
 
-// Class representing the thumb. When the thumb is clicked it is separated into
-// its own layer and the ink drop layer is made a child of the thumb layer
-// allowing the two to animate in sync.
-class ToggleButton::ThumbView : public views::View {
+// Class representing the thumb (the circle that slides horizontally).
+class ToggleButton::ThumbView : public InkDropHostView {
  public:
-  ThumbView() : color_ratio_(0.) {}
-  ~ThumbView() override {}
-
-  void AddInkDropLayer(ui::Layer* ink_drop_layer) {
+  ThumbView() : color_ratio_(0.) {
     SetPaintToLayer(true);

[varkha, 2016/10/21 00:35:18]

The old implementation was more lines but it was only using extra layers when
the toggle is interacted with (and flattened otherwise). The new impl is always
using an extra layer - something we were trying to minimize before with the
ripple. Something probably only noticeable when there are many toggle buttons at
the same time but still worth consideration.

[Evan Stade, 2016/10/21 17:19:15]

It's not clear to me that creating and destroying this layer on every ink drop
is a performance improvement, but the real reason I added it is because the
shadow extends slightly beyond the bounds of the view.  That said, it's not
truly necessary if we leave the vertical margin constants alone, I mainly
changed those to match the overall height Sebastien spec'd.

[varkha, 2016/10/21 17:55:36]

+bruthig@ and sadrul@ to see if they have stronger opinions here. I think this
is what we are doing with the rest of the ripple-enabled buttons because we
tried to keep texture memory down as a concern.

[bruthig, 2016/10/21 18:36:40]

Even though the ThumbView is always painting to a Layer the ink drop layers will
be cleaned up when not active.  Considering that the ShelfButtons paint to their
own layer I don't have any concerns with having 1 extra layer per toggle. 
Having said that, if it's easy enough to not require a layer then why not
eliminate the layer...

In summary, no strong opinion ;)

FTR I'd wait for sadrul@ to give an opinion too.

[sadrul, 2016/10/24 14:46:12]

Always having the layer seem unrelated to the bugs being fixed. Can you move the
layer-change to a separate CL so we can discuss that separately, and not block
the bug fix?

[Evan Stade, 2016/10/27 17:12:14]

Done.

     layer()->SetFillsBoundsOpaquely(false);
-    layer()->Add(ink_drop_layer);
-  }
-
-  void RemoveInkDropLayer(ui::Layer* ink_drop_layer) {
-    layer()->Remove(ink_drop_layer);
-    SetPaintToLayer(false);
   }
+  ~ThumbView() override {}
 
   void Update(const gfx::Rect& bounds, double color_ratio) {
     SetBoundsRect(bounds);
@@ -59,7 +44,18 @@ class ToggleButton::ThumbView : public views::View {
     SchedulePaint();
   }
 
+  static gfx::Insets GetShadowInsets() {
+    gfx::Insets blur(kShadowBlur);
+    gfx::Insets offset(-kShadowOffsetY, -kShadowOffsetX, kShadowOffsetY,
+                       kShadowOffsetX);
+    return blur + offset;
+  }
+
  private:
+  static const int kShadowOffsetX = 0;
+  static const int kShadowOffsetY = 1;
+  static const int kShadowBlur = 2;
+
   // views::View:
   const char* GetClassName() const override {
     return "ToggleButton::ThumbView";
@@ -67,23 +63,34 @@ class ToggleButton::ThumbView : public views::View {
 
   void OnPaint(gfx::Canvas* canvas) override {
     std::vector<gfx::ShadowValue> shadows;
-    shadows.emplace_back(gfx::Vector2d(0, 1), 4.f,
-                         SkColorSetA(SK_ColorBLACK, 0x99));
+    shadows.emplace_back(
+        gfx::Vector2d(kShadowOffsetX, kShadowOffsetY), 2 * kShadowBlur,
+        SkColorSetA(GetNativeTheme()->GetSystemColor(
+                        ui::NativeTheme::kColorId_LabelEnabledColor),
+                    0x99));
     SkPaint thumb_paint;
     thumb_paint.setLooper(gfx::CreateShadowDrawLooperCorrectBlur(shadows));
-    thumb_paint.setStyle(SkPaint::kFill_Style);
     thumb_paint.setAntiAlias(true);
     const SkColor thumb_on_color = GetNativeTheme()->GetSystemColor(
         ui::NativeTheme::kColorId_ProminentButtonColor);
-    // TODO(estade): get this color from the theme?
-    const SkColor thumb_off_color = SK_ColorWHITE;
+    const SkColor thumb_off_color = GetNativeTheme()->GetSystemColor(
+        ui::NativeTheme::kColorId_DialogBackground);
     const SkAlpha blend = static_cast<SkAlpha>(SK_AlphaOPAQUE * color_ratio_);
     thumb_paint.setColor(
         color_utils::AlphaBlend(thumb_on_color, thumb_off_color, blend));
-    gfx::Rect thumb_bounds = GetLocalBounds();
-    thumb_bounds.Inset(gfx::Insets(kThumbVerticalMargin));
-    canvas->DrawCircle(gfx::RectF(thumb_bounds).CenterPoint(),
-                       thumb_bounds.height() / 2.f, thumb_paint);
+
+    gfx::RectF thumb_bounds(GetLocalBounds());
+    thumb_bounds.Inset(GetShadowInsets());
+    thumb_bounds.Inset(gfx::InsetsF(0.5f));
+    gfx::RectF bounds_in_px = thumb_bounds;
+    // We want the circle to have an integer pixel diameter and to be aligned
+    // with pixel boundaries, so we scale dip bounds to pixel bounds, round, and
+    // unscale back to dip bounds.
+    bounds_in_px.Scale(canvas->image_scale());
+    gfx::RectF bounds_in_dip(gfx::ToEnclosingRect(bounds_in_px));
+    bounds_in_dip.Scale(1.f / canvas->image_scale());
+    canvas->DrawCircle(bounds_in_dip.CenterPoint(),
+                       bounds_in_dip.height() / 2.f, thumb_paint);
   }
 
   // Color ratio between 0 and 1 that controls the thumb color.
@@ -99,12 +106,12 @@ ToggleButton::ToggleButton(ButtonListener* listener)
     : CustomButton(listener),
       is_on_(false),
       slide_animation_(this),
-      thumb_view_(new ToggleButton::ThumbView()) {
+      thumb_view_(new ThumbView()) {
   slide_animation_.SetSlideDuration(80 /* ms */);
   slide_animation_.SetTweenType(gfx::Tween::LINEAR);
   SetBorder(Border::CreateEmptyBorder(
       gfx::Insets(kTrackVerticalMargin, kTrackHorizontalMargin)));
-  AddChildView(thumb_view_.get());
+  AddChildView(thumb_view_);
   SetInkDropMode(InkDropMode::ON);
   set_has_ink_drop_action_on_click(true);
 }
@@ -136,13 +143,12 @@ gfx::Rect ToggleButton::GetThumbBounds() const {
   // The thumb is a circle, so the width should match the height.
   thumb_bounds.set_width(thumb_bounds.height());
   thumb_bounds.set_x(GetMirroredXForRect(thumb_bounds));
+  thumb_bounds.Inset(-ThumbView::GetShadowInsets());

[varkha, 2016/10/21 00:35:18]

Thank you!

   return thumb_bounds;
 }
 
 void ToggleButton::UpdateThumb() {
-  gfx::Rect thumb_bounds = GetThumbBounds();
-  thumb_bounds.Inset(gfx::Insets(-kThumbVerticalMargin));
-  thumb_view_->Update(thumb_bounds, slide_animation_.GetCurrentValue());
+  thumb_view_->Update(GetThumbBounds(), slide_animation_.GetCurrentValue());
 }
 
 gfx::Size ToggleButton::GetPreferredSize() const {
@@ -161,13 +167,9 @@ void ToggleButton::OnPaint(gfx::Canvas* canvas) {
   gfx::RectF track_rect(GetContentsBounds());
   SkPaint track_paint;
   track_paint.setAntiAlias(true);
-  const SkColor track_on_color =
-      SkColorSetA(GetNativeTheme()->GetSystemColor(
-                      ui::NativeTheme::kColorId_ProminentButtonColor),
-                  0xFF / 2);
   const double color_ratio = slide_animation_.GetCurrentValue();
   track_paint.setColor(color_utils::AlphaBlend(
-      track_on_color, kTrackOffColor,
+      GetTrackColor(true), GetTrackColor(false),
       static_cast<SkAlpha>(SK_AlphaOPAQUE * color_ratio)));
   canvas->DrawRoundRect(track_rect, track_rect.height() / 2, track_paint);
 }
@@ -202,10 +204,7 @@ std::unique_ptr<InkDropRipple> ToggleButton::CreateInkDropRipple() const {
 }
 
 SkColor ToggleButton::GetInkDropBaseColor() const {
-  return is_on()
-             ? GetNativeTheme()->GetSystemColor(
-                   ui::NativeTheme::kColorId_ProminentButtonColor)
-             : kTrackOffColor;
+  return GetTrackColor(is_on());
 }
 
 bool ToggleButton::ShouldShowInkDropHighlight() const {
@@ -223,4 +222,14 @@ void ToggleButton::AnimationProgressed(const gfx::Animation* animation) {
   CustomButton::AnimationProgressed(animation);
 }
 
+SkColor ToggleButton::GetTrackColor(bool is_on) const {
+  const SkAlpha kOffTrackAlpha = 0x29;
+  const SkAlpha kOnTrackAlpha = kOffTrackAlpha * 2;
+  ui::NativeTheme::ColorId color_id =
+      is_on ? ui::NativeTheme::kColorId_ProminentButtonColor
+            : ui::NativeTheme::kColorId_LabelEnabledColor;
+  return SkColorSetA(GetNativeTheme()->GetSystemColor(color_id),
+                     is_on ? kOnTrackAlpha : kOffTrackAlpha);
+}
+
 }  // namespace views