Remove support for SK_CROP_RECT_IS_INT, now that it is no longer used in Blink or Chrome.

Remove support for SK_CROP_RECT_IS_INT, now that it is no longer used in Blink or Chrome.

BUG=
R=reed@google.com, reed

Committed: https://code.google.com/p/skia/source/detail?r=11812

[Stephen White, 2013-10-16 14:30:23]

reed@: PTAL. Thanks!

[reed1, 2013-10-16 14:51:53]

lgtm, w/ requests for some dox/clarifications

https://codereview.chromium.org/27521002/diff/1/include/core/SkImageFilter.h
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/27521002/diff/1/include/core/SkImageFilter.h#...
include/core/SkImageFilter.h:36: uint32_t fFlags;
Is your longer-term plan to keep these fields exposed, or to wrap/hide them, in
the spirit of other structs like SkRRect?

https://codereview.chromium.org/27521002/diff/1/include/core/SkImageFilter.h#...
include/core/SkImageFilter.h:46: bool isSet() const
Can we have a more descriptive name, and/or some dox for this predicate?

[Stephen White, 2013-10-16 14:57:15]

https://codereview.chromium.org/27521002/diff/1/include/core/SkImageFilter.h
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/27521002/diff/1/include/core/SkImageFilter.h#...
include/core/SkImageFilter.h:36: uint32_t fFlags;
On 2013/10/16 14:51:53, reed1 wrote:
> Is your longer-term plan to keep these fields exposed, or to wrap/hide them,
in
> the spirit of other structs like SkRRect?

I was going to leave it as a struct, in the spirit of SkRect.

https://codereview.chromium.org/27521002/diff/1/include/core/SkImageFilter.h#...
include/core/SkImageFilter.h:46: bool isSet() const
On 2013/10/16 14:51:53, reed1 wrote:
> Can we have a more descriptive name, and/or some dox for this predicate?

Done.

[reed1, 2013-10-16 14:59:27]

https://codereview.chromium.org/27521002/diff/1/include/core/SkImageFilter.h
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/27521002/diff/1/include/core/SkImageFilter.h#...
include/core/SkImageFilter.h:36: uint32_t fFlags;
On 2013/10/16 14:57:15, Stephen White wrote:
> On 2013/10/16 14:51:53, reed1 wrote:
> > Is your longer-term plan to keep these fields exposed, or to wrap/hide them,
> in
> > the spirit of other structs like SkRRect?
> 
> I was going to leave it as a struct, in the spirit of SkRect.

Not sure if you're serious, but rect doesn't have bitfields in a flag-int.
Everywhere we have that sort of sophistication, we usually guard it (e.g. Paint,
Matrix), where we care about the client not writing undefined bits.

[Stephen White, 2013-10-16 15:03:46]

On 2013/10/16 14:59:27, reed1 wrote:
> https://codereview.chromium.org/27521002/diff/1/include/core/SkImageFilter.h
> File include/core/SkImageFilter.h (right):
> 
>
https://codereview.chromium.org/27521002/diff/1/include/core/SkImageFilter.h#...
> include/core/SkImageFilter.h:36: uint32_t fFlags;
> On 2013/10/16 14:57:15, Stephen White wrote:
> > On 2013/10/16 14:51:53, reed1 wrote:
> > > Is your longer-term plan to keep these fields exposed, or to wrap/hide
them,
> > in
> > > the spirit of other structs like SkRRect?
> > 
> > I was going to leave it as a struct, in the spirit of SkRect.
> 
> Not sure if you're serious, but rect doesn't have bitfields in a flag-int.
> Everywhere we have that sort of sophistication, we usually guard it (e.g.
Paint,
> Matrix), where we care about the client not writing undefined bits.

Oh sorry, perhaps I misunderstood what you're asking for. I thought you just
wanted to make all the fields private (can do). Did you mean something else?

[reed1, 2013-10-16 15:12:35]

I think its worth exploring what the minimal way to expose these per-edge-flags
is.

