Bug 79356 – Little optimization in CSSParser::parseShorthand.

• Component: CSS
• Status: RESOLVED FIXED

[Alexis Menard (darktears)]

Little optimization in CSSParser::parseShorthand.

[Alexis Menard (darktears)]

Created attachment 128455 [details]
Patch

[Kentaro Hara]

Comment on attachment 128455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128455&action=review

> Source/WebCore/css/CSSParser.cpp:2682
> +    bool fnd[6]= { false }; // Trust me ;)

"bool fnd[6] = {false, false, false, false, false, false}" might be better (just for clarification).

"bool fnd[6] = {false}" is equivalent to "bool fnd[6] = {false, 0, 0, 0, 0, 0}". This is OK because false and 0 have the same meaning in this method, but is a bit confusing.

> Source/WebCore/css/CSSParser.cpp:2686
>          for (int propIndex = 0; !found && propIndex < numProperties; ++propIndex) {

BTW, (I am not sure how much it is important for performance but) maybe we can remove this for loop, by moving the propIndex declaration to outside while (...). Anyway you can try it in another patch.

[Alexis Menard (darktears)]

Created attachment 128460 [details]
Patch

[Kentaro Hara]

Comment on attachment 128460 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128460&action=review

> Source/WebCore/css/CSSParser.cpp:-2687
> -        for (int propIndex = 0; !found && propIndex < numProperties; ++propIndex) {

I am sorry, I was wrong. Let us keep it as-is.

### I was thinking that we might be able to remove the for loop something like this. (The following code is wrong.)

int propIndex = 0;
while (m_valueList->current()) {
    if (propIndex >= numProperties)
        return false;
    if (parseValue(properties[propIndex], important)) {
        fnd[propIndex] = true;
        propIndex++;
    }
}
if (propIndex == numProperties)
    return true;

[Alexis Menard (darktears)]

Created attachment 128470 [details]
Patch

[Kentaro Hara]

Comment on attachment 128470 [details]
Patch

Thanks for the patch!

[WebKit Review Bot]

Comment on attachment 128470 [details]
Patch

Rejecting attachment 128470 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[Tony Chang]

Comment on attachment 128470 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128470&action=review

> Source/WebCore/css/CSSParser.cpp:2680
>      bool found = false;

Can we declare found in the while loop?

> Source/WebCore/css/CSSParser.cpp:2682
> +    bool fnd[6]= { false, false, false, false, false, false }; // Trust me ;)

I'm not sure what the comment means.  I would probably just remove it.

> Source/WebCore/css/CSSParser.cpp:2688
>              if (!fnd[propIndex]) {
> -                if (parseValue(properties[propIndex], important))
> +                if (parseValue(properties[propIndex], important)) {

Can we merge these if statements with &&?

[Kentaro Hara]

(In reply to comment #8)
> > Source/WebCore/css/CSSParser.cpp:2682
> > +    bool fnd[6]= { false, false, false, false, false, false }; // Trust me ;)
> 
> I'm not sure what the comment means.  I would probably just remove it.

The comment means "6 is enough size". We can update the comment to clarify that.

[Alexis Menard (darktears)]

Created attachment 128704 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 128704 [details]
Patch for landing

Clearing flags on attachment: 128704

Committed r108784: <http://trac.webkit.org/changeset/108784>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.