Improve dashed line drawing

Improve dashed line drawing

This patch improves our rendering of dashed lines for borders and
text decoration. Specifically, the dashes are now shorter and the
gaps even shorter, more in keeping with the professional styles
for dashed strokes, and Edge/IE (that have the nicest dashed line
drawing of all browsers). And in most cases corners now have dashes
intersecting at the corner, as they should for clear delineation of
the corner.

R=fmalita@chromium.org
BUG=349985
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2737063002
Cr-Commit-Position: refs/heads/master@{#456754}
Committed: https://chromium.googlesource.com/chromium/src/+/77f3bbabe3fcfb626cd268a018c683a948438c5f

[Stephen Chennney, 2017-03-07 21:21:20]

Description was changed from

==========
Improve dashed line drawing

This patch improves our rendering of dashed lines for borders and
text decoration. Specifically, the dashes are now shorter and the
gaps even shorter, more in keeping with the professional styles
for dashed strokes, and Edge/IE (that have the nicest dashed line
drawing of all browsers). And in most cases corners now have dashes
intersecting at the corner, as they should for clear delineation of
the corner.

R=fmalita@chromium.org
BUG=349985
==========

to

==========
Improve dashed line drawing

This patch improves our rendering of dashed lines for borders and
text decoration. Specifically, the dashes are now shorter and the
gaps even shorter, more in keeping with the professional styles
for dashed strokes, and Edge/IE (that have the nicest dashed line
drawing of all browsers). And in most cases corners now have dashes
intersecting at the corner, as they should for clear delineation of
the corner.

R=fmalita@chromium.org
BUG=349985
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Stephen Chennney, 2017-03-07 21:22:47]

This is just a first pass - it probably needs cleanup (like moving the gap size
calculations to a helper for re-use). The new results are almost universally
better than the old ones.

[Stephen Chennney, 2017-03-08 16:49:56]

Cleaned things up and fixed a bug. Test results are still not right due to the
bug fix, but code is ready for review.

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

On 2017/03/07 21:22:47, Stephen Chennney wrote:
> The new results are almost universally
> better than the old ones.

Nice, I agree the new results look much better!

https://codereview.chromium.org/2737063002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp (right):

https://codereview.chromium.org/2737063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1057: if (pathLength
>= 2 * dashLength + gapLength) {
nit: 2 * perDashLength

https://codereview.chromium.org/2737063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1062: float minGap =
(pathLength - minNumDashes * gapLength) / (minNumDashes - 1);
Shouldn't this use dashLength instead of gapLength?  (the total available gap
space is pathLenght - total dashLenght)

https://codereview.chromium.org/2737063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1063: float maxGap =
(pathLength - maxNumDashes * gapLength) / (maxNumDashes - 1);
ditto

https://codereview.chromium.org/2737063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1065: fabs(minGap -
gapLength) < fabs(maxGap - gapLength) ? minGap : maxGap;
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);

https://codereview.chromium.org/2737063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1073: float multiplier
= pathLength / (2 * dashLength + gapLength);
nit: 2 * perDashLength

https://codereview.chromium.org/2737063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1073: float multiplier
= pathLength / (2 * dashLength + gapLength);
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)?

https://codereview.chromium.org/2737063002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/StrokeData.cpp (right):

https://codereview.chromium.org/2737063002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/StrokeData.cpp:69: static float
epsilon = 1.0e-2f;
nit: can we leave this close to its only user, and make constexpr?

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

Thanks. I'll probably not get to this again before end of day.

https://codereview.chromium.org/2737063002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp (right):

https://codereview.chromium.org/2737063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1057: if (pathLength
>= 2 * dashLength + gapLength) {
On 2017/03/08 16:51:56, f(malita) wrote:
> nit: 2 * perDashLength

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

https://codereview.chromium.org/2737063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1062: float minGap =
(pathLength - minNumDashes * gapLength) / (minNumDashes - 1);
On 2017/03/08 16:51:56, f(malita) wrote:
> Shouldn't this use dashLength instead of gapLength?  (the total available gap
> space is pathLenght - total dashLenght)

Yes. Fixed in the second patch.

https://codereview.chromium.org/2737063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1065: fabs(minGap -
gapLength) < fabs(maxGap - gapLength) ? minGap : maxGap;
On 2017/03/08 16:51:56, f(malita) wrote:
> 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);

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.

https://codereview.chromium.org/2737063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1073: float multiplier
= pathLength / (2 * dashLength + gapLength);
On 2017/03/08 16:51:56, f(malita) wrote:
> 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)?

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?

https://codereview.chromium.org/2737063002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/StrokeData.cpp (right):

https://codereview.chromium.org/2737063002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/StrokeData.cpp:69: static float
epsilon = 1.0e-2f;
On 2017/03/08 16:51:56, f(malita) wrote:
> nit: can we leave this close to its only user, and make constexpr?

Yes, I should revert that back to where it was before and make it constexpr.

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

https://codereview.chromium.org/2737063002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp (right):

