Summary: | [CSS3 Media Queries] Aspect ratio value re-parsed when media query expression is evaluated | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexander Shalamov <alexander.shalamov> | ||||||
Component: | CSS | Assignee: | 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
Alexander Shalamov
2012-10-11 00:55:03 PDT
Created attachment 168166 [details]
Patch 1
Use CSSAspectRatioValue instead of CSSValueList to avoid aspect-ratio re-parsing.
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 (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. 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 on attachment 168177 [details] Patch 2 Clearing flags on attachment: 168177 Committed r131037: <http://trac.webkit.org/changeset/131037> All reviewed patches have been landed. Closing bug. |