Paint dotted borders using circular dots

Paint dotted borders using circular dots

Firefox, Edge and IE draw dotted borders using circular dots,
while Chrome was using square dots, like Safari. I claim that
most users would expect dots to be circular, so change the
behavior.

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/2711983002
Cr-Commit-Position: refs/heads/master@{#454890}
Committed: https://chromium.googlesource.com/chromium/src/+/399f2e5e66b0ab059852ac7f1e82f334640c3b38

[Stephen Chennney, 2017-02-23 22:01:24]

Description was changed from

==========
Paint dotted borders using circular dots

Firefox, Edge and IE draw dotted borders using circular dots,
while Chrome was using square dots, like Safari. I claim that
most users would expect dots to be circular, so change the
behavior.

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

to

==========
Paint dotted borders using circular dots

Firefox, Edge and IE draw dotted borders using circular dots,
while Chrome was using square dots, like Safari. I claim that
most users would expect dots to be circular, so change the
behavior.

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

[Stephen Chennney, 2017-03-01 17:06:02]

While waiting for baselines, do you think I should break the assorted method
refactoring out into a separate patch?

[f(malita), 2017-03-01 17:50:15]

On 2017/03/01 17:06:02, Stephen Chennney wrote:
> While waiting for baselines, do you think I should break the assorted method
> refactoring out into a separate patch?

If not too much work, it would make it easier to focus on the dotted change.

[Stephen Chennney, 2017-03-01 17:54:07]

On 2017/03/01 17:50:15, f(malita) wrote:
> On 2017/03/01 17:06:02, Stephen Chennney wrote:
> > While waiting for baselines, do you think I should break the assorted method
> > refactoring out into a separate patch?
> 
> If not too much work, it would make it easier to focus on the dotted change.

I'll do that. Right now it's incomprehensible.

[Stephen Chennney, 2017-03-01 21:31:51]

I suggest you look at the test results from the 2nd patch to browse the changes.

The only test that really looks worse to me is
fast/borders/borderRadiusDotted04.html, where we cannot handle the variable
width stroke correctly. Somehow Firefox does the right thing, while we clip big
circles to get the narrow path portions.

Most other tests look better or, at worse, look differently broken.

[Stephen Chennney, 2017-03-01 21:42:59]

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

[commit-bot: I haz the power, 2017-03-01 21:43:44]

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-01 23:00:26]

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

[commit-bot: I haz the power, 2017-03-01 23:00:29]

Dry run: 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-02 13:58:46]

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

[commit-bot: I haz the power, 2017-03-02 13:58:59]

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-02 15:35:39]

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

[commit-bot: I haz the power, 2017-03-02 15:35:41]

Dry run: This issue passed the CQ dry run.

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

Thanks for filling this gap!  Looks good, mostly questions/nits.

https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp (right):

