Bug 19660

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

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

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

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

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

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.

Fixed.

Reopening to implement (again).

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

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.

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

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 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.

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 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.

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

Created attachment 263101 [details] Patch

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 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 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

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 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

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 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.

Created attachment 263131 [details] Patch

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.

Landed in r191128.

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

(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?

(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?

(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?

ping?