[webkit-reviews] review denied: [Bug 42965] CSS: Add fast-path for rgba() color parsing : [Attachment 62690] Proposed patch v2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Andreas Kling
<andreas.kling at nokia.com>'s request for review:
Bug 42965: CSS: Add fast-path for rgba() color parsing
https://bugs.webkit.org/show_bug.cgi?id=42965

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +static inline bool parseAlphaDouble(const UChar*& string, const UChar* end,
UChar terminator, double& value)
> +{
> +    int length = end - string;
> +    if (!length)
> +	   return false;
> +
> +    Vector<char, 256> bytes(length + 1);
> +    for (int i = 0; i < length; ++i) {
> +	   if (!isASCIIDigit(string[i]) && string[i] != '.' && string[i] !=
')')
> +	       return false;
> +	   bytes[i] = string[i];
> +    }
> +    bytes[length] = '\0';
> +    char* foundTerminator;
> +    value = WTF::strtod(bytes.data(), &foundTerminator);
> +    value = max(0.0, min(value, 1.0));
> +    string += (foundTerminator - bytes.data()) + 1;
> +    return *foundTerminator == terminator;
> +}

For the typical alpha values like 0, 1, and the tenths and simple powers of too
we can do a *lot* faster than calling the general purpose strtod function every
time, perhaps even avoiding floating point math altogether.

> +	   rgb = makeRGBA(red, green, blue, static_cast<int>(lroundf(255.0f *
static_cast<float>(alpha))));

I don’t think it’s correct to multiply the alpha value by 255. At other call
sites you'll see code more like this:

    nextafter(256, 0) * alpha

We need some test cases that cover the edge cases, the boundaries between alpha
of 254 and 255 for example, and alpha of 0 and 1. The relevant functions to
look at are CSSParser::parseColorParameters and the various functions in the
Color.cpp file. There may be some cases where multiplying by 255 is right.
Maybe we can also do code sharing.

If the test cases prove that you are not changing behavior, then I suppose you
could put this patch up for review again.

review- because of the 255/256 thing.