[webkit-reviews] review denied: [Bug 41467] SVG Text assertion on SVGInlineTextBox : [Attachment 70095] Adding layout test for checking SVG Text assertion

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Renata Hodovan
<reni at inf.u-szeged.hu>'s request for review:
Bug 41467: SVG Text assertion on SVGInlineTextBox
https://bugs.webkit.org/show_bug.cgi?id=41467

Attachment 70095: Adding layout test for checking SVG Text assertion
https://bugs.webkit.org/attachment.cgi?id=70095&action=review

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

> LayoutTests/ChangeLog:8
> +	   Adding layout test for checking SVG Text assertion.

Example: <text><tspan>S</tspan><tspan>O</tspan></text>
The old SVG text engine fired an assertion, when the glyph 'O' isn't present in
the <font>, but only if a <missing-glyph> element is included in the SVG
<font>.
<tspan>SO</tspan> didn't trigger that assertion. Adding a new layout test, to
make sure we never see the assertion again.

> LayoutTests/svg/text/text-assert.svg:8
> +    <font>
> +	<font-face font-family="Arial"/>
> +	<missing-glyph/>
> +	<glyph horiz-adv-x="667" unicode="S" d="M40 230z"/>
> +    </font>

Can you reindent here, with 4 spaces?

> LayoutTests/svg/text/text-assert.svg:11
> +<!-- Assertion triggers only if <missing-glyph> element above is included
and only if a nonexistant glyph is referenced in a _second_ tspan. Moving the
'o' next to the 'S' just works. Weird.-->

I'd change the comment, as it no longer applies:
<!-- The old SVG text engine fired an assertion, if the <missing-glyph> element
is included in the <font> and if
     a nonexistant glyph is referenced in a different tspan. Moving the 'o'
next to the 'S' doesn't trigger it. -->

> LayoutTests/svg/text/text-assert.svg:12
> +<text style="font-family: Helvetica" y="158"
x="311.00006"><tspan>S</tspan><tspan>o</tspan></text>

Just use x="10" y="20".

> LayoutTests/svg/text/text-assert.svg:14
> +<text y="30">If you see "So" the test passed.</text>

y="50" here.

Hopefully you can commit soon, for now you need to upload a new version, sorry
:(