[webkit-reviews] review denied: [Bug 52542] SVG CSS property types with <number> don't support exponents : [Attachment 134986] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 52542: SVG CSS property types with <number> don't support exponents
https://bugs.webkit.org/show_bug.cgi?id=52542

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=134986&action=review


I have some new comments:

> Source/WebCore/css/CSSParser.cpp:79
> +#if ENABLE(SVG)
> +#include "SVGParserUtilities.h"
> +#endif

I don't think you need to guard this.

> Source/WebCore/css/CSSParser.cpp:8606
> +	       if (isASCIIAlphaCaselessEqual(*m_currentCharacter, 'e')) {

At this point it's guaranteed that m_currentCharacter exists okay. So
dereferencing the UChar* pointer is fine.

> Source/WebCore/css/CSSParser.cpp:8607
> +		   if (m_currentCharacter[1] == '-' || m_currentCharacter[1] ==
'+' || isASCIIDigit(m_currentCharacter[1])) {

But who guarantees that the UChar* m_currentCharacter + 1 that you're
dereferencing here exists?

The old code passes m_tokenStart to charactersToDouble as const UChar*
parameter, and m_currentCharacter - m_tokenStart as size.
I'd strongly suggest to reuse that way of accessing the string.
const UChar* currentCharacter = m_tokenStart;
unsigned length = m_currentCharacter - m_tokenStart;
if (length > 1 && isASCIIAlphaCaselessEqual(*currentCharacter, 'e')) {
// Now its guaranteed that the passed in string is at least 'eX' (e + another
item)
}

If I'm missing something, and this is already guaranteed by the CSSParser code,
you should at least assert that the length is > 1.

> Source/WebCore/css/CSSParser.cpp:8612
> +		   while (isASCIIDigit(m_currentCharacter[offset]))

If the branch above isn't taken that means the first character following the
'e' is not '+', not '-' nor an ascii digit - why do you continue parsing at
this point? You could have exited already.

> Source/WebCore/css/CSSParser.cpp:8615
> +	       if (!parseSVGNumber(m_tokenStart, m_currentCharacter + offset -
m_tokenStart, yylval->number))

This would then read as if (!parseSVGNumber(m_tokenStart, length + offset,
yylval->number).

> Source/WebCore/css/CSSParser.cpp:8616
> +		   break;

If parseSVGNumber() fails, is yylval->number reset to a specific value? (Not
sure about that, that's why I'm asking).

> Source/WebCore/svg/SVGParserUtilities.cpp:152
> +

One newline too much.

> LayoutTests/svg/css/script-tests/scientific-numbers.js:13
>
+shouldBeEqualToString("document.defaultView.getComputedStyle(text).fontSize",
"16px");

You still use the long form here, getComputedStyle(text).fontSize should work,
without the document.defaultView prefix :-)