Merge ReferenceFilter into Filter

Merge ReferenceFilter into Filter

The number of differences between the different subclasses of Filter is
decreasing. This brings in a sink and source reference into the
baseclass and makes use of that for users of the SVGFilter subclass too.
Instances of 'targetBoundingBox' is renamed to 'referenceBox' and the
absoluteFilterRegion() method is simplified.

BUG=109224
Committed: https://crrev.com/e57a376a142ca72f6adeeac938bd4287fd54934c
git-svn-id: svn://svn.chromium.org/blink/trunk@202273 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[fs, 2015-09-04 16:25:32]

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

[fs, 2015-09-04 16:25:32]

Step 1 on the road to consolidating the subclasses of Filter - if this passes
muster I'll follow-up with a similar CL for SVGFilter.

[fs, 2015-09-14 18:33:50]

On 2015/09/04 16:25:32, fs wrote:
> Step 1 on the road to consolidating the subclasses of Filter - if this passes
> muster I'll follow-up with a similar CL for SVGFilter.

Ping

[Stephen White, 2015-09-14 18:49:34]

Thanks for taking this on -- I've been meaning to do this.

https://codereview.chromium.org/1326183002/diff/1/Source/core/paint/SVGFilter...
File Source/core/paint/SVGFilterPainter.cpp (right):

https://codereview.chromium.org/1326183002/diff/1/Source/core/paint/SVGFilter...
Source/core/paint/SVGFilterPainter.cpp:47: // FIXME: maybe filterData should
just hold onto SourceGraphic after creation?
You can probably get rid of the comment now -- we're no longer looking it up by
ID (which was the gross part).

https://codereview.chromium.org/1326183002/diff/1/Source/platform/graphics/fi...
File Source/platform/graphics/filters/Filter.h (right):

https://codereview.chromium.org/1326183002/diff/1/Source/platform/graphics/fi...
Source/platform/graphics/filters/Filter.h:52: FloatRect
mapLocalRectToAbsoluteRect(const FloatRect& rect) const
Nit: maybe we should just de-inline these functions. I strongly doubt they're
showing up as hot anywhere.

https://codereview.chromium.org/1326183002/diff/1/Source/platform/graphics/fi...
Source/platform/graphics/filters/Filter.h:85: FloatRect m_referenceBox;
Could you rationalize the namechange here? Is it because it's not strictly a
bounding box (in the userSpaceOnUse case?)

[fs, 2015-09-15 10:42:56]

https://codereview.chromium.org/1326183002/diff/1/Source/core/paint/SVGFilter...
File Source/core/paint/SVGFilterPainter.cpp (right):

https://codereview.chromium.org/1326183002/diff/1/Source/core/paint/SVGFilter...
Source/core/paint/SVGFilterPainter.cpp:47: // FIXME: maybe filterData should
just hold onto SourceGraphic after creation?
On 2015/09/14 18:49:34, Stephen White wrote:
> You can probably get rid of the comment now -- we're no longer looking it up
by
> ID (which was the gross part).

Done.

https://codereview.chromium.org/1326183002/diff/1/Source/platform/graphics/fi...
File Source/platform/graphics/filters/Filter.h (right):

https://codereview.chromium.org/1326183002/diff/1/Source/platform/graphics/fi...
Source/platform/graphics/filters/Filter.h:52: FloatRect
mapLocalRectToAbsoluteRect(const FloatRect& rect) const
On 2015/09/14 18:49:34, Stephen White wrote:
> Nit: maybe we should just de-inline these functions. I strongly doubt they're
> showing up as hot anywhere.

Done.

https://codereview.chromium.org/1326183002/diff/1/Source/platform/graphics/fi...
Source/platform/graphics/filters/Filter.h:85: FloatRect m_referenceBox;
On 2015/09/14 18:49:34, Stephen White wrote:
> Could you rationalize the namechange here? Is it because it's not strictly a
> bounding box (in the userSpaceOnUse case?)

I think the origin of the term might be:
https://drafts.csswg.org/css-transforms-1/#transform-box (and the term
"reference box" has been popping up in various specs, although it doesn't
actually seem to have made it to the Filter Effects spec AFAICS). As far as I'm
concerned it's more about having a more neutral term between SVG and HTML ("SVG
layout box" vs. "CSS layout box"). For uSOU I suppose it could weigh in for
HTML, which doesn't have a (well-defined) CTM concept.

If you prefer the old naming I can revert it back to that. I've mostly been
trying to replace/generalize usages of objectBoundingBox where uses could
generalize to both SVG and HTML in the future.

[Stephen White, 2015-09-15 14:25:35]

LGTM

https://codereview.chromium.org/1326183002/diff/1/Source/platform/graphics/fi...
File Source/platform/graphics/filters/Filter.h (right):

https://codereview.chromium.org/1326183002/diff/1/Source/platform/graphics/fi...
Source/platform/graphics/filters/Filter.h:85: FloatRect m_referenceBox;
On 2015/09/15 10:42:56, fs wrote:
> On 2015/09/14 18:49:34, Stephen White wrote:
> > Could you rationalize the namechange here? Is it because it's not strictly a
> > bounding box (in the userSpaceOnUse case?)
> 
> I think the origin of the term might be:
> https://drafts.csswg.org/css-transforms-1/#transform-box (and the term
> "reference box" has been popping up in various specs, although it doesn't
> actually seem to have made it to the Filter Effects spec AFAICS). As far as
I'm
> concerned it's more about having a more neutral term between SVG and HTML
("SVG
> layout box" vs. "CSS layout box"). For uSOU I suppose it could weigh in for
> HTML, which doesn't have a (well-defined) CTM concept.
> 
> If you prefer the old naming I can revert it back to that. I've mostly been
> trying to replace/generalize usages of objectBoundingBox where uses could
> generalize to both SVG and HTML in the future.

OK, that makes sense. Thanks for the pointer.

[f(malita), 2015-09-15 14:30:47]

LGTM 2

[f(malita), 2015-09-15 14:30:52]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2015-09-15 14:31:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1326183002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1326183002/20001

[commit-bot: I haz the power, 2015-09-15 15:22:49]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202273

[commit-bot: I haz the power, 2015-09-23 12:45:06]

Patchset 2 (id:??) landed as
https://crrev.com/e57a376a142ca72f6adeeac938bd4287fd54934c