Paint dotted borders using circular dots

third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp

Index: third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp
diff --git a/third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp b/third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp
index 8d10bb36913b631082af996af9c3d0ff7a1bdb97..e08369502df3912d9914acc6d7c6aab78323ac8e 100644
--- a/third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp
+++ b/third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp
@@ -976,8 +976,9 @@ void BoxBorderPainter::drawBoxSideFromPath(GraphicsContext& graphicsContext,
       return;
     case BorderStyleDotted:
     case BorderStyleDashed: {
-      drawDashedDottedBoxSideFromPath(graphicsContext, borderPath, thickness,
-                                      drawThickness, color, borderStyle);
+      drawDashedDottedBoxSideFromPath(graphicsContext, borderRect, borderPath,
+                                      thickness, drawThickness, color,
+                                      borderStyle);
       return;
     }
     case BorderStyleDouble: {
@@ -1011,6 +1012,7 @@ void BoxBorderPainter::drawBoxSideFromPath(GraphicsContext& graphicsContext,
 
 void BoxBorderPainter::drawDashedDottedBoxSideFromPath(
     GraphicsContext& graphicsContext,
+    const LayoutRect& borderRect,
     const Path& borderPath,
     float thickness,
     float drawThickness,
@@ -1018,6 +1020,12 @@ void BoxBorderPainter::drawDashedDottedBoxSideFromPath(
     EBorderStyle borderStyle) const {
   graphicsContext.setStrokeColor(color);
 
+  if (borderStyle == BorderStyleDotted && thickness > 3) {

[f(malita), 2017/03/03 15:34:08]

Why is dotted predicated on thickness > 3?

These are not device pixels, and can be transformed arbitrarily, no?

[Stephen Chennney, 2017/03/03 22:19:22]

Done.

+    drawWideDottedBoxSideFromPath(graphicsContext, borderRect, borderPath,
+                                  thickness);
+    return;
+  }
+
   // The stroke is doubled here because the provided path is the
   // outside edge of the border so half the stroke is clipped off.
   // The extra multiplier is so that the clipping mask can antialias
@@ -1034,7 +1042,6 @@ void BoxBorderPainter::drawDashedDottedBoxSideFromPath(
   // do the same thing as StrokeData::setupPaintDashPathEffect and should be
   // refactored to re-use that code. It would require
   // GraphicsContext::strokePath to take a length parameter.
-
   float dashLength =
       thickness * ((borderStyle == BorderStyleDashed) ? 3.0f : 1.0f);
   float gapLength = dashLength;
@@ -1062,6 +1069,68 @@ void BoxBorderPainter::drawDashedDottedBoxSideFromPath(
   graphicsContext.strokePath(borderPath);
 }
 
+void BoxBorderPainter::drawWideDottedBoxSideFromPath(
+    GraphicsContext& graphicsContext,
+    const LayoutRect& borderRect,
+    const Path& borderPath,

[f(malita), 2017/03/03 15:34:08]

It's kind of annoying that we don't use the provided path (as hinted by the
method name), but instead recompute it from borderRect.

I understand why, but it indicates we're computing the path too early upstack,
and in this case we're throwing it away.  Maybe add a TODO for us to look at
refactoring these methods?

[Stephen Chennney, 2017/03/03 22:19:22]

Done.

+    float thickness) const {
+  // Convert the path to be down the middle of the dots.
+  const LayoutRectOutsets centerOffsets(
+      -roundf(m_edges[BSTop].usedWidth() * 0.5),

[f(malita), 2017/03/03 15:34:08]

Is rounding needed here?  I'm always wary of snapping in non-device coordinates,
and for dots in particular pixel-alignment doesn't seem important.

Except maybe to match the geometry of different style border sides - but then
again, we're computing a "synthetic" mid-border path, so I'm not sure it
matters.

[Stephen Chennney, 2017/03/03 22:19:22]

Not needed, probably. Have changed it and we'll see what happens to test
results.

+      -roundf(m_edges[BSRight].usedWidth() * 0.5),
+      -roundf(m_edges[BSBottom].usedWidth() * 0.5),
+      -roundf(m_edges[BSLeft].usedWidth() * 0.5));
+  FloatRoundedRect innerClip = m_style.getRoundedInnerBorderFor(

[f(malita), 2017/03/03 15:34:08]

Naming nit: not an inner clip.  Maybe centeredBorder or similar?

Or better yet, just inline in addRounderRect() below and skip the name.

[Stephen Chennney, 2017/03/03 22:19:22]

Inlined.

+      borderRect, centerOffsets, m_includeLogicalLeftEdge,
+      m_includeLogicalRightEdge);
+  Path centerlinePath;
+  centerlinePath.addRoundedRect(innerClip);
+
+  graphicsContext.setStrokeThickness(thickness);
+  graphicsContext.setStrokeStyle(DottedStroke);
+
+  // TODO(schenney): This code for setting up the dash effect is largely
+  // duplicated from StrokeData::setupPaintDashPathEffect and both this code
+  // and the method above should be refactored to re-use that code. It would
+  // require GraphicsContext::strokePath to take a length parameter.
+  graphicsContext.setLineCap(RoundCap);
+
+  // Adjust the width to get equal dot spacing as much as possible.
+  float perDotLength = thickness * 2;
+  static float epsilon = 1.0e-2f;
+  float pathLength = centerlinePath.length();
+
+  if (pathLength < perDotLength + thickness) {
+    // Exactly 2 dots with whatever space we can get
+    DashArray lineDash;
+    lineDash.push_back(0);
+    lineDash.push_back(pathLength - thickness - epsilon);
+    graphicsContext.setLineDash(lineDash, 0);
+  } else {
+    // Determine what number of dots gives the minimum deviation from
+    // idealGap between dots. Set the gap to that width.
+    float minNumDots = floorf((pathLength + thickness) / perDotLength);
+    float maxNumDots = minNumDots + 1;
+    float minGap = (pathLength - minNumDots * thickness) / (minNumDots - 1);
+    float maxGap = (pathLength - maxNumDots * thickness) / (maxNumDots - 1);
+    if (fabs(minGap - thickness) < fabs(maxGap - thickness)) {
+      DashArray lineDash;
+      lineDash.push_back(0);
+      lineDash.push_back(minGap + thickness - epsilon);
+      graphicsContext.setLineDash(lineDash, 0);
+    } else {
+      DashArray lineDash;
+      lineDash.push_back(0);
+      lineDash.push_back(maxGap + thickness - epsilon);
+      graphicsContext.setLineDash(lineDash, 0);
+    }

[f(malita), 2017/03/03 15:34:08]

nit: these two branches are mostly the same; maybe consolidate as

  auto gap = fabs(minGap - thickness) < fabs(maxGap - thickness)
      ? minGap : maxGap;
  ...

[Stephen Chennney, 2017/03/03 22:19:22]

Done.

+  }
+
+  // TODO(schenney): stroking the border path causes issues with tight corners:
+  // https://bugs.webkit.org/show_bug.cgi?id=58711
+  graphicsContext.strokePath(centerlinePath);
+}
+
 void BoxBorderPainter::drawDoubleBoxSideFromPath(
     GraphicsContext& graphicsContext,
     const LayoutRect& borderRect,