simplify handle-affine: subclass overrides just describe their leaf behavior

simplify handle-affine: subclass overrides just describe their leaf behavior

added new test case (that would have failed before) of blur with a colorfilter input

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1915943002

Committed: https://skia.googlesource.com/skia/+/96a04f329926099f0002f97883242793ff04f61c

[reed1, 2016-04-25 13:20:09]

Description was changed from

==========
simplify handle-affine: subclass overrides just describe their leaf behavior

BUG=skia:
==========

to

==========
simplify handle-affine: subclass overrides just describe their leaf behavior

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[reed1, 2016-04-25 13:20:40]

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

[reed1, 2016-04-25 13:21:09]

Description was changed from

==========
simplify handle-affine: subclass overrides just describe their leaf behavior

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
simplify handle-affine: subclass overrides just describe their leaf behavior

added new test case (that would have failed before) of blur with a colorfilter
input

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[reed1, 2016-04-25 13:21:13]

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

[commit-bot: I haz the power, 2016-04-25 13:21:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915943002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915943002/1

[reed1, 2016-04-25 13:38:18]

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

[commit-bot: I haz the power, 2016-04-25 13:38:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915943002/30001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915943002/30001

[commit-bot: I haz the power, 2016-04-25 13:51:58]

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

[commit-bot: I haz the power, 2016-04-25 13:51:59]

Dry run: This issue passed the CQ dry run.

[reed1, 2016-04-25 14:00:56]

ptal

[robertphillips, 2016-04-25 14:20:11]

lgtm

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

https://codereview.chromium.org/1915943002/diff/30001/src/core/SkImageFilter....
src/core/SkImageFilter.cpp:340: }
Move this first to short-circuit DAG traversal?

[reed1, 2016-04-25 14:22:15]

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

https://codereview.chromium.org/1915943002/diff/30001/src/core/SkImageFilter....
src/core/SkImageFilter.cpp:340: }
On 2016/04/25 14:20:11, robertphillips wrote:
> Move this first to short-circuit DAG traversal?

Thought about that. Not sure it makes it clearer (since now there will be 3
return statements), and I have no evidence that there is a performance problem
that justifies making this harder to read.

[reed1, 2016-04-25 14:24:19]

I expect there may be other subclasses that can opt-in (e.g. paintimagefilter?)
but I think adding those and writing good tests for them (i.e. visual
confirmation tests) probably justifies its own CL.

[Stephen White, 2016-04-25 14:27:03]

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

https://codereview.chromium.org/1915943002/diff/30001/src/core/SkImageFilter....
src/core/SkImageFilter.cpp:340: }
On 2016/04/25 14:22:15, reed1 wrote:
> On 2016/04/25 14:20:11, robertphillips wrote:
> > Move this first to short-circuit DAG traversal?
> 
> Thought about that. Not sure it makes it clearer (since now there will be 3
> return statements), and I have no evidence that there is a performance problem
> that justifies making this harder to read.

I agree with Robert. An early-out to avoid the recursion makes sense, and is
consistent with other recursive functions in SkImageFilter (filterInput,
computeFastBounds, etc).

if (!this->onCanHandleAffine()) { 
    return false;
}

... (as above)


return true;

[reed1, 2016-04-25 14:32:36]

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

https://codereview.chromium.org/1915943002/diff/30001/src/core/SkImageFilter....
src/core/SkImageFilter.cpp:340: }
On 2016/04/25 14:27:03, Stephen White wrote:
> On 2016/04/25 14:22:15, reed1 wrote:
> > On 2016/04/25 14:20:11, robertphillips wrote:
> > > Move this first to short-circuit DAG traversal?
> > 
> > Thought about that. Not sure it makes it clearer (since now there will be 3
> > return statements), and I have no evidence that there is a performance
problem
> > that justifies making this harder to read.
> 
> I agree with Robert. An early-out to avoid the recursion makes sense, and is
> consistent with other recursive functions in SkImageFilter (filterInput,
> computeFastBounds, etc).
> 
> if (!this->onCanHandleAffine()) { 
>     return false;
> }
> 
> ... (as above)
> 
> 
> return true;

So be it :)

[reed1, 2016-04-25 14:32:50]

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

[commit-bot: I haz the power, 2016-04-25 14:32:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915943002/70001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915943002/70001

[Stephen White, 2016-04-25 14:34:15]

LGTM

[reed1, 2016-04-25 14:34:42]

The CQ bit was unchecked by reed@google.com

[reed1, 2016-04-25 14:34:50]

The CQ bit was checked by reed@google.com

[reed1, 2016-04-25 14:34:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1915943002/#ps70001
(title: "early out if onCanHandleComplexCTM returns false")

[commit-bot: I haz the power, 2016-04-25 14:34:56]

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

[commit-bot: I haz the power, 2016-04-25 14:39:55]

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

[commit-bot: I haz the power, 2016-04-25 14:39:57]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)

[reed1, 2016-04-25 14:46:12]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2016-04-25 14:46:15]

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

[commit-bot: I haz the power, 2016-04-25 14:50:19]

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

[commit-bot: I haz the power, 2016-04-25 14:50:20]

Try jobs failed on following builders:
  Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)

[reed1, 2016-04-25 16:06:37]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2016-04-25 16:06:40]

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

[commit-bot: I haz the power, 2016-04-25 16:25:17]

Description was changed from

==========
simplify handle-affine: subclass overrides just describe their leaf behavior

added new test case (that would have failed before) of blur with a colorfilter
input

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
simplify handle-affine: subclass overrides just describe their leaf behavior

added new test case (that would have failed before) of blur with a colorfilter
input

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/+/96a04f329926099f0002f97883242793ff04f61c
==========

[commit-bot: I haz the power, 2016-04-25 16:25:18]

Committed patchset #5 (id:70001) as
https://skia.googlesource.com/skia/+/96a04f329926099f0002f97883242793ff04f61c