[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
bugzilla-daemon at webkit.org
Tue Apr 5 07:44:55 PDT 2011
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.
More information about the webkit-reviews
mailing list