CSS Properties and Values API: Implement computation / computational independence

third_party/WebKit/Source/core/css/PropertyRegistration.cpp

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/css/PropertyRegistration.h"
 
 #include "core/css/CSSSyntaxDescriptor.h"
+#include "core/css/CSSValueList.h"
 #include "core/css/CSSVariableReferenceValue.h"
 #include "core/css/PropertyDescriptor.h"
 #include "core/css/PropertyRegistry.h"
 #include "core/css/parser/CSSVariableParser.h"
 #include "core/dom/Document.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/dom/StyleChangeReason.h"
 
 namespace blink {
 
-static bool computationallyIdempotent(const CSSValue& value)
+static bool computationallyIndependent(const CSSValue& value)

[alancutter (OOO until 2018), 2016/09/20 00:15:55]

What happens to CSS wide keywords? Either check them or assert they're not given
as input.
Please add tests for them.

[Timothy Loh, 2016/09/21 01:12:04]

This is tested in register-property-syntax-parsing.html already. I'll add an
assert.

 {
-    // TODO(timloh): Implement this
+    if (value.isVariableReferenceValue())
+        return !toCSSVariableReferenceValue(value).variableDataValue()->needsVariableResolution();

[sashab, 2016/09/19 23:25:03]

Can variables not have percentages etc in them? It feels like this should be
recursive on the variable's value.

[alancutter (OOO until 2018), 2016/09/20 00:15:55]

It's very weird to be checking needsVariableResolution here. It doesn't make
sense for a CSSVariableReferenceValue to not have var() references, we should be
using CSSTokenStreamValues instead, please add it as a separate patch and assert
in CSSVariableReferenceValue::create() that the CSSVariableData has unresolved
references.

[Timothy Loh, 2016/09/21 01:12:04]

If there are variable references then this returns false, if there aren't (i.e.
it's just a regular token stream) then we don't do any computation on it
regardless of the contents.

[Timothy Loh, 2016/09/21 01:12:04]

There's no CSSTokenStreamValue class -- CSSVariableReferenceValue is the only
CSSValue subclass which stores a token stream. I can rename this later if you
want, I don't think it makes sense to have two CSSValue subclasses that are
effectively identical.

+
+    // TODO(timloh): Images and transform-function values can also contain lengths.

[sashab, 2016/09/19 23:25:03]

Are there tests that fail because of this?

[Timothy Loh, 2016/09/21 01:12:04]

Nope I'll add them when I resolve the TODO.

+
+    if (value.isValueList()) {
+        for (const CSSValue* innerValue : toCSSValueList(value)) {
+            if (!computationallyIndependent(*innerValue))
+                return false;
+        }
+        return true;
+    }
+
+    if (!value.isPrimitiveValue())

[sashab, 2016/09/19 23:25:03]

Just to prevent the kind of mess we currently have in CSSToStyleMap, I'd prefer
this written as

if (value.isPrimitiveValue()) {
  ...
}

return true;

[alancutter (OOO until 2018), 2016/09/20 00:15:55]

I think it's preferred to exit functions early on simple checks to avoid nested
code and provide guarantees for the remainder of the function. I can't find
references to this though...

[Timothy Loh, 2016/09/21 01:12:04]

I'll change this, it probably made more sense before I added the other cases
above.

+        return true;
+
+    const CSSPrimitiveValue& primitiveValue = toCSSPrimitiveValue(value);
+    if (!primitiveValue.isLength() && !primitiveValue.isCalculatedPercentageWithLength())
+        return true;
+
+    CSSPrimitiveValue::CSSLengthArray lengthArray;
+    primitiveValue.accumulateLengthArray(lengthArray);
+    for (size_t i = 0; i < lengthArray.values.size(); i++) {
+        if (lengthArray.typeFlags.get(i)
+            && i != CSSPrimitiveValue::UnitTypePixels
+            && i != CSSPrimitiveValue::UnitTypePercentage)
+            return false;
+    }
     return true;
 }
 
 void PropertyRegistration::registerProperty(ExecutionContext* executionContext, const PropertyDescriptor& descriptor, ExceptionState& exceptionState)
 {
     // Bindings code ensures these are set.
     DCHECK(descriptor.hasName());
     DCHECK(descriptor.hasInherits());
     DCHECK(descriptor.hasSyntax());
 
@@ skipping 15 matching lines @@
         exceptionState.throwDOMException(SyntaxError, "The syntax provided is not a valid custom property syntax.");
         return;
     }
 
     if (descriptor.hasInitialValue()) {
         const CSSValue* initial = syntaxDescriptor.parse(descriptor.initialValue());
         if (!initial) {
             exceptionState.throwDOMException(SyntaxError, "The initial value provided does not parse for the given syntax.");
             return;
         }
-        if (!computationallyIdempotent(*initial)) {
-            exceptionState.throwDOMException(SyntaxError, "The initial value provided is not computationally idempotent.");
+        if (!computationallyIndependent(*initial)) {
+            exceptionState.throwDOMException(SyntaxError, "The initial value provided is not computationally independent.");
             return;
         }
         registry.registerProperty(atomicName, syntaxDescriptor, descriptor.inherits(), initial);
     } else {
         if (!syntaxDescriptor.isTokenStream()) {
             exceptionState.throwDOMException(SyntaxError, "An initial value must be provided if the syntax is not '*'");
             return;
         }
         registry.registerProperty(atomicName, syntaxDescriptor, descriptor.inherits(), nullptr);
     }
@@ skipping 10 matching lines @@
     if (!registry.registration(atomicProperty)) {
         exceptionState.throwDOMException(NotFoundError, "CSS.unregisterProperty() called with non-registered property " + property);
         return;
     }
     registry.unregisterProperty(atomicProperty);
 
     document->setNeedsStyleRecalc(SubtreeStyleChange, StyleChangeReasonForTracing::create(StyleChangeReason::PropertyUnregistration));
 }
 
 } // namespace blink