Bug 82365

Summary: Update shape-inside/shape-outside CSS Exclusion properties
Product: WebKit Reporter: Bear Travis <betravis>
Component: CSSAssignee: Bear Travis <betravis>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, macpherson, menard, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 82363    
Attachments:
Description Flags
Proposed Patch
none
updated patch
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-02
none
updated patch
none
updated patch
none
simplified patch, split with 82667 none

Description Bear Travis 2012-03-27 12:03:13 PDT
The draft specification renames -webkit-wrap-shape-inside and -webkit-wrap-shape-outside to -webkit-shape-inside and -webkit-shape-outside
http://dev.w3.org/csswg/css3-exclusions/
Comment 1 Bear Travis 2012-03-27 13:22:35 PDT
Created attachment 134122 [details]
Proposed Patch

Simple name-change patch updating the exclusions css properties "-webkit-wrap-shape-inside" and "-webkit-wrap-shape-outside" to "-webkit-shape-inside" and "-webkit-shape-outside".
The current spec is located here:
http://dev.w3.org/csswg/css3-exclusions/
Comment 2 Alexis Menard (darktears) 2012-03-27 13:42:24 PDT
Comment on attachment 134122 [details]
Proposed Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2329
> +        case CSSPropertyWebkitShapeOutside:

static const int computedProperties[] = doesn't list it. That's odd and it's a bug to me. Maybe you could add a test case to cover where computedProperties is used in this file.

> Source/WebCore/css/CSSParser.cpp:2400
> +            return parseWrapShape((propId == CSSPropertyWebkitShapeInside), important);

Why you don't remove this function to parseShape (or some better name)?
Comment 3 Alexis Menard (darktears) 2012-03-27 13:43:14 PDT
(In reply to comment #2)
> (From update of attachment 134122 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134122&action=review
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2329
> > +        case CSSPropertyWebkitShapeOutside:
> 
> static const int computedProperties[] = doesn't list it. That's odd and it's a bug to me. Maybe you could add a test case to cover where computedProperties is used in this file.

And make sure CSSPropertyWebkitShapeOutside and CSSPropertyWebkitShapeInside are taken into account.

> 
> > Source/WebCore/css/CSSParser.cpp:2400
> > +            return parseWrapShape((propId == CSSPropertyWebkitShapeInside), important);
> 
> Why you don't remove this function to parseShape (or some better name)?
Comment 4 Bear Travis 2012-03-28 14:30:42 PDT
Created attachment 134402 [details]
updated patch

thanks for the quick feedback.
I have added shape-inside and shape-outside to the list of computed style properties, and updated the tests that check these properties.
I have also renamed the parsing methods to make them more clear. parseShape > parseClipShape, and parseWrapShape > parseExclusionShape.
Comment 5 Alexis Menard (darktears) 2012-03-28 14:50:44 PDT
Comment on attachment 134402 [details]
updated patch

Ok you need to seek a reviewer now :)
Comment 6 WebKit Review Bot 2012-03-28 18:15:59 PDT
Comment on attachment 134402 [details]
updated patch

Attachment 134402 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12193013

New failing tests:
svg/css/getComputedStyle-basic.xhtml
Comment 7 WebKit Review Bot 2012-03-28 18:16:06 PDT
Created attachment 134461 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 8 Bear Travis 2012-03-29 10:59:55 PDT
Created attachment 134618 [details]
updated patch

updating the expected test results for the failing test case
svg computed styles should include -webkit-shape properties
Comment 9 Ryosuke Niwa 2012-03-29 14:31:38 PDT
Comment on attachment 134618 [details]
updated patch

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

> LayoutTests/ChangeLog:11
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the description but after the bug title and the bug url.

> LayoutTests/platform/chromium-mac-snowleopard/svg/css/getComputedStyle-basic-expected.txt:450
> +rect: style.getPropertyValue(-webkit-shape-inside) : auto
> +rect: style.getPropertyCSSValue(-webkit-shape-inside) : [object CSSPrimitiveValue]
> +rect: style.getPropertyValue(-webkit-shape-outside) : auto
> +rect: style.getPropertyCSSValue(-webkit-shape-outside) : [object CSSPrimitiveValue]

I don't understand. Why are we adding these lines now?

And ditto for all lines that are added below.

> Source/WebCore/ChangeLog:11
> +        Reviewed by NOBODY (OOPS!).

Ditto.
Comment 10 Bear Travis 2012-03-29 14:57:26 PDT
Created attachment 134672 [details]
updated patch

updating changelogs

This patch adds -webkit-shape-inside and -webkit-shape-outside to the list of computed shape properties (CSSComputedStyleDeclaration::computedProperties)
Because they were not there before, they were not showing up in the result of a getComputedStyle js call.

With this addition, we need to update the tests that look at all the properties of the computed style.
Comment 11 Ryosuke Niwa 2012-03-29 14:58:45 PDT
(In reply to comment #10)
> This patch adds -webkit-shape-inside and -webkit-shape-outside to the list of computed shape properties (CSSComputedStyleDeclaration::computedProperties)
> Because they were not there before, they were not showing up in the result of a getComputedStyle js call.

Can we separate this change? It's hard to review a patch when it does more than one thing at a time.
Comment 12 Bear Travis 2012-03-29 15:01:59 PDT
(In reply to comment #11)
> Can we separate this change? It's hard to review a patch when it does more than one thing at a time.

Yes, I will separate this out into another bug and patch.
Comment 13 Bear Travis 2012-03-29 15:27:02 PDT
Created attachment 134682 [details]
simplified patch, split with 82667

simplified change after separating out the code to add the computed properties (bug 82667)
Comment 14 WebKit Review Bot 2012-03-29 18:17:53 PDT
Comment on attachment 134682 [details]
simplified patch, split with 82667

Clearing flags on attachment: 134682

Committed r112629: <http://trac.webkit.org/changeset/112629>
Comment 15 WebKit Review Bot 2012-03-29 18:18:11 PDT
All reviewed patches have been landed.  Closing bug.