Revamp filter primitive region calculations for Filter Effects

Reland of Revamp filter primitive region calculations for Filter Effects

This moves the filter primitive region calculation to
SVGFilterPrimitiveStandardAttributes::setStandardAttributes, folding in
FilterEffect::applyEffectBoundaries and getting rid of
FilterEffect::m_effectBoundaries and related flags.
What's left of FilterEffect::determineFilterPrimitiveSubregion() is
renamed to determineMaximumEffectRect(), and callsites updated.

BUG=642035
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[fs, 2016-09-01 12:14:57]

Description was changed from

==========
Revamp filter primitive region calculations for Filter Effects

This moves most of the filter primitive region calculation to
SVGFilterPrimitiveStandardAttributes::setStandardAttributes, folding in
FilterEffect::applyEffectBoundaries and getting rid of
FilterEffect::m_effectBoundaries and related flags. What's left in
FilterEffect (at least for now) is the computation of the default filter
primitive region.
FilterEffect::determineFilterPrimitiveSubregion() is renamed to
determineMaximumEffectRect(), and callsites updated.

BUG=642035
==========

to

==========
Revamp filter primitive region calculations for Filter Effects

This moves most of the filter primitive region calculation to
SVGFilterPrimitiveStandardAttributes::setStandardAttributes, folding in
FilterEffect::applyEffectBoundaries and getting rid of
FilterEffect::m_effectBoundaries and related flags. What's left in
FilterEffect (at least for now) is the computation of the default filter
primitive region.
FilterEffect::determineFilterPrimitiveSubregion() is renamed to
determineMaximumEffectRect(), and callsites updated.

BUG=642035
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[fs, 2016-09-01 12:15:03]

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

[commit-bot: I haz the power, 2016-09-01 12:15:13]

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-09-01 13:32:36]

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

[commit-bot: I haz the power, 2016-09-01 13:32:36]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[fs, 2016-09-01 15:36:17]

Description was changed from

==========
Revamp filter primitive region calculations for Filter Effects

This moves most of the filter primitive region calculation to
SVGFilterPrimitiveStandardAttributes::setStandardAttributes, folding in
FilterEffect::applyEffectBoundaries and getting rid of
FilterEffect::m_effectBoundaries and related flags. What's left in
FilterEffect (at least for now) is the computation of the default filter
primitive region.
FilterEffect::determineFilterPrimitiveSubregion() is renamed to
determineMaximumEffectRect(), and callsites updated.

BUG=642035
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Revamp filter primitive region calculations for Filter Effects

This moves most of the filter primitive region calculation to
SVGFilterPrimitiveStandardAttributes::setStandardAttributes, folding in
FilterEffect::applyEffectBoundaries and getting rid of
FilterEffect::m_effectBoundaries and related flags. What's left in
FilterEffect (at least for now) is the computation of the default filter
primitive region.
FilterEffect::determineFilterPrimitiveSubregion() is renamed to
determineMaximumEffectRect(), and callsites updated.

BUG=642035, 641834
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[fs, 2016-09-01 16:17:22]

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

[commit-bot: I haz the power, 2016-09-01 16:17:40]

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-09-01 18:06:20]

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

[commit-bot: I haz the power, 2016-09-01 18:06:21]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fs, 2016-09-01 18:22:06]

Description was changed from

==========
Revamp filter primitive region calculations for Filter Effects

This moves most of the filter primitive region calculation to
SVGFilterPrimitiveStandardAttributes::setStandardAttributes, folding in
FilterEffect::applyEffectBoundaries and getting rid of
FilterEffect::m_effectBoundaries and related flags. What's left in
FilterEffect (at least for now) is the computation of the default filter
primitive region.
FilterEffect::determineFilterPrimitiveSubregion() is renamed to
determineMaximumEffectRect(), and callsites updated.

BUG=642035, 641834
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Revamp filter primitive region calculations for Filter Effects

This moves the filter primitive region calculation to
SVGFilterPrimitiveStandardAttributes::setStandardAttributes, folding in
FilterEffect::applyEffectBoundaries and getting rid of
FilterEffect::m_effectBoundaries and related flags.
What's left of FilterEffect::determineFilterPrimitiveSubregion() is
renamed to determineMaximumEffectRect(), and callsites updated.

BUG=642035, 641834
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[fs, 2016-09-01 18:22:26]

Description was changed from

==========
Revamp filter primitive region calculations for Filter Effects

