[webkit-reviews] review denied: [Bug 78385] REGRESSION(r105057): Dynamically changing <tspan> offsets is broken : [Attachment 126611] patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Tim Horton
<timothy_horton at apple.com>'s request for review:
Bug 78385: REGRESSION(r105057): Dynamically changing <tspan> offsets is broken
https://bugs.webkit.org/show_bug.cgi?id=78385

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

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


Thanks Tim for your quick investigation! That saved me a lot time, as I could
easily grasp the problem:

> LayoutTests/svg/text/tspan-dynamic-positioning.svg:1
> +<svg onload="load()" xmlns="http://www.w3.org/2000/svg" version="1.1"
width="600" height="500">

Pixel tests should use layoutTestController.display() now, see eg. bug 77736.
Please import fast/repaint/resources/repaint.js, and attach <svg
onload="runRepaintTest()".

> LayoutTests/svg/text/tspan-dynamic-positioning.svg:5
> +	   function move()

s/move/repaintTest/ - which gets invoked by runRepaintTest, once if finished
layout and an initial paint, which is what you want here.

> LayoutTests/svg/text/tspan-dynamic-positioning.svg:11
> +	       if (window.layoutTestController)
> +		   layoutTestController.notifyDone();

This is then unnecessary.

> LayoutTests/svg/text/tspan-dynamic-positioning.svg:19
> +	   function load()
> +	   {
> +	       if (window.layoutTestController)
> +		   layoutTestController.waitUntilDone();
> +	       setTimeout(move, 0);
> +	   }

Ditto, can be removed as well - also it gives you the nice gray overlay rect,
so we can really be sure that repainting worked :-)
Please always use this for future tests, and tell anyone to do so.

> Source/WebCore/ChangeLog:13
> +	   Test: svg/text/tspan-dynamic-positioning.svg.

Trailing period.

> Source/WebCore/rendering/svg/RenderSVGText.cpp:220
> +	   m_layoutAttributesBuilder.clearTextPositioningElements();

While this works for sure, it would regress performance. The text positioning
elements cache should only ever change when textDOMChanged() gets called.
We only need to invalidate the text positioning elements list, if a) the text
length of any RenderSVGInlineText changes b) the <text> DOM subtree mutates in
some way. This is already captured correctly, and we have tests covering that.

When reading through the code, I just realized that
buildLayoutAttributesIfNeeded is wrong. It shouldn't early exit if
m_textPositions is not empty, we still need to call buildLayoutAttributes!
The only thing we can save is the collectTextPositioningElements and thus a
full walk of the SVG <text> subtree - - we just forgot to layout in that case.

A shame, we didn't have a test yet covering just that.