Move SkMatrixImageFilter into core, and add a factory fn for it.

Move SkMatrixImageFilter into core, and add a factory fn for it.

BUG=skia:

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

[Stephen White, 2015-03-19 18:46:07]

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

[Stephen White, 2015-03-19 18:46:08]

PTAL. Thanks!

[Stephen White, 2015-03-19 18:47:42]

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

https://codereview.chromium.org/1011273003/diff/1/include/core/SkImageFilter....
include/core/SkImageFilter.h:199: static SkImageFilter* CreateMatrix(const
SkMatrix& matrix,
BTW, I was thinking of changing this to CreateMatrixFilter(), since
CreateMatrix() is somewhat ambiguous. CreateMatrixImageFilter() seems a bit
long, but I could go with that too.

[reed1, 2015-03-19 20:50:01]

In the end, I think we want to move SkMatrixImageFilter.h into src/core (i.e.
private).

If chrome/blink are currently referencing the header, how should this be staged?

one idea:
1. move real header into src/core now (possibly w/ a new name to avoid dup
names)
2. make dummy header in include/effects that just forwards the Create call to
the new api.
3. when chrome/blink have been updated, just remove the dummy header from
effects.

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

https://codereview.chromium.org/1011273003/diff/1/include/core/SkImageFilter....
include/core/SkImageFilter.h:199: static SkImageFilter* CreateMatrix(const
SkMatrix& matrix,
On 2015/03/19 18:47:42, Stephen White wrote:
> BTW, I was thinking of changing this to CreateMatrixFilter(), since
> CreateMatrix() is somewhat ambiguous. CreateMatrixImageFilter() seems a bit
> long, but I could go with that too.

Agreed, CreateMatrixFilter is more consistent with other std factories in
SkColorFilter and SkShader.

[Stephen White, 2015-03-19 21:08:30]

On 2015/03/19 20:50:01, reed1 wrote:
> In the end, I think we want to move SkMatrixImageFilter.h into src/core (i.e.
> private).
> 
> If chrome/blink are currently referencing the header, how should this be
staged?
> 
> one idea:
> 1. move real header into src/core now (possibly w/ a new name to avoid dup
> names)
> 2. make dummy header in include/effects that just forwards the Create call to
> the new api.
> 3. when chrome/blink have been updated, just remove the dummy header from
> effects.

How about we copy the header to src/core, but also leave a copy in
include/effects? Include guards should protect it from being multiply-defined,
then we circle back and remove the include/effects copy after Blink has been
updated to call the new factory.

> https://codereview.chromium.org/1011273003/diff/1/include/core/SkImageFilter.h
> File include/core/SkImageFilter.h (right):
> 
>
https://codereview.chromium.org/1011273003/diff/1/include/core/SkImageFilter....
> include/core/SkImageFilter.h:199: static SkImageFilter* CreateMatrix(const
> SkMatrix& matrix,
> On 2015/03/19 18:47:42, Stephen White wrote:
> > BTW, I was thinking of changing this to CreateMatrixFilter(), since
> > CreateMatrix() is somewhat ambiguous. CreateMatrixImageFilter() seems a bit
> > long, but I could go with that too.
> 
> Agreed, CreateMatrixFilter is more consistent with other std factories in
> SkColorFilter and SkShader.

Done.

[reed1, 2015-03-19 21:19:29]

1. I am nervous having two copies of the same named file, since some clients
(like skia itself) don't scope the dir.

2. Since chrome/blink do scope, don't we need a header in include/effects for
them?

[Stephen White, 2015-03-19 21:21:46]

On 2015/03/19 21:19:29, reed1 wrote:
> 1. I am nervous having two copies of the same named file, since some clients
> (like skia itself) don't scope the dir.

Not sure I understand the risk. It's the same file?

> 2. Since chrome/blink do scope, don't we need a header in include/effects for
> them?

It turns out that the the include in Blink was never scoped, which is why the
first version of this patch worked. Now it's not important, since I'm leaving a
copy in include/effects.

[reed1, 2015-03-20 12:50:56]

Ah, if the current includers are not scoped, then I say we don't need to add a
copy of the same file into src/core (yet). Lets land w/o out that, (just move it
into include/core) and then after we update the call-sites, we can move it from
include/core to src/core

[robertphillips, 2015-03-20 12:52:13]

I don't like having two copies of the same header lying around. How about we:

leave SkMatrixImageFilter.h in include\core (for now)
update Chrome to use the new factory in SkImageFilter
move SkMatrixImageFilter.h to src\core

[Stephen White, 2015-03-20 13:13:24]

On 2015/03/20 12:50:56, reed1 wrote:
> Ah, if the current includers are not scoped, then I say we don't need to add a
> copy of the same file into src/core (yet). Lets land w/o out that, (just move
it
> into include/core) and then after we update the call-sites, we can move it
from
> include/core to src/core

OK, reverted to path set 2.

[reed1, 2015-03-20 13:21:44]

add skbug.com/3568 to track the cleanup.
lgtm

[reed1, 2015-03-20 13:22:45]

have we already updated all of our internal callers to just use the factory (and
not the class)? We should do that before landing.

[Stephen White, 2015-03-20 13:29:11]

On 2015/03/20 13:22:45, reed1 wrote:
> have we already updated all of our internal callers to just use the factory
(and
> not the class)? We should do that before landing.

Done at PS5.

[robertphillips, 2015-03-20 13:31:17]

lgtm

[reed1, 2015-03-20 13:31:27]

yum

[Stephen White, 2015-03-20 13:32:00]

The CQ bit was checked by senorblanco@chromium.org

[Stephen White, 2015-03-20 13:32:00]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/1011273003/#ps80001
(title: "Changed all callers to use SkImageFilter::CreateMatrixFilter() instead
of SKMIF::Create()")

[commit-bot: I haz the power, 2015-03-20 13:32:17]

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

[commit-bot: I haz the power, 2015-03-20 13:38:25]

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