This moves the filter primitive region calculation to
SVGFilterPrimitiveStandardAttributes::setStandardAttributes, folding in
FilterEffect::applyEffectBoundaries and getting rid of
FilterEffect::m_effectBoundaries and related flags.
What's left of FilterEffect::determineFilterPrimitiveSubregion() is
renamed to determineMaximumEffectRect(), and callsites updated.

BUG=642035, 641834
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Revamp filter primitive region calculations for Filter Effects

This moves the filter primitive region calculation to
SVGFilterPrimitiveStandardAttributes::setStandardAttributes, folding in
FilterEffect::applyEffectBoundaries and getting rid of
FilterEffect::m_effectBoundaries and related flags.
What's left of FilterEffect::determineFilterPrimitiveSubregion() is
renamed to determineMaximumEffectRect(), and callsites updated.

BUG=642035
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[fs, 2016-09-01 20:05:38]

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

[commit-bot: I haz the power, 2016-09-01 20:06:15]

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

[fs, 2016-09-01 21:47:51]

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

[fs, 2016-09-01 21:47:52]

Ok, I think this is ready to go now. Changes to tests are because the primitive
subregion is (correctly) no longer subject to mapRect (on that note there are
some related pessimizations to get rid of next...)

[commit-bot: I haz the power, 2016-09-01 22:51:39]

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

[commit-bot: I haz the power, 2016-09-01 22:51:41]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-09-03 04:50:04]

LGTM, much nicer and matches Gecko

https://codereview.chromium.org/2303703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGFilterPrimitiveStandardAttributes.cpp
(right):

https://codereview.chromium.org/2303703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGFilterPrimitiveStandardAttributes.cpp:118:
// special-case the percentages are relative to the dimensions of the
Do you know what the spec means by "special case" here? I'm wondering why they
even mentioned the percentage values instead of saying "the default subregion is
the filter region".

https://codereview.chromium.org/2303703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/FilterEffect.cpp
(right):

https://codereview.chromium.org/2303703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FilterEffect.cpp:139:
FloatRect inputUnion;
Nit: absoluteInputUnion?

https://codereview.chromium.org/2303703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FilterEffect.cpp:168:
m_maxEffectRect = absoluteSubregion;
(Not for this patch, possibly one of the pessimizations :)
It seems brittle that we rely on determineMaximumEffectRect being called before
maxEffectRect is used, and that the determined max effect rect type (forward,
clip, etc) happens to be what the maxEffectRect caller wants. And... yeah.

https://codereview.chromium.org/2303703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/FilterEffect.h (right):

https://codereview.chromium.org/2303703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FilterEffect.h:59: typedef
int DetermineMaxEffectRectFlags;
For a future patch, it doesn't look like these are ever actually used as flags.

[fs, 2016-09-04 09:33:34]

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

[commit-bot: I haz the power, 2016-09-04 09:33:39]

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

[fs, 2016-09-04 09:34:25]

https://codereview.chromium.org/2303703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGFilterPrimitiveStandardAttributes.cpp
(right):

https://codereview.chromium.org/2303703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGFilterPrimitiveStandardAttributes.cpp:118:
// special-case the percentages are relative to the dimensions of the
On 2016/09/03 at 04:50:03, pdr. wrote:
> Do you know what the spec means by "special case" here? I'm wondering why they
even mentioned the percentage values instead of saying "the default subregion is
the filter region".

The "special case" stems from percentages not generally resolving against the
filter region (but rather against the oBB or viewport as usual.) I don't know
why this formulation was chosen though.

https://codereview.chromium.org/2303703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/FilterEffect.cpp
(right):

https://codereview.chromium.org/2303703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FilterEffect.cpp:139:
FloatRect inputUnion;
On 2016/09/03 at 04:50:03, pdr. wrote:
> Nit: absoluteInputUnion?

Done.

https://codereview.chromium.org/2303703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FilterEffect.cpp:168:
m_maxEffectRect = absoluteSubregion;
On 2016/09/03 at 04:50:03, pdr. wrote:
> (Not for this patch, possibly one of the pessimizations :)
> It seems brittle that we rely on determineMaximumEffectRect being called
before maxEffectRect is used, and that the determined max effect rect type
(forward, clip, etc) happens to be what the maxEffectRect caller wants. And...
yeah.

Yeah...

https://codereview.chromium.org/2303703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/FilterEffect.h (right):

