[webkit-reviews] review granted: [Bug 130879] Clear SVGInlineTextBox fragments when the text changes. : [Attachment 228012] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 130879: Clear SVGInlineTextBox fragments when the text changes.
https://bugs.webkit.org/show_bug.cgi?id=130879

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

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


> Source/WebCore/ChangeLog:16
> +	   Also cleans up virtual, OVERRIDE and FINAL for SVGRootInlineBox,
which was all messed up.

I don’t understand this comment. The patch adds a few final. but I don’t see
any cleanup of virtual or override.

Maybe this is just a comment from Blink that makes no sense here.

> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:70
> +    InlineTextBox* nextBox = nextTextBox();
> +    if (nextBox)

I’d suggest putting the variable definition inside the if statement.

> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:71
> +	   nextBox->dirtyLineBoxes();

I’m slightly concerned that this is a recursive algorithm, which might end up
using stack proportional to the number of successive SVGInlineTextBox objects.
Is there a clean way to do this in an iterative fashion rather than recursive?
Maybe the compiler’s tail call optimization will take care of it?

> LayoutTests/ChangeLog:16
> +	   This patch modifies SVGInlineTextBox::dirtyLineBoxes to clear all
> +	   following text boxes when invoked. Typically this method is called
> +	   when the underlying text string changes, and that change needs to
> +	   be propagated to all the boxes that use the text beyond the point
> +	   where the text is first modified.
> +	   
> +	   Also cleans up virtual, OVERRIDE and FINAL for SVGRootInlineBox,
which was all messed up.

We should not be repeating these comments here. The comments here should just
talk about the tests.

> LayoutTests/svg/custom/unicode-in-tspan-multi-svg-crash.html:36
> +    <p>Test Passes if there is no crash in Debug or Asan builds. There
should be no characters preceding "Test".</p>

Seems pretty weak to have a test where passing is “not crashing”. It seems to
me that we could come up with a test that shows checks for correct behavior,
maybe a reference test.

But I guess it’s efficient to just use the test the Blink contributor provided
with his patch, even though it has serious shortcomings.