Move ReferenceFilterBuilder::build to FilterEffectBuilder

Move ReferenceFilterBuilder::build to FilterEffectBuilder

This function resolves a filter reference and then builds a filter
(sub)DAG for painting. Split it into the two fairly distinct parts,
leaving ReferenceFilterBuilder as a class that only does element
lookup/resolution, while the DAG-building takes place in
FilterEffectBuilder (with a little help from SVGFilterBuilder.)
Use the new function in SVGFilterPainter.

Also pass Element& to ReferenceFilterBuilder::build, and remove a
redundant null-check of ComputedStyle in
PaintLayer::updateOrRemoveFilterEffectBuilder (already checked by
paintsWithFilters and assumed later in the function.)

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

[fs, 2016-06-08 10:51:35]

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

[fs, 2016-06-08 10:51:36]

https://codereview.chromium.org/2044153002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/FilterEffectBuilder.cpp (right):

https://codereview.chromium.org/2044153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/FilterEffectBuilder.cpp:309: if (nodeMap &&
filterRegion.isEmpty())
This is a bit ugly, and could also be achieved using a dedicated flag of some
sort. Nonetheless it's a workaround until we've fixed the bug mentioned.

[Stephen White, 2016-06-08 13:53:45]

LGTM

I don't *think* it will be affected, but one case to keep an eye on is
filter_terrain (in smoothness.tough_filters_cases), which exercises the caching
of partially-invalidated filter DAGs.

https://codereview.chromium.org/2044153002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2044153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:2736: const ComputedStyle&
style = layoutObject()->styleRef();
<rambling> I could be misremembering, but IIRC this didn't used to work with
filter nodes loaded in external documents. But maybe it does now. At any rate,
it's a reference so it's non-null, even if the style isn't up-to-date.
</rambling>

https://codereview.chromium.org/2044153002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/SVGFilterPainter.cpp (left):

https://codereview.chromium.org/2044153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGFilterPainter.cpp:111: SVGFilterBuilder
builder(filterData->filter->getSourceGraphic(), filterData->nodeMap.get());
Does this mean SVGFilterBuilder can be removed? Or is it still used elsewhere?

[fs, 2016-06-08 14:08:42]

On 2016/06/08 at 13:53:45, senorblanco wrote:
> LGTM
> 
> I don't *think* it will be affected, but one case to keep an eye on is
filter_terrain (in smoothness.tough_filters_cases), which exercises the caching
of partially-invalidated filter DAGs.

The cache will still remain. Although optional, it's still provided for the same
code-path as before (SVGFilterPainter). Thanks for the heads-up though still.

https://codereview.chromium.org/2044153002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2044153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:2736: const ComputedStyle&
style = layoutObject()->styleRef();
On 2016/06/08 at 13:53:45, Stephen White wrote:
> <rambling> I could be misremembering, but IIRC this didn't used to work with
filter nodes loaded in external documents. But maybe it does now. At any rate,
it's a reference so it's non-null, even if the style isn't up-to-date.
</rambling>

This is the style of whatever LayoutBox the PaintLayer is associated with, so
it's not related to the actual filter node (if any). At this point we'll have
indirectly null-checked it regardless (via paintsWithFilter IIRC.) Also, the
window where layoutObject() is non-null but style() isn't is not one where we'll
be calling this method.

https://codereview.chromium.org/2044153002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/SVGFilterPainter.cpp (left):

https://codereview.chromium.org/2044153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGFilterPainter.cpp:111: SVGFilterBuilder
builder(filterData->filter->getSourceGraphic(), filterData->nodeMap.get());
On 2016/06/08 at 13:53:45, Stephen White wrote:
> Does this mean SVGFilterBuilder can be removed? Or is it still used elsewhere?

It's still used by the new method in FilterEffectBuilder. There should only be
one user (outside layout-tree dumping) though, so maybe it can eventually be
folded.

[pdr., 2016-06-08 23:12:51]

LGTM with or without my suggestion.

https://codereview.chromium.org/2044153002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/FilterEffectBuilder.h (right):

https://codereview.chromium.org/2044153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/FilterEffectBuilder.h:54: static Filter*
buildReferenceFilter(SVGFilterElement&, const FloatRect& referenceBox, const
SkPaint* fillPaint, const SkPaint* strokePaint, FilterEffect* previousEffect,
float zoom, SVGFilterGraphNodeMap* = nullptr);
The first buildReferenceFilter wants a zoomed referenceBoxSize whereas the
second wants referenceBox without zoom. WDYT about renaming referenceBoxSize ->
zoomedReferenceBoxSize or refactoring the callers to remove zoom?

[fs, 2016-06-09 08:33:07]

https://codereview.chromium.org/2044153002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/FilterEffectBuilder.h (right):

https://codereview.chromium.org/2044153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/FilterEffectBuilder.h:54: static Filter*
buildReferenceFilter(SVGFilterElement&, const FloatRect& referenceBox, const
SkPaint* fillPaint, const SkPaint* strokePaint, FilterEffect* previousEffect,
float zoom, SVGFilterGraphNodeMap* = nullptr);
On 2016/06/08 at 23:12:50, pdr. wrote:
> The first buildReferenceFilter wants a zoomed referenceBoxSize whereas the
second wants referenceBox without zoom. WDYT about renaming referenceBoxSize ->
zoomedReferenceBoxSize or refactoring the callers to remove zoom?

Yes, I think renaming is reasonable. In time I think we might be able to hoist
the computation of the reference box, but not sure if we'll be able to get rid
of the zoom...

So, renamed to zoomedReferenceBoxSize where applicable.

[fs, 2016-06-09 13:27:51]

The CQ bit was checked by fs@opera.com

[fs, 2016-06-09 13:27:52]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
senorblanco@chromium.org
Link to the patchset: https://codereview.chromium.org/2044153002/#ps40001
(title: "zoomedReferenceBoxSize")

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2044153002/40001

[commit-bot: I haz the power, 2016-06-09 13:59:49]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-09 14:01:08]

Description was changed from

==========
Move ReferenceFilterBuilder::build to FilterEffectBuilder

This function resolves a filter reference and then builds a filter
(sub)DAG for painting. Split it into the two fairly distinct parts,
leaving ReferenceFilterBuilder as a class that only does element
lookup/resolution, while the DAG-building takes place in
FilterEffectBuilder (with a little help from SVGFilterBuilder.)
Use the new function in SVGFilterPainter.

Also pass Element& to ReferenceFilterBuilder::build, and remove a
redundant null-check of ComputedStyle in
PaintLayer::updateOrRemoveFilterEffectBuilder (already checked by
paintsWithFilters and assumed later in the function.)

BUG=109224,533457
==========

to

==========
Move ReferenceFilterBuilder::build to FilterEffectBuilder

This function resolves a filter reference and then builds a filter
(sub)DAG for painting. Split it into the two fairly distinct parts,
leaving ReferenceFilterBuilder as a class that only does element
lookup/resolution, while the DAG-building takes place in
FilterEffectBuilder (with a little help from SVGFilterBuilder.)
Use the new function in SVGFilterPainter.

Also pass Element& to ReferenceFilterBuilder::build, and remove a
redundant null-check of ComputedStyle in
PaintLayer::updateOrRemoveFilterEffectBuilder (already checked by
paintsWithFilters and assumed later in the function.)

BUG=109224,533457

Committed: https://crrev.com/20a5c045d0eb975be198b14204ce5eb6a5ab60e2
Cr-Commit-Position: refs/heads/master@{#398866}
==========

[commit-bot: I haz the power, 2016-06-09 14:01:10]

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