[webkit-reviews] review granted: [Bug 57831] text-tspan-02-b.svg from SVG 1.1 2nd edition fails : [Attachment 88208] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 57831: text-tspan-02-b.svg from SVG 1.1 2nd edition fails
https://bugs.webkit.org/show_bug.cgi?id=57831

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

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
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(RenderSVG
Text* 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.