Bug 99003

Summary: [CSS3 Media Queries] Aspect ratio value re-parsed when media query expression is evaluated
Product: WebKit Reporter: Alexander Shalamov <alexander.shalamov>
Component: CSSAssignee: Alexander Shalamov <alexander.shalamov>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, eric.carlson, feature-media-reviews, kenneth, macpherson, menard, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch 1
none
Patch 2 none

Description Alexander Shalamov 2012-10-11 00:55:03 PDT
Aspect ratio css value is parsed every time when media query expression is evaluated.
That would degrade performance if there are multiple media queries or multiple media query listeners.

Aspect ratio css value should be constructed once and should not be parsed during expression evaluation.
Comment 1 Alexander Shalamov 2012-10-11 01:19:17 PDT
Created attachment 168166 [details]
Patch 1

Use CSSAspectRatioValue instead of CSSValueList to avoid aspect-ratio re-parsing.
Comment 2 Kenneth Rohde Christiansen 2012-10-11 02:13:28 PDT
Comment on attachment 168166 [details]
Patch 1

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

> Source/WebCore/css/MediaQueryExp.cpp:177
> +            for (int i = 0; i < 3; ++i, valueList->next()) {

I would use unsigned

> Source/WebCore/css/MediaQueryExp.cpp:180
> +                    !i ? numeratorValue = value->fValue : denominatorValue = value->fValue;

I think an if sentence would be more readable

> Source/WebCore/css/MediaQueryExp.cpp:182
> +                else if (i == 1 && value->unit == CSSParserValue::Operator && value->iValue == '/')
> +                    continue;

So can you get an integer in 0 and then nothing in 1? and then another integer in 2? What if there is no operator
Comment 3 Alexander Shalamov 2012-10-11 02:27:16 PDT
(In reply to comment #2)
> (From update of attachment 168166 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168166&action=review
> 
> So can you get an integer in 0 and then nothing in 1? and then another integer in 2? What if there is no operator

if there is nothing in 1, then else if (i == 1... will check that value at 1 must be operator '/', otherwise aspect-ratio is invalid.

http://www.w3.org/TR/css3-mediaqueries/#values

"The <ratio> value is a positive (not zero or negative) <integer> followed by optional whitespace, followed by a solidus (‘/’), followed by optional whitespace, followed by a positive <integer>."

I'm fixing patch according to your comments. Will upload in few minutes.
Comment 4 Alexander Shalamov 2012-10-11 02:40:56 PDT
Created attachment 168177 [details]
Patch 2

- Use unsigned instead of int in for loop. MediaQueryExp.cpp:177
- Use if statement instead of ternary operator. MediaQueryExp.cpp:180
Comment 5 WebKit Review Bot 2012-10-11 03:11:59 PDT
Comment on attachment 168177 [details]
Patch 2

Clearing flags on attachment: 168177

Committed r131037: <http://trac.webkit.org/changeset/131037>
Comment 6 WebKit Review Bot 2012-10-11 03:12:02 PDT
All reviewed patches have been landed.  Closing bug.