[webkit-reviews] review denied: [Bug 52542] SVG CSS property types with <number> don't support exponents : [Attachment 135000] 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 135000: Patch
https://bugs.webkit.org/attachment.cgi?id=135000&action=review

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


Next round of comments, poor Dirk :-)

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

Are we sure that at least one element follows the 'e' ?
Can we ASSERT(m_currentCharacter - m_tokenStart > 1) here?
Before accessing: if (m_currentCharacter[1].. below

> Source/WebCore/css/CSSParser.cpp:8612
> +		   if (m_currentCharacter[1] == '-' || m_currentCharacter[1] ==
'+' || isASCIIDigit(m_currentCharacter[1])) {
> +		       offset += 2;
> +		       while (isASCIIDigit(m_currentCharacter[offset]))
> +			   ++offset;
> +		       // Use FLOATTOKEN if we have exponents.
> +		       dotSeen = true;
> +		   }
> +	       }

++m_currentCharacter;
if (*m_currentCharacter == '-' || *m_currentCharacter == '+' ||
isASCIIDigit(*m_currentCharacter)) {
    ++m_currentCharacter;
    while (isASCIIDigit(*m_currentCharacter))
	++m_currentCharacter;
    // Use FLOATTOKEN if the string contains exponents.
    dotSeen = true;
}

if (!parseNumber(m_tokenStart, m_currentCharacter, yylval->number, false))
    break;

You'll need a new parseNumber() variant taking a double&, but you can remove
the new parseSVGNumber() method that you've added.

> Source/WebCore/svg/SVGParserUtilities.h:36
> +bool parseSVGNumber(UChar*& ptr, size_t length, double& number);

Can be avoided.

> LayoutTests/svg/css/script-tests/scientific-numbers.js:274
> +text.setAttribute("font-size", "50e0.0");
> +shouldBeEqualToString("getComputedStyle(text).fontSize", "16px");

Hard to understand the test, without seeing what you're trying to assign.

I propose following:
function setFontSize(valueString) {
    text.setAttribute("font-size", valueString);
}

function test(valueString, expectedString) {
    debug("text.setAttribute('font-size', '" + valueString + "');");
    setFontSize(valueString);
    shouldBeEqualToString("getComputedStyle(text).fontSize", expectedString);
}

and use:

debug("Valid values");
test("50px", "50px");
test("100px", "100px");

setFontSize("16px");

debug("");
debug("Invalid values");
test("50e0.0", "16px");
test("50e 0", "16px");
....

setFontSize("16px");

debug("");
debug("Next category...");