Bug 132747

Summary:

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

Product:

WebKit

Reporter:

Rebecca Hauck <rhauck>

Component:

CSS

Assignee:

Zoltan Horvath <zoltan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, commit-queue, esprehn+autocc, glenn, gyuyoung.kim, kondapallykalyan, macpherson, menard, rniwa, zoltan

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

98664

Attachments:

Description	Flags
Test case for bug	none
Patch	none
Patch	buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion	none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion	none
Patch	none
Patch	bjonesbe: review-
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2	none
Patch	none
Patch	none
Patch	none
Patch	none

Rebecca Hauck

Reported 2014-05-09 11:54:50 PDT

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.

Attachments
Test case for bug (1.42 KB, text/html) 2014-05-09 11:55 PDT, Rebecca Hauck	no flags	Details
Patch (23.10 KB, patch) 2014-05-21 13:01 PDT, Zoltan Horvath	no flags	Details Formatted Diff Diff
Patch (23.10 KB, patch) 2014-05-21 13:11 PDT, Zoltan Horvath	buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (541.24 KB, application/zip) 2014-05-21 14:58 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (539.76 KB, application/zip) 2014-05-21 15:57 PDT, Build Bot	no flags	Details
Patch (20.98 KB, patch) 2014-05-21 21:36 PDT, Zoltan Horvath	no flags	Details Formatted Diff Diff
Patch (20.66 KB, patch) 2014-05-21 21:37 PDT, Zoltan Horvath	bjonesbe: review-	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (532.55 KB, application/zip) 2014-05-21 22:39 PDT, Build Bot	no flags	Details
Patch (20.71 KB, patch) 2014-05-27 15:53 PDT, Zoltan Horvath	no flags	Details Formatted Diff Diff
Patch (20.68 KB, patch) 2014-05-27 15:56 PDT, Zoltan Horvath	no flags	Details Formatted Diff Diff
Patch (20.68 KB, patch) 2014-05-27 15:57 PDT, Zoltan Horvath	no flags	Details Formatted Diff Diff
Patch (20.68 KB, patch) 2014-05-27 16:00 PDT, Zoltan Horvath	no flags	Details Formatted Diff Diff
Show Obsolete (11) View All Add attachment proposed patch, testcase, etc.

Rebecca Hauck

Comment 1 2014-05-09 11:55:54 PDT

Created attachment 231173 [details] Test case for bug

Zoltan Horvath

Comment 2 2014-05-21 13:01:06 PDT

Created attachment 231837 [details] Patch

Zoltan Horvath

Comment 3 2014-05-21 13:11:37 PDT

Created attachment 231838 [details] Patch

Build Bot

Comment 4 2014-05-21 14:58:23 PDT

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

Comment 5 2014-05-21 14:58:27 PDT

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 6 2014-05-21 15:57:48 PDT

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

Comment 7 2014-05-21 15:57:52 PDT

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

Comment 8 2014-05-21 21:36:21 PDT

Created attachment 231852 [details] Patch

Zoltan Horvath

Comment 9 2014-05-21 21:37:16 PDT

Created attachment 231853 [details] Patch

Build Bot

Comment 10 2014-05-21 22:39:40 PDT

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

Comment 11 2014-05-21 22:39:43 PDT

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 12 2014-05-27 13:49:30 PDT

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

Comment 13 2014-05-27 15:53:07 PDT

Created attachment 232153 [details] Patch Comments are addressed in the patch.

WebKit Commit Bot

Comment 14 2014-05-27 15:54:24 PDT

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

Comment 15 2014-05-27 15:56:51 PDT

Created attachment 232155 [details] Patch All comments are addressed in this patch.

Zoltan Horvath

Comment 16 2014-05-27 15:57:56 PDT

Created attachment 232156 [details] Patch All comments are addressed in this patch.

Zoltan Horvath

Comment 17 2014-05-27 16:00:07 PDT

Created attachment 232157 [details] Patch

WebKit Commit Bot

Comment 18 2014-05-27 17:29:22 PDT

Comment on attachment 232157 [details] Patch Clearing flags on attachment: 232157 Committed r169406: <http://trac.webkit.org/changeset/169406>

WebKit Commit Bot

Comment 19 2014-05-27 17:29:27 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.