Make SkImageFilter::applyCropRect() optionally compute srcBounds.

Make SkImageFilter::applyCropRect() optionally compute srcBounds.

BUG=skia:4526
Committed: https://skia.googlesource.com/skia/+/4449398b324a7d6226a71e766561ace0028b5153

[Stephen White, 2015-10-28 18:57:31]

senorblanco@chromium.org changed reviewers:
+ reed@google.com

[Stephen White, 2015-10-28 18:57:31]

reed@: PTAL.

This is just the SkImageFilter::applyCropRect() change broken out from
https://codereview.chromium.org/1415653003/.

[reed1, 2015-10-28 19:27:31]

reed@google.com changed reviewers:
+ robertphillips@google.com

[reed1, 2015-10-28 19:27:31]

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

https://codereview.chromium.org/1410553007/diff/1/include/core/SkImageFilter....
include/core/SkImageFilter.h:372: *  If srcBounds is non-null, it will be
returned there.
... after it is intersected with the computed dstBounds?

Its a little hard to follow, as we talk about what-happens-to-srcBounds in two
diff places.

https://codereview.chromium.org/1410553007/diff/1/include/core/SkImageFilter....
include/core/SkImageFilter.h:375: *  context's clipBounds, and returns the
result in "bounds". Finally,
s/bounds/dstBounds ?

[Stephen White, 2015-10-28 20:17:21]

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

https://codereview.chromium.org/1410553007/diff/1/include/core/SkImageFilter....
include/core/SkImageFilter.h:372: *  If srcBounds is non-null, it will be
returned there.
On 2015/10/28 19:27:31, reed1 wrote:
> ... after it is intersected with the computed dstBounds?
> 
> Its a little hard to follow, as we talk about what-happens-to-srcBounds in two
> diff places.

Yeah, I'm having trouble phrasing this, since srcBounds is really computed in
two places, before and after dstBounds.  I've reworked
it a bit; let me know what you think.

https://codereview.chromium.org/1410553007/diff/1/include/core/SkImageFilter....
include/core/SkImageFilter.h:375: *  context's clipBounds, and returns the
result in "bounds". Finally,
On 2015/10/28 19:27:31, reed1 wrote:
> s/bounds/dstBounds ?

Done.

[reed1, 2015-10-28 20:29:00]

Thanks, that helps (I wonder if pseudo code in the comment would work)

BTW -- does applyCropRect need the full context, or just its clipBounds? If so,
it might be a little clearer to only pass that (as I was wondering as I read the
desc if we were going to transform anything by the ctm in the context).

[Stephen White, 2015-10-28 20:36:58]

On 2015/10/28 20:29:00, reed1 wrote:
> Thanks, that helps (I wonder if pseudo code in the comment would work)
> 
> BTW -- does applyCropRect need the full context, or just its clipBounds? If
so,
> it might be a little clearer to only pass that (as I was wondering as I read
the
> desc if we were going to transform anything by the ctm in the context).

The filter's crop rect is transformed by the CTM.

[reed1, 2015-10-28 20:50:30]

Ah, glad I asked :) Lets mention that in the comment as well.

I know this is becoming a very detailed comment, but I have sort of worked with
imagefilters for a long time, but I still don't have a clear enough picture in
my head about flow, and comments at this level are very helpful.

[Stephen White, 2015-10-28 20:51:12]

On 2015/10/28 20:50:30, reed1 wrote:
> Ah, glad I asked :) Lets mention that in the comment as well.
> 
> I know this is becoming a very detailed comment, but I have sort of worked
with
> imagefilters for a long time, but I still don't have a clear enough picture in
> my head about flow, and comments at this level are very helpful.

That's kind of how I feel about the rest of Skia. :)

[reed1, 2015-10-28 20:53:48]

Of course, so please ask for pointed improvements to the documentation as you
run across sections.

[reed1, 2015-10-28 20:54:33]

I think the failure is mine (not fully groking imagefilters), but comments
definitely help.

[Stephen White, 2015-10-28 20:56:01]

On 2015/10/28 20:50:30, reed1 wrote:
> Ah, glad I asked :) Lets mention that in the comment as well.

Done.

[reed1, 2015-10-28 21:02:15]

thanks

lgtm

[Stephen White, 2015-10-28 21:02:46]

The CQ bit was checked by senorblanco@chromium.org

[commit-bot: I haz the power, 2015-10-28 21:02:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1410553007/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1410553007/40001

[commit-bot: I haz the power, 2015-10-28 21:17:19]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/4449398b324a7d6226a71e766561ace0028b5153