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: | P2 | ||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||
Bug Blocks: | 98664 | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Rebecca Hauck
2014-05-09 11:54:50 PDT
Created attachment 231173 [details]
Test case for bug
Created attachment 231837 [details]
Patch
Created attachment 231838 [details]
Patch
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 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
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 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
Created attachment 231852 [details]
Patch
Created attachment 231853 [details]
Patch
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 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
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. Created attachment 232153 [details]
Patch
Comments are addressed in the patch.
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.
Created attachment 232155 [details]
Patch
All comments are addressed in this patch.
Created attachment 232156 [details]
Patch
All comments are addressed in this patch.
Created attachment 232157 [details]
Patch
Comment on attachment 232157 [details] Patch Clearing flags on attachment: 232157 Committed r169406: <http://trac.webkit.org/changeset/169406> All reviewed patches have been landed. Closing bug. |