- Having getters (e.g. rect(), flags(), certainly can't hurt)

- You have a constructor that takes both a rect and flags... is that sufficient,
or do you need to change either later on?

- If you need to mutate, do you need to do it incrementally, or is a setter with
the same params as the constructor sufficient?

- Do we have to expose that the bool-per-edge is..
   - just a bool (could it ever be tri-state?
   - stored as a bit -vs- as a bool/enum?

I don't know the call-sites nor the impl enough to evaluate any of these. If you
think that any sort of restriction/hiding will break functionality, I'm fine to
leave it as is, but our history with skia's api has led us to rue exposing even
simple things (e.g. paint has "flags" some of which now need to become enums...)

[Stephen White, 2013-10-16 15:27:27]

On 2013/10/16 15:12:35, reed1 wrote:
> I think its worth exploring what the minimal way to expose these
per-edge-flags
> is.
> 
> - Having getters (e.g. rect(), flags(), certainly can't hurt)

Sure. The only class that really needs to access the flags directly is
SkImageFilter (in applyCropRect()). The isSet() method is used by some derived
classes, for certain optimizations. E.g., SkColorFilterImageFilter can only be
expressed as an SkColorFilter alone if there's no crop rect.

> - You have a constructor that takes both a rect and flags... is that
sufficient,
> or do you need to change either later on?

The whole SkImageFilter API is immutable after construction, so this struct
essentially is too. We could hide that with getters.

> - If you need to mutate, do you need to do it incrementally, or is a setter
with
> the same params as the constructor sufficient?

N/A

> - Do we have to expose that the bool-per-edge is..
>    - just a bool (could it ever be tri-state?
>    - stored as a bit -vs- as a bool/enum?

I considered and implemented versions with both 4-bools and a uint32_t in the
codereview that landed the API. Both the implementation and callsites were
cleaner with the latter, so I went with that.

> I don't know the call-sites nor the impl enough to evaluate any of these. If
you
> think that any sort of restriction/hiding will break functionality, I'm fine
to
> leave it as is, but our history with skia's api has led us to rue exposing
even
> simple things (e.g. paint has "flags" some of which now need to become
enums...)

I think we can bake in immutability, since it's transitively immutable being
part of the SkImageFilter API. I'll give that a shot.

[Stephen White, 2013-10-16 15:43:28]

On 2013/10/16 15:27:27, Stephen White wrote:
> On 2013/10/16 15:12:35, reed1 wrote:
> > I think its worth exploring what the minimal way to expose these
> per-edge-flags
> > is.
> > 
> > - Having getters (e.g. rect(), flags(), certainly can't hurt)
> 
> Sure. The only class that really needs to access the flags directly is
> SkImageFilter (in applyCropRect()). The isSet() method is used by some derived
> classes, for certain optimizations. E.g., SkColorFilterImageFilter can only be
> expressed as an SkColorFilter alone if there's no crop rect.
> 
> > - You have a constructor that takes both a rect and flags... is that
> sufficient,
> > or do you need to change either later on?
> 
> The whole SkImageFilter API is immutable after construction, so this struct
> essentially is too. We could hide that with getters.
> 
> > - If you need to mutate, do you need to do it incrementally, or is a setter
> with
> > the same params as the constructor sufficient?
> 
> N/A
> 
> > - Do we have to expose that the bool-per-edge is..
> >    - just a bool (could it ever be tri-state?
> >    - stored as a bit -vs- as a bool/enum?
> 
> I considered and implemented versions with both 4-bools and a uint32_t in the
> codereview that landed the API. Both the implementation and callsites were
> cleaner with the latter, so I went with that.
> 
> > I don't know the call-sites nor the impl enough to evaluate any of these. If
> you
> > think that any sort of restriction/hiding will break functionality, I'm fine
> to
> > leave it as is, but our history with skia's api has led us to rue exposing
> even
> > simple things (e.g. paint has "flags" some of which now need to become
> enums...)
> 
> I think we can bake in immutability, since it's transitively immutable being
> part of the SkImageFilter API. I'll give that a shot.

As discussed, I'm going to land this patch as-is, and upload immutability as a
separate patch.

[Stephen White, 2013-10-16 15:46:08]

Committed patchset #2 manually as r11812 (presubmit successful).