Bug 19660 – Implement CSS Variables

• Component: CSS
• Status: RESOLVED FIXED

[Dave Hyatt]

This bug covers the initial landing of CSS variables support in WebKit.

[Dave Hyatt]

Created attachment 21824 [details]
Initial cut at CSS variables.

[Dave Hyatt]

Created attachment 21830 [details]
Updated to deal with Darin's big cleanup patch to the style system

[Dave Hyatt]

Created attachment 21839 [details]
Updated to deal with conflicts.

[Dave Hyatt]

Created attachment 21847 [details]
Ready for review.  Now with many tests and changelogs.

[Darin Adler]

Comment on attachment 21847 [details]
Ready for review.  Now with many tests and changelogs.

You fixed the "sepcification" typo but added a new typo:

// he CSS3 specification defines the format of a HSL color as

It should be "The", not "he".

+    bool ok = false;
+    if (m_variableNames.size()) {
+        ok = true;
+        declaration->addParsedVariable(variableName, m_variableValues[0]);
+        clearVariables();
+    } else
+        clearVariables();

Why not put the clearVariables function call outside the if statement?

+            break;
+        } else if (valueList->valueAt(i)->unit == CSSParserValue::Function) {

You don't need an else here after the break.

+    addProperty(propId, val.get(), important);

This doesn't need to be "val.get()" -- it should be "val.release()" since this is the last use of val in this function. Or just plain "val" would work too, but be less efficient. No need for get() in any case.

+        RefPtr<CSSPrimitiveValue> primitiveValue = CSSPrimitiveValue::createIdentifier(iValue);
+        primitiveValue->setPrimitiveType(CSSPrimitiveValue::CSS_PARSER_OPERATOR);
+        parsedValue = primitiveValue;

This should say "primitiveValue.release()" to avoid a tiny bit of refcount churn.

+    CSSParserValueList* args;
+
+    ~CSSParserFunction() { delete args; }

Maybe you could use OwnPtr instead here?

+            char c = static_cast<char>(m_value.ident);

What range of characters does this support? This expression won't work well for characters outside the 0-7F range.

+    // use with care!!!
+    void setPrimitiveType(unsigned short type) { m_type = type; }

What a crappy comment (I know you were just bringing this back). What does "with care" even mean? I use all functions with care. The argument should be the enum type, not just unsigned short, right?

But really we should get rid of this; add a new create function to use instead of createIdentifier for Operator and use a different union member and not bring this function back.

+    RefPtr<CSSMutableStyleDeclaration> resolvedDecl = m_resolvedVariablesDeclarations.get(decl);

You could just use a raw pointer here and add the get() call on this line.

+    virtual CSSParserValue parserValue() const { ASSERT_NOT_REACHED(); return CSSParserValue(); }

Can this function be pure virtual instead?

+    static PassRefPtr<CSSValueList> createFromParserValueList(CSSParserValueList* list)

Why not just name this "create"?

+    if (m_list)
+        return m_list->cssText();
+    return "";

+    CSSValue* val = getParsedVariable(variableName);
+    if (val)
+        return val->cssText();

I prefer the early return style in cases like these. Return in the error case then continue in the normal case. It's great when you have non-trivial code.

+    CSSVariableDependentValue(const PassRefPtr<CSSValueList>&);

This needs to just be a PassRefPtr, not a const PassRefPtr&.

+    ~CSSVariableDependentValue();

Please say virtual explicitly here.

+    CSSParser parser(useStrictParsing());
+    if (!parser.parseVariable(this, variableName, variableValue)) // If the parse succeeds, it will call addParsedVariable (our internal method for doing the add) with the parsed Value*.

+    RefPtr<MediaList> m_lstMedia;

Please just call this m_media. I've renamed it in other places in the existing classes.

+    void setDeclaration(CSSVariablesDeclaration* decl) { m_variables = decl; }

Please use PassRefPtr here.

+    CSSVariablesRule(CSSStyleSheet* parent, MediaList*);

Please use PassRefPtr for the MediaList.

+CSSVariablesRule::CSSVariablesRule(CSSStyleSheet* parent, MediaList* mediaList)
+    : CSSRule(parent)
+{
+    m_lstMedia = mediaList;
+}

Please use constructor syntax to initialize the mediaList -- it avoids an unneeded store and branch that happens later.

I don't see any tests for the cssText functions. I'd like to make sure we have some.

There's nothing fundamentally flawed here, so r=me

[Dave Hyatt]

Thanks for the comments.  This was actually already reviewed by Beth (who forgot to + the bug I guess).  I will make another checkin to clean things up based off your suggestions.

[Dave Hyatt]

Fixed.

[Dave Hyatt]

Reopening to implement (again).

[Dave Hyatt]

Created attachment 263084 [details]
Patch (not for review)

[WebKit Commit Bot]

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


ERROR: Source/WebCore/rendering/style/RenderStyle.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/rendering/style/RenderStyle.cpp:2034:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSValueList.cpp:24:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/css/CSSValueList.cpp:222:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/css/CSSValueList.cpp:243:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/css/StyleResolver.cpp:60:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/css/CSSVariableDependentValue.cpp:27:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/css/CSSVariableDependentValue.cpp:28:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/css/CSSParserValues.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/css/CSSVariableValue.cpp:73:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/css/CSSVariableValue.cpp:83:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/css/CSSPrimitiveValue.h:157:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/css/CSSPrimitiveValue.h:160:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/css/CSSVariableValue.h:57:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 14 in 46 files


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

[Dave Hyatt]

Created attachment 263089 [details]
Patch (not for review)

[WebKit Commit Bot]

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


ERROR: Source/WebCore/rendering/style/RenderStyle.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/css/CSSPrimitiveValue.h:157:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/css/CSSPrimitiveValue.h:160:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 3 in 48 files


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

[Build Bot]

Comment on attachment 263089 [details]
Patch (not for review)

Attachment 263089 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/287667

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 263096 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 263089 [details]
Patch (not for review)

Attachment 263089 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/287666

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 263097 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Dave Hyatt]

