Bug 79356

Summary: Little optimization in CSSParser::parseShorthand.
Product: WebKit Reporter: Alexis Menard (darktears) <menard>
Component: CSSAssignee: Alexis Menard (darktears) <menard>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, haraken, kling, macpherson, morrita, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Alexis Menard (darktears) 2012-02-23 04:03:11 PST
Little optimization in CSSParser::parseShorthand.
Comment 1 Alexis Menard (darktears) 2012-02-23 04:10:41 PST
Created attachment 128455 [details]
Patch
Comment 2 Kentaro Hara 2012-02-23 04:28:52 PST
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.
Comment 3 Alexis Menard (darktears) 2012-02-23 05:01:17 PST
Created attachment 128460 [details]
Patch
Comment 4 Kentaro Hara 2012-02-23 05:56:55 PST
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;
Comment 5 Alexis Menard (darktears) 2012-02-23 06:13:11 PST
Created attachment 128470 [details]
Patch
Comment 6 Kentaro Hara 2012-02-23 06:14:03 PST
Comment on attachment 128470 [details]
Patch

Thanks for the patch!
Comment 7 WebKit Review Bot 2012-02-23 07:42:33 PST
Comment on attachment 128470 [details]
Patch

Rejecting attachment 128470 [details] from review queue.

haraken@chromium.org 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.
Comment 8 Tony Chang 2012-02-23 10:23:30 PST
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 &&?
Comment 9 Kentaro Hara 2012-02-23 14:03:18 PST
(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.
Comment 10 Alexis Menard (darktears) 2012-02-24 03:53:44 PST
Created attachment 128704 [details]
Patch for landing
Comment 11 WebKit Review Bot 2012-02-24 06:16:37 PST
Comment on attachment 128704 [details]
Patch for landing

Clearing flags on attachment: 128704

Committed r108784: <http://trac.webkit.org/changeset/108784>
Comment 12 WebKit Review Bot 2012-02-24 06:16:45 PST
All reviewed patches have been landed.  Closing bug.