[webkit-reviews] review denied: [Bug 63076] Assertion failure in RenderSVGInlineText::characterStartsNewTextChunk : [Attachment 98006] Proposed fix: Notify RenderSVGInlineText to invalidate RenderSVGText once it is attached.

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 63076: Assertion failure in
RenderSVGInlineText::characterStartsNewTextChunk
https://bugs.webkit.org/show_bug.cgi?id=63076

Attachment 98006: Proposed fix: Notify RenderSVGInlineText to invalidate
RenderSVGText once it is attached.
https://bugs.webkit.org/attachment.cgi?id=98006&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=98006&action=review

Great job Julien, still r- because the test looks weird:

> LayoutTests/svg/custom/crash-text-in-textpath.xhtml:5
> +		  
window.document.childNodes[0].childNodes[1].childNodes[1].scrollIntoView('foo')


This reads very confusing to me.
Why does this need to be a html test?

> LayoutTests/svg/custom/crash-text-in-textpath.xhtml:16
> +		       <set onload="main()" ></set>
> +		       foo    
> +		       <script xlink:href="#bar"></script>

Why is a set with onload handler, text and a _script_ needed here? Can't we
reduce this as much as possible?

> LayoutTests/svg/custom/crash-text-in-textpath.xhtml:20
> +	       <text x="10px" y="50px">Test for bug <a
href="https://bugs.webkit.org/show_bug.cgi?id=63076">63076</a>: Assertion
failure in RenderSVGInlineText::characterStartsNewTextChunk</text>
> +	       <text x="10px" y="100px"> This test passes if it does not crash
</text>

You can omit the 'px'.

> LayoutTests/svg/custom/crash-text-in-textpath.xhtml:24
> +	       <script type="text/ecmascript"><![CDATA[ 
> +		   if (window.layoutTestController)
> +		       layoutTestController.dumpAsText();
> +	       ]]></script>

The CDATA should be omitted as well.

> Source/WebCore/rendering/svg/RenderSVGInlineText.cpp:235
> +{
> +    // We don't need to handle removed from RenderTree as destroy() will be
called in this case.
> +    if (RenderSVGText* textRenderer =
RenderSVGText::locateRenderSVGTextAncestor(this))
> +	   textRenderer->setNeedsPositioningValuesUpdate();

"We don't need to handle removedFromRenderTree as destroy() will be called in
this case." ?