Bug 127785 - [CSS Shapes] Adjust inset sizing syntax to the latest spec
Summary: [CSS Shapes] Adjust inset sizing syntax to the latest spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zoltan Horvath
URL:
Keywords:
Depends on:
Blocks: 124173
  Show dependency treegraph
 
Reported: 2014-01-28 11:31 PST by Zoltan Horvath
Modified: 2014-02-07 09:38 PST (History)
6 users (show)

See Also:


Attachments
Patch (43.38 KB, patch)
2014-01-28 11:35 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
Patch (44.03 KB, patch)
2014-01-28 14:01 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
Patch - linear parsing logic and less variable assignments (43.35 KB, patch)
2014-01-28 15:20 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
Patch - linear parsing logic and less variable assignments (43.08 KB, patch)
2014-01-28 16:10 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Horvath 2014-01-28 11:31:02 PST
According to the latest CSS Shapes specification [1], the width arguments of inset should follow the syntax of the margin shorthand, which let us set all four insets with one, two or four values.
This patch updates the behavior and updates the affected tests.

[1] http://dev.w3.org/csswg/css-shapes/#funcdef-inset
Comment 1 Zoltan Horvath 2014-01-28 11:35:23 PST
Created attachment 222469 [details]
Patch
Comment 2 Bem Jones-Bey 2014-01-28 11:47:07 PST
Comment on attachment 222469 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=222469&action=review

> Source/WebCore/css/CSSParser.cpp:5409
> +    if (widthArguments.size() == 1) {

I think a switch would be more readable here.

> Source/WebCore/css/CSSParser.cpp:5422
> +        shape->setBottom(widthArguments[2]);

You need to set Left here, and double check your tests, if this passed, you missed a test.
Comment 3 Bem Jones-Bey 2014-01-28 11:48:27 PST
Comment on attachment 222469 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=222469&action=review

> LayoutTests/fast/shapes/parsing/parsing-test-utils.js:19
>      ["inset(10px 9px 8px)", "inset(10px 9px 8px)", "inset(10px 9px 8px 0px round 0px 0px 0px 0px / 0px 0px 0px 0px)"],

Here's the wrong test!
Comment 4 Zoltan Horvath 2014-01-28 14:01:32 PST
Created attachment 222483 [details]
Patch

(In reply to comment #2)
> (From update of attachment 222469 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222469&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:5409
> > +    if (widthArguments.size() == 1) {
> 
> I think a switch would be more readable here.

I modified to use switch.

> > Source/WebCore/css/CSSParser.cpp:5422
> > +        shape->setBottom(widthArguments[2]);
> 
> You need to set Left here, and double check your tests, if this passed, you missed a test.

That behavior was the expected in my mind. :( I added the corrected case and I modified the affected test.
Comment 5 Zoltan Horvath 2014-01-28 15:20:54 PST
Created attachment 222505 [details]
Patch - linear parsing logic and less variable assignments
Comment 6 Zoltan Horvath 2014-01-28 16:10:15 PST
Created attachment 222526 [details]
Patch - linear parsing logic and less variable assignments
Comment 7 Bem Jones-Bey 2014-01-28 16:23:09 PST
Comment on attachment 222526 [details]
Patch - linear parsing logic and less variable assignments

r=me
Comment 8 WebKit Commit Bot 2014-01-28 17:17:43 PST
Comment on attachment 222526 [details]
Patch - linear parsing logic and less variable assignments

Clearing flags on attachment: 222526

Committed r162989: <http://trac.webkit.org/changeset/162989>
Comment 9 WebKit Commit Bot 2014-01-28 17:17:45 PST
All reviewed patches have been landed.  Closing bug.