[CSS3 Media Queries] Aspect ratio value re-parsed when media query expression is evaluated

[Alexander Shalamov]

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.

[Alexander Shalamov]

Created attachment 168166 [details]
Patch 1

Use CSSAspectRatioValue instead of CSSValueList to avoid aspect-ratio re-parsing.

[Kenneth Rohde Christiansen]

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

[Alexander Shalamov]

(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.

[Alexander Shalamov]

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

[WebKit Review Bot]

Comment on attachment 168177 [details]
Patch 2

Clearing flags on attachment: 168177

Committed r131037: <http://trac.webkit.org/changeset/131037>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.