Change SkImageFilter's cropRect from SkIRect to a CropRect struct

Change SkImageFilter's cropRect from SkIRect to a CropRect struct, containing an SkRect and flags indicating which parameters are set.

NOTE: this will require SK_CROP_RECT_IS_INT=1 to be set in Chrome until Blink has been updated to use SkImageFilter::CropRect. Include https://codereview.chromium.org/26528002/ with the Skia roll.

Note also that SK_CROP_RECT_IS_INT is a temporary measure until all call sites in Blink have been updated to use SkRect.

R=reed@google.com

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

[Stephen White, 2013-10-07 22:08:57]

reed: PTAL. Thanks!

[reed1, 2013-10-08 22:04:17]

lgtm

I think the idea of using magic values is a little tricky, both as a public api
and down in the innards. Seems like we'll end up with various helpers like
is_inherited(rect.fLeft) etc. Perhaps for clarity/sanity, we can just create a
new type for imagefilters that is closer to webkit...

struct CropRectOrSomeOtherName {
    SkRect fRect;
    uint32_t fFlags;   // bits for each edge
};

We ended up creating such a composite class for SkRRect, which at first seemed
overkill, but now we just love it (ask Robert).

[robertphillips, 2013-10-09 13:51:25]

I do worry that using magic values might bite us later (w.r.t. complexity and
maintenance). If we need this functionality, I think it would be much clearer to
make the behavior explicit (i.e., have a CropRect class).

The questions I have are:
  Would we need to have the CropRect class in Skia at all or could it live in
Chrome?
  If it needs to be in Skia (which I expect is the case), can it be isolated in
effects?

[Stephen White, 2013-10-09 14:07:29]

On 2013/10/09 13:51:25, robertphillips wrote:
> I do worry that using magic values might bite us later (w.r.t. complexity and
> maintenance). If we need this functionality, I think it would be much clearer
to
> make the behavior explicit (i.e., have a CropRect class).
> 
> The questions I have are:
>   Would we need to have the CropRect class in Skia at all or could it live in
> Chrome?

It would be much easier in Skia. It could be done in Chrome, but we'd need an
opaque/abstract class in Skia, and a callback/virtual to apply it.

>   If it needs to be in Skia (which I expect is the case), can it be isolated
in
> effects?

Yes, it could be declared as SkCropRect in SkImageFilter.h, or scoped to the
class (SkImageFilter::CropRect), as you prefer.

Are you firm on the uint32_t for bits, or could you live with 4 bools for the
flags? Could be smaller/cleaner that way.

[reed1, 2013-10-09 14:14:55]

On 2013/10/09 14:07:29, Stephen White wrote:
> On 2013/10/09 13:51:25, robertphillips wrote:
> > I do worry that using magic values might bite us later (w.r.t. complexity
and
> > maintenance). If we need this functionality, I think it would be much
clearer
> to
> > make the behavior explicit (i.e., have a CropRect class).
> > 
> > The questions I have are:
> >   Would we need to have the CropRect class in Skia at all or could it live
in
> > Chrome?
> 
> It would be much easier in Skia. It could be done in Chrome, but we'd need an
> opaque/abstract class in Skia, and a callback/virtual to apply it.
> 
> >   If it needs to be in Skia (which I expect is the case), can it be isolated
> in
> > effects?
> 
> Yes, it could be declared as SkCropRect in SkImageFilter.h, or scoped to the
> class (SkImageFilter::CropRect), as you prefer.
> 
> Are you firm on the uint32_t for bits, or could you live with 4 bools for the
> flags? Could be smaller/cleaner that way.

I have (almost) no opinion on the impl of said CropRect struct. What ever is
cleanest (at least first go 'round) sgtm.

I do (for now) prefer to keep it scoped to SkImageFilter...
SkImageFilter::CropRect.

[Stephen White, 2013-10-09 16:21:46]

OK, done. PTAL.

[Stephen White, 2013-10-09 18:38:00]

Patch set 5 is an alternate version, using a single fFlags member, as originally
suggested. I kind of like this version better, since I can have a single
constructor to rule them all.

[reed1, 2013-10-09 19:57:21]

struct looks fine.

What is the plan to transition Blink over to using it?

in our drawBItmapRect case, we basically lived with both APIs for a while, and
had the old one internally just call the new one. That way we could start to
transition callers to use the new (since it existed), and then only removed the
duplicate APIs when it was safe.

[Stephen White, 2013-10-09 20:07:42]

On 2013/10/09 19:57:21, reed1 wrote:
> struct looks fine.
> 
> What is the plan to transition Blink over to using it?
> 
> in our drawBItmapRect case, we basically lived with both APIs for a while, and
> had the old one internally just call the new one. That way we could start to
> transition callers to use the new (since it existed), and then only removed
the
> duplicate APIs when it was safe.

See above:

NOTE: this will require SK_CROP_RECT_IS_INT=1 to be set in Chrome until Blink
has been updated to use SkImageFilter::CropRect. Include
https://codereview.chromium.org/26528002/ with the Skia roll. 
Note also that SK_CROP_RECT_IS_INT is a temporary measure until all call sites
in Blink have been updated to use SkRect.

So the plan is: land this change, roll Skia with the SK_CROP_RECT_IS_INT,
upstream that to Blink, fix the Blink call sites and remove SK_CROP_RECT_IS_INT
from Blink, remove it from Chrome after the Blink roll, then remove it from
Skia.

[Stephen White, 2013-10-09 20:18:01]

On 2013/10/09 20:07:42, Stephen White wrote:
> On 2013/10/09 19:57:21, reed1 wrote:
> > struct looks fine.
> > 
> > What is the plan to transition Blink over to using it?
> > 
> > in our drawBItmapRect case, we basically lived with both APIs for a while,
and
> > had the old one internally just call the new one. That way we could start to
> > transition callers to use the new (since it existed), and then only removed
> the
> > duplicate APIs when it was safe.
> 
> See above:
> 
> NOTE: this will require SK_CROP_RECT_IS_INT=1 to be set in Chrome until Blink
> has been updated to use SkImageFilter::CropRect. Include
> https://codereview.chromium.org/26528002/ with the Skia roll. 
> Note also that SK_CROP_RECT_IS_INT is a temporary measure until all call sites
> in Blink have been updated to use SkRect.
> 
> So the plan is: land this change, roll Skia with the SK_CROP_RECT_IS_INT,
> upstream that to Blink, fix the Blink call sites and remove
SK_CROP_RECT_IS_INT
> from Blink, remove it from Chrome after the Blink roll, then remove it from
> Skia.

This seemed cleaner than having two API's in this case, since it would have
needed two copies of every constructor, factory function, etc for each filter,
esp. since the constructors call the base class constructors.

[reed1, 2013-10-09 20:23:03]

lgtm

seems like we need to be testing (in skia) both settings, to ensure that you can
freely flip the bit on the blink side. Adding Eric to the cc.

[Stephen White, 2013-10-09 20:45:46]

Come to think of it, it should be safe to preemptively land the
SK_CROP_RECT_IS_INT change in Chrome before landing this, to avoid breaking
Skia's Chrome canary.

[Stephen White, 2013-10-10 13:51:29]

Committed patchset #5 manually as r11692 (presubmit successful).