Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1601103006:
Add SkPath::isLastContourClosed() (Closed)
DescriptionAdd SkPath::isLastContourClosed()
Adds a simple method for checking if the last command/verb in the
current contour is a 'close'.
This will simplify determining "closedness" for blink::Path, and aid
in the implementation of algorithms such as:
https://drafts.fxtf.org/motion-1/#motion-processing (second item in list)
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1601103006
Committed: https://skia.googlesource.com/skia/+/b1475b0d41efc580207183a4e25d14e920b57360
Patch Set 1 #
Total comments: 4
Patch Set 2 : isClosed -> isLastContourClosed; add braces #
Messages
Total messages: 21 (9 generated)
Description was changed from ========== Add SkPath::isClosed() Adds a simple method for checking if the last command/verb in the current contour is a 'close'. ========== to ========== Add SkPath::isClosed() Adds a simple method for checking if the last command/verb in the current contour is a 'close'. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Add SkPath::isClosed() Adds a simple method for checking if the last command/verb in the current contour is a 'close'. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add SkPath::isClosed() Adds a simple method for checking if the last command/verb in the current contour is a 'close'. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
fs@opera.com changed reviewers: + caryclark@google.com, reed@google.com
The CQ bit was checked by fs@opera.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/1601103006/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1601103006/1
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2016-01-19 23:24 UTC
PTAL, intended to replace cases such as: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
looks fine -- Mike alone can approve the API change
The CQ bit was unchecked by fs@opera.com
On 2016/01/19 at 17:56:23, caryclark wrote: > looks fine -- Mike alone can approve the API change Mike, wdyt?
What is the motivation for the API? It doesn't look scary, but all query methods are slightly risky, since they can impose (subtle) assumptions on the underlying impl.
On 2016/01/20 at 16:08:03, reed wrote: > What is the motivation for the API? It doesn't look scary, but all query methods are slightly risky, since they can impose (subtle) assumptions on the underlying impl. The motivation is for cases like https://drafts.fxtf.org/motion-1/#motion-processing (second item in list) and similar cases. Currently this is implemented like: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... which uses a whitespace pre-processed string to answer the same query. In Blink we usually have other ways of finding out too, but those are O(N) ATM (could be fixed of course, but would make for more craziness than what this amounts too IMHO.) The main pros of this solution would be that it doesn't require high-level path knowledge (don't need to have both the blink::Path and the data that created it) and is quick. I guess that makes the main motivation "speed".
Thanks for the explanation. I think a link to the spec w/ a brief comment pointing out the need for "closedness" would be good in the CL description. https://codereview.chromium.org/1601103006/diff/1/include/core/SkPath.h File include/core/SkPath.h (right): https://codereview.chromium.org/1601103006/diff/1/include/core/SkPath.h#newco... include/core/SkPath.h:192: bool isClosed() const; nit: lets make the name clearer bool isLastContourClosed() const https://codereview.chromium.org/1601103006/diff/1/src/core/SkPath.cpp File src/core/SkPath.cpp (right): https://codereview.chromium.org/1601103006/diff/1/src/core/SkPath.cpp#newcode323 src/core/SkPath.cpp:323: if (0 == verbCount) nit: skia always uses { } if (0 == verbCount) { return false; }
Description was changed from ========== Add SkPath::isClosed() Adds a simple method for checking if the last command/verb in the current contour is a 'close'. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add SkPath::isLastContourClosed() Adds a simple method for checking if the last command/verb in the current contour is a 'close'. This will simplify determining "closedness" for blink::Path, and aid in the implementation of algorithms such as: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Add SkPath::isLastContourClosed() Adds a simple method for checking if the last command/verb in the current contour is a 'close'. This will simplify determining "closedness" for blink::Path, and aid in the implementation of algorithms such as: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add SkPath::isLastContourClosed() Adds a simple method for checking if the last command/verb in the current contour is a 'close'. This will simplify determining "closedness" for blink::Path, and aid in the implementation of algorithms such as: https://drafts.fxtf.org/motion-1/#motion-processing (second item in list) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
On 2016/01/20 at 16:41:41, reed wrote: > Thanks for the explanation. I think a link to the spec w/ a brief comment pointing out the need for "closedness" would be good in the CL description. Added short blurb and the link from above. https://codereview.chromium.org/1601103006/diff/1/include/core/SkPath.h File include/core/SkPath.h (right): https://codereview.chromium.org/1601103006/diff/1/include/core/SkPath.h#newco... include/core/SkPath.h:192: bool isClosed() const; On 2016/01/20 at 16:41:41, reed1 wrote: > nit: lets make the name clearer > > bool isLastContourClosed() const Done. https://codereview.chromium.org/1601103006/diff/1/src/core/SkPath.cpp File src/core/SkPath.cpp (right): https://codereview.chromium.org/1601103006/diff/1/src/core/SkPath.cpp#newcode323 src/core/SkPath.cpp:323: if (0 == verbCount) On 2016/01/20 at 16:41:41, reed1 wrote: > nit: skia always uses { } > > if (0 == verbCount) { > return false; > } Done.
lgtm
The CQ bit was checked by fs@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1601103006/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1601103006/20001
Message was sent while issue was closed.
Description was changed from ========== Add SkPath::isLastContourClosed() Adds a simple method for checking if the last command/verb in the current contour is a 'close'. This will simplify determining "closedness" for blink::Path, and aid in the implementation of algorithms such as: https://drafts.fxtf.org/motion-1/#motion-processing (second item in list) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add SkPath::isLastContourClosed() Adds a simple method for checking if the last command/verb in the current contour is a 'close'. This will simplify determining "closedness" for blink::Path, and aid in the implementation of algorithms such as: https://drafts.fxtf.org/motion-1/#motion-processing (second item in list) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/b1475b0d41efc580207183a4e25d14e920b57360 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/b1475b0d41efc580207183a4e25d14e920b57360 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
