[Slimming Paint] Implement deferred SVG filters

[Slimming Paint] Implement deferred SVG filters

This patch implements filters in slimming paint. The general approach is
to create a new graphics context in the filter and use that for painting
the contents of the filter. This solves the issue of nested drawing
recorders.

With this patch we pass 64 additional tests in svg/filters and the
remaining 30 failures have been skipped in the new virtual test
suite (these failures are primarily feImage).

BUG=451606
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189079

[pdr., 2015-01-23 23:51:12]

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

[pdr., 2015-01-23 23:51:26]

It lives!

[chrishtr, 2015-01-24 00:02:02]

fs should look at this CL also, I'm not 100% familiar with all of its
subtleties.

https://codereview.chromium.org/871983003/diff/60001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourceFilter.cpp (right):

https://codereview.chromium.org/871983003/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceFilter.cpp:126: if
(context->displayItemList())
if (RuntimeEnabledFeatures::slimmingPaintEnabled()) {
  ASSERT(context->displayItemList());
  context->displayItemList()->replay(context);
}

https://codereview.chromium.org/871983003/diff/60001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourceFilter.h (right):

https://codereview.chromium.org/871983003/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceFilter.h:77: void
finishEffect(RenderObject*, GraphicsContext*, GraphicsContext*);
Add names to these two graphics contexts, way too confusing otherwise. Is there
a way to avoid passing two contexts to the same method?

https://codereview.chromium.org/871983003/diff/60001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGRenderingContext.cpp (right):

https://codereview.chromium.org/871983003/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGRenderingContext.cpp:165: GraphicsContext*
filterContents = m_filterContentGraphicsContext ?
m_filterContentGraphicsContext.get() : m_paintInfo.context;
Declare filterContents before line 158 and init to m_paintInfo.context, then
override it after line 160. This avoids two conditionals that always trigger on
the same conditions.

Also, do you mean filterContext, not filterContents?

https://codereview.chromium.org/871983003/diff/60001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGRenderingContext.h (right):

https://codereview.chromium.org/871983003/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGRenderingContext.h:97: OwnPtr<DisplayItemList>
m_filterContentDisplayItemList;
Switch this order, so that we are not in a state with a stale pointer in the
destructor of GraphicsContext.

[pdr., 2015-01-24 01:13:36]

TY PTAL

https://codereview.chromium.org/871983003/diff/60001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourceFilter.cpp (right):

https://codereview.chromium.org/871983003/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceFilter.cpp:126: if
(context->displayItemList())
On 2015/01/24 at 00:02:02, chrishtr wrote:
> if (RuntimeEnabledFeatures::slimmingPaintEnabled()) {
>   ASSERT(context->displayItemList());
>   context->displayItemList()->replay(context);
> }

Done.

https://codereview.chromium.org/871983003/diff/60001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourceFilter.h (right):

https://codereview.chromium.org/871983003/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceFilter.h:77: void
finishEffect(RenderObject*, GraphicsContext*, GraphicsContext*);
On 2015/01/24 at 00:02:02, chrishtr wrote:
> Add names to these two graphics contexts, way too confusing otherwise. Is
there a way to avoid passing two contexts to the same method?

Done: used "primaryContext" and "contentContext".

https://codereview.chromium.org/871983003/diff/60001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGRenderingContext.cpp (right):

https://codereview.chromium.org/871983003/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGRenderingContext.cpp:165: GraphicsContext*
filterContents = m_filterContentGraphicsContext ?
m_filterContentGraphicsContext.get() : m_paintInfo.context;
On 2015/01/24 at 00:02:02, chrishtr wrote:
> Declare filterContents before line 158 and init to m_paintInfo.context, then
override it after line 160. This avoids two conditionals that always trigger on
the same conditions.
> 
> Also, do you mean filterContext, not filterContents?

Done. Refactored this even further and removed the filterConte(nt/xt).

https://codereview.chromium.org/871983003/diff/60001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGRenderingContext.h (right):

https://codereview.chromium.org/871983003/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGRenderingContext.h:97: OwnPtr<DisplayItemList>
m_filterContentDisplayItemList;
On 2015/01/24 at 00:02:02, chrishtr wrote:
> Switch this order, so that we are not in a state with a stale pointer in the
destructor of GraphicsContext.

Done

[chrishtr, 2015-01-24 01:20:22]

https://codereview.chromium.org/871983003/diff/30007/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourceFilter.h (right):

https://codereview.chromium.org/871983003/diff/30007/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceFilter.h:77: void
finishEffect(RenderObject*, GraphicsContext* /* primaryContext */,
GraphicsContext* /* contentContext */);
I don't think this explains that well to the caller what to pass.

[chrishtr, 2015-01-24 01:20:25]

lgtm

[pdr., 2015-01-24 04:22:22]

On 2015/01/24 at 01:20:22, chrishtr wrote:
>
https://codereview.chromium.org/871983003/diff/30007/Source/core/rendering/sv...
> File Source/core/rendering/svg/RenderSVGResourceFilter.h (right):
> 
>
https://codereview.chromium.org/871983003/diff/30007/Source/core/rendering/sv...
> Source/core/rendering/svg/RenderSVGResourceFilter.h:77: void
finishEffect(RenderObject*, GraphicsContext* /* primaryContext */,
GraphicsContext* /* contentContext */);
> I don't think this explains that well to the caller what to pass.

Fixed. I took a slightly different approach of just keeping the new graphics
context in the filter instead of in the rendering context. I think this is
cleaner overall and means we don't have any functions that take two graphics
contexts.

[fs, 2015-01-26 09:47:11]

lgtm

https://codereview.chromium.org/871983003/diff/150001/Source/core/rendering/s...
File Source/core/rendering/svg/SVGRenderingContext.cpp (right):

https://codereview.chromium.org/871983003/diff/150001/Source/core/rendering/s...
Source/core/rendering/svg/SVGRenderingContext.cpp:52: m_paintInfo.rect =
m_originalPaintInfo->rect;
m_paintInfo.rect is "dead" at this point (not used below this point, and scopes
out at the end of this function.) Might be worth adding a note/assert/something
to that effect?

(With this line of reasoning I suppose m_originalPaintInfo->context could be
passed to the other two finishEffect()s as well... since m_paintInfo.context ===
m_originalPaintInfo->context after this point. A think a note will do fine
though at this point.)

[pdr., 2015-01-28 01:04:39]

https://codereview.chromium.org/871983003/diff/150001/Source/core/rendering/s...
File Source/core/rendering/svg/SVGRenderingContext.cpp (right):

https://codereview.chromium.org/871983003/diff/150001/Source/core/rendering/s...
Source/core/rendering/svg/SVGRenderingContext.cpp:52: m_paintInfo.rect =
m_originalPaintInfo->rect;
On 2015/01/26 at 09:47:11, fs wrote:
> m_paintInfo.rect is "dead" at this point (not used below this point, and
scopes out at the end of this function.) Might be worth adding a
note/assert/something to that effect?
> 
> (With this line of reasoning I suppose m_originalPaintInfo->context could be
passed to the other two finishEffect()s as well... since m_paintInfo.context ===
m_originalPaintInfo->context after this point. A think a note will do fine
though at this point.)

Yeah, I thought it would be clean to fully clean up even though it isn't used
today (or, likely ever). I'll add a note.

[pdr., 2015-01-28 01:29:47]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2015-01-28 01:30:03]

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

[commit-bot: I haz the power, 2015-01-28 02:48:40]

Committed patchset #10 (id:170001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189079