Bug 75725 – Remove m_isSpaceSeparatedValueList from CSSValue

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

[Tony Chang]

Remove m_isSpaceSeparatedValueList from CSSValueList

[Tony Chang]

Created attachment 121466 [details]
Patch

[Tony Chang]

This will make it easier to add a slash separated css value list since multiple css properties now use slashes to break up the various parts.

[Alexis Menard (darktears)]

Comment on attachment 121466 [details]
Patch

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

That would work and we can possibly add a new ValueListSlashSeparatedClass value.

> Source/WebCore/css/CSSValueList.h:68
> +    bool isSpaceSeparated() const { return classType() == ValueListSpaceSeparatedClass || classType() == WebKitCSSFilterSpaceSeparatedClass || classType() == ValueListSpaceSeparatedClass; }

Why two times classType() == ValueListSpaceSeparatedClass ?

[Tony Chang]

Created attachment 121470 [details]
Patch

[Tony Chang]

(In reply to comment #3)
> (From update of attachment 121466 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121466&action=review
> 
> > Source/WebCore/css/CSSValueList.h:68
> > +    bool isSpaceSeparated() const { return classType() == ValueListSpaceSeparatedClass || classType() == WebKitCSSFilterSpaceSeparatedClass || classType() == ValueListSpaceSeparatedClass; }
> 
> Why two times classType() == ValueListSpaceSeparatedClass ?

Oops, I was over aggressive with copy/paste. Removed.

[Alexis Menard (darktears)]

Comment on attachment 121470 [details]
Patch

That looks good, less hackish than https://bugs.webkit.org/show_bug.cgi?id=75714

[Early Warning System Bot]

Comment on attachment 121470 [details]
Patch

Attachment 121470 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11170052

[Alexis Menard (darktears)]

Comment on attachment 121470 [details]
Patch

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

> Source/WebCore/css/CSSValueList.h:68
> +    bool isSpaceSeparated() const { return classType() == ValueListSpaceSeparatedClass || classType() == WebKitCSSFilterSpaceSeparatedClass; }

You need to protect the classType() == WebKitCSSFilterSpaceSeparatedClass in a #if ENABLE(CSS_FILTERS). We don't enable them in the Qt port.

[Tony Chang]

Created attachment 121487 [details]
Patch

[Tony Chang]

There will be a style warning for the semicolon being on its own line, but this seems like the pattern in other places.

+    return classType() == ValueListSpaceSeparatedClass
+#if ENABLE(CSS_FILTERS)
+        || classType() == WebKitCSSFilterSpaceSeparatedClass
+#endif
+        ;

[WebKit Review Bot]

Attachment 121487 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/css/CSSValueList.cpp:57:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexis Menard (darktears)]

Tony I think this use case was solved by https://bugs.webkit.org/show_bug.cgi?id=75841 which fixed our need for border-radius. Would you mind closing it?

[Tony Chang]

The patch on 75841 seems less good in that we're still going to run out of ClassTypeBits (we're at 31 and have space for 32) soon, but if that's what has been decided on, then I'm happy to close this bug.