CSS Aspect Ratio Property Parsing Stage
Created attachment 112139 [details] Patch
Created attachment 112144 [details] Patch
Comment on attachment 112144 [details] Patch Attachment 112144 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10197761 New failing tests: fast/borders/border-radius-groove-01.html fast/borders/border-radius-parsing.html fast/borders/border-radius-groove-03.html fast/box-shadow/spread-multiple-normal.html fast/box-shadow/inset-with-extraordinary-radii-and-border.html fast/box-shadow/spread-multiple-inset.html fast/box-shadow/inset-box-shadows.html fast/backgrounds/gradient-background-leakage.html fast/borders/border-radius-wide-border-04.html fast/backgrounds/border-radius-split-background-image.html fast/borders/border-radius-groove-02.html fast/borders/border-radius-wide-border-01.html fast/borders/border-radius-different-width-001.html fast/borders/border-radius-wide-border-03.html fast/borders/border-radius-wide-border-02.html fast/box-shadow/shadow-tiling-artifact.html fast/borders/border-radius-inset-outset.html fast/backgrounds/border-radius-split-background.html fast/borders/border-styles-split.html fast/borders/border-radius-circle.html
Created attachment 112148 [details] Patch
Comment on attachment 112148 [details] Patch Attachment 112148 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10199745 New failing tests: fast/borders/border-radius-groove-01.html fast/borders/border-radius-parsing.html fast/borders/border-radius-groove-03.html fast/box-shadow/spread-multiple-normal.html fast/box-shadow/inset-with-extraordinary-radii-and-border.html fast/box-shadow/spread-multiple-inset.html fast/box-shadow/inset-box-shadows.html fast/backgrounds/gradient-background-leakage.html fast/borders/border-radius-wide-border-04.html fast/backgrounds/border-radius-split-background-image.html fast/borders/border-radius-groove-02.html fast/borders/border-radius-wide-border-01.html fast/borders/border-radius-different-width-001.html fast/borders/border-radius-wide-border-03.html fast/borders/border-radius-wide-border-02.html fast/box-shadow/shadow-tiling-artifact.html fast/borders/border-radius-inset-outset.html fast/backgrounds/border-radius-split-background.html fast/borders/border-styles-split.html fast/borders/border-radius-circle.html
Comment on attachment 112148 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112148&action=review Please add tests. You should be able to write tests the verify parsing happens correctly and that non-positive numbers are considered invalid. See the -webkit-calc tests for examples. > Source/WebCore/css/CSSParser.cpp:1513 > + return parseAspectRatio(important); indent is off > Source/WebCore/css/CSSParser.cpp:5651 > + if (!validUnit(lvalue, FLength, m_strict) || !validUnit(rvalue, FLength, m_strict)) Does this exclude negative values? If not, we should make sure to return false for negative values as per the spec. > Source/WebCore/css/CSSParser.cpp:5654 > + if (!rvalue->fValue) Do you not need to check lvalue->fValue here? Is this to deal with divide by 0? It also seems non-sensical to have a 0 lvalue and the spec says both numbers should be non-zero.
Is this in any CSS spec yet?
Ojan, I don't see any tests here: https://bugs.webkit.org/show_bug.cgi?id=57082 I will address the rest of your comments. Thanks, Fady (In reply to comment #6) > (From update of attachment 112148 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=112148&action=review > > Please add tests. You should be able to write tests the verify parsing happens correctly and that non-positive numbers are considered invalid. See the -webkit-calc tests for examples. > > > Source/WebCore/css/CSSParser.cpp:1513 > > + return parseAspectRatio(important); > > indent is off > > > Source/WebCore/css/CSSParser.cpp:5651 > > + if (!validUnit(lvalue, FLength, m_strict) || !validUnit(rvalue, FLength, m_strict)) > > Does this exclude negative values? If not, we should make sure to return false for negative values as per the spec. > > > Source/WebCore/css/CSSParser.cpp:5654 > > + if (!rvalue->fValue) > > Do you not need to check lvalue->fValue here? Is this to deal with divide by 0? It also seems non-sensical to have a 0 lvalue and the spec says both numbers should be non-zero.
(In reply to comment #7) > Is this in any CSS spec yet? No there is no formal spec yet, but an informal spec can be found here: http://www.xanthir.com/blog/b4810 There has been discussion at www-style: http://lists.w3.org/Archives/Public/www-style/2010Oct/0046.html http://lists.w3.org/Archives/Public/www-style/2011Oct/0550.html. It's just blocked on Tab and Fantasai having enough time to put in into a formal spec.
I like the proposal. I'll take a look soon.
Created attachment 112980 [details] Patch
Comment on attachment 112980 [details] Patch Attachment 112980 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10242453 New failing tests: fast/css/border-radius-non-negative.html fast/box-shadow/shadow-tiling-artifact.html fast/borders/webkit-border-radius.html inspector/elements/elements-panel-styles.html fast/borders/border-radius-circle.html fast/backgrounds/border-radius-split-background.html fast/borders/border-radius-parsing.html
(In reply to comment #12) > (From update of attachment 112980 [details]) > Attachment 112980 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/10242453 > > New failing tests: > fast/css/border-radius-non-negative.html > fast/box-shadow/shadow-tiling-artifact.html > fast/borders/webkit-border-radius.html > inspector/elements/elements-panel-styles.html > fast/borders/border-radius-circle.html > fast/backgrounds/border-radius-split-background.html > fast/borders/border-radius-parsing.html Do we want to support floating point numbers for the numerator and denominator? For example, should 1.2 / 2 be allowed? Currently, the implementation does not allow for this.
Comment on attachment 112980 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112980&action=review > Source/WebCore/css/CSSStyleSelector.cpp:2524 > + return; Move to list of unimplemented properties (before WebkitFontSizeDelta).
Created attachment 113072 [details] Patch
Comment on attachment 112980 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112980&action=review Additional small change in the patch I just uploaded: floating point numerators and denominators are now supported. >> Source/WebCore/css/CSSStyleSelector.cpp:2524 >> + return; > > Move to list of unimplemented properties (before WebkitFontSizeDelta). Done. Thanks.
Comment on attachment 113072 [details] Patch Attachment 113072 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10241860 New failing tests: fast/css/border-radius-non-negative.html fast/box-shadow/shadow-tiling-artifact.html fast/borders/webkit-border-radius.html inspector/elements/elements-panel-styles.html fast/borders/border-radius-circle.html fast/backgrounds/border-radius-split-background.html fast/borders/border-radius-parsing.html
Comment on attachment 113072 [details] Patch Looks good to me.
Comment on attachment 113072 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113072&action=review > Source/WebCore/css/CSSPropertyNames.in:186 > +-webkit-aspect-ratio can this be kept in alphabetical order, below -webkit-appearance?
Created attachment 113241 [details] Patch
Comment on attachment 113072 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113072&action=review >> Source/WebCore/css/CSSPropertyNames.in:186 >> +-webkit-aspect-ratio > > can this be kept in alphabetical order, below -webkit-appearance? Done.
Comment on attachment 113241 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113241&action=review Just needs a little restructuring...I think it'll be fine to commit after that. > Source/WebCore/rendering/style/StyleBoxData.h:86 > + float m_aspectRatio; > + > bool m_hasAutoZIndex : 1; > + bool m_hasAspectRatio : 1; These should probably be on StyleRareNonInheritedData.h. See what the new flexbox properties do as an example. As it is, every RenderStyle holds a StyleBoxData, so this is adding a float to every RenderStyle. With StyleRareNonInheritedData, it would only add the memory use if aspectRatio is set. It's safe to say that aspectRatio will be used rarely. Even on pages that use it, they'll only use it on a small subset of the DOM. > LayoutTests/fast/css/aspect-ratio-parsing-tests-expected.txt:6 > +PASS testParsing("aspectRatioTest", "2/1", "-webkit-aspect-ratio") is "2" This serialization doesn't match http://www.xanthir.com/blog/b4810. It should be "2/1".
Created attachment 113585 [details] Patch
Comment on attachment 113585 [details] Patch Attachment 113585 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10301034 New failing tests: fast/borders/only-one-border-with-width.html fast/css/border-radius-non-negative.html media/audio-repaint.html fast/backgrounds/gradient-background-leakage.html fast/box-shadow/shadow-tiling-artifact.html fast/borders/webkit-border-radius.html inspector/elements/elements-panel-styles.html fast/borders/border-radius-circle.html fast/backgrounds/border-radius-split-background.html fast/borders/border-radius-parsing.html
Comment on attachment 113585 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113585&action=review Yay! > Source/WebCore/css/CSSParser.cpp:5679 > + addProperty(CSSPropertyWebkitAspectRatio, aspectRatioValue.release(), important); CSSAspectRatioValue::create returns a PassRefPtr, so you could make this a one-liner: addProperty(CSSPropertyWebkitAspectRatio, CSSAspectRatioValue::create(lvalue->fValue, rvalue->fValue), important); I guess that's a bit unwieldy. Up to you.
Created attachment 113609 [details] Patch
Created attachment 113610 [details] Patch
(In reply to comment #25) > (From update of attachment 113585 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113585&action=review > > Yay! > > > Source/WebCore/css/CSSParser.cpp:5679 > > + addProperty(CSSPropertyWebkitAspectRatio, aspectRatioValue.release(), important); > > CSSAspectRatioValue::create returns a PassRefPtr, so you could make this a one-liner: > addProperty(CSSPropertyWebkitAspectRatio, CSSAspectRatioValue::create(lvalue->fValue, rvalue->fValue), important); > > I guess that's a bit unwieldy. Up to you. Done.
Created attachment 113612 [details] Patch
Created attachment 113613 [details] Patch
Comment on attachment 113613 [details] Patch Attachment 113613 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10316036 New failing tests: fast/borders/only-one-border-with-width.html fast/css/border-radius-non-negative.html media/audio-repaint.html fast/backgrounds/gradient-background-leakage.html fast/backgrounds/border-radius-split-background.html fast/borders/webkit-border-radius.html inspector/elements/elements-panel-styles.html fast/borders/border-radius-circle.html fast/box-shadow/shadow-tiling-artifact.html fast/borders/border-radius-parsing.html
Created attachment 113654 [details] Patch
Just iterating through the patch to get all the platforms to build. I believe mac is still broken because the xcodeproj needs to be updated. I will do that shortly.
Created attachment 113660 [details] Patch
Created attachment 113662 [details] Patch
Created attachment 113664 [details] Patch
Comment on attachment 113664 [details] Patch Attachment 113664 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10310182
(In reply to comment #31) > (From update of attachment 113613 [details]) > Attachment 113613 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/10316036 > > New failing tests: > fast/borders/only-one-border-with-width.html > fast/css/border-radius-non-negative.html > media/audio-repaint.html > fast/backgrounds/gradient-background-leakage.html > fast/backgrounds/border-radius-split-background.html > fast/borders/webkit-border-radius.html > inspector/elements/elements-panel-styles.html > fast/borders/border-radius-circle.html > fast/box-shadow/shadow-tiling-artifact.html > fast/borders/border-radius-parsing.html Fady, do you know what's up with these? If you run these tests locally with your change, do they fail? How about with a chromium webkit build?
Comment on attachment 113664 [details] Patch Attachment 113664 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10333005 New failing tests: fast/borders/only-one-border-with-width.html fast/css/border-radius-non-negative.html media/audio-repaint.html fast/backgrounds/gradient-background-leakage.html fast/box-shadow/shadow-tiling-artifact.html fast/borders/webkit-border-radius.html inspector/elements/elements-panel-styles.html fast/borders/border-radius-circle.html fast/backgrounds/border-radius-split-background.html fast/borders/border-radius-parsing.html
(In reply to comment #38) > (In reply to comment #31) > > (From update of attachment 113613 [details] [details]) > > Attachment 113613 [details] [details] did not pass chromium-ews (chromium-xvfb): > > Output: http://queues.webkit.org/results/10316036 > > > > New failing tests: > > fast/borders/only-one-border-with-width.html > > fast/css/border-radius-non-negative.html > > media/audio-repaint.html > > fast/backgrounds/gradient-background-leakage.html > > fast/backgrounds/border-radius-split-background.html > > fast/borders/webkit-border-radius.html > > inspector/elements/elements-panel-styles.html > > fast/borders/border-radius-circle.html > > fast/box-shadow/shadow-tiling-artifact.html > > fast/borders/border-radius-parsing.html > > Fady, do you know what's up with these? If you run these tests locally with your change, do they fail? How about with a chromium webkit build? I will look into it. I won't commit until everything is green.
(In reply to comment #37) > (From update of attachment 113664 [details]) > Attachment 113664 [details] did not pass qt-ews (qt): > Output: http://queues.webkit.org/results/10310182 (In reply to comment #38) > (In reply to comment #31) > > (From update of attachment 113613 [details] [details]) > > Attachment 113613 [details] [details] did not pass chromium-ews (chromium-xvfb): > > Output: http://queues.webkit.org/results/10316036 > > > > New failing tests: > > fast/borders/only-one-border-with-width.html > > fast/css/border-radius-non-negative.html > > media/audio-repaint.html > > fast/backgrounds/gradient-background-leakage.html > > fast/backgrounds/border-radius-split-background.html > > fast/borders/webkit-border-radius.html > > inspector/elements/elements-panel-styles.html > > fast/borders/border-radius-circle.html > > fast/box-shadow/shadow-tiling-artifact.html > > fast/borders/border-radius-parsing.html > > Fady, do you know what's up with these? If you run these tests locally with your change, do they fail? How about with a chromium webkit build? Looks like I misplaced case CSSPropertyWebkitAspectRatio: return parseAspectRatio(important); while I was reordering it to make it alphabetical.
Created attachment 113685 [details] Patch
Comment on attachment 113685 [details] Patch Taking it through the commit queue, just in case.
Comment on attachment 113685 [details] Patch Clearing flags on attachment: 113685 Committed r99332: <http://trac.webkit.org/changeset/99332>
All reviewed patches have been landed. Closing bug.