[webkit-reviews] review denied: [Bug 46066] SVG: Optimize stringToLengthType() : [Attachment 68042] Proposed patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Andreas Kling
<andreas.kling at nokia.com>'s request for review:
Bug 46066: SVG: Optimize stringToLengthType()
https://bugs.webkit.org/show_bug.cgi?id=46066

Attachment 68042: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=68042&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=68042&action=prettypatch

> WebCore/svg/SVGLength.cpp:97
> +    if (characters[length - 1] == '%')

Can you put UChar b = characters[length - 1]; right before this check? The
names should indicate the content. Can you use 'lastChar' for b or simply
'end'? a also needs a better name. Just a bit strange that we give back number,
if the string doesn't end with a length type that we know.

> WebCore/svg/SVGLength.cpp:104
> +    UChar a = characters[length - 2];
> +    UChar b = characters[length - 1];
Should be const.

I was looking at setValueAsString, the caller of this function. And it looks
bogus to me, to just check the last or last two chars. Following example would
parse on WebKit, while it doesn't on other Viewers:
<rect width="20    mm" height="20*SomethinStupid*px"/>

Can't you just use 'ptr' and 'end' of setValueAsString? 'ptr' is the first char
after the parsed number of the string. We don't need String at all on
lengthTypeToString. This change will need new layouttests.