[webkit-reviews] review denied: [Bug 132747] [CSS Shapes] serialization of the computed value should omit the default radii : [Attachment 231853] Patch

[bugzilla-daemon at webkit.org]

Bem Jones-Bey <bjonesbe at adobe.com> has denied Zoltan Horvath
<zoltan at webkit.org>'s request for review:
Bug 132747: [CSS Shapes] serialization of the computed value should omit the
default radii
https://bugs.webkit.org/show_bug.cgi?id=132747

Attachment 231853: Patch
https://bugs.webkit.org/attachment.cgi?id=231853&action=review

------- Additional Comments from Bem Jones-Bey <bjonesbe at adobe.com>
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.