Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1805963002:
allow one zero length dash (Closed)
Descriptionallow one zero length dash
If the constructed stroke that represents a dash has a
single dash of length zero, and the end cap is square or
round, draw the cap.
The old code initialized the initial dash length to zero,
making it ambiguous whether the first length is zero or
not.
R=robertphillips@google.com
BUG=583299
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1805963002
Committed: https://skia.googlesource.com/skia/+/5e1a24808415df2748822e8082e21a361362cdfe
Committed: https://skia.googlesource.com/skia/+/eb75c7db3a7372de68347d0df8d58acebc33a9ad
Patch Set 1 #Patch Set 2 : allow one zero length dash #
Total comments: 2
Patch Set 3 : use explicit bad params flag #
Total comments: 2
Patch Set 4 : fix comment #Patch Set 5 : add another comment #Patch Set 6 : refactor dash param validation #Patch Set 7 : fix nonsense param test #Patch Set 8 : allow (0,0) dash to return a nullptr patheffect #Patch Set 9 : fix dash nanobench #
Messages
Total messages: 51 (23 generated)
Description was changed from ========== wip; test for 583299 BUG=583299 ========== to ========== wip; test for 583299 BUG=583299 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/1805963002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1805963002/20001
Description was changed from ========== wip; test for 583299 BUG=583299 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== allow one zero length dash If the constructed stroke that represents a dash has a single dash of length zero, and the end cap is square or round, draw the cap. The old code initialized the initial dash length to zero, making it ambiguous whether the first length is zero or not. R=robertphillips@google.com BUG=583299 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
caryclark@google.com changed reviewers: + robertphillips@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
this also only effects the newly added gm; ptal
https://codereview.chromium.org/1805963002/diff/20001/src/effects/SkDashPathE... File src/effects/SkDashPathEffect.cpp (right): https://codereview.chromium.org/1805963002/diff/20001/src/effects/SkDashPathE... src/effects/SkDashPathEffect.cpp:16: : fPhase(0) Don't we use a -1 initial dash length as a signal for bad data (e.g., in CalcDashParameters) ?
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/1805963002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1805963002/40001
https://codereview.chromium.org/1805963002/diff/20001/src/effects/SkDashPathE... File src/effects/SkDashPathEffect.cpp (right): https://codereview.chromium.org/1805963002/diff/20001/src/effects/SkDashPathE... src/effects/SkDashPathEffect.cpp:16: : fPhase(0) On 2016/03/16 18:01:39, robertphillips wrote: > Don't we use a -1 initial dash length as a signal for bad data (e.g., in > CalcDashParameters) ? Switched to explict flag. Please let me know if you like this better.
I do like it better. Do the comments in DashPathEffectTest need cleaning up too? https://codereview.chromium.org/1805963002/diff/40001/src/utils/SkDashPath.cpp File src/utils/SkDashPath.cpp (right): https://codereview.chromium.org/1805963002/diff/40001/src/utils/SkDashPath.cp... src/utils/SkDashPath.cpp:226: Is this comment right - was it ever right?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1805963002/diff/40001/src/utils/SkDashPath.cpp File src/utils/SkDashPath.cpp (right): https://codereview.chromium.org/1805963002/diff/40001/src/utils/SkDashPath.cp... src/utils/SkDashPath.cpp:226: On 2016/03/16 19:00:52, robertphillips wrote: > Is this comment right - was it ever right? Done.
lgtm
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/1805963002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1805963002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
caryclark@google.com changed reviewers: + reed@google.com
On 2016/03/16 19:00:52, robertphillips wrote: > I do like it better. > > Do the comments in DashPathEffectTest need cleaning up too? > > https://codereview.chromium.org/1805963002/diff/40001/src/utils/SkDashPath.cpp > File src/utils/SkDashPath.cpp (right): > > https://codereview.chromium.org/1805963002/diff/40001/src/utils/SkDashPath.cp... > src/utils/SkDashPath.cpp:226: > Is this comment right - was it ever right? Fixed comments in DashPathEffectTest
Mike, this now includes a public include change. Could you take a look?
Can we move the "valid" check into the factory, and just return nullptr in that case. This is the pattern we are moving toward in other class, as it communicates to the client that we're not going to perform the request, and simplifies the actual impl (no need for a self-check at runtime).
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/1805963002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1805963002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
lgtm
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/1805963002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1805963002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
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/1805963002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1805963002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by caryclark@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from robertphillips@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/1805963002/#ps140001 (title: "allow (0,0) dash to return a nullptr patheffect")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1805963002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1805963002/140001
Message was sent while issue was closed.
Description was changed from ========== allow one zero length dash If the constructed stroke that represents a dash has a single dash of length zero, and the end cap is square or round, draw the cap. The old code initialized the initial dash length to zero, making it ambiguous whether the first length is zero or not. R=robertphillips@google.com BUG=583299 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== allow one zero length dash If the constructed stroke that represents a dash has a single dash of length zero, and the end cap is square or round, draw the cap. The old code initialized the initial dash length to zero, making it ambiguous whether the first length is zero or not. R=robertphillips@google.com BUG=583299 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/5e1a24808415df2748822e8082e21a361362cdfe ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://skia.googlesource.com/skia/+/5e1a24808415df2748822e8082e21a361362cdfe
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/1808303004/ by bungeman@google.com. The reason for reverting is: Causes the dash bench to crash. Example crash: https://build.chromium.org/p/client.skia/builders/Perf-Ubuntu-GCC-GCE-CPU-AVX....
Message was sent while issue was closed.
Description was changed from ========== allow one zero length dash If the constructed stroke that represents a dash has a single dash of length zero, and the end cap is square or round, draw the cap. The old code initialized the initial dash length to zero, making it ambiguous whether the first length is zero or not. R=robertphillips@google.com BUG=583299 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/5e1a24808415df2748822e8082e21a361362cdfe ========== to ========== allow one zero length dash If the constructed stroke that represents a dash has a single dash of length zero, and the end cap is square or round, draw the cap. The old code initialized the initial dash length to zero, making it ambiguous whether the first length is zero or not. R=robertphillips@google.com BUG=583299 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/5e1a24808415df2748822e8082e21a361362cdfe ==========
The CQ bit was checked by caryclark@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from robertphillips@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/1805963002/#ps110006 (title: "fix dash nanobench")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1805963002/110006 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1805963002/110006
Message was sent while issue was closed.
Description was changed from ========== allow one zero length dash If the constructed stroke that represents a dash has a single dash of length zero, and the end cap is square or round, draw the cap. The old code initialized the initial dash length to zero, making it ambiguous whether the first length is zero or not. R=robertphillips@google.com BUG=583299 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/5e1a24808415df2748822e8082e21a361362cdfe ========== to ========== allow one zero length dash If the constructed stroke that represents a dash has a single dash of length zero, and the end cap is square or round, draw the cap. The old code initialized the initial dash length to zero, making it ambiguous whether the first length is zero or not. R=robertphillips@google.com BUG=583299 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/5e1a24808415df2748822e8082e21a361362cdfe Committed: https://skia.googlesource.com/skia/+/eb75c7db3a7372de68347d0df8d58acebc33a9ad ==========
Message was sent while issue was closed.
Committed patchset #9 (id:110006) as https://skia.googlesource.com/skia/+/eb75c7db3a7372de68347d0df8d58acebc33a9ad |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
