Improve dashed line drawing

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 3308fa3809bd15a074be97eac0de8d8b02d0fcc2..11cdcbeb1c96e17a49806e5ca068477cc4c38cd2 100644
--- a/third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp
+++ b/third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp
@@ -1034,41 +1034,48 @@ void BoxBorderPainter::drawDashedDottedBoxSideFromPath(
     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
   // the edges to prevent jaggies.
   graphicsContext.setStrokeThickness(drawThickness * 1.1f);
   graphicsContext.setStrokeStyle(
       borderStyle == BorderStyleDashed ? DashedStroke : DottedStroke);
 
-  // If the number of dashes that fit in the path is odd and non-integral
-  // then we will have an awkwardly-sized dash at the end of the path. To
-  // try to avoid that here, we simply make the whitespace dashes ever so
-  // slightly bigger.
   // TODO(schenney): This code for setting up the dash effect is trying to
   // 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 dashLength = drawThickness;
   float gapLength = dashLength;
-  float numberOfDashes = centerlinePath.length() / dashLength;
+  if (borderStyle == BorderStyleDashed) {
+    dashLength *= StrokeData::dashLengthRatio(drawThickness);
+    gapLength *= StrokeData::dashGapRatio(drawThickness);
+  }
+  float perDashLength = dashLength + gapLength;
+  float pathLength = centerlinePath.length();
   // Don't try to show dashes if we have less than 2 dashes + 2 gaps.
   // FIXME: should do this test per side.
-  if (numberOfDashes >= 4) {
-    bool evenNumberOfFullDashes = !((int)numberOfDashes % 2);
-    bool integralNumberOfDashes = !(numberOfDashes - (int)numberOfDashes);
-    if (!evenNumberOfFullDashes && !integralNumberOfDashes) {
-      float numberOfGaps = numberOfDashes / 2;
-      gapLength += (dashLength / numberOfGaps);
-    }
+  if (pathLength >= 2 * dashLength + gapLength) {

[f(malita), 2017/03/08 16:51:56]

nit: 2 * perDashLength

[Stephen Chennney, 2017/03/08 17:33:26]

I changed this to be just 2 * dashLength because the gap code handles it
correctly. The 2nd patch fixes other issues with short lengths.

[f(malita), 2017/03/08 17:48:02]

Acknowledged.

+    // Determine what number of dashes gives the minimum deviation from
+    // idealGap between dashes. Set the gap to that width.
+    float minNumDashes = floorf((pathLength + gapLength) / perDashLength);
+    float maxNumDashes = minNumDashes + 1;
+    float minGap = (pathLength - minNumDashes * gapLength) / (minNumDashes - 1);

[f(malita), 2017/03/08 16:51:56]

Shouldn't this use dashLength instead of gapLength?  (the total available gap
space is pathLenght - total dashLenght)

[Stephen Chennney, 2017/03/08 17:33:26]

Yes. Fixed in the second patch.

[f(malita), 2017/03/08 17:48:02]

Acknowledged.

+    float maxGap = (pathLength - maxNumDashes * gapLength) / (maxNumDashes - 1);

[f(malita), 2017/03/08 16:51:56]

ditto

+    auto gap =
+        fabs(minGap - gapLength) < fabs(maxGap - gapLength) ? minGap : maxGap;

[f(malita), 2017/03/08 16:51:56]

Also, I can't quite work it out in my head but it feels like the above should be
expressible via a rounding op...

E.g., if we always minimize the variance for perDashLength 

  auto numDashes = roundf((pathLength + gapLength) / perDashLength);

isn't it sufficient to guarantee that the gap differential is also minimal?

  auto gap = (pathLength - numDashes * dashLength) / (numDashes - 1);

[Stephen Chennney, 2017/03/08 17:33:26]

It might be. I'll look at it further. Even then, it might give close-enough
results. And this is refactored out into a helper on StrokeData in the 2nd
patch.

 
     DashArray lineDash;
     lineDash.push_back(dashLength);
-    lineDash.push_back(gapLength);
+    lineDash.push_back(gap);
     graphicsContext.setLineDash(lineDash, dashLength);
-  }
+  } else if (pathLength > dashLength) {
+    // Exactly 2 dashes proportionally sized
+    float multiplier = pathLength / (2 * dashLength + gapLength);

[f(malita), 2017/03/08 16:51:56]

should this be

float multiplier = (pathLength + gapLength) / 2 * perDashLength;

instead, to align the end of the solid dash on the endpoint (rather than the end
of the gap)?

[f(malita), 2017/03/08 16:51:56]

nit: 2 * perDashLength

[Stephen Chennney, 2017/03/08 17:33:26]

If there are 2 dashes at the endpoints then there is only one gap. So we want to
scale both dashLength and gapLength such that the painting fills up path length
with 2 dashes and the intervening gap. Does that make sense?

[f(malita), 2017/03/08 17:48:02]

Ah, yes, I was parsing that as 2 * (dashLength + gapLength) for some reason :)

+    DashArray lineDash;
+    lineDash.push_back(dashLength * multiplier);
+    lineDash.push_back(gapLength * multiplier);
+    graphicsContext.setLineDash(lineDash, 0);
+  }  // else don't dash at all
 
   // FIXME: stroking the border path causes issues with tight corners:
   // https://bugs.webkit.org/show_bug.cgi?id=58711