Bug 44107 – CSS: Make rgb() and rgba() fast paths case-insensitive

• Component: CSS
• Status: RESOLVED FIXED

[Andreas Kling]

The fast path in CSSParser::parseColor() could easily catch RGB() and RGBA() as well.

Instead of equalIgnoringCase(), we can do inline character comparison and gain some speed while we're at it, too.

[Andreas Kling]

Created attachment 64588 [details]
Proposed patch

[Ariya Hidayat]

Comment on attachment 64588 [details]
Proposed patch

> -    if (name.startsWith("rgba(")) {
> +    if (mightBeRGBA(characters, length)) {

Will there be a (minor) behavior difference IF the input is invalid e.g. "rgba(foo"?

[Andreas Kling]

(In reply to comment #2)
> (From update of attachment 64588 [details])
> > -    if (name.startsWith("rgba(")) {
> > +    if (mightBeRGBA(characters, length)) {
> 
> Will there be a (minor) behavior difference IF the input is invalid e.g. "rgba(foo"?

The invalid string "rgba(foo" will fail via Color::setNamedColor() below instead of inside the fast rgba() parser.

Behavior won't change but you have a point - it will be slightly slower for invalid strings. I will update the patch.

[Andreas Kling]

Created attachment 64719 [details]
Proposed patch v2

Updated to avoid performance regression for invalid colors.

[Darin Adler]

Comment on attachment 64719 [details]
Proposed patch v2

> +static inline bool mightBeRGBA(const UChar* characters, unsigned length)
> +{
> +    if (length < 5)
> +        return false;
> +    return characters[4] == '('
> +        && (characters[0] == 'r' || characters[0] == 'R')
> +        && (characters[1] == 'g' || characters[1] == 'G')
> +        && (characters[2] == 'b' || characters[2] == 'B')
> +        && (characters[3] == 'a' || characters[3] == 'A');
> +}

This kind of check can be done considerably more efficiently like this:

    && (characters[0] | 0x20) == 'r'
    && (characters[1] | 0x20) == 'b'

r=me as-is but consider my faster version

[Andreas Kling]

(In reply to comment #5)
> r=me as-is but consider my faster version

Indeed, Ariya was saying the same thing on IRC. I will land with that modification. Thanks!

[Andreas Kling]

Committed r65626: <http://trac.webkit.org/changeset/65626>