Bug 75841 – Extend CSSValueList to allow slash separated lists.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alexis Menard (darktears)]

Extend CSSValueList to allow slash separated lists.

[Alexis Menard (darktears)]

Created attachment 121642 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 121643 [details]
Patch

[Andreas Kling]

Comment on attachment 121643 [details]
Patch

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.

[Alexis Menard (darktears)]

Created attachment 121646 [details]
Patch

[Andreas Kling]

Comment on attachment 121646 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121646&action=review

r=me

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

Unnecessary comment.

[Alexis Menard (darktears)]

Created attachment 121648 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 121648 [details]
Patch for landing

Clearing flags on attachment: 121648

Committed r104452: <http://trac.webkit.org/changeset/104452>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.