Created attachment 263101 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/rendering/style/RenderStyle.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/css/CSSPrimitiveValue.h:157:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/css/CSSPrimitiveValue.h:160:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 3 in 50 files


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

[Simon Fraser (smfr)]

Comment on attachment 263101 [details]
Patch

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

Reviewed for style, not substance.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3622
> +    const HashMap<AtomicString, RefPtr<CSSValue>>& customProperties = style->customProperties();

auto? or just remove the local variable.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3644
> +    const auto& customProperties = style->customProperties();

Remove local variable?

> Source/WebCore/css/CSSCustomPropertyValue.h:38
> +    static Ref<CSSCustomPropertyValue> create(const AtomicString& name, RefPtr<CSSValue>& value)

Should that be a RefPtr<>&& ? Or even a Ref<>&& ? Can it be null?

> Source/WebCore/css/CSSCustomPropertyValue.h:63
> +    bool isInvalid() const { return m_value == nullptr; }

!m_value

> Source/WebCore/css/CSSCustomPropertyValue.h:90
> +    bool m_containsVariables;

Put the bool at the end to reduce padding.

> Source/WebCore/css/CSSFunctionValue.cpp:65
> +bool CSSFunctionValue::buildParserValueSubstitutingVariables(CSSParserValue* result, const HashMap<AtomicString, RefPtr<CSSValue>>& customProperties) const

CSSParserValue&

