Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1793373002:
Add asRRect method to SkClipStack::Element (Closed)
|
Created:
4 years, 9 months ago by Chris Dalton Modified:
4 years, 9 months ago Reviewers:
bsalomon CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionAdd asRRect method to SkClipStack::Element
Adds an asRRect method alongside asPath, for clip implementations that
can be generalized to round rects.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1793373002
Committed: https://skia.googlesource.com/skia/+/b893a4c569d0719a395abffb266d1d61af847e2f
Patch Set 1 #
Total comments: 2
Patch Set 2 : comments #
Total comments: 1
Messages
Total messages: 15 (5 generated)
Description was changed from ========== Add asRRect method to SkClipStack Adds an asRRect method alongside asPath, for clip implementations that can be generalized to round rects. BUG=skia: ========== to ========== Add asRRect method to SkClipStack Adds an asRRect method alongside asPath, for clip implementations that can be generalized to round rects. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
cdalton@nvidia.com changed reviewers: + bsalomon@google.com
lgtm https://codereview.chromium.org/1793373002/diff/1/include/core/SkClipStack.h File include/core/SkClipStack.h (right): https://codereview.chromium.org/1793373002/diff/1/include/core/SkClipStack.h#... include/core/SkClipStack.h:102: const SkRRect* asRRect() const { Maybe we should just loosen the restrictions on when getRRect() can be called? This shares the "as" Name with "asPath". However, that method always succeeds, whereas this one does not.
On 2016/03/16 13:17:12, bsalomon wrote: > lgtm oops, didn't mean to say 1gtm, not lgtm (pending discussion on naming issue). > https://codereview.chromium.org/1793373002/diff/1/include/core/SkClipStack.h > File include/core/SkClipStack.h (right): > > https://codereview.chromium.org/1793373002/diff/1/include/core/SkClipStack.h#... > include/core/SkClipStack.h:102: const SkRRect* asRRect() const { > Maybe we should just loosen the restrictions on when getRRect() can be called? > This shares the "as" Name with "asPath". However, that method always succeeds, > whereas this one does not.
https://codereview.chromium.org/1793373002/diff/1/include/core/SkClipStack.h File include/core/SkClipStack.h (right): https://codereview.chromium.org/1793373002/diff/1/include/core/SkClipStack.h#... include/core/SkClipStack.h:102: const SkRRect* asRRect() const { On 2016/03/16 13:17:11, bsalomon wrote: > Maybe we should just loosen the restrictions on when getRRect() can be called? > This shares the "as" Name with "asPath". However, that method always succeeds, > whereas this one does not. Right, I gave it the "as" name because they both have the property of generalizing specialized types to a more general type of shape. The problem with loosening the restrictions on getRRect() is that it seems we would then have to loosen the restrictions on getPath() as well, just out if principle. What if we keep the "as" name, but assert it's not a path instead of returning null when we can't generalize it? Then it's the caller's responsibility to check it isn't a path before calling.
On 2016/03/16 15:17:56, Chris Dalton wrote: > https://codereview.chromium.org/1793373002/diff/1/include/core/SkClipStack.h > File include/core/SkClipStack.h (right): > > https://codereview.chromium.org/1793373002/diff/1/include/core/SkClipStack.h#... > include/core/SkClipStack.h:102: const SkRRect* asRRect() const { > On 2016/03/16 13:17:11, bsalomon wrote: > > Maybe we should just loosen the restrictions on when getRRect() can be called? > > This shares the "as" Name with "asPath". However, that method always succeeds, > > whereas this one does not. > > Right, I gave it the "as" name because they both have the property of > generalizing specialized types to a more general type of shape. > > The problem with loosening the restrictions on getRRect() is that it seems we > would then have to loosen the restrictions on getPath() as well, just out if > principle. > > What if we keep the "as" name, but assert it's not a path instead of returning > null when we can't generalize it? Then it's the caller's responsibility to check > it isn't a path before calling. That seems like a reasonable compromise.
On 2016/03/16 15:20:23, bsalomon wrote: > On 2016/03/16 15:17:56, Chris Dalton wrote: > > https://codereview.chromium.org/1793373002/diff/1/include/core/SkClipStack.h > > File include/core/SkClipStack.h (right): > > > > > https://codereview.chromium.org/1793373002/diff/1/include/core/SkClipStack.h#... > > include/core/SkClipStack.h:102: const SkRRect* asRRect() const { > > On 2016/03/16 13:17:11, bsalomon wrote: > > > Maybe we should just loosen the restrictions on when getRRect() can be > called? > > > This shares the "as" Name with "asPath". However, that method always > succeeds, > > > whereas this one does not. > > > > Right, I gave it the "as" name because they both have the property of > > generalizing specialized types to a more general type of shape. > > > > The problem with loosening the restrictions on getRRect() is that it seems we > > would then have to loosen the restrictions on getPath() as well, just out if > > principle. > > > > What if we keep the "as" name, but assert it's not a path instead of returning > > null when we can't generalize it? Then it's the caller's responsibility to > check > > it isn't a path before calling. > > That seems like a reasonable compromise. Done. Let me know what you think of this approach.
Description was changed from ========== Add asRRect method to SkClipStack Adds an asRRect method alongside asPath, for clip implementations that can be generalized to round rects. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add asRRect method to SkClipStack::Element Adds an asRRect method alongside asPath, for clip implementations that can be generalized to round rects. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
https://codereview.chromium.org/1793373002/diff/20001/src/core/SkClipStack.cpp File src/core/SkClipStack.cpp (right): https://codereview.chromium.org/1793373002/diff/20001/src/core/SkClipStack.cp... src/core/SkClipStack.cpp:24: fRRect.setEmpty(); Do we really care about doing this? The reason the path is reset is because it can hold an arbitrarily large amount of memory. I don't think it's a big deal either way. Could also just do this in debug. Otherwise, lgtm.
The CQ bit was checked by cdalton@nvidia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1793373002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1793373002/20001
Message was sent while issue was closed.
Description was changed from ========== Add asRRect method to SkClipStack::Element Adds an asRRect method alongside asPath, for clip implementations that can be generalized to round rects. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add asRRect method to SkClipStack::Element Adds an asRRect method alongside asPath, for clip implementations that can be generalized to round rects. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/b893a4c569d0719a395abffb266d1d61af847e2f ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/b893a4c569d0719a395abffb266d1d61af847e2f |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
