[webkit-reviews] review granted: [Bug 83405] REGRESSION(r105057): Infinite loop inside SVGTextLayoutEngine::currentLogicalCharacterMetrics : [Attachment 141986] Patch v5

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 83405: REGRESSION(r105057): Infinite loop inside
SVGTextLayoutEngine::currentLogicalCharacterMetrics
https://bugs.webkit.org/show_bug.cgi?id=83405

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=141986&action=review


A lot to review here, but it all looks good to me. r=me

> Source/WebCore/rendering/svg/RenderSVGInline.cpp:136
> +    RenderSVGText* textRenderer =
RenderSVGText::locateRenderSVGTextAncestor(this);
> +    if (!child->isSVGInlineText() || !textRenderer) {
> +	   RenderInline::removeChild(child);
> +	   return;
> +    }

Seems a shame to call RenderSVGText::locateRenderSVGTextAncestor when
!child->isSVGInlineText(). I suggest restructuring the code so we don’t do
that. One way is to write this:

    RenderSVGText* textRenderer = child->isSVGInlineText() ?
RenderSVGText::locateRenderSVGTextAncestor(this) : 0;
    if (!textRenderer)

But there may also be a clean way to write it.

> Source/WebCore/rendering/svg/RenderSVGInline.h:62
> +    virtual void removeChild(RenderObject*);

Please add OVERRIDE.

> Source/WebCore/rendering/svg/RenderSVGText.cpp:184
> +    if (newLayoutAttributes.isEmpty()) {
> +	   m_layoutAttributes.clear();
> +	   return;
> +    }

This special case does not seem more optimal than falling into the general case
below. I suggest leaving it out unless there is something I am missing.

>> Source/WebCore/rendering/svg/RenderSVGText.h:80
>> +	virtual void removeChild(RenderObject*);
> 
> I think this needs OVERRIDE? Please check others as well.

Please add OVERRIDE.

> Source/WebCore/rendering/svg/RenderSVGText.h:81
> +    virtual void willBeDestroyed();

Please add OVERRIDE.

>> Source/WebCore/rendering/svg/RenderSVGText.h:93
>> +	bool shallHandleSubtreeMutations() const;
> 
> shall sounds a bit strange. How about canHandleSubtreeMutations()?

We normally use “should” for functions like this, and I think it would work
fine here.