https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1023: if (borderStyle
== BorderStyleDotted && thickness > 3) {
Why is dotted predicated on thickness > 3?

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

https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1075: const Path&
borderPath,
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?

https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1079:
-roundf(m_edges[BSTop].usedWidth() * 0.5),
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.

https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1083: FloatRoundedRect
innerClip = m_style.getRoundedInnerBorderFor(
Naming nit: not an inner clip.  Maybe centeredBorder or similar?

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

https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1126: }
nit: these two branches are mostly the same; maybe consolidate as

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

https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp:508:
(getStrokeStyle() == DottedStroke && width <= 3)) {
Same question about 3.

https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp:1317: (penStyle
== DottedStroke && strokeWidth <= 3)) {
Same.  Let's at least add a helper to encapsulate this logic
(shouldBeDashed()?).

[Stephen Chennney, 2017-03-03 16:21:58]

On 2017/03/03 15:34:08, f(malita) wrote:
> Thanks for filling this gap!  Looks good, mostly questions/nits.
> 
>
https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp (right):
> 
>
https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1023: if
(borderStyle
> == BorderStyleDotted && thickness > 3) {
> Why is dotted predicated on thickness > 3?
> 
> These are not device pixels, and can be transformed arbitrarily, no?
> 
>
https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1075: const Path&
> borderPath,
> 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?
> 
>
https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1079:
> -roundf(m_edges[BSTop].usedWidth() * 0.5),
> 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.
> 
>
https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1083:
FloatRoundedRect
> innerClip = m_style.getRoundedInnerBorderFor(
> Naming nit: not an inner clip.  Maybe centeredBorder or similar?
> 
> Or better yet, just inline in addRounderRect() below and skip the name.
> 
>
https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1126: }
> nit: these two branches are mostly the same; maybe consolidate as
> 
>   auto gap = fabs(minGap - thickness) < fabs(maxGap - thickness)
>       ? minGap : maxGap;
>   ...
> 
>
https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp (right):
> 
>
https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp:508:
> (getStrokeStyle() == DottedStroke && width <= 3)) {
> Same question about 3.
> 
>
https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp:1317:
(penStyle
> == DottedStroke && strokeWidth <= 3)) {
> Same.  Let's at least add a helper to encapsulate this logic
> (shouldBeDashed()?).

Thanks. I'll update the code later today. I agree with your suggestions.

Regarding device pixels and the threshold, all I checked is that the right thing
happened under zoom (it does), but I agree it may do the wrong thing if there's
a CSS transform or other non-zoom-based rescaling. I'm not sure what to do about
it, however. We would have to move the logic down to Skia to not antialias
single pixel line caps.

Regarding the fact we compute a path then throw it away, there is already a TODO
that we really should be passing the mid-path rather than the outer path. See
the bottom of BoxBorderPainter::drawDashedDottedBoxSideFromPath. I think that
basically covers it.

[Stephen Chennney, 2017-03-03 22:19:23]

I'll probably need new baselines, so running try jobs. Otherwise I think I've
addressed all your concerns.

https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp (right):

https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1023: if (borderStyle
== BorderStyleDotted && thickness > 3) {
On 2017/03/03 15:34:08, f(malita) wrote:
> Why is dotted predicated on thickness > 3?
> 
> These are not device pixels, and can be transformed arbitrarily, no?

Done.

https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1075: const Path&
borderPath,
On 2017/03/03 15:34:08, f(malita) wrote:
> 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?

Done.

https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1079:
-roundf(m_edges[BSTop].usedWidth() * 0.5),
On 2017/03/03 15:34:08, f(malita) wrote:
> 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.

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

https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1083: FloatRoundedRect
innerClip = m_style.getRoundedInnerBorderFor(
On 2017/03/03 15:34:08, f(malita) wrote:
> Naming nit: not an inner clip.  Maybe centeredBorder or similar?
> 
> Or better yet, just inline in addRounderRect() below and skip the name.

Inlined.

https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1126: }
On 2017/03/03 15:34:08, f(malita) wrote:
> nit: these two branches are mostly the same; maybe consolidate as
> 
>   auto gap = fabs(minGap - thickness) < fabs(maxGap - thickness)
>       ? minGap : maxGap;
>   ...

Done.

https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/2711983002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp:1317: (penStyle
== DottedStroke && strokeWidth <= 3)) {
On 2017/03/03 15:34:08, f(malita) wrote:
> Same.  Let's at least add a helper to encapsulate this logic
> (shouldBeDashed()?).

Helper done.

[Stephen Chennney, 2017-03-03 22:20:05]

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

[commit-bot: I haz the power, 2017-03-03 22:20:40]

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-03 23:18:27]

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

[commit-bot: I haz the power, 2017-03-03 23:18:28]

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-04 14:24:14]

Thanks.

LGTM % dashing changes.

https://codereview.chromium.org/2711983002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp (right):

https://codereview.chromium.org/2711983002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1026:
m_includeLogicalRightEdge));
I like the consolidation, but it changes dashing and some of the new results
seem to have worse dash distribution than before.

I think we should leave dashing changes for a separate CL (it could use some
other improvements - e.g. Firefox ensures corners are always drawn
symmetrically, which is nice).

