Bug 132747 – [CSS Shapes] serialization of the computed value should omit the default radii

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Rebecca Hauck]

For the following shape definitions:

circle(at 50px 50px)
circle(closest-side at 50px 50px)
ellipse(at 50px 50px)
ellipse(closest-side at 50px 50px)

The computed style should be circle(at 50px 50px) or ellipse(at 50px 50px), omitting the default radii.

The attached test for these 4 cases fails as the default radii is not omitted.

[Rebecca Hauck]

Created attachment 231173 [details]
Test case for bug

[Zoltan Horvath]

Created attachment 231837 [details]
Patch

[Zoltan Horvath]

Created attachment 231838 [details]
Patch

[Build Bot]

Comment on attachment 231838 [details]
Patch

Attachment 231838 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4714561223524352

New failing tests:
fast/shapes/shape-outside-floats/shape-outside-floats-circle-005.html
fast/shapes/shape-outside-floats/shape-outside-floats-circle-003.html

[Build Bot]

Created attachment 231843 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 231838 [details]
Patch

Attachment 231838 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5334979986849792

New failing tests:
fast/shapes/shape-outside-floats/shape-outside-floats-circle-005.html
fast/shapes/shape-outside-floats/shape-outside-floats-circle-003.html

[Build Bot]

Created attachment 231844 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Zoltan Horvath]

Created attachment 231852 [details]
Patch

[Zoltan Horvath]

Created attachment 231853 [details]
Patch

[Build Bot]

Comment on attachment 231853 [details]
Patch

Attachment 231853 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4722087885275136

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html

[Build Bot]

Created attachment 231858 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Bem Jones-Bey]

Comment on attachment 231853 [details]
Patch

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

> Source/WebCore/css/CSSBasicShapes.cpp:117
> +    String radius = m_radius ? m_radius->cssText() : String();
> +    if (m_radius && m_radius->getValueID() == CSSValueClosestSide)

You should just get rid of the ternary and make this a set of if statements. (for example here you get the cssText even if you're just going to throw it away.)

> Source/WebCore/css/CSSBasicShapes.cpp:184
> +        bool isRadiusXCSSValueClosestSide = m_radiusX->getValueID() == CSSValueClosestSide;
> +        bool isRadiusYCSSValueClosestSide = m_radiusY && m_radiusY->getValueID() == CSSValueClosestSide;
> +        if ((!isRadiusXCSSValueClosestSide) || (isRadiusXCSSValueClosestSide && m_radiusY && !isRadiusYCSSValueClosestSide))

I think you should change the name of these booleans to shouldSerializeRadusXValue (m_radiusX->getValueID() != CSSValueClosestSide) and shouldSerializeRadiusYValue (m_radiusY && m_radiusY->getValueID() != CSSValueClosestSide). Then you won't need as many negations in your if statements. You also won't need to check m_radiusY in both setting the boolean and in the if statements, which I found rather confusing.

> Source/WebCore/css/CSSBasicShapes.cpp:186
> +        if (m_radiusY && !isRadiusYCSSValueClosestSide)

Ditto.

[Zoltan Horvath]

Created attachment 232153 [details]
Patch

Comments are addressed in the patch.

[WebKit Commit Bot]

Attachment 232153 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSBasicShapes.cpp:186:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/css/CSSBasicShapes.cpp:187:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/css/CSSBasicShapes.cpp:188:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zoltan Horvath]

Created attachment 232155 [details]
Patch

All comments are addressed in this patch.

[Zoltan Horvath]

Created attachment 232156 [details]
Patch

All comments are addressed in this patch.

[Zoltan Horvath]

Created attachment 232157 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 232157 [details]
Patch

Clearing flags on attachment: 232157

Committed r169406: <http://trac.webkit.org/changeset/169406>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.