Remove software path from filters: FEGaussianBlur.

Remove software path from filters: FEGaussianBlur.

BUG=357672
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170356

[pdr., 2014-03-27 21:36:57]

+cc senorblanco

[Stephen White, 2014-03-27 21:40:14]

https://codereview.chromium.org/215303003/diff/1/Source/platform/graphics/fil...
File Source/platform/graphics/filters/FilterEffect.h (right):

https://codereview.chromium.org/215303003/diff/1/Source/platform/graphics/fil...
Source/platform/graphics/filters/FilterEffect.h:203: virtual void
applySoftware() { };
I don't think this is correct. I think this will turn all filters which don't
have applySkia() into no-ops.

I think the correct solution is to rename FEGaussianBlur::applySkia() to
FEGaussianBlur::applySoftware(), and remove the existing
FEGaussianBlur::applySoftware() (as you've done).

[Stephen White, 2014-03-27 21:41:13]

On 2014/03/27 21:40:14, Stephen White wrote:
>
https://codereview.chromium.org/215303003/diff/1/Source/platform/graphics/fil...
> File Source/platform/graphics/filters/FilterEffect.h (right):
> 
>
https://codereview.chromium.org/215303003/diff/1/Source/platform/graphics/fil...
> Source/platform/graphics/filters/FilterEffect.h:203: virtual void
> applySoftware() { };
> I don't think this is correct. I think this will turn all filters which don't
> have applySkia() into no-ops.
> 
> I think the correct solution is to rename FEGaussianBlur::applySkia() to
> FEGaussianBlur::applySoftware(), and remove the existing
> FEGaussianBlur::applySoftware() (as you've done).

BTW, this shouldn't require any changes to FilterEffect.

Once all applySkia()s have been renamed and old code removed, we can remove it
from FilterEffect.

[Savago, 2014-03-27 23:16:30]

https://codereview.chromium.org/215303003/diff/1/Source/platform/graphics/fil...
File Source/platform/graphics/filters/FilterEffect.h (right):

https://codereview.chromium.org/215303003/diff/1/Source/platform/graphics/fil...
Source/platform/graphics/filters/FilterEffect.h:203: virtual void
applySoftware() { };
applySoftware() is a pure abstract virtual member function, which will require
all its descendants to reimplement it, otherwise no instances are allowed to be
created.

Since I'm looking to implement this patch step-by-step (or a Filter at time),
the path of less friction was to provide a default empty implementation of this
method. 

This would allow to simply remove the aforementioned member function from
FEGaussianBlur, while allowing instances of it to be created while requiring *no
changes* on the other FE*Filters.

When the last filter class is completed, then we can rename applySkia() to
applySoftware().

[Stephen White, 2014-03-27 23:27:48]

https://codereview.chromium.org/215303003/diff/1/Source/platform/graphics/fil...
File Source/platform/graphics/filters/FilterEffect.h (right):

https://codereview.chromium.org/215303003/diff/1/Source/platform/graphics/fil...
Source/platform/graphics/filters/FilterEffect.h:203: virtual void
applySoftware() { };
On 2014/03/27 23:16:31, Savago wrote:
> applySoftware() is a pure abstract virtual member function, which will require
> all its descendants to reimplement it, otherwise no instances are allowed to
be
> created.
> 
> Since I'm looking to implement this patch step-by-step (or a Filter at time),
> the path of less friction was to provide a default empty implementation of
this
> method. 
> 
> This would allow to simply remove the aforementioned member function from
> FEGaussianBlur, while allowing instances of it to be created while requiring
*no
> changes* on the other FE*Filters.

It's a good goal, but you can do this without requiring changes to FilterEffect
either.

Just rename FEGaussianBlur::applySkia() to FEGaussianBlur::applySoftware() as I
suggested (and remove the return value). Then the pure virtual of
FilterEffect::applySoftware() is overridden. This should only require changes to
FEGaussianBlur.*.

> When the last filter class is completed, then we can rename applySkia() to
> applySoftware().

[Savago, 2014-03-27 23:43:53]

Stephen

Thanks for commenting, I appreciate the feedback.

There is the issue that Filter.cpp calls both applySoftware() and applySkia().
Making no changes on both its .h and .cpp would require the filter classes to
have both methods.

I'm assuming that changing the call site in the .cpp is the way that you prefer?

If we purge the calls to applySkia() and just call applySoftware() to allow to
execute the renamed method in FEGaussianBlur as you suggested, that means that
only FEGaussianBlur would execute the skia path (while the other classes that
are unpatched) would execute the software path.

That would be a change on the behavior... and probably not what we want. To
continue executing the skia path, it would require to patch the other classes
(i.e. rename applySkia() to applySoftware() in all of them).

[Stephen White, 2014-03-27 23:49:30]

On 2014/03/27 23:43:53, Savago wrote:
> Stephen
> 
> Thanks for commenting, I appreciate the feedback.
> 
> There is the issue that Filter.cpp calls both applySoftware() and applySkia().
> Making no changes on both its .h and .cpp would require the filter classes to
> have both methods.

Look again. The code in FilterEffect::applyRecursive() looks like this:

    if (applySkia())
        return;

    applySoftware();

and the base class implementation of applySkia() returns false. So it's
perfectly fine to have
applySoftware() without an applySkia() (a number of the filters currently do).
We'd just be
changing FEGaussianBlur to do that.

> I'm assuming that changing the call site in the .cpp is the way that you
prefer?
> 
> If we purge the calls to applySkia() and just call applySoftware() to allow to
> execute the renamed method in FEGaussianBlur as you suggested, that means that
> only FEGaussianBlur would execute the skia path (while the other classes that
> are unpatched) would execute the software path.

They'll continue to do what they did before: applySkia() if they have it,
applySoftware() if they don't.

> That would be a change on the behavior... and probably not what we want. To
> continue executing the skia path, it would require to patch the other classes
> (i.e. rename applySkia() to applySoftware() in all of them).

[Savago, 2014-03-27 23:54:14]

I stand corrected, didn't noticed that the default in applySkia() was to return
a false.
:-)

[Savago, 2014-03-28 15:51:01]

The last patch I've uploaded yesterday should address the reviewer's comments.

[Stephen White, 2014-03-28 17:12:27]

LGTM. Thanks for the extra effort.

(BTW, reviewers are not notified when a new patch is uploaded; you have to
"publish & mail comments" again.)

[Stephen White, 2014-03-28 17:13:39]

https://codereview.chromium.org/215303003/diff/60001/Source/platform/graphics...
File Source/platform/graphics/filters/FEGaussianBlur.h (left):

https://codereview.chromium.org/215303003/diff/60001/Source/platform/graphics...
Source/platform/graphics/filters/FEGaussianBlur.h:81: inline void
FEGaussianBlur::kernelPosition(int boxBlur, unsigned& std, int& dLeft, int&
dRight)
BTW, it looks like this function could go as well.

[Savago, 2014-03-28 17:15:50]

Stephen

Thanks for the review. I will upload a new patch with the suggestions.

[Savago, 2014-03-28 17:17:01]

By the way, since the plan is to do a filter at time, it may make sense to open
a bug report in chromium to help keep track of the work. I will update the patch
to reference the new bug.

[Savago, 2014-03-28 18:33:27]

Bug report created, issue CL updated, new patch uploaded.

[Stephen White, 2014-03-28 18:34:34]

Still LGTM. :)

[Savago, 2014-03-28 18:57:21]

The CQ bit was checked by a.cavalcanti@samsung.com

[commit-bot: I haz the power, 2014-03-28 18:58:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.cavalcanti@samsung.com/215303003/70001

[commit-bot: I haz the power, 2014-03-28 19:00:57]

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

[commit-bot: I haz the power, 2014-03-28 19:00:58]

Internal error: failed to checkout. Please try again.

[Stephen White, 2014-03-28 19:06:17]

On 2014/03/28 19:00:58, I haz the power (commit-bot) wrote:
> Internal error: failed to checkout. Please try again.

Let me give it a try.

[Stephen White, 2014-03-28 19:06:22]

The CQ bit was checked by senorblanco@chromium.org

[commit-bot: I haz the power, 2014-03-28 19:06:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.cavalcanti@samsung.com/215303003/70001

[commit-bot: I haz the power, 2014-03-28 19:33:17]

Change committed as 170356