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

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


Nice job, still some caveats, r- for test coverage:

> Source/WebCore/css/CSSParser.cpp:8603
> +	       int offset = 1;

why int? I'd use unsigned.

> Source/WebCore/css/CSSParser.cpp:8605
> +	       if (m_currentCharacter[offset] == '+') {

Is this safe to access? Is it guaranteed that m_currentCharacter isn't accessed
out of boundaries, aka. that 'e' is always followed by another character.

> Source/WebCore/css/CSSParser.cpp:8606
> +		   offset++;

++offset.

> Source/WebCore/css/CSSParser.cpp:8609
> +		   offset++;

Ditto.

> Source/WebCore/css/CSSParser.cpp:8612
> +	       double exponent;

No need for this local here, just declare it in the if (offset > digits) branch
directly,

> Source/WebCore/css/CSSParser.cpp:8616
> +		   exponent = charactersToDouble(m_currentCharacter + digits,
offset - digits);

Can this yield "invalid" doubles, is this clamped somehow? Shall we sanitize
the parsed exponent here? (thinking of NaN handling, or Inf)

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

You can just use "getComputedStyle(text).fontSize".

> LayoutTests/svg/css/script-tests/scientific-numbers.js:232
> +

This needs to be extended;
leading whitespace, trailing whitespace, mid white-space (" 5e10", " 5e1 0",
etc. (also including negative values, for both number and exponent).
Arbitary characters inside of exponent, exponents leading to double overflow,
etc.