Improve appearance of FocusableBorder at fractional scale factors by

ui/views/controls/focusable_border.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 "ui/views/controls/focusable_border.h"
 
 #include "cc/paint/paint_flags.h"
 #include "third_party/skia/include/core/SkPath.h"
 #include "ui/base/material_design/material_design_controller.h"
 #include "ui/gfx/canvas.h"
@@ skipping 25 matching lines @@
 
 void FocusableBorder::Paint(const View& view, gfx::Canvas* canvas) {
   // In harmony, the focus indicator is a FocusRing.
   if (ui::MaterialDesignController::IsSecondaryUiMaterial() && view.HasFocus())
     return;
 
   cc::PaintFlags flags;
   flags.setStyle(cc::PaintFlags::kStroke_Style);
   flags.setColor(GetCurrentColor(view));
 
+  gfx::ScopedCanvas scoped(canvas);
+  float dsf = canvas->UndoDeviceScaleFactor();
+
+  const int stroke_width_px =
+      ui::MaterialDesignController::IsSecondaryUiMaterial() ? 1 : dsf;

[Peter Kasting, 2017/02/14 00:30:32]

It seems like the old pre-Harmony code was trying to draw a 2 DIP border, and
the new code draws something like a 1 DIP border.  Don't we want a 2* in here?

I also wonder if this shouldn't be gfx::ToRoundedInt().  It looks like you're
truncating-to-int, which seems a bit surprising.  If that's really what we want,
we probably want an explicit ToFlooredInt() + a comment about why flooring is
correct.

[Evan Stade, 2017/02/14 00:37:47]

That was just a hacky way to get a 1dip border. 1dip was inside the rect, 1
outside (and therefore outside the view, i.e. not visible). Instead, we use the
correct stroke width and inset the bounds by half the stroke.
I've been assuming flooring is correct for dip-sized strokes, but I'll ping
Sebastien.

[Peter Kasting, 2017/02/14 00:45:59]

I see.

Why was the old code producing poorer-looking output than the new code?  Was it
because we were drawing the path without AA but not rounding or flooring the
path width, so on some edges the fractional pixel would result in filling the
whole next pixel, and in some it wouldn't?  In which case, the reason this is
better is basically because of the use of "int" in this line.

+  flags.setStrokeWidth(SkIntToScalar(stroke_width_px));
+
+  // Scale the rect and snap to pixel boundaries.
+  gfx::RectF rect(gfx::ScaleToEnclosingRect(view.GetLocalBounds(), dsf));
+  rect.Inset(gfx::InsetsF(stroke_width_px / 2.0f));
+
+  SkPath path;
   if (ui::MaterialDesignController::IsSecondaryUiMaterial()) {
-    gfx::ScopedCanvas scoped(canvas);
-    float dsf = canvas->UndoDeviceScaleFactor();
-    // Scale the rect and snap to pixel boundaries.
-    gfx::RectF rect(gfx::ScaleToEnclosingRect(view.GetLocalBounds(), dsf));
-    rect.Inset(gfx::InsetsF(0.5f));
-    SkPath path;
+    flags.setAntiAlias(true);
     float corner_radius_px = kCornerRadiusDp * dsf;
     path.addRoundRect(gfx::RectFToSkRect(rect), corner_radius_px,
                       corner_radius_px);
-    const int kStrokeWidthPx = 1;
-    flags.setStrokeWidth(SkIntToScalar(kStrokeWidthPx));
-    flags.setAntiAlias(true);
-    canvas->DrawPath(path, flags);
   } else {
-    SkPath path;
-    path.addRect(gfx::RectToSkRect(view.GetLocalBounds()),
-                 SkPath::kCW_Direction);
-    flags.setStrokeWidth(SkIntToScalar(2));
-    canvas->DrawPath(path, flags);
+    path.addRect(gfx::RectFToSkRect(rect), SkPath::kCW_Direction);
   }
+
+  canvas->DrawPath(path, flags);
 }
 
 gfx::Insets FocusableBorder::GetInsets() const {
   return insets_;
 }
 
 gfx::Size FocusableBorder::GetMinimumSize() const {
   return gfx::Size();
 }
 
@@ skipping 12 matching lines @@
   SkColor color = view.GetNativeTheme()->GetSystemColor(color_id);
   if (ui::MaterialDesignController::IsSecondaryUiMaterial() &&
       !view.enabled()) {
     return color_utils::BlendTowardOppositeLuma(color,
                                                 gfx::kDisabledControlAlpha);
   }
   return color;
 }
 
 }  // namespace views