[webkit-reviews] review denied: [Bug 85580] Implement CSS Variables Standard. : [Attachment 144419] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 85580: Implement CSS Variables Standard.
https://bugs.webkit.org/show_bug.cgi?id=85580

Attachment 144419: Patch
https://bugs.webkit.org/attachment.cgi?id=144419&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=144419&action=review


This is difficult to review. It is supposed be an experimental feature so
things don't need to be optimal and regressing performance may be ok. We should
make sure the architectural direction is right though as it is likely to stick.


r- is for a bunch of smaller issues but please think if you can come up with
architecture that does not involve RenderStyle (or explain why it makes sense).


> Source/WebCore/ChangeLog:26
> +	   Tests: fast/css/variables/colors-test-expected.html
> +		  fast/css/variables/colors-test.html
> +		  fast/css/variables/complex-cycle-expected.html
> +		  fast/css/variables/complex-cycle.html

I don't see the tests in the patch.

> Source/WebCore/css/CSSGrammar.y:138
> +%token VAR_SYM

VARFUNCTION would seem more appropriate with the current syntax.

> Source/WebCore/css/CSSParser.cpp:1196
> +	       AtomicString name =
static_cast<CSSVariableValue*>(property.value())->name();

const AtomicString&

> Source/WebCore/css/CSSParser.cpp:2840
> +bool CSSParser::storeVariableDeclaration(const CSSParserString& name,
PassOwnPtr<CSSParserValueList> value, bool important)

What does the returned boolean signify? It is always true.

> Source/WebCore/css/CSSParser.cpp:2848
> +    StringBuilder builder;
> +    for (unsigned i = 0, size = value->size(); i < size; i++) {
> +	   if (i)
> +	       builder.append(' ');
> +	   builder.append(value->valueAt(i)->createCSSValue()->cssText());
> +    }
> +    addProperty(CSSPropertyVariable, CSSVariableValue::create(name,
builder.toString()), important, false);

