[webkit-reviews] review denied: [Bug 16708] CSS: Slow parsing of rgb() with percent values : [Attachment 86172] updated patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Andras Becsi <abecsi at webkit.org>'s
request for review:
Bug 16708: CSS: Slow parsing of rgb() with percent values
https://bugs.webkit.org/show_bug.cgi?id=16708

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=86172&action=review

I think we need some more test cases beyond the ones added here. I don’t think
the test cases cover all the branches in the code. For example, I don’t see any
test cases with multiple periods and a percentage sign or with any other
illegal characters.

> Source/WebCore/css/CSSParser.cpp:3901
> +static inline int parseDoubleIfValid(const UChar* string, const UChar* end,
UChar terminator, double& value)

I think it’s unclear that this function result is the number of characters
consumed. There should be a comment saying so.

I don’t think it makes sense to mark this function inline. It’s called in three
different places and it’s pretty long too.

> Source/WebCore/css/CSSParser.cpp:3933
> +    if (processedLength) {
> +	   value = 0.0;
> +	   bytes[processedLength] = static_cast<char>(terminator);
> +	   bytes[processedLength + 1] = '\0';
> +	   char* foundTerminator;
> +	   value = WTF::strtod(bytes.data(), &foundTerminator);
> +	   return (*foundTerminator == static_cast<char>(terminator)) ?
processedLength : 0;
> +    }
> +    return 0;

This should use early return instead of putting all the code inside an if.

It’s not a good idea to cast a UChar into a char. If the terminator is
guaranteed to be an ASCII character, then it should be passed in to this
function as a char, not a UChar. If it’s not, then this code won’t work.

Does this code really need to use WTF::strtod?

> Source/WebCore/css/CSSParser.cpp:3936
> +static inline bool parseColorIntOrPercentage(const UChar*& string, const
UChar* end, UChar terminator, CSSPrimitiveValue::UnitTypes& expect, int& value)


I don’t think it makes sense to mark a function inline that’s this long and
called in so many places.

> Source/WebCore/css/CSSParser.cpp:3939
> +    double localValue = 0.0;

We normally just say 0, not 0.0.

> Source/WebCore/css/CSSParser.cpp:3982
> +	   localValue =  localValue / 100.0 * 256.0;

There’s an extra space here. Multiplying by 256 is wrong. You should multiply
by nextafter(256, 0).

> Source/WebCore/css/CSSParser.cpp:3984
> +	   if (localValue > 255)
> +	       localValue = 255;

This probably wouldn’t be necessary if we multiplied by the right value. Unless
we allow percentage values over 100.

> Source/WebCore/css/CSSParser.cpp:4034
> +	   double d;

Can you use a word name for this local variable rather than just "d"?

> Source/WebCore/css/CSSParser.cpp:4035
> +	   if (parseDoubleIfValid(string, end, terminator, d)) {

This code does significant extra work that it didn’t before, including calling
strtod. That doesn’t seem good for performance.

> Source/WebCore/css/CSSParser.cpp:4062
> +    double d = 0.0;
> +    if (parseDoubleIfValid(string, end, terminator, d)) {
> +	   value = negative ? 0 : static_cast<int>(d * nextafter(256.0, 0.0));
> +	   string = end;
> +	   return true;
> +    }
> +    return false;

Can you use a word name for this local variable rather than just "d"? Not sure
why you are initializing it.

This should be early return style, not “nest the success case inside an if”
style.