[webkit-reviews] review denied: [Bug 72924] CSSValue: reorder ClassType enum to allow faster comparisons, add COMPILE_ASSERT on class size. : [Attachment 116162] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 72924: CSSValue: reorder ClassType enum to allow faster comparisons, add
COMPILE_ASSERT on class size.
https://bugs.webkit.org/show_bug.cgi?id=72924

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

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116162&action=review


Good idea, I'd like one more iteration though.

> Source/WebCore/css/CSSValue.h:97
> +	   /* Primitive class types must appear before PrimitiveClass */

Coding style, we use C++ comments (//), and end them in periods.

> Source/WebCore/css/CSSValue.h:103
> +	   /* Image generator classes */

Ditto.

> Source/WebCore/css/CSSValue.h:109
> +	   /* Timing function classes */

Ditto.

> Source/WebCore/css/CSSValue.h:114
> +	   /* Other class types */

Ditto.

> Source/WebCore/css/CSSValue.h:139
> +	   /* List class types must appear after ValueListClass */

Ditto.

> Source/WebCore/css/CSSValue.h:145
> +	   /* Do not append non-list class types here */

Ditto.

> Source/WebCore/css/CSSValue.h:157
> +	   COMPILE_ASSERT((sizeof(CSSValue) - sizeof(RefCounted<CSSValue>)) <=
4, CSS_value_packs_into_four_bytes);

This should be in CSSValue.cpp instead.

> Source/WebCore/css/CSSValue.h:173
> -    unsigned m_primitiveUnitType : 7; // CSSPrimitiveValue::UnitTypes
> +    unsigned char m_primitiveUnitType : 7; // CSSPrimitiveValue::UnitTypes

I suppose you're making this change because of MSVC? At least note it in the
ChangeLog as it's unrelated to the rest of the patch.