add virtuals to optimize composing colorfilters

add virtuals to optimize composing colorfilters

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/8a8d841d485ff36b64d838575eb3735c1ddcf929

[reed1, 2015-03-02 20:59:09]

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

[reed1, 2015-03-02 20:59:10]

ptal

[reed1, 2015-03-02 21:01:44]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2015-03-02 21:02:22]

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

[commit-bot: I haz the power, 2015-03-02 21:02:22]

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-03 03:02 UTC

[commit-bot: I haz the power, 2015-03-02 21:03:42]

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

[commit-bot: I haz the power, 2015-03-02 21:03:43]

Try jobs failed on following builders:
  Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot on client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-Cl...)
  Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-GC...)

[Stephen White, 2015-03-02 21:10:10]

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

https://codereview.chromium.org/968993004/diff/40001/include/core/SkColorFilt...
include/core/SkColorFilter.h:107: virtual SkColorFilter* newComposed(const
SkColorFilter* inner) const { return NULL; }
Naming nit: maybe this could be preConcat()? (or postConcat(), whatever the
semantics are).

https://codereview.chromium.org/968993004/diff/40001/src/effects/SkColorMatri...
File src/effects/SkColorMatrix.cpp (right):

https://codereview.chromium.org/968993004/diff/40001/src/effects/SkColorMatri...
src/effects/SkColorMatrix.cpp:135: #if 0
Please remove #if 0'ed code.

[reed1, 2015-03-02 21:19:20]

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

https://codereview.chromium.org/968993004/diff/40001/include/core/SkColorFilt...
include/core/SkColorFilter.h:107: virtual SkColorFilter* newComposed(const
SkColorFilter* inner) const { return NULL; }
On 2015/03/02 21:10:10, Stephen White wrote:
> Naming nit: maybe this could be preConcat()? (or postConcat(), whatever the
> semantics are).

preConcat for matrices, compose for patheffects, shaders, colorfilters and
imagefilters. I'd rather keep the current theme (compose) for now, but if the
dox aren't clear enough, I will definitely try to improve them.

https://codereview.chromium.org/968993004/diff/40001/src/effects/SkColorMatri...
File src/effects/SkColorMatrix.cpp (right):

https://codereview.chromium.org/968993004/diff/40001/src/effects/SkColorMatri...
src/effects/SkColorMatrix.cpp:135: #if 0
On 2015/03/02 21:10:10, Stephen White wrote:
> Please remove #if 0'ed code.

Ooops. Done.

[reed1, 2015-03-02 21:32:47]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2015-03-02 21:32:51]

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

[commit-bot: I haz the power, 2015-03-02 21:32:52]

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-03 03:32 UTC

[Stephen White, 2015-03-02 21:32:52]

LGTM w/optional nits

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

https://codereview.chromium.org/968993004/diff/40001/include/core/SkColorFilt...
include/core/SkColorFilter.h:107: virtual SkColorFilter* newComposed(const
SkColorFilter* inner) const { return NULL; }
On 2015/03/02 21:19:19, reed1 wrote:
> On 2015/03/02 21:10:10, Stephen White wrote:
> > Naming nit: maybe this could be preConcat()? (or postConcat(), whatever the
> > semantics are).
> 
> preConcat for matrices, compose for patheffects, shaders, colorfilters and
> imagefilters. I'd rather keep the current theme (compose) for now, but if the
> dox aren't clear enough, I will definitely try to improve them.

Hmm.. createComposed()? The factories (and various classes in Ganesh) provide a
precedent for createFoo(). OTOH SkSurface and friends provide a precedent for
newFoo(). I leave it up to you.

https://codereview.chromium.org/968993004/diff/60001/src/effects/SkTableColor...
File src/effects/SkTableColorFilter.cpp (right):

https://codereview.chromium.org/968993004/diff/60001/src/effects/SkTableColor...
src/effects/SkTableColorFilter.cpp:286: if
(!innerFilter->asComponentTable(&innerBM)) {
Should we be checking for NULL innerFilter here? Maybe asserting?

[reed1, 2015-03-02 21:32:55]

The CQ bit was unchecked by reed@google.com

[reed1, 2015-03-02 21:39:12]

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

https://codereview.chromium.org/968993004/diff/40001/include/core/SkColorFilt...
include/core/SkColorFilter.h:107: virtual SkColorFilter* newComposed(const
SkColorFilter* inner) const { return NULL; }
On 2015/03/02 21:32:52, Stephen White wrote:
> On 2015/03/02 21:19:19, reed1 wrote:
> > On 2015/03/02 21:10:10, Stephen White wrote:
> > > Naming nit: maybe this could be preConcat()? (or postConcat(), whatever
the
> > > semantics are).
> > 
> > preConcat for matrices, compose for patheffects, shaders, colorfilters and
> > imagefilters. I'd rather keep the current theme (compose) for now, but if
the
> > dox aren't clear enough, I will definitely try to improve them.
> 
> Hmm.. createComposed()? The factories (and various classes in Ganesh) provide
a
> precedent for createFoo(). OTOH SkSurface and friends provide a precedent for
> newFoo(). I leave it up to you.

Yea, I was going with surface/canvas use of new as opposed to create. For sure
we should consider consolidating conventions at some point.

https://codereview.chromium.org/968993004/diff/60001/src/effects/SkTableColor...
File src/effects/SkTableColorFilter.cpp (right):

https://codereview.chromium.org/968993004/diff/60001/src/effects/SkTableColor...
src/effects/SkTableColorFilter.cpp:286: if
(!innerFilter->asComponentTable(&innerBM)) {
On 2015/03/02 21:32:52, Stephen White wrote:
> Should we be checking for NULL innerFilter here? Maybe asserting?

We mostly don't do null checks for required parameters, except sometimes in
factories. I think it will reliably crash (as it should), which is the same as
an assert I think.

[reed1, 2015-03-02 21:39:18]

The CQ bit was checked by reed@google.com

[reed1, 2015-03-02 21:39:18]

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/968993004/#ps80001 (title:
"fix param warning")

[commit-bot: I haz the power, 2015-03-02 21:40:32]

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

[commit-bot: I haz the power, 2015-03-02 21:46:07]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/8a8d841d485ff36b64d838575eb3735c1ddcf929