Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1766243004:
don't create zero length intervals (Closed)
Descriptiondon't create zero length intervals
Dashing a pattern without zero-length intervals should
not create them if the end of the on interval coincides
with the beginning of the initial dash offset.
R=reed@google.com
BUG=591993
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1766243004
Committed: https://skia.googlesource.com/skia/+/18bbd00190623fb6cdb119df4a118ac3c1aed52a
Committed: https://skia.googlesource.com/skia/+/d3cfd94228e6e58bb43dc4f799b4e443fba027a3
Patch Set 1 #
Messages
Total messages: 25 (9 generated)
Description was changed from ========== don't create zero length intervals Dashing a pattern without zero-length intervals should not create them if the end of the on interval coincides with the beginning of the initial dash offset. R=reed@google.com BUG=591993 ========== to ========== don't create zero length intervals Dashing a pattern without zero-length intervals should not create them if the end of the on interval coincides with the beginning of the initial dash offset. R=reed@google.com BUG=591993 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by caryclark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1766243004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1766243004/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Verified that the only change in Gold is the new GM included here and one existing test (dashing3, 1st column, 3rd row) that has this exact pattern -- round cap, offset == on dash length. The GM I wrote doesn't draw anything when it passes -- maybe there's a better way to do this -- but it matches the fiddle in the bug
reed@google.com changed reviewers: + fmalita@chromium.org
1. Does
if (phase >= intervals[i]) { ...
also fix the bug? (>= instead of >)
2. How ought dashing behave with zero length "on" segments in general. e.g.
intervals[] = { 0, 5, 0, 5, };
Should that draw nothing, or draw 2 dots (depending on the stroke cap/join
settings of course)?
Asking since we (re)visited these sorts of questions earlier w.r.t. moveTo/close
etc.
On 2016/03/08 15:08:06, reed1 wrote:
> 1. Does
>
> if (phase >= intervals[i]) { ...
>
> also fix the bug? (>= instead of >)
Yes, it fixes this bug, but it introduces a regression for the reason you state
below.
It causes dashing with zero length on segments to be skipped.
>
>
> 2. How ought dashing behave with zero length "on" segments in general. e.g.
>
> intervals[] = { 0, 5, 0, 5, };
>
> Should that draw nothing, or draw 2 dots (depending on the stroke cap/join
> settings of course)?
>
> Asking since we (re)visited these sorts of questions earlier w.r.t.
moveTo/close
> etc.
Two dots are drawn, which, as I understand it, is the desired behavior w.r.t.
SVG.
I tested the 0, 50, 0, 50 and I only saw one dot
On 2016/03/08 16:22:41, reed1 wrote: > I tested the 0, 50, 0, 50 and I only saw one dot tho maybe i need to make the host path longer. can test again
Just saw this https://bugs.chromium.org/p/chromium/issues/detail?id=583299 suggesting that there are indeed more bugs in this area.
I don't see how to make it clearer, but I do find the fixed code subtle and dense logically. The GM doesn't draw anything when its working correctly. Is a GM the right thing for this, or perhaps one that draws something visible, or a unittest instead? lgtm for change, but unsure about best way to exercise/test.
On 2016/03/08 16:32:13, reed1 wrote: > lgtm for change, but unsure about best way to exercise/test. LGTM too. In Blink there's a class of tests which follow the conversion of "all-green rect == good", "non-green rect or green rect with artifacts == bad". It's probably not ideal for Skia GMs since there's no precedent, just a thought. I guess a unit test reading back a few pixels from dot area and checking they're not drawn is the best idea I have.
The CQ bit was checked by caryclark@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1766243004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1766243004/1
Message was sent while issue was closed.
Description was changed from ========== don't create zero length intervals Dashing a pattern without zero-length intervals should not create them if the end of the on interval coincides with the beginning of the initial dash offset. R=reed@google.com BUG=591993 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== don't create zero length intervals Dashing a pattern without zero-length intervals should not create them if the end of the on interval coincides with the beginning of the initial dash offset. R=reed@google.com BUG=591993 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/18bbd00190623fb6cdb119df4a118ac3c1aed52a ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://skia.googlesource.com/skia/+/18bbd00190623fb6cdb119df4a118ac3c1aed52a
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/1779803002/ by robertphillips@google.com. The reason for reverting is: This may, or may not, be blocking the DEPS roll with: svg/W3C-SVG-1.1/painting-stroke-04-t.svg - LayoutSVGPath {path} at (50,127) size 380x26 [stroke={[type=SOLID] [color=#000000] [stroke width=25.00] [dash offset=10.00] [dash array={10.00, 10.00}]}] [data="M 50 140 L 430 140"] + LayoutSVGPath {path} at (60,127) size 370x26 [stroke={[type=SOLID] [color=#000000] [stroke width=25.00] [dash offset=10.00] [dash array={10.00, 10.00}]}] [data="M 50 140 L 430 140"].
Message was sent while issue was closed.
Description was changed from ========== don't create zero length intervals Dashing a pattern without zero-length intervals should not create them if the end of the on interval coincides with the beginning of the initial dash offset. R=reed@google.com BUG=591993 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/18bbd00190623fb6cdb119df4a118ac3c1aed52a ========== to ========== don't create zero length intervals Dashing a pattern without zero-length intervals should not create them if the end of the on interval coincides with the beginning of the initial dash offset. R=reed@google.com BUG=591993 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/18bbd00190623fb6cdb119df4a118ac3c1aed52a ==========
The CQ bit was checked by caryclark@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1766243004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1766243004/1
Message was sent while issue was closed.
Description was changed from ========== don't create zero length intervals Dashing a pattern without zero-length intervals should not create them if the end of the on interval coincides with the beginning of the initial dash offset. R=reed@google.com BUG=591993 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/18bbd00190623fb6cdb119df4a118ac3c1aed52a ========== to ========== don't create zero length intervals Dashing a pattern without zero-length intervals should not create them if the end of the on interval coincides with the beginning of the initial dash offset. R=reed@google.com BUG=591993 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/18bbd00190623fb6cdb119df4a118ac3c1aed52a Committed: https://skia.googlesource.com/skia/+/d3cfd94228e6e58bb43dc4f799b4e443fba027a3 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://skia.googlesource.com/skia/+/d3cfd94228e6e58bb43dc4f799b4e443fba027a3 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