https://codereview.chromium.org/2303703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FilterEffect.h:59: typedef
int DetermineMaxEffectRectFlags;
On 2016/09/03 at 04:50:03, pdr. wrote:
> For a future patch, it doesn't look like these are ever actually used as
flags.

Well, I guess it depends on what property of this method will perish first =)

[commit-bot: I haz the power, 2016-09-04 10:43:03]

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

[commit-bot: I haz the power, 2016-09-04 10:43:04]

Dry run: This issue passed the CQ dry run.

[fs, 2016-09-04 12:12:43]

On 2016/09/03 at 04:50:04, pdr wrote:
> LGTM, much nicer and matches Gecko

Additional LGTM needed for modules/canvas2d/CanvasRenderingContext2DState.cpp
(given the nature of the change, I may TBR that after a timeout.)

[haraken, 2016-09-04 12:22:10]

On 2016/09/04 12:12:43, fs wrote:
> On 2016/09/03 at 04:50:04, pdr wrote:
> > LGTM, much nicer and matches Gecko
> 
> Additional LGTM needed for modules/canvas2d/CanvasRenderingContext2DState.cpp
> (given the nature of the change, I may TBR that after a timeout.)

modules/ LGTM

[fs, 2016-09-04 12:54:11]

On 2016/09/04 at 12:22:10, haraken wrote:
> On 2016/09/04 12:12:43, fs wrote:
> > On 2016/09/03 at 04:50:04, pdr wrote:
> > > LGTM, much nicer and matches Gecko
> > 
> > Additional LGTM needed for
modules/canvas2d/CanvasRenderingContext2DState.cpp
> > (given the nature of the change, I may TBR that after a timeout.)
> 
> modules/ LGTM

Thanks! =D

[fs, 2016-09-04 12:54:18]

The CQ bit was checked by fs@opera.com

[fs, 2016-09-04 12:54:18]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2303703002/#ps60001
(title: "absoluteInputUnion")

[commit-bot: I haz the power, 2016-09-04 12:54:29]

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-09-04 12:58:26]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-04 13:00:06]

Description was changed from

==========
Revamp filter primitive region calculations for Filter Effects

This moves the filter primitive region calculation to
SVGFilterPrimitiveStandardAttributes::setStandardAttributes, folding in
FilterEffect::applyEffectBoundaries and getting rid of
FilterEffect::m_effectBoundaries and related flags.
What's left of FilterEffect::determineFilterPrimitiveSubregion() is
renamed to determineMaximumEffectRect(), and callsites updated.

BUG=642035
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Revamp filter primitive region calculations for Filter Effects

This moves the filter primitive region calculation to
SVGFilterPrimitiveStandardAttributes::setStandardAttributes, folding in
FilterEffect::applyEffectBoundaries and getting rid of
FilterEffect::m_effectBoundaries and related flags.
What's left of FilterEffect::determineFilterPrimitiveSubregion() is
renamed to determineMaximumEffectRect(), and callsites updated.