https://codereview.chromium.org/2711983002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/StrokeData.cpp (right):

https://codereview.chromium.org/2711983002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/StrokeData.cpp:74: (m_style ==
DottedStroke && m_thickness <= 3)) {
nit: GraphicsContext::strokeIsDashed?

[Stephen Chennney, 2017-03-05 21:50:33]

Moved strokeIsDashed around

https://codereview.chromium.org/2711983002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp (right):

https://codereview.chromium.org/2711983002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1026:
m_includeLogicalRightEdge));
On 2017/03/04 14:24:14, f(malita) wrote:
> I like the consolidation, but it changes dashing and some of the new results
> seem to have worse dash distribution than before.
> 
> I think we should leave dashing changes for a separate CL (it could use some
> other improvements - e.g. Firefox ensures corners are always drawn
> symmetrically, which is nice).

How about we land this and then fix dashing? The distribution changed because
the path is effectively shorter so we draw fewer dashes in some cases, and
visually they are farther apart. I think we should reduce the dash/gap length to
2 x strokewidth (or whatever is closer to other browsers). Fixing corners would
require  clipping the path to the edge we are drawing, so maybe drawing more
paths too. Possible, but challenging.

https://codereview.chromium.org/2711983002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/StrokeData.cpp (right):

https://codereview.chromium.org/2711983002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/StrokeData.cpp:74: (m_style ==
DottedStroke && m_thickness <= 3)) {
On 2017/03/04 14:24:14, f(malita) wrote:
> nit: GraphicsContext::strokeIsDashed?

Missed it. Will do.

[Stephen Chennney, 2017-03-05 21:51:39]

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

[commit-bot: I haz the power, 2017-03-05 21:51: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-05 23:19:00]

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

[commit-bot: I haz the power, 2017-03-05 23:19:02]

Dry run: This issue passed the CQ dry run.

[f(malita), 2017-03-06 17:29:08]

On 2017/03/05 21:50:33, Stephen Chennney wrote:
> Moved strokeIsDashed around
> 
>
https://codereview.chromium.org/2711983002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp (right):
> 
>
https://codereview.chromium.org/2711983002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1026:
> m_includeLogicalRightEdge));
> On 2017/03/04 14:24:14, f(malita) wrote:
> > I like the consolidation, but it changes dashing and some of the new results
> > seem to have worse dash distribution than before.
> > 
> > I think we should leave dashing changes for a separate CL (it could use some
> > other improvements - e.g. Firefox ensures corners are always drawn
> > symmetrically, which is nice).
> 
> How about we land this and then fix dashing?


SGTM

[Stephen Chennney, 2017-03-06 18:08:59]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2017-03-06 18:09:01]

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/2711983002/#ps140001
(title: "Refactor isDashedStroke")

[commit-bot: I haz the power, 2017-03-06 18:09:33]

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-06 18:23:01]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488823739965010,
"parent_rev": "fe4abc21cb6a537fffdaa0d8695365f52a58a731", "commit_rev":
"399f2e5e66b0ab059852ac7f1e82f334640c3b38"}

[commit-bot: I haz the power, 2017-03-06 18:23:41]

Description was changed from

==========
Paint dotted borders using circular dots

Firefox, Edge and IE draw dotted borders using circular dots,
while Chrome was using square dots, like Safari. I claim that
most users would expect dots to be circular, so change the
behavior.

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

to

==========
Paint dotted borders using circular dots

Firefox, Edge and IE draw dotted borders using circular dots,
while Chrome was using square dots, like Safari. I claim that
most users would expect dots to be circular, so change the
behavior.

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/2711983002
Cr-Commit-Position: refs/heads/master@{#454890}
Committed:
https://chromium.googlesource.com/chromium/src/+/399f2e5e66b0ab059852ac7f1e82...
==========

[commit-bot: I haz the power, 2017-03-06 18:23:43]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/399f2e5e66b0ab059852ac7f1e82...