Bug 19660

Summary: Implement CSS Variables
Product: WebKit Reporter: Dave Hyatt <hyatt>
Component: CSSAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Normal CC: bugzilla, buildbot, commit-queue, dino, gavin.sharp, jonlee, ossy, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
URL: http://disruptive-innovations.com/zoo/cssvariables/
Bug Depends on: 150321, 150325    
Bug Blocks:    
Attachments:
Description Flags
Initial cut at CSS variables.
none
Updated to deal with Darin's big cleanup patch to the style system
none
Updated to deal with conflicts.
none
Ready for review. Now with many tests and changelogs.
darin: review+
Patch (not for review)
none
Patch (not for review)
buildbot: commit-queue-
Archive of layout-test-results from ews101 for mac-mavericks
none
Archive of layout-test-results from ews105 for mac-mavericks-wk2
none
Patch
dino: review+, buildbot: commit-queue-
Archive of layout-test-results from ews103 for mac-mavericks
none
Archive of layout-test-results from ews104 for mac-mavericks-wk2
none
Patch bfulgham: review+

Description Dave Hyatt 2008-06-18 15:01:10 PDT
This bug covers the initial landing of CSS variables support in WebKit.
Comment 1 Dave Hyatt 2008-06-18 15:04:15 PDT
Created attachment 21824 [details]
Initial cut at CSS variables.
Comment 2 Dave Hyatt 2008-06-19 01:26:18 PDT
Created attachment 21830 [details]
Updated to deal with Darin's big cleanup patch to the style system
Comment 3 Dave Hyatt 2008-06-19 09:50:02 PDT
Created attachment 21839 [details]
Updated to deal with conflicts.
Comment 4 Dave Hyatt 2008-06-19 13:05:32 PDT
Created attachment 21847 [details]
Ready for review.  Now with many tests and changelogs.
Comment 5 Darin Adler 2008-06-19 15:05:11 PDT
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
Comment 6 Dave Hyatt 2008-06-19 15:08:06 PDT
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.

Comment 7 Dave Hyatt 2008-06-19 15:36:28 PDT
Fixed.

Comment 8 Dave Hyatt 2015-10-14 10:15:18 PDT
Reopening to implement (again).
Comment 9 Dave Hyatt 2015-10-14 10:16:16 PDT
Created attachment 263084 [details]
Patch (not for review)
Comment 10 WebKit Commit Bot 2015-10-14 10:18:44 PDT
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.
Comment 11 Dave Hyatt 2015-10-14 11:32:34 PDT
Created attachment 263089 [details]
Patch (not for review)
Comment 12 WebKit Commit Bot 2015-10-14 11:39:49 PDT
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.
Comment 13 Build Bot 2015-10-14 12:13:00 PDT
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.
Comment 14 Build Bot 2015-10-14 12:13:04 PDT
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
Comment 15 Build Bot 2015-10-14 12:14:43 PDT
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.
Comment 16 Build Bot 2015-10-14 12:14:47 PDT
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
Comment 17 Dave Hyatt 2015-10-14 13:04:58 PDT
Created attachment 263101 [details]
Patch
Comment 18 WebKit Commit Bot 2015-10-14 13:07:18 PDT
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.
Comment 19 Simon Fraser (smfr) 2015-10-14 13:25:43 PDT
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 };
Comment 20 Build Bot 2015-10-14 13:51:32 PDT
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
Comment 21 Build Bot 2015-10-14 13:51:35 PDT
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
Comment 22 Build Bot 2015-10-14 13:56:08 PDT
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
Comment 23 Build Bot 2015-10-14 13:56:12 PDT
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
Comment 24 Dean Jackson 2015-10-14 16:17:32 PDT
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.
Comment 25 Dave Hyatt 2015-10-14 18:31:33 PDT
Created attachment 263131 [details]
Patch
Comment 26 WebKit Commit Bot 2015-10-14 18:34:20 PDT
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.
Comment 27 Dave Hyatt 2015-10-15 12:49:43 PDT
Landed in r191128.
Comment 28 Jon Lee 2015-10-15 13:40:43 PDT
rdar://problem/14563910
Comment 29 Csaba Osztrogonác 2015-10-20 01:29:55 PDT
(In reply to comment #27)
> Landed in r191128.

It caused a build failure and a build warning, 
see bug150321 and bug150325 for details.
Comment 30 Csaba Osztrogonác 2015-11-09 05:00:58 PST
(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?
Comment 31 Csaba Osztrogonác 2015-11-12 01:44:53 PST
(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?
Comment 32 Csaba Osztrogonác 2015-11-16 03:15:17 PST
(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?
Comment 33 Csaba Osztrogonác 2015-11-19 04:47:56 PST
ping?