BUG=642035
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/59aa6aab66c0a3c12644d44c686c02d484814da6
Cr-Commit-Position: refs/heads/master@{#416472}
==========

[commit-bot: I haz the power, 2016-09-04 13:00:07]

Patchset 4 (id:??) landed as
https://crrev.com/59aa6aab66c0a3c12644d44c686c02d484814da6
Cr-Commit-Position: refs/heads/master@{#416472}

[Matt Giuca, 2016-09-05 05:33:53]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2313583002/ by mgiuca@chromium.org.

The reason for reverting is: Introduced failure on WebKit Mac10.11 (retina) bot
in effect-reference-hw.html.

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.11%20%28....

[fs, 2016-09-05 07:40:02]

fs@opera.com changed reviewers:
+ haraken@chromium.org

[fs, 2016-09-05 11:05:41]

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

[commit-bot: I haz the power, 2016-09-05 11:05:54]

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

[fs, 2016-09-05 11:21:49]

Description was changed from

==========
Revamp filter primitive region calculations for Filter Effects

This moves the filter primitive region calculation to
SVGFilterPrimitiveStandardAttributes::setStandardAttributes, folding in
FilterEffect::applyEffectBoundaries and getting rid of
FilterEffect::m_effectBoundaries and related flags.
What's left of FilterEffect::determineFilterPrimitiveSubregion() is
renamed to determineMaximumEffectRect(), and callsites updated.

BUG=642035
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/59aa6aab66c0a3c12644d44c686c02d484814da6
Cr-Commit-Position: refs/heads/master@{#416472}
==========

to

==========
Revamp filter primitive region calculations for Filter Effects

This moves the filter primitive region calculation to
SVGFilterPrimitiveStandardAttributes::setStandardAttributes, folding in
FilterEffect::applyEffectBoundaries and getting rid of
FilterEffect::m_effectBoundaries and related flags.
What's left of FilterEffect::determineFilterPrimitiveSubregion() is
renamed to determineMaximumEffectRect(), and callsites updated.

BUG=642035
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2016-09-05 12:38:38]

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

[commit-bot: I haz the power, 2016-09-05 12:38:39]

Dry run: This issue passed the CQ dry run.

[fs, 2016-09-05 12:45:16]

Description was changed from

==========
Revamp filter primitive region calculations for Filter Effects

This moves the filter primitive region calculation to
SVGFilterPrimitiveStandardAttributes::setStandardAttributes, folding in
FilterEffect::applyEffectBoundaries and getting rid of
FilterEffect::m_effectBoundaries and related flags.
What's left of FilterEffect::determineFilterPrimitiveSubregion() is
renamed to determineMaximumEffectRect(), and callsites updated.

BUG=642035
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Reland of Revamp filter primitive region calculations for Filter Effects

This moves the filter primitive region calculation to
SVGFilterPrimitiveStandardAttributes::setStandardAttributes, folding in
FilterEffect::applyEffectBoundaries and getting rid of
FilterEffect::m_effectBoundaries and related flags.
What's left of FilterEffect::determineFilterPrimitiveSubregion() is
renamed to determineMaximumEffectRect(), and callsites updated.

BUG=642035
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[fs, 2016-09-05 12:45:23]

The CQ bit was checked by fs@opera.com

[fs, 2016-09-05 12:45:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2303703002/#ps100001
(title: "Baselines again; Manual for mac10.11-retina")

[commit-bot: I haz the power, 2016-09-05 12:45:35]

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-09-05 12:49:57]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-05 12:51:27]

Description was changed from

==========
Reland of Revamp filter primitive region calculations for Filter Effects

This moves the filter primitive region calculation to
SVGFilterPrimitiveStandardAttributes::setStandardAttributes, folding in
FilterEffect::applyEffectBoundaries and getting rid of
FilterEffect::m_effectBoundaries and related flags.
What's left of FilterEffect::determineFilterPrimitiveSubregion() is
renamed to determineMaximumEffectRect(), and callsites updated.

BUG=642035
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Reland of Revamp filter primitive region calculations for Filter Effects

This moves the filter primitive region calculation to
SVGFilterPrimitiveStandardAttributes::setStandardAttributes, folding in
FilterEffect::applyEffectBoundaries and getting rid of
FilterEffect::m_effectBoundaries and related flags.
What's left of FilterEffect::determineFilterPrimitiveSubregion() is
renamed to determineMaximumEffectRect(), and callsites updated.

BUG=642035
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[commit-bot: I haz the power, 2016-09-05 12:51:28]

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

[Stephen White, 2016-09-13 15:48:02]

Post-landing LGTM.

Great stuff! I'm really glad to see this cleaned up.

https://codereview.chromium.org/2303703002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/filters/FETile.cpp (right):

https://codereview.chromium.org/2303703002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/filters/FETile.cpp:50: if
(inputEffect(0)->getFilterEffectType() == FilterEffectTypeSourceInput)
Would it not be possible to have make filterPrimitiveSubregion() virtual, and
have SourceGraphic return filterRegion() as its filterPrimitiveSubregion()? It
would allow us to eliminate the special case here. Or is that a bad idea for
other reasons?

[fs, 2016-09-13 16:11:52]

https://codereview.chromium.org/2303703002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/filters/FETile.cpp (right):

https://codereview.chromium.org/2303703002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/filters/FETile.cpp:50: if
(inputEffect(0)->getFilterEffectType() == FilterEffectTypeSourceInput)
On 2016/09/13 at 15:48:02, Stephen White wrote:
> Would it not be possible to have make filterPrimitiveSubregion() virtual, and
have SourceGraphic return filterRegion() as its filterPrimitiveSubregion()? It
would allow us to eliminate the special case here. Or is that a bad idea for
other reasons?

I think that more general here would be to check clipsToBounds() (== has filter
primitive region), so specializing to SourceGraphic probably isn't necessarily
the right thing. (Also the filter [and hence region] of the source may differ
from the filter of the feTile in some cases.)