[webkit-reviews] review denied: [Bug 102901] Implement CSSValue::operator==() : [Attachment 177501] Patch 4

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Alexander Shalamov
<alexander.shalamov at gmail.com>'s request for review:
Bug 102901: Implement CSSValue::operator==()
https://bugs.webkit.org/show_bug.cgi?id=102901

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

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=177501&action=review


This introduces lots of new code, some of it pretty complex, and I suspect we
lack sufficient test coverage for it. Since the operators are already used it
should be possible to write a layout test that systematically tests them in all
the value types (at minimum verifying that equal values really compare equal).

> Source/WebCore/css/CSSBasicShapes.cpp:81
> +    if (type() == shape.type()) {
> +	   const CSSBasicShapeRectangle& rhs = static_cast<const
CSSBasicShapeRectangle&>(shape);

It would be nicer to use early returns. 

if (type() != shape.type())
    return false;
...

to avoid indenting large blocks. This comment applies to many places in this
patch.

> Source/WebCore/css/CSSBasicShapes.cpp:82
> +	   return *m_x == *rhs.m_x && *m_y == *rhs.m_y && *m_width ==
*rhs.m_width && *m_height == *rhs.m_height

Do we know m_x and pals can't be null? This patch has lots of unexplained
dereferencing.

> Source/WebCore/css/CSSBasicShapes.cpp:84
> +	       && m_radiusX ? rhs.m_radiusX && *m_radiusX == *rhs.m_radiusX :
!rhs.m_radiusX
> +	       && m_radiusY ? rhs.m_radiusY && *m_radiusY == *rhs.m_radiusY :
!rhs.m_radiusY;

This is fairly hard to read. It might be useful to have an inline template
function for comparing CSSValue pointers. 

Some CSSValues, especially CSSPrimitiveValues are shared via CSSValuePool.
Equality test should always involve pointer comparison first.

> Source/WebCore/css/CSSBorderImageSliceValue.cpp:53
> +bool CSSBorderImageSliceValue::operator==(const CSSBorderImageSliceValue&
rhs) const

I think elsewhere we generally use 'other' as variable name in operator==.
'rsh' sounds strange.

> Source/WebCore/css/CSSPrimitiveValue.cpp:1261
> +    if (m_primitiveUnitType == rhs.m_primitiveUnitType)
> +	   switch (m_primitiveUnitType) {

Use early return.

> Source/WebCore/css/CSSValue.cpp:293
> +    if (m_classType == rhs.m_classType) {
> +	   switch (m_classType) {

Use early return.