[webkit-reviews] review granted: [Bug 102901] Implement CSSValue::operator==() : [Attachment 183732] Patch 6

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted 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 183732: Patch 6
https://bugs.webkit.org/attachment.cgi?id=183732&action=review

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


r=me with some comments.

> LayoutTests/cssom/cssvalue-comparison-expected.txt:4
> +PASS Two CSSValues "20%" for property "width" are equal. 

This could also include a test per type sanity checking that non-equal values
don't compare equal.

> Source/WebCore/css/CSSValue.cpp:81
> +template<class ChildClassType>
> +inline static bool compareCSSValues(const CSSValue& first, const CSSValue&
second)
> +{
> +    return static_cast<const
ChildClassType&>(first).equals(static_cast<const ChildClassType&>(second));
> +}

This helper could be just above the only function that uses it.

> Source/WebCore/css/CSSValue.h:246
> +    size_t size = firstVector.size();
> +    if (size == secondVector.size()) {
> +	   for (size_t i = 0; i < size; i++) {

Could do early return instead.

> Source/WebCore/css/FontValue.cpp:75
> +    return compareCSSValuePtr<CSSPrimitiveValue>(style, other.style)
> +	   && compareCSSValuePtr<CSSPrimitiveValue>(variant, other.variant)
> +	   && compareCSSValuePtr<CSSPrimitiveValue>(weight, other.weight)
> +	   && compareCSSValuePtr<CSSPrimitiveValue>(size, other.size)
> +	   && compareCSSValuePtr<CSSPrimitiveValue>(lineHeight,
other.lineHeight)
> +	   && compareCSSValuePtr<CSSValueList>(family, other.family);

Shouldn't the compiler be able to figure out the template argument type without
providing it explicitly pretty much everywhere?