Bug 57831 – text-tspan-02-b.svg from SVG 1.1 2nd edition fails

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

text-tspan-02-b.svg from SVG 1.1 2nd edition fails.
We fail to correctly span the rotate attribute across text children.

[Nikolas Zimmermann]

Created attachment 88208 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 88208 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88208&action=review

> Source/WebCore/ChangeLog:26
> +        (WebCore::SVGTextLayoutAttributes::reserveCapacity): Also reserveCapacity for the text metrics list (minor optimiyation).

Typo: optimyation

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1997
> +                if (isSVGText && pos > 0 && !atStart) {

I don't think you mentioned the !atStart part of this change in your ChangeLog.

> Source/WebCore/rendering/svg/SVGTextLayoutAttributes.cpp:53
> +    // Doesn't touch m_textMetricsValues on purpose.

Instead of just "on purpose", maybe you can explain why we don't touch it. (Ditto for other similar functions in this patch.)

> Source/WebCore/rendering/svg/SVGTextLayoutAttributes.cpp:68
> +    if (position >= values.size())

When do we expect this to happen?

> Source/WebCore/rendering/svg/SVGTextLayoutAttributes.h:40
>      void reserveCapacity(unsigned length);
> +    void fillWithEmptyValue(unsigned length);
> +
> +    void appendEmptyValue();
> +    void appendSingleValueFromAttribute(const SVGTextLayoutAttributes&, unsigned position);

The length and position arguments should really be of type size_t.

> Source/WebCore/rendering/svg/SVGTextLayoutAttributesBuilder.cpp:2
> - * Copyright (C) Research In Motion Limited 2010. All rights reserved.
> + * Copyright (C) Research In Motion Limited 2010-2011. All rights reserved.

I think we normally prefer comma-separated years.

> Source/WebCore/rendering/svg/SVGTextLayoutAttributesBuilder.cpp:126
> +    if (start->isSVGText()) 
> +        ASSERT(m_textPositions.isEmpty());

You could turn this into just an assertion: ASSERT(!start->isSVGText() || m_textPositions.isEmpty());

> Source/WebCore/rendering/svg/SVGTextLayoutAttributesBuilder.cpp:138
> +        unsigned atPosition = m_textPositions.size();

Should be size_t.

> Source/WebCore/rendering/svg/SVGTextLayoutAttributesBuilder.cpp:148
> +        TextPosition& position = m_textPositions.at(atPosition);

Could use [] instead of .at().

> Source/WebCore/rendering/svg/SVGTextLayoutAttributesBuilder.cpp:154
> +void SVGTextLayoutAttributesBuilder::buildLayoutAttributesForAllCharacters(RenderSVGText* textRoot, unsigned textLength)

Maybe you should assert that textLength is non-zero?

> Source/WebCore/rendering/svg/SVGTextLayoutAttributesBuilder.cpp:177
> +    unsigned size = m_textPositions.size();
> +    for (unsigned i = 0; i < size; ++i)

Should be size_t

> Source/WebCore/rendering/svg/SVGTextLayoutAttributesBuilder.cpp:178
> +        fillAttributesAtPosition(m_textPositions.at(i));

Could use [] instead of .at().

> Source/WebCore/rendering/svg/SVGTextLayoutAttributesBuilder.cpp:257
> +    unsigned valuesSize = values.size();
> +    for (unsigned i = 0; i < valuesSize; ++i)

Should be size_t.

> Source/WebCore/rendering/svg/SVGTextLayoutAttributesBuilder.h:2
> - * Copyright (C) Research In Motion Limited 2010. All rights reserved.
> + * Copyright (C) Research In Motion Limited 2010-2011. All rights reserved.

I think we usually prefer comma-separated years.

[Nikolas Zimmermann]

(In reply to comment #2)
> (From update of attachment 88208 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88208&action=review
> 
> > Source/WebCore/ChangeLog:26
> > +        (WebCore::SVGTextLayoutAttributes::reserveCapacity): Also reserveCapacity for the text metrics list (minor optimiyation).
> 
> Typo: optimyation
Fixed.

> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1997
> > +                if (isSVGText && pos > 0 && !atStart) {
> 
> I don't think you mentioned the !atStart part of this change in your ChangeLog.
> 
> > Source/WebCore/rendering/svg/SVGTextLayoutAttributes.cpp:53
> > +    // Doesn't touch m_textMetricsValues on purpose.
> 
> Instead of just "on purpose", maybe you can explain why we don't touch it. (Ditto for other similar functions in this patch.)
I refactored the code, to avoid the need at all for the comment. SVGTextLayoutAttributesBuilder only needs to store the global x/y/dx/dy/rotate lists, not the metrics lists.


> > Source/WebCore/rendering/svg/SVGTextLayoutAttributes.cpp:68
> > +    if (position >= values.size())
> 
> When do we expect this to happen?
Trailing whitespace, if xml:space is default. Covered by LayoutTests/svg/batik/text/xmlSpaces.svg
 
> > Source/WebCore/rendering/svg/SVGTextLayoutAttributes.h:40
> >      void reserveCapacity(unsigned length);
> > +    void fillWithEmptyValue(unsigned length);
> > +
> > +    void appendEmptyValue();
> > +    void appendSingleValueFromAttribute(const SVGTextLayoutAttributes&, unsigned position);
> 
> The length and position arguments should really be of type size_t.
Okay, as discussed on IRC that should be done seperated, as it affects much more than just this part of SVG.

> 
> > Source/WebCore/rendering/svg/SVGTextLayoutAttributesBuilder.cpp:2
> > - * Copyright (C) Research In Motion Limited 2010. All rights reserved.
> > + * Copyright (C) Research In Motion Limited 2010-2011. All rights reserved.
> 
> I think we normally prefer comma-separated years.
RIM has a different style.

> 
> > Source/WebCore/rendering/svg/SVGTextLayoutAttributesBuilder.cpp:126
> > +    if (start->isSVGText()) 
> > +        ASSERT(m_textPositions.isEmpty());
> 
> You could turn this into just an assertion: ASSERT(!start->isSVGText() || m_textPositions.isEmpty());
Great.


> 
> > Source/WebCore/rendering/svg/SVGTextLayoutAttributesBuilder.cpp:148
> > +        TextPosition& position = m_textPositions.at(atPosition);
> 
> Could use [] instead of .at().
Fixed.

> 
> > Source/WebCore/rendering/svg/SVGTextLayoutAttributesBuilder.cpp:154
> > +void SVGTextLayoutAttributesBuilder::buildLayoutAttributesForAllCharacters(RenderSVGText* textRoot, unsigned textLength)
> 
> Maybe you should assert that textLength is non-zero?
Right, fixed.

> 
> > Source/WebCore/rendering/svg/SVGTextLayoutAttributesBuilder.cpp:178
> > +        fillAttributesAtPosition(m_textPositions.at(i));
> 
> Could use [] instead of .at().
Fixed.

Posting new patch soon.

[Nikolas Zimmermann]

Created attachment 88248 [details]
Patch v2

Fixed Adams comments.

[Dirk Schulze]

Comment on attachment 88248 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=88248&action=review

LGTM. r=me

> Source/WebCore/rendering/svg/SVGTextLayoutAttributesBuilder.h:25
> +#include <wtf/HashMap.h>

You can remove this include.

[Nikolas Zimmermann]

Landed in r82947.