https://codereview.chromium.org/2737063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1057: if (pathLength
>= 2 * dashLength + gapLength) {
On 2017/03/08 17:33:26, Stephen Chennney wrote:
> On 2017/03/08 16:51:56, f(malita) wrote:
> > nit: 2 * perDashLength
> 
> I changed this to be just 2 * dashLength because the gap code handles it
> correctly. The 2nd patch fixes other issues with short lengths.

Acknowledged.

https://codereview.chromium.org/2737063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1062: float minGap =
(pathLength - minNumDashes * gapLength) / (minNumDashes - 1);
On 2017/03/08 17:33:26, Stephen Chennney wrote:
> On 2017/03/08 16:51:56, f(malita) wrote:
> > Shouldn't this use dashLength instead of gapLength?  (the total available
gap
> > space is pathLenght - total dashLenght)
> 
> Yes. Fixed in the second patch.

Acknowledged.

https://codereview.chromium.org/2737063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1073: float multiplier
= pathLength / (2 * dashLength + gapLength);
On 2017/03/08 17:33:26, Stephen Chennney wrote:
> On 2017/03/08 16:51:56, f(malita) wrote:
> > 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)?
> 
> 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?

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

[Stephen Chennney, 2017-03-09 22:18:36]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 22:19:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stephen Chennney, 2017-03-09 22:20:53]

I tried modifying the gap resizing to round the number of gaps using roundf and
then compute a gap from that, but the results that changed all became much
worse, with one set losing its gaps entirely it seems, and another growing them
way too big.

So I'd like to leave the gap computation as it is.

I think I've addressed all the other requested changes, and we'll see if the
test results are right now. We're still going to wait on
https://codereview.chromium.org/2733453002/ before landing so as not to cause
others much pain.

[Stephen Chennney, 2017-03-09 22:41:24]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 22:41:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 00:31:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 00:31:29]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[f(malita), 2017-03-10 18:35:52]

lgtm

[Stephen Chennney, 2017-03-10 19:42:07]

It's going to take some effort to get all the baselines. :-(

[Stephen Chennney, 2017-03-10 21:08:13]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2017-03-10 21:08:16]

The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/2737063002/#ps100001
(title: "New expectations")

[commit-bot: I haz the power, 2017-03-10 21:08:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 22:00:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 22:00:08]

Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[Stephen Chennney, 2017-03-11 13:23:57]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2017-03-11 13:24:00]

The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/2737063002/#ps120001
(title: "New baselines")

[commit-bot: I haz the power, 2017-03-11 13:24:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-11 15:56:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-11 15:56:04]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Stephen Chennney, 2017-03-13 13:53:19]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2017-03-13 13:53:22]

The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/2737063002/#ps140001
(title: "Expectations for files that don't rebaseline")

[commit-bot: I haz the power, 2017-03-13 13:53:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 14:42:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 14:42:24]

Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[Stephen Chennney, 2017-03-13 17:10:07]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2017-03-13 17:10:09]

The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/2737063002/#ps160001
(title: "Trying again")

[commit-bot: I haz the power, 2017-03-13 17:10:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 18:52:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 18:52:08]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Stephen Chennney, 2017-03-13 19:47:11]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2017-03-13 19:47:13]

The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/2737063002/#ps180001
(title: "Fix mac expectations")

[commit-bot: I haz the power, 2017-03-13 19:48:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 23:37:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 23:37:13]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Stephen Chennney, 2017-03-14 14:16:35]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2017-03-14 14:16:38]

The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/2737063002/#ps200001
(title: "More expectations. Flaky?")

[commit-bot: I haz the power, 2017-03-14 14:16:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[f(malita), 2017-03-14 14:20:50]

I think your change clashed with the SK_SUPPORT_LEGACY_BROKEN_LERP
mega-rebaseline, and subsequent cleanup.  Sorry!

[f(malita), 2017-03-14 14:22:43]

On 2017/03/14 14:20:50, f(malita) wrote:
> I think your change clashed with the SK_SUPPORT_LEGACY_BROKEN_LERP
> mega-rebaseline, and subsequent cleanup.  Sorry!

(FWIW, baselines should be stable now, so hopefully this will go in)

[commit-bot: I haz the power, 2017-03-14 17:52:08]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1489500995383110,
"parent_rev": "bbd052efa719a473ee143cdb8f6e84bcbfbf0876", "commit_rev":
"77f3bbabe3fcfb626cd268a018c683a948438c5f"}

[commit-bot: I haz the power, 2017-03-14 17:53:08]

Description was changed from

==========
Improve dashed line drawing

This patch improves our rendering of dashed lines for borders and
text decoration. Specifically, the dashes are now shorter and the
gaps even shorter, more in keeping with the professional styles
for dashed strokes, and Edge/IE (that have the nicest dashed line
drawing of all browsers). And in most cases corners now have dashes
intersecting at the corner, as they should for clear delineation of
the corner.

R=fmalita@chromium.org
BUG=349985
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Improve dashed line drawing

This patch improves our rendering of dashed lines for borders and
text decoration. Specifically, the dashes are now shorter and the
gaps even shorter, more in keeping with the professional styles
for dashed strokes, and Edge/IE (that have the nicest dashed line
drawing of all browsers). And in most cases corners now have dashes
intersecting at the corner, as they should for clear delineation of
the corner.

R=fmalita@chromium.org
BUG=349985
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2737063002
Cr-Commit-Position: refs/heads/master@{#456754}
Committed:
https://chromium.googlesource.com/chromium/src/+/77f3bbabe3fcfb626cd268a018c6...
==========

[commit-bot: I haz the power, 2017-03-14 17:53:12]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/77f3bbabe3fcfb626cd268a018c6...