add impl limit for number of leaf-nodes in composecolorfilter

add impl limit for number of leaf-nodes in composecolorfilter

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/dc812222a7488d2a0e39b4a09c81c9f000d4b869

[reed1, 2015-03-04 22:22:22]

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

[reed1, 2015-03-04 22:22:23]

ptal

[reed1, 2015-03-04 22:22:25]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2015-03-04 22:23:00]

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

[commit-bot: I haz the power, 2015-03-04 22:23:00]

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-03-05 04:23 UTC

[Stephen White, 2015-03-04 22:38:12]

https://codereview.chromium.org/972153010/diff/20001/src/core/SkColorFilter.cpp
File src/core/SkColorFilter.cpp (right):

https://codereview.chromium.org/972153010/diff/20001/src/core/SkColorFilter.c...
src/core/SkColorFilter.cpp:116: count = 1; // filter is "leaf" filter
Why not just have the base class implementation of privateComposedFilterCount()
return 1? Then you wouldn't need this function at all.

https://codereview.chromium.org/972153010/diff/20001/tests/ColorFilterTest.cpp
File tests/ColorFilterTest.cpp (right):

https://codereview.chromium.org/972153010/diff/20001/tests/ColorFilterTest.cp...
tests/ColorFilterTest.cpp:47: //            SkDebugf("max compose level was
%d\n", i - 1);
Nit: please remove commented-out code.

[reed2, 2015-03-05 01:38:11]

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

[reed2, 2015-03-05 01:38:12]

https://codereview.chromium.org/972153010/diff/20001/src/core/SkColorFilter.cpp
File src/core/SkColorFilter.cpp (right):

https://codereview.chromium.org/972153010/diff/20001/src/core/SkColorFilter.c...
src/core/SkColorFilter.cpp:116: count = 1; // filter is "leaf" filter
On 2015/03/04 22:38:12, Stephen White wrote:
> Why not just have the base class implementation of
privateComposedFilterCount()
> return 1? Then you wouldn't need this function at all.

Yep, I figured that out too driving home. Will change to base-1.

https://codereview.chromium.org/972153010/diff/20001/tests/ColorFilterTest.cpp
File tests/ColorFilterTest.cpp (right):

https://codereview.chromium.org/972153010/diff/20001/tests/ColorFilterTest.cp...
tests/ColorFilterTest.cpp:47: //            SkDebugf("max compose level was
%d\n", i - 1);
On 2015/03/04 22:38:12, Stephen White wrote:
> Nit: please remove commented-out code.

Acknowledged.

[commit-bot: I haz the power, 2015-03-05 04:23:08]

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

[commit-bot: I haz the power, 2015-03-05 04:23:09]

No LGTM from a valid reviewer yet. Please ask for an LGTM from a
full Skia committer from
https://skia.googlesource.com/skia/+/master/CQ_COMMITTERS

[reed1, 2015-03-05 15:13:41]

ptal

[reed1, 2015-03-05 15:13:45]

The CQ bit was checked by reed@google.com

[reed1, 2015-03-05 15:13:54]

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

[commit-bot: I haz the power, 2015-03-05 15:14:19]

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

[commit-bot: I haz the power, 2015-03-05 15:14:21]

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-03-05 21:14 UTC

[Stephen White, 2015-03-05 15:19:11]

LGTM

IWBN if you could run the layout tests (in particular, css3/filters/ and svg/)
locally, just to make sure there are no regressions.

I'm really excited to see the perf benefits here. It would be great to have some
benches to make sure we don't regress in the future.

https://codereview.chromium.org/972153010/diff/40001/include/core/SkColorFilt...
File include/core/SkColorFilter.h (right):

https://codereview.chromium.org/972153010/diff/40001/include/core/SkColorFilt...
include/core/SkColorFilter.h:172: virtual int privateComposedFilterCount() const
{ return 1; }
Nit: maybe this could just be privateFilterCount()?

https://codereview.chromium.org/972153010/diff/40001/tests/ColorFilterTest.cpp
File tests/ColorFilterTest.cpp (right):

https://codereview.chromium.org/972153010/diff/40001/tests/ColorFilterTest.cp...
tests/ColorFilterTest.cpp:39: // Test that CreateComposeFilter() has some finite
limit (i.e. that the factory can return null)
Maybe we should add some more tests that reasonable numbers (less than the
limit) correctly return the number of leaf nodes?

[robertphillips, 2015-03-05 15:20:43]

lgtm + nits

https://codereview.chromium.org/972153010/diff/40001/include/core/SkColorFilt...
File include/core/SkColorFilter.h (right):

https://codereview.chromium.org/972153010/diff/40001/include/core/SkColorFilt...
include/core/SkColorFilter.h:166: *  Returns 1 if this is a single filter (not a
composition of other filters), otherwise it
reutrns

https://codereview.chromium.org/972153010/diff/40001/tests/ColorFilterTest.cpp
File tests/ColorFilterTest.cpp (right):

https://codereview.chromium.org/972153010/diff/40001/tests/ColorFilterTest.cp...
tests/ColorFilterTest.cpp:39: // Test that CreateComposeFilter() has some finite
limit (i.e. that the factory can return null)
kWayTooMany ?

[commit-bot: I haz the power, 2015-03-05 15:21:05]

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