[webkit-reviews] review granted: [Bug 99243] FEImage::m_document is never cleared. Why not? : [Attachment 171478] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has granted Stephen Chenney
<schenney at chromium.org>'s request for review:
Bug 99243: FEImage::m_document is never cleared. Why not?
https://bugs.webkit.org/show_bug.cgi?id=99243

Attachment 171478: Patch
https://bugs.webkit.org/attachment.cgi?id=171478&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=171478&action=review


> Source/WebCore/ChangeLog:8
> +	   Adding a comment to explain why the failure to clear m_document is
not a problem.

Please say that this will be refactored with another patch.

>>> Source/WebCore/svg/graphics/filters/SVGFEImage.h:58
>>> +	 // m_document will never be a dangling reference. See
https://bugs.webkit.org/show_bug.cgi?id=99243
>> 
>> Better justification would be to explain why here as well.  This is not an
Element, so it doesn't keep the document alive.  I assume this is held off of a
renderer?  Can this also be held off of a CSSValue?
> 
> This object is only held by a SVGFilterBuilder which in turn is held by a
RenderSVGResourceFilter::FilterData object, which in turn is held by a
RenderSVGResourceFilter object which is a renderer. Nothing else at all holds
references to FEImage. Any changes to CSS properties (including SVG attributes)
that affect the filter cause the FilterData and hence FEImage object to be
destroyed and recreated. CSS Filters cannot use SVGFEImage.
> 
> The only way that FEImage::m_document is used is to look up the renderer for
the image it is drawing. It will only do that during applyResource, which is
only called when the target for the filter (the thing that will draw it) is
painted, which only happens if the target is in the document. If the target is
in the document then FEImage::m_document is valid. If the target is not in the
document then it both cannot be painted and there is no FEImage object to have
a dangling reference.

Please add a link to this bug report, and open the bug report after landing
again. Can be dependent on your refactoring patch later. After refactoring you
just close it.