[webkit-reviews] review denied: [Bug 75841] Extend CSSValueList to allow slash separated lists. : [Attachment 121643] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied Alexis Menard
<alexis.menard at openbossa.org>'s request for review:
Bug 75841: Extend CSSValueList to allow slash separated lists.
https://bugs.webkit.org/show_bug.cgi?id=75841

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

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


Great solution to the bit shortage, just needs a bit of polish.

> Source/WebCore/css/CSSInitialValue.h:51
> +    bool m_isImplicit : 1;

No need for this to be a bitfield.

> Source/WebCore/css/CSSValue.h:161
> +	   , m_valueListSeparator(CommaSeparator)

SpaceSeparator strikes me as a slightly more suitable default value here, since
that would make all fields 0 in a newly constructed CSSValue.
The actual value doesn't matter anyway since all CSSValueList constructors
overwrite it.

> Source/WebCore/css/CSSValue.h:184
> +    unsigned char m_valueListSeparator : ValueListSeparatorBits; //
CSSValueList type

The "// CSSValueList type" comment adds nothing.

> Source/WebCore/css/CSSValueList.cpp:129
> -	       if (isSpaceSeparated())
> +	       switch (m_valueListSeparator) {
> +	       case SpaceSeparator:
>		   result += " ";
> -	       else
> +		   break;
> +	       case CommaSeparator:
>		   result += ", ";
> +		   break;
> +	       case SlashSeparator:
> +		   result += " / ";
> +		   break;
> +	       default:
> +		   ASSERT_NOT_REACHED();
> +	       }

There's no need for this to be inside the loop. We can determine the right
separator string beforehand and store it in a String.

> Source/WebCore/css/CSSValueList.h:45
> +    static PassRefPtr<CSSValueList> createSlashSeparated()
> +    {
> +	   return adoptRef(new CSSValueList(SlashSeparator));
>      }

We shouldn't add a create*() method if we're not going to use it.