Consolidate FilterOperation and FilterEffect mapRect implementations

Consolidate FilterOperation and FilterEffect mapRect implementations

The FilterOperations and FilterEffects for drop-shadow and gaussian blur
has very similar but slightly different implementations.
Restructure the code a bit so that the entire thing can be shared/reused
between the two different code-paths. The new canonical location is the
corresponding FilterEffect (FEGaussianBlur and FEDropShadow.)

Committed: https://crrev.com/b7c3f49d73334795c7e836874d75dbdb40edfd57
Cr-Commit-Position: refs/heads/master@{#424263}

[fs, 2016-10-06 16:15:51]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 16:16:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 16:26:20]

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

[commit-bot: I haz the power, 2016-10-06 16:26:21]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[fs, 2016-10-06 17:30:43]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 17:32:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fs, 2016-10-06 18:46:57]

Description was changed from

==========
Consolidate FilterOperation and FilterEffect mapRect implementations
==========

to

==========
Consolidate FilterOperation and FilterEffect mapRect implementations

The FilterOperations and FilterEffects for drop-shadow and gaussian blur
has very similar but slightly different implementations.
Restructure the code a bit so that the entire thing can be shared/reused
between the two different code-paths. The new canonical location is the
corresponding FilterEffect (FEGaussianBlur and FEDropShadow.)
==========

[fs, 2016-10-06 18:47:20]

fs@opera.com changed reviewers:
+ schenney@chromium.org, senorblanco@chromium.org

[commit-bot: I haz the power, 2016-10-06 18:58:48]

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

[commit-bot: I haz the power, 2016-10-06 18:58:48]

Dry run: This issue passed the CQ dry run.

[Stephen Chennney, 2016-10-10 19:43:25]

Was on vacation. Sorry for the delay.

https://codereview.chromium.org/2393993004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/FilterOperation.cpp (right):

https://codereview.chromium.org/2393993004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/FilterOperation.cpp:154: float stdDeviation
= m_stdDeviation;
Why does this not do the floatValueForLength that the blur effect does?

https://codereview.chromium.org/2393993004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/FEGaussianBlur.cpp
(right):

https://codereview.chromium.org/2393993004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FEGaussianBlur.cpp:48: //
compatible with Firefox' behavior.
Where do we do this limiting? It would be nice of the comment to say where the
code is (or else the comment is out of date and should be removed).

[fs, 2016-10-10 20:40:03]

https://codereview.chromium.org/2393993004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/FilterOperation.cpp (right):

https://codereview.chromium.org/2393993004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/FilterOperation.cpp:154: float stdDeviation
= m_stdDeviation;
On 2016/10/10 at 19:43:25, Stephen Chennney wrote:
> Why does this not do the floatValueForLength that the blur effect does?

This caught me by surprise too, but it turns out that this m_stdDeviation -
unlike the one in BlurFilterOperation - is a plain 'int'. It'd probably make
sense to "upgrade" it to a Length (like in Blur...) Similarly with m_location.
That's best left to a separate CL though I think.

https://codereview.chromium.org/2393993004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/FEGaussianBlur.cpp
(right):

https://codereview.chromium.org/2393993004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FEGaussianBlur.cpp:48: //
compatible with Firefox' behavior.
On 2016/10/10 at 19:43:25, Stephen Chennney wrote:
> Where do we do this limiting? It would be nice of the comment to say where the
code is (or else the comment is out of date and should be removed).

It looks like this comment has outstayed its welcome. It appears to have been
added in 695f3cdf1bfd5c171556245d73d842d3bb5ba32c (back in 2010) together with a
gMaxKernelSize (set to 1000) which was clamped against.
https://codereview.chromium.org/793873002 the dropped the clamping but
supposedly forgot to remove the comment. I guess remove it then.

[fs, 2016-10-10 20:43:53]

https://codereview.chromium.org/2393993004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/FEGaussianBlur.cpp
(right):

https://codereview.chromium.org/2393993004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FEGaussianBlur.cpp:48: //
compatible with Firefox' behavior.
On 2016/10/10 at 20:40:03, fs wrote:
> On 2016/10/10 at 19:43:25, Stephen Chennney wrote:
> > Where do we do this limiting? It would be nice of the comment to say where
the code is (or else the comment is out of date and should be removed).
> 
> It looks like this comment has outstayed its welcome. It appears to have been
added in 695f3cdf1bfd5c171556245d73d842d3bb5ba32c (back in 2010) together with a
gMaxKernelSize (set to 1000) which was clamped against.
https://codereview.chromium.org/793873002 the dropped the clamping but
supposedly forgot to remove the comment. I guess remove it then.

Dropped.

[Stephen Chennney, 2016-10-10 20:52:51]

Thanks. LGTM.

[fs, 2016-10-10 20:53:44]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-10 20:54:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-10 22:11:28]

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

[commit-bot: I haz the power, 2016-10-10 22:11:29]

Dry run: This issue passed the CQ dry run.

[fs, 2016-10-10 22:13:55]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2016-10-10 22:14:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-10 22:21:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-10 22:23:37]

Description was changed from

==========
Consolidate FilterOperation and FilterEffect mapRect implementations

The FilterOperations and FilterEffects for drop-shadow and gaussian blur
has very similar but slightly different implementations.
Restructure the code a bit so that the entire thing can be shared/reused
between the two different code-paths. The new canonical location is the
corresponding FilterEffect (FEGaussianBlur and FEDropShadow.)
==========

to

==========
Consolidate FilterOperation and FilterEffect mapRect implementations

The FilterOperations and FilterEffects for drop-shadow and gaussian blur
has very similar but slightly different implementations.
Restructure the code a bit so that the entire thing can be shared/reused
between the two different code-paths. The new canonical location is the
corresponding FilterEffect (FEGaussianBlur and FEDropShadow.)

Committed: https://crrev.com/b7c3f49d73334795c7e836874d75dbdb40edfd57
Cr-Commit-Position: refs/heads/master@{#424263}
==========

[commit-bot: I haz the power, 2016-10-10 22:23:38]

Patchset 3 (id:??) landed as
https://crrev.com/b7c3f49d73334795c7e836874d75dbdb40edfd57
Cr-Commit-Position: refs/heads/master@{#424263}