Letting the parser parse the string as expr and then smashing it back together
into string by concatenating cssTexts seems less than ideal (and the API method
cssText shouldn't be used for internal purposes in general). Can't we have a
grammar that just gives the string?

> Source/WebCore/css/CSSPrimitiveValue.cpp:1090
> +#if ENABLE(CSS_VARIABLES)
> +String CSSPrimitiveValue::customCssText(const HashMap<AtomicString, String>&
variables) const
> +{
> +    if (m_primitiveUnitType == CSS_VARIABLE_NAME &&
variables.contains(m_value.string))
> +	   return variables.get(m_value.string);
> +    return customCssText();
> +}
> +#endif

As far as I can see the idea here (and in other similar functions you add) is
to find the value for a variable name. This little to do with the CSSOM
cssText() function (which is for serializing style and shouldn't be used for
internal purposes) so I'm confused about the naming and the factoring in
general. Why invoke this at all if it is not CSS_VARIABLE_NAME?

> Source/WebCore/css/CSSVariableValue.h:17
> + * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> + *	  its contributors may be used to endorse or promote products derived
> + *	  from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY

Are you sure this is the legal text you want?

> Source/WebCore/css/StyleResolver.cpp:2848
> +#if ENABLE(CSS_VARIABLES)
> +    // First apply all variable definitions, as they may be used during
application of later properties.
> +    applyMatchedProperties<VariableDefinitions>(matchResult, false, 0,
matchResult.matchedProperties.size() - 1, applyInheritedOnly);
> +    applyMatchedProperties<VariableDefinitions>(matchResult, true,
matchResult.ranges.firstAuthorRule, matchResult.ranges.lastAuthorRule,
applyInheritedOnly);
> +    applyMatchedProperties<VariableDefinitions>(matchResult, true,
matchResult.ranges.firstUserRule, matchResult.ranges.lastUserRule,
applyInheritedOnly);
> +    applyMatchedProperties<VariableDefinitions>(matchResult, true,
matchResult.ranges.firstUARule, matchResult.ranges.lastUARule,
applyInheritedOnly);
> +#endif

Additional iterations searching for variables are likely to slow down common
case. We need a better solution. Should be ok for experimental implementation
though.

> Source/WebCore/css/StyleResolver.cpp:3176
> +static bool hasVariableReference(CSSValue* value)
> +{
> +    if (value->isPrimitiveValue() &&
static_cast<CSSPrimitiveValue*>(value)->isVariableName())
> +	   return true;
> +
> +    for (CSSValueListIterator i = value; i.hasMore(); i.advance()) {
> +	   if (hasVariableReference(i.value()))
> +	       return true;
> +    }
> +
> +    return false;
> +}

An efficient implementation would cache the existence of variable ref to a bit
in the top level CSSValue.

> Source/WebCore/css/StyleResolver.cpp:3183
> +void StyleResolver::resolveVariables(CSSPropertyID id, CSSValue* value,
HashSet<String>* knownExpressions)
>  {
> +    if (!hasVariableReference(value)) {
> +	   applyProperty(id, value);
> +	   return;
> +    }

The top level call site already calls hasVariableReference(). This could move
to the loop below.

> Source/WebCore/css/StyleResolver.cpp:3185
> +    String expression = value->cssText(*style()->variables());

As noted above calling something called cssText() here is strange and (as noted
below) having variables part of the RenderStyle seems suspicious.

> Source/WebCore/css/StyleResolver.cpp:3188
> +    if (!knownExpressions->add(getPropertyName(id) + expression).isNewEntry)

> +	   return; // cycle detected.

The key shouldn't be formed by concatenating strings. You can use pair<> of any
hashable type as hash key.

I don't understand how this prevents cycles. Could you explain?

> Source/WebCore/css/StyleResolver.cpp:3192
> +    RefPtr<StylePropertySet> resultSet = StylePropertySet::create();
> +    if (!CSSParser::parseValue(resultSet.get(), id, expression, false,
CSSStrictMode, 0))
> +	   return; // expression failed to parse.

Parsing CSS values on style resolve time is going to be horribly slow. A good
approach might be to cache the parsed values to the variable declaration value
itself (with id/type key).

> Source/WebCore/css/StyleResolver.cpp:3205
> +void StyleResolver::applyProperty(CSSPropertyID id, CSSValue* value)
> +{
> +#if ENABLE(CSS_VARIABLES)
> +    if (hasVariableReference(value)) {
> +	   HashSet<String> knownExpressions;

Initializing and tearing down a HashSet for applying a single property seems
costly.

> Source/WebCore/css/StyleResolver.cpp:3225
> -    // check lookup table for implementations and use when available
> -    const PropertyHandler& handler = m_styleBuilder.propertyHandler(id);
> -    if (handler.isValid()) {
> -	   if (isInherit)
> -	       handler.applyInheritValue(this);
> -	   else if (isInitial)
> -	       handler.applyInitialValue(this);
> -	   else
> -	       handler.applyValue(this, value);
> -	   return;
> +    // Check lookup table for implementations and use when available.
> +    if (id >= firstCSSProperty) {

Why this change?

> Source/WebCore/rendering/style/RenderStyle.h:216
> +#if ENABLE(CSS_VARIABLES)
> +    DataRef<StyleVariableData> m_variables;
> +#endif

Having variables as part of the RenderStyle seems architecturally suspicious.
The RenderStyle is the input for rendering and layout. Rendering sees variables
as resolved values so it doesn't need to know about them. We don't have a
precedent, this is the first CSS property type that has no effect after style
resolve. I'm not sure what the right way to go here is.

In practical terms adding a pointer to RenderStyle has significant memory cost
whether variables are used in a document or not.

> Source/WebCore/rendering/style/StyleVariableData.h:52
> +class StyleVariableData : public RefCounted<StyleVariableData> {
> +public:
> +    static PassRefPtr<StyleVariableData> create() { return adoptRef(new
StyleVariableData()); }
> +    PassRefPtr<StyleVariableData> copy() const { return adoptRef(new
StyleVariableData(*this)); }
> +
> +    bool operator==(const StyleVariableData& other) const { return
other.m_data == m_data; }
> +    bool operator!=(const StyleVariableData& other) const { return !(*this
== other); }
> +
> +    void setVariable(const AtomicString& name, const String& value) {
m_data.set(name, value); }
> +    bool hasVariable(const AtomicString& name) const { return
m_data.contains(name); }
> +    String variable(const AtomicString& name) const { return
m_data.get(name); }
> +
> +    HashMap<AtomicString, String> m_data;

HashMaps are large. This class is very cost if there are definitions on
multiple levels and the instances don't get shared.