Bug 70707 – CSS Aspect Ratio Property Parsing Stage

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Fady Samuel]

CSS Aspect Ratio Property Parsing Stage

[Fady Samuel]

Created attachment 112139 [details]
Patch

[Fady Samuel]

Created attachment 112144 [details]
Patch

[WebKit Review Bot]

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

[Fady Samuel]

Created attachment 112148 [details]
Patch

[WebKit Review Bot]

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

[Ojan Vafai]

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.

[Simon Fraser (smfr)]

Is this in any CSS spec yet?

[Fady Samuel]

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.

[Fady Samuel]

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

[Dave Hyatt]

I like the proposal. I'll take a look soon.

[Fady Samuel]

Created attachment 112980 [details]
Patch

[WebKit Review Bot]

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

[Fady Samuel]

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

[Luke Macpherson]

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

[Fady Samuel]

Created attachment 113072 [details]
Patch

[Fady Samuel]

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.

[WebKit Review Bot]

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

[Dave Hyatt]

Comment on attachment 113072 [details]
Patch

Looks good to me.

[Jarred Nicholls]

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?

[Fady Samuel]

Created attachment 113241 [details]
Patch

[Fady Samuel]

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.

[Ojan Vafai]

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

[Fady Samuel]

Created attachment 113585 [details]
Patch

[WebKit Review Bot]

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

[Ojan Vafai]

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.

[Fady Samuel]

Created attachment 113609 [details]
Patch

[Fady Samuel]

Created attachment 113610 [details]
Patch

[Fady Samuel]

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

[Fady Samuel]

Created attachment 113612 [details]
Patch

[Fady Samuel]

Created attachment 113613 [details]
Patch

[WebKit Review Bot]

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

[Fady Samuel]

Created attachment 113654 [details]
Patch

[Fady Samuel]

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.

[Fady Samuel]

Created attachment 113660 [details]
Patch

[Fady Samuel]

Created attachment 113662 [details]
Patch

[Fady Samuel]

Created attachment 113664 [details]
Patch

[Early Warning System Bot]

Comment on attachment 113664 [details]
Patch

Attachment 113664 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10310182

[Ojan Vafai]

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

[WebKit Review Bot]

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

[Fady Samuel]

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

[Fady Samuel]

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

[Fady Samuel]

Created attachment 113685 [details]
Patch

[Fady Samuel]

Comment on attachment 113685 [details]
Patch

Taking it through the commit queue, just in case.

[WebKit Review Bot]

Comment on attachment 113685 [details]
Patch

Clearing flags on attachment: 113685

Committed r99332: <http://trac.webkit.org/changeset/99332>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.