Split SVGFilterbuilder into "builder" and "node map" parts

Split SVGFilterbuilder into "builder" and "node map" parts

This separates the longer lived state (the LayoutObject->FilterEffect
and FilterEffect dependents maps) from the state that's only used/valid
while building a (sub)filter-graph. The former is moved to the new
class SVGFilterGraphNodeMap while the latter remain in
SVGFilterBuilder. SVGFilterBuilder can thus be converted to something
that is allocated on the stack and only kept for the building
operation. FilterData is changed to the keep a SVGFilterGraphNodeMap
instead of a builder.
The graph-building code in ReferenceFilterBuilder::build and
LayoutSVGResourceFilter::buildPrimitives is consolidated into
SVGFilterBuilder::buildGraph, with the more "relaxed" behavior of the
former kept. This should only result in a change in behavior for the
case where externally referenced filters are used.

BUG=109224, 533457
Committed: https://crrev.com/e795c33a0120bf58d20fc324111c9108b3fc815e
Cr-Commit-Position: refs/heads/master@{#353125}

[fs, 2015-10-02 14:17:16]

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

[fs, 2015-10-02 14:17:17]

The image might be more complete with the following in mind:

https://codereview.chromium.org/1378163003
https://codereview.chromium.org/1383013002

(The former could be improved upon a bit, but that's probably easier to do with
this landed first.)

[Stephen White, 2015-10-02 14:56:07]

This seems like a great direction. I'd like to take a more detailed look,
but I probably won't be able to before Tuesday next week. If you don't
hear from me by Wednesday, ping me again.

[fs, 2015-10-02 14:57:17]

On 2015/10/02 at 14:56:07, senorblanco wrote:
> This seems like a great direction. I'd like to take a more detailed look,
> but I probably won't be able to before Tuesday next week. If you don't
> hear from me by Wednesday, ping me again.

Sure thing, will do. Thanks for the heads-up!

[fs, 2015-10-07 15:59:26]

On 2015/10/02 at 14:57:17, fs wrote:
> On 2015/10/02 at 14:56:07, senorblanco wrote:
> > This seems like a great direction. I'd like to take a more detailed look,
> > but I probably won't be able to before Tuesday next week. If you don't
> > hear from me by Wednesday, ping me again.

Now it's "by Wednesday" - at least in my time zone! =P

[Stephen White, 2015-10-07 19:44:01]

A number of css3/filters tests are currently marked for rebaseline (and the ARB
seems to be in a coma). Could you run them locally just to verify correctness?
(esp. for the reference filters with external files.) Thanks!

https://codereview.chromium.org/1382163003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/graphics/filters/SVGFilterBuilder.cpp
(right):

https://codereview.chromium.org/1382163003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/filters/SVGFilterBuilder.cpp:144:
m_filter->setLastEffect(m_lastEffect);
It seems a shame to add the Filter as a member of SVGFilterBuilder, if its only
purpose is to call setLastEffect() on it. Could we do this outside the builder?

https://codereview.chromium.org/1382163003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/graphics/filters/SVGFilterBuilder.h
(right):

https://codereview.chromium.org/1382163003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/filters/SVGFilterBuilder.h:50: void
addPrimitive(PassRefPtrWillBeRawPtr<FilterEffect>, LayoutObject*);
Nit: while we're here, could we switch the order of these args, so it's key,
value?

[fs, 2015-10-08 10:53:12]

On 2015/10/07 at 19:44:01, senorblanco wrote:
> A number of css3/filters tests are currently marked for rebaseline (and the
ARB seems to be in a coma). Could you run them locally just to verify
correctness? (esp. for the reference filters with external files.) Thanks!

I enabled the NeedsManualRebaseline css3/filters/* tests (and a few more of
them) and they all passed when testing locally.

https://codereview.chromium.org/1382163003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/graphics/filters/SVGFilterBuilder.cpp
(right):

https://codereview.chromium.org/1382163003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/filters/SVGFilterBuilder.cpp:144:
m_filter->setLastEffect(m_lastEffect);
On 2015/10/07 at 19:44:01, Stephen White wrote:
> It seems a shame to add the Filter as a member of SVGFilterBuilder, if its
only purpose is to call setLastEffect() on it. Could we do this outside the
builder?

We still need it so that we can pass it to SVGFE*Element::build(...) though. I
change it to be an argument of buildGraph instead, sinking the setLastEffect
call out into callers. Better? (For disclosure, I was going to just drop the
Filter* to SVGFE*Element::build(...) and let them read it from the passed
builder instead - I do agree it's hanging pretty loose though...)

https://codereview.chromium.org/1382163003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/graphics/filters/SVGFilterBuilder.h
(right):

https://codereview.chromium.org/1382163003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/filters/SVGFilterBuilder.h:50: void
addPrimitive(PassRefPtrWillBeRawPtr<FilterEffect>, LayoutObject*);
On 2015/10/07 at 19:44:01, Stephen White wrote:
> Nit: while we're here, could we switch the order of these args, so it's key,
value?

Done.

[Stephen White, 2015-10-08 18:48:03]

LGTM

https://codereview.chromium.org/1382163003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/graphics/filters/SVGFilterBuilder.cpp
(right):

https://codereview.chromium.org/1382163003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/filters/SVGFilterBuilder.cpp:144:
m_filter->setLastEffect(m_lastEffect);
On 2015/10/08 10:53:11, fs wrote:
> On 2015/10/07 at 19:44:01, Stephen White wrote:
> > It seems a shame to add the Filter as a member of SVGFilterBuilder, if its
> only purpose is to call setLastEffect() on it. Could we do this outside the
> builder?
> 
> We still need it so that we can pass it to SVGFE*Element::build(...) though. I
> change it to be an argument of buildGraph instead, sinking the setLastEffect
> call out into callers. Better? (For disclosure, I was going to just drop the
> Filter* to SVGFE*Element::build(...) and let them read it from the passed
> builder instead - I do agree it's hanging pretty loose though...)

Ah yes, I missed that. Definitely better as an arg.

[fs, 2015-10-08 19:27:19]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2015-10-08 19:29:42]

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

[commit-bot: I haz the power, 2015-10-08 20:21:43]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-08 20:22:47]

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