refactor code to apply the croprect

refactor code to apply the croprect

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/189186337eb91b0b21c15db42377bbf3ae4acf23

[reed1, 2015-07-06 15:29:12]

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

[reed1, 2015-07-06 15:29:13]

ptal

[reed1, 2015-07-06 15:29:18]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2015-07-06 15:29:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1218993002/100001

[commit-bot: I haz the power, 2015-07-06 15:29:32]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2015-07-06 21:29 UTC

[commit-bot: I haz the power, 2015-07-06 15:30:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-06 15:30:14]

Try jobs failed on following builders:
  Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
  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...)
  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-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
  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...)

(exceeded global retry quota)

[Stephen White, 2015-07-06 15:37:27]

senorblanco@chromium.org changed reviewers:
+ senorblanco@chromium.org

[Stephen White, 2015-07-06 15:37:28]

LGTM (once the bots are happy)

https://codereview.chromium.org/1218993002/diff/100001/include/core/SkImageFi...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/1218993002/diff/100001/include/core/SkImageFi...
include/core/SkImageFilter.h:86: bool applyTo(const SkIRect& imageBounds, const
Context&, SkIRect* cropped) const;
Nit: could this be srcBounds, as it is in the .cpp?

https://codereview.chromium.org/1218993002/diff/100001/src/core/SkImageFilter...
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/1218993002/diff/100001/src/core/SkImageFilter...
src/core/SkImageFilter.cpp:77: if (!(fFlags & kHasLeft_CropEdge)) {
Nit: the logic seemed clearer to me as it was (without the !s). Out of
curiosity, why the change?

[robertphillips, 2015-07-06 16:02:06]

https://codereview.chromium.org/1218993002/diff/100001/src/core/SkImageFilter...
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/1218993002/diff/100001/src/core/SkImageFilter...
src/core/SkImageFilter.cpp:65: 
srcBounds -> imageBounds ?

https://codereview.chromium.org/1218993002/diff/100001/src/core/SkImageFilter...
src/core/SkImageFilter.cpp:72: 
transformedR -> xformedCropR ?

https://codereview.chromium.org/1218993002/diff/100001/src/core/SkImageFilter...
src/core/SkImageFilter.cpp:76: 
So srcBounds is in device space ?
Add to dox ?

[reed1, 2015-07-16 19:06:53]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-16 19:07:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1218993002/140001

[reed1, 2015-07-16 19:08:37]

The CQ bit was unchecked by reed@google.com

[reed1, 2015-07-16 19:08:47]

https://codereview.chromium.org/1218993002/diff/100001/include/core/SkImageFi...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/1218993002/diff/100001/include/core/SkImageFi...
include/core/SkImageFilter.h:86: bool applyTo(const SkIRect& imageBounds, const
Context&, SkIRect* cropped) const;
On 2015/07/06 15:37:27, Stephen White wrote:
> Nit: could this be srcBounds, as it is in the .cpp?

This can equally apply to the (logical) src or dst bounds in a filter.
The newer spec (2014?) seems to say that crops are in fact only apply to the
dst..

Either way, I think the functionality of this method is independent of how the
caller thinks of the first parameter.

https://codereview.chromium.org/1218993002/diff/100001/src/core/SkImageFilter...
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/1218993002/diff/100001/src/core/SkImageFilter...
src/core/SkImageFilter.cpp:72: 
On 2015/07/06 16:02:06, robertphillips wrote:
> transformedR -> xformedCropR ?

Done.

https://codereview.chromium.org/1218993002/diff/100001/src/core/SkImageFilter...
src/core/SkImageFilter.cpp:76: 
On 2015/07/06 16:02:06, robertphillips wrote:
> So srcBounds is in device space ?
> Add to dox ?

Done.

https://codereview.chromium.org/1218993002/diff/100001/src/core/SkImageFilter...
src/core/SkImageFilter.cpp:77: if (!(fFlags & kHasLeft_CropEdge)) {
On 2015/07/06 15:37:27, Stephen White wrote:
> Nit: the logic seemed clearer to me as it was (without the !s). Out of
> curiosity, why the change?

Done.

[Stephen White, 2015-07-16 19:18:41]

Still LGTM

[reed1, 2015-07-16 20:02:18]

Looking at the change to use the crop's WIDTH instead of RIGHT, I think thats
confusing. Perhaps if we changed the enum to be kHasWidth it would be clearer.

But then again, we've discussed eliminating these flags all together, and having
the client resolve them, so perhaps this concern is only short-term...

[reed1, 2015-07-16 20:02:23]

The CQ bit was checked by reed@google.com

[reed1, 2015-07-16 20:02:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
senorblanco@chromium.org
Link to the patchset: https://codereview.chromium.org/1218993002/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-07-16 20:02:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1218993002/160001

[commit-bot: I haz the power, 2015-07-16 20:17:16]

Committed patchset #9 (id:160001) as
https://skia.googlesource.com/skia/+/189186337eb91b0b21c15db42377bbf3ae4acf23

[Stephen White, 2015-07-16 21:01:26]

On 2015/07/16 20:02:18, reed1 wrote:
> Looking at the change to use the crop's WIDTH instead of RIGHT, I think thats
> confusing. Perhaps if we changed the enum to be kHasWidth it would be clearer.
> 
> But then again, we've discussed eliminating these flags all together, and
having
> the client resolve them, so perhaps this concern is only short-term...

Yeah, that was my first version: https://codereview.chromium.org/1234873005/

but there were no API owners around at the time. :)

I'll rework it.