Bug 70707 - CSS Aspect Ratio Property Parsing Stage
Summary: CSS Aspect Ratio Property Parsing Stage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fady Samuel
URL:
Keywords:
Depends on:
Blocks: 47738
  Show dependency treegraph
 
Reported: 2011-10-23 21:20 PDT by Fady Samuel
Modified: 2011-11-04 16:07 PDT (History)
9 users (show)

See Also:


Attachments
Patch (10.15 KB, patch)
2011-10-23 21:22 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (10.20 KB, patch)
2011-10-23 21:49 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (10.14 KB, patch)
2011-10-23 22:33 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (15.85 KB, patch)
2011-10-29 19:07 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (16.04 KB, patch)
2011-10-31 12:33 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (16.10 KB, patch)
2011-11-01 15:39 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (20.19 KB, patch)
2011-11-03 17:07 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (20.19 KB, patch)
2011-11-03 19:32 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (20.11 KB, patch)
2011-11-03 19:58 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (20.11 KB, patch)
2011-11-03 20:01 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (20.11 KB, patch)
2011-11-03 20:20 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (20.72 KB, patch)
2011-11-04 07:40 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (20.16 KB, patch)
2011-11-04 08:43 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (24.65 KB, patch)
2011-11-04 08:56 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (24.81 KB, patch)
2011-11-04 09:15 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (25.42 KB, patch)
2011-11-04 11:28 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fady Samuel 2011-10-23 21:20:31 PDT
CSS Aspect Ratio Property Parsing Stage
Comment 1 Fady Samuel 2011-10-23 21:22:00 PDT
Created attachment 112139 [details]
Patch
Comment 2 Fady Samuel 2011-10-23 21:49:23 PDT
Created attachment 112144 [details]
Patch
Comment 3 WebKit Review Bot 2011-10-23 22:22:10 PDT
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
Comment 4 Fady Samuel 2011-10-23 22:33:13 PDT
Created attachment 112148 [details]
Patch
Comment 5 WebKit Review Bot 2011-10-23 23:12:20 PDT
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 6 Ojan Vafai 2011-10-24 10:04:30 PDT
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.
Comment 7 Simon Fraser (smfr) 2011-10-24 10:22:21 PDT
Is this in any CSS spec yet?
Comment 8 Fady Samuel 2011-10-24 14:42:16 PDT
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.
Comment 9 Fady Samuel 2011-10-24 14:44:46 PDT
(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.
Comment 10 Dave Hyatt 2011-10-25 15:47:14 PDT
I like the proposal. I'll take a look soon.
Comment 11 Fady Samuel 2011-10-29 19:07:56 PDT
Created attachment 112980 [details]
Patch
Comment 12 WebKit Review Bot 2011-10-29 19:54:18 PDT
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
Comment 13 Fady Samuel 2011-10-30 16:57:35 PDT
(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 14 Luke Macpherson 2011-10-31 11:22:38 PDT
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).
Comment 15 Fady Samuel 2011-10-31 12:33:19 PDT
Created attachment 113072 [details]
Patch
Comment 16 Fady Samuel 2011-10-31 12:35:36 PDT
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 17 WebKit Review Bot 2011-10-31 13:40:31 PDT
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 18 Dave Hyatt 2011-11-01 13:58:35 PDT
Comment on attachment 113072 [details]
Patch

Looks good to me.
Comment 19 Jarred Nicholls 2011-11-01 14:04:58 PDT
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?
Comment 20 Fady Samuel 2011-11-01 15:39:09 PDT
Created attachment 113241 [details]
Patch
Comment 21 Fady Samuel 2011-11-01 15:40:01 PDT
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 22 Ojan Vafai 2011-11-01 16:23:25 PDT
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".
Comment 23 Fady Samuel 2011-11-03 17:07:41 PDT
Created attachment 113585 [details]
Patch
Comment 24 WebKit Review Bot 2011-11-03 18:13:34 PDT
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 25 Ojan Vafai 2011-11-03 18:42:23 PDT
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.
Comment 26 Fady Samuel 2011-11-03 19:32:46 PDT
Created attachment 113609 [details]
Patch
Comment 27 Fady Samuel 2011-11-03 19:58:14 PDT
Created attachment 113610 [details]
Patch
Comment 28 Fady Samuel 2011-11-03 19:58:52 PDT
(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.
Comment 29 Fady Samuel 2011-11-03 20:01:37 PDT
Created attachment 113612 [details]
Patch
Comment 30 Fady Samuel 2011-11-03 20:20:09 PDT
Created attachment 113613 [details]
Patch
Comment 31 WebKit Review Bot 2011-11-03 21:16:53 PDT
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
Comment 32 Fady Samuel 2011-11-04 07:40:23 PDT
Created attachment 113654 [details]
Patch
Comment 33 Fady Samuel 2011-11-04 07:45:30 PDT
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.
Comment 34 Fady Samuel 2011-11-04 08:43:09 PDT
Created attachment 113660 [details]
Patch
Comment 35 Fady Samuel 2011-11-04 08:56:52 PDT
Created attachment 113662 [details]
Patch
Comment 36 Fady Samuel 2011-11-04 09:15:24 PDT
Created attachment 113664 [details]
Patch
Comment 37 Early Warning System Bot 2011-11-04 09:40:39 PDT
Comment on attachment 113664 [details]
Patch

Attachment 113664 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10310182
Comment 38 Ojan Vafai 2011-11-04 09:51:21 PDT
(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 39 WebKit Review Bot 2011-11-04 10:00:17 PDT
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
Comment 40 Fady Samuel 2011-11-04 10:01:37 PDT
(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.
Comment 41 Fady Samuel 2011-11-04 11:28:14 PDT
(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.
Comment 42 Fady Samuel 2011-11-04 11:28:32 PDT
Created attachment 113685 [details]
Patch
Comment 43 Fady Samuel 2011-11-04 15:22:41 PDT
Comment on attachment 113685 [details]
Patch

Taking it through the commit queue, just in case.
Comment 44 WebKit Review Bot 2011-11-04 16:07:37 PDT
Comment on attachment 113685 [details]
Patch

Clearing flags on attachment: 113685

Committed r99332: <http://trac.webkit.org/changeset/99332>
Comment 45 WebKit Review Bot 2011-11-04 16:07:45 PDT
All reviewed patches have been landed.  Closing bug.