> Source/WebCore/css/CSSFunctionValue.cpp:73
> +    result->function = new CSSParserFunction;
> +    result->function->name.init(m_name);
> +    bool success = true;
> +    if (m_args) {
> +        CSSParserValueList* argList = new CSSParserValueList;

Should eliminate these bare 'new's using unique_ptr.

> Source/WebCore/css/CSSParser.cpp:1916
> +    m_valueList.reset(new CSSParserValueList());

Can we get rid of the bare 'new' calls?

> Source/WebCore/css/CSSParser.cpp:1948
> +    

whitespace

> Source/WebCore/css/CSSParserValues.cpp:107
> +    

whitespace

> Source/WebCore/css/CSSValueList.cpp:207
> +bool CSSValueList::checkVariables(HashMap<AtomicString, RefPtr<CSSValue>>& customProperties, HashSet<AtomicString>& seenProperties, HashSet<AtomicString>& invalidProperties) const

Should probably have a typedef for HashMap<AtomicString, RefPtr<CSSValue>> since it's used all over.

> Source/WebCore/css/CSSValueList.cpp:210
> +    for (unsigned i = 0; i < m_values.size(); i++) {
> +        auto* value = item(i);

Modern enumeration?

> Source/WebCore/css/CSSValueList.cpp:245
> +    result->valueList = new CSSParserValueList();

Another new.

> Source/WebCore/css/CSSValueList.cpp:251
> +    for (unsigned i = 0; i < m_values.size(); ++i) {

Modern loop?

> Source/WebCore/css/CSSVariableDependentValue.h:67
> +        , m_serialized(false)

Use C++11 initializer.

> Source/WebCore/css/CSSVariableValue.h:51
> +    bool equals(const CSSVariableValue&) const;

Why not operator==?

> Source/WebCore/css/StyleResolver.cpp:822
> -
> +    

Whitespace.

> Source/WebCore/css/StyleResolver.cpp:992
> +    

Whitespace.

> Source/WebCore/css/StyleResolver.h:379
> +        

again

> Source/WebCore/rendering/style/RenderStyle.cpp:2001
> +    for (WTF::KeyValuePair<AtomicString, RefPtr<CSSValue>> entry : customProperties) {

auto?

> Source/WebCore/rendering/style/RenderStyle.cpp:2018
> +    for (WTF::KeyValuePair<AtomicString, RefPtr<CSSValue>> entry : customProperties) {

auto?

> Source/WebCore/rendering/style/StyleCustomPropertyData.h:47
> +        for (WTF::KeyValuePair<AtomicString, RefPtr<CSSValue>> entry : m_values) {

auto

> Source/WebCore/rendering/style/StyleCustomPropertyData.h:73
> +    bool m_containsVariables;

{ false };

[Build Bot]

Comment on attachment 263101 [details]
Patch

Attachment 263101 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/287917

New failing tests:
fast/css/table-border-spacing.html
fast/css/cssom-remove-shorthand-property.html
fast/dom/domListEnumeration.html
fast/css/parsing-expr-error-recovery.html
fast/css/style-enumerate-properties.html

[Build Bot]

Created attachment 263106 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 263101 [details]
Patch

Attachment 263101 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/287922

New failing tests:
fast/css/table-border-spacing.html
fast/css/cssom-remove-shorthand-property.html
fast/dom/domListEnumeration.html
fast/css/parsing-expr-error-recovery.html
fast/css/style-enumerate-properties.html

[Build Bot]

Created attachment 263107 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Dean Jackson]

Comment on attachment 263101 [details]
Patch

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

I guess you could split this up a bit. e.g.

- RenderStyle customProperties() returning a ref
- some bits of CSSParser.cpp
- moving toString to containsVariables (or most of the CSSParserValues changes)
- the CSSPrimitiveValue changes

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:10087
> +		7C4C96D81AD4483500365A51 /* ReadableStreamBuiltins.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ReadableStreamBuiltins.cpp; sourceTree = "<group>"; };
> +		7C4C96D81AD4483500365A52 /* CountQueuingStrategyBuiltins.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = CountQueuingStrategyBuiltins.cpp; sourceTree = "<group>"; };

What's this change?

> Source/WebCore/css/CSSCustomPropertyValue.h:82
> +        , m_value(nullptr)
> +        , m_containsVariables(false)

Use initializer syntax for these two...

> Source/WebCore/css/CSSCustomPropertyValue.h:90
> +    RefPtr<CSSValue> m_value;
> +    bool m_containsVariables;

.... down here. Actually, you only need it for the boolean.

> Source/WebCore/css/CSSParser.cpp:1925
> +    for (size_t i = 0; i < m_parsedProperties.size(); ++i) {
> +        if (m_parsedProperties[i].id() == propID)
> +            return m_parsedProperties[i].value();
> +    }

Is there a reason this can't be a modern for loop?

> Source/WebCore/css/CSSParser.cpp:1948
> -
> +    

Oops.

> Source/WebCore/css/CSSParserValues.cpp:107
> -
> +    

Oops again.

> Source/WebCore/css/CSSPrimitiveValue.cpp:1148
> -        return quoteCSSStringIfNeeded(m_value.string);
> +        return m_value.string;

No longer necessary?

> Source/WebCore/css/CSSVariableDependentValue.h:47
> +            m_stringValue = m_valueList ? m_valueList->customCSSText() : "";

Use emptyString()

> Source/WebCore/css/CSSVariableDependentValue.h:67
> +        , m_serialized(false)

Use initializer syntax.

> Source/WebCore/css/StyleResolver.cpp:823
> -
> +    
>      if (state.style()->hasViewportUnits())

Ooops.

> Source/WebCore/css/StyleResolver.cpp:992
> -
> +    

Oops.

> Source/WebCore/rendering/style/StyleCustomPropertyData.h:62
> +    void setCustomPropertyValue(const AtomicString& name, const RefPtr<CSSValue>& value)
> +    {
> +        m_values.set(name, value);
> +        if (value->isVariableDependentValue())
> +            m_containsVariables = true;
> +    }

I assume that we never set the value twice? i.e. something can't be set from something that contains variables to something that doesn't?

> Source/WebCore/rendering/style/StyleCustomPropertyData.h:78
> +        , m_containsVariables(false)

Another case of initializer syntax.

[Dave Hyatt]

Created attachment 263131 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/rendering/style/RenderStyle.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/css/CSSPrimitiveValue.h:157:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/css/CSSPrimitiveValue.h:160:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 3 in 50 files


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

[Dave Hyatt]

Landed in r191128.

[Jon Lee]

rdar://problem/14563910

[Csaba Osztrogonác]

(In reply to comment #27)
> Landed in r191128.

It caused a build failure and a build warning, 
see bug150321 and bug150325 for details.

[Csaba Osztrogonác]

(In reply to comment #29)
> (In reply to comment #27)
> > Landed in r191128.
> 
> It caused a build failure and a build warning, 
> see bug150321 and bug150325 for details.

bug150325 is still valid, are you planning to 
fix the regression which is caused by this patch?

[Csaba Osztrogonác]

(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #27)
> > > Landed in r191128.
> > 
> > It caused a build failure and a build warning, 
> > see bug150321 and bug150325 for details.
> 
> bug150325 is still valid, are you planning to 
> fix the regression which is caused by this patch?

ping?

[Csaba Osztrogonác]

(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #27)
> > > Landed in r191128.
> > 
> > It caused a build failure and a build warning, 
> > see bug150321 and bug150325 for details.
> 
> bug150325 is still valid, are you planning to 
> fix the regression which is caused by this patch?

ping?

[Csaba Osztrogonác]

ping?