Bug 130879 – Clear SVGInlineTextBox fragments when the text changes.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Clear SVGInlineTextBox fragments when the text changes.

[Myles C. Maxfield]

Created attachment 228012 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/15695951>

[Darin Adler]

Comment on attachment 228012 [details]
Patch

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.

[Myles C. Maxfield]

Comment on attachment 228012 [details]
Patch

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.

It is. I'll update this comment.

>> 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.

Without the patch, this line doesn't get shown at all on my mac. Instead, garbage characters are shown instead. A dump-as-text test should be reasonable in this case.

[Myles C. Maxfield]

Comment on attachment 228012 [details]
Patch

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.
> 
> It is. I'll update this comment.

Done.

>> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:70
>> +    if (nextBox)
> 
> I’d suggest putting the variable definition inside the if statement.

Done.

>> 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?

Done.

>> LayoutTests/ChangeLog:16
>> +        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.

Done.

[Myles C. Maxfield]

http://trac.webkit.org/changeset/166420