Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1461763004:
add SkPath::isRRect (Closed)
Descriptionadd SkPath::isRRect
Add helper to track when a round rect was added to a path,
and then return the SkRRect specification that describes it.
Move the implementation for SkPath::RawIter to SkPathRef so it can be used there as well.
R=reed@google.com,robertphillips@google.com
Committed: https://skia.googlesource.com/skia/+/da707bf5635c70d4c3c284a0b05d92489b76788e
Patch Set 1 #Patch Set 2 : remove erroneous qualification #
Total comments: 14
Patch Set 3 : fix compile; address comments #Patch Set 4 : remove unused test function #Patch Set 5 : more testing #Patch Set 6 : remove unused test function #Patch Set 7 : fix raw iter typo #Patch Set 8 : incorporated Rob's comments #
Messages
Total messages: 41 (19 generated)
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/1461763004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1461763004/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
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/1461763004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1461763004/20001
Description was changed from ========== add isRRect Add helper to track when a round rect was added to a path, and then return the SkRRect specification that describes it. R=reed@google.com,robertphillips@google.com ========== to ========== add isRRect Add helper to track when a round rect was added to a path, and then return the SkRRect specification that describes it. Move the implementation for SkPath::RawIter to SkPathRef so it can be used there as well. R=reed@google.com,robertphillips@google.com ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
Description was changed from ========== add isRRect Add helper to track when a round rect was added to a path, and then return the SkRRect specification that describes it. Move the implementation for SkPath::RawIter to SkPathRef so it can be used there as well. R=reed@google.com,robertphillips@google.com ========== to ========== add SkPath::isRRect Add helper to track when a round rect was added to a path, and then return the SkRRect specification that describes it. Move the implementation for SkPath::RawIter to SkPathRef so it can be used there as well. R=reed@google.com,robertphillips@google.com ==========
not complete -- but go ahead and take a look. I'll finish it when I get back in an hour or so
https://codereview.chromium.org/1461763004/diff/20001/include/core/SkPathRef.h File include/core/SkPathRef.h (right): https://codereview.chromium.org/1461763004/diff/20001/include/core/SkPathRef.... include/core/SkPathRef.h:460: fIsRRect = false; do we remember why calling this forces us to wack these bits? who are the callers? https://codereview.chromium.org/1461763004/diff/20001/include/core/SkPathRef.... include/core/SkPathRef.h:474: SkBool8 fIsOval; we now have 5 bytes in a row, do we want to move all of them to the end of the list, just for tighter packing? https://codereview.chromium.org/1461763004/diff/20001/src/core/SkPath.cpp File src/core/SkPath.cpp (right): https://codereview.chromium.org/1461763004/diff/20001/src/core/SkPath.cpp#new... src/core/SkPath.cpp:1126: SkPathRef::Editor ed(&fPathRef); Should we move these into the last else case, so we only set the bit if we are a rrect (and not a rect or oval)? I sort of think so.
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/1461763004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1461763004/40001
https://codereview.chromium.org/1461763004/diff/20001/include/core/SkPathRef.h File include/core/SkPathRef.h (right): https://codereview.chromium.org/1461763004/diff/20001/include/core/SkPathRef.... include/core/SkPathRef.h:460: fIsRRect = false; On 2015/11/19 19:28:10, reed1 wrote: > do we remember why calling this forces us to wack these bits? who are the > callers? This is only called by the editor. Added a const flavor that doesn't clear the bits. https://codereview.chromium.org/1461763004/diff/20001/include/core/SkPathRef.... include/core/SkPathRef.h:474: SkBool8 fIsOval; On 2015/11/19 19:28:10, reed1 wrote: > we now have 5 bytes in a row, do we want to move all of them to the end of the > list, just for tighter packing? Done. https://codereview.chromium.org/1461763004/diff/20001/src/core/SkPath.cpp File src/core/SkPath.cpp (right): https://codereview.chromium.org/1461763004/diff/20001/src/core/SkPath.cpp#new... src/core/SkPath.cpp:1126: SkPathRef::Editor ed(&fPathRef); On 2015/11/19 19:28:10, reed1 wrote: > Should we move these into the last else case, so we only set the bit if we are a > rrect (and not a rect or oval)? I sort of think so. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
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/1461763004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1461763004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Mac10.8-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac10.8-Clang-...)
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/1461763004/50006 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1461763004/50006
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
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/1461763004/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1461763004/90001
lgtm
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/1461763004/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1461763004/110001
https://codereview.chromium.org/1461763004/diff/20001/include/core/SkPath.h File include/core/SkPath.h (right): https://codereview.chromium.org/1461763004/diff/20001/include/core/SkPath.h#n... include/core/SkPath.h:933: RawIter(const SkPath& path) Is there an extra ' ' here ? https://codereview.chromium.org/1461763004/diff/20001/include/core/SkPath.h#n... include/core/SkPath.h:956: private: So this is both derived from and includes an SkPathRef::Iter ? https://codereview.chromium.org/1461763004/diff/20001/include/core/SkPathRef.h File include/core/SkPathRef.h (right): https://codereview.chromium.org/1461763004/diff/20001/include/core/SkPathRef.... include/core/SkPathRef.h:116: Should this be 'setPathRef' ? https://codereview.chromium.org/1461763004/diff/20001/include/core/SkPathRef.... include/core/SkPathRef.h:180: if (fIsRRect && rrect) { this->getRRect ?
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/1461763004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1461763004/120001
https://codereview.chromium.org/1461763004/diff/20001/include/core/SkPath.h File include/core/SkPath.h (right): https://codereview.chromium.org/1461763004/diff/20001/include/core/SkPath.h#n... include/core/SkPath.h:933: RawIter(const SkPath& path) On 2015/11/19 22:27:28, robertphillips wrote: > Is there an extra ' ' here ? Done. https://codereview.chromium.org/1461763004/diff/20001/include/core/SkPath.h#n... include/core/SkPath.h:956: private: On 2015/11/19 22:27:28, robertphillips wrote: > So this is both derived from and includes an SkPathRef::Iter ? Ack! Fixed typo. Thanks for the extra eyeballs. https://codereview.chromium.org/1461763004/diff/20001/include/core/SkPathRef.h File include/core/SkPathRef.h (right): https://codereview.chromium.org/1461763004/diff/20001/include/core/SkPathRef.... include/core/SkPathRef.h:116: On 2015/11/19 22:27:28, robertphillips wrote: > Should this be 'setPathRef' ? Done. https://codereview.chromium.org/1461763004/diff/20001/include/core/SkPathRef.... include/core/SkPathRef.h:180: if (fIsRRect && rrect) { On 2015/11/19 22:27:28, robertphillips wrote: > this->getRRect ? Done.
The CQ bit was unchecked by caryclark@google.com
The CQ bit was checked by caryclark@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the patchset: https://codereview.chromium.org/1461763004/#ps120001 (title: "incorporated Rob's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1461763004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1461763004/120001
Message was sent while issue was closed.
Committed patchset #8 (id:120001) as https://skia.googlesource.com/skia/+/da707bf5635c70d4c3c284a0b05d92489b76788e |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
