CSS Properties and Values API: Implement computation / computational independence

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

Index: third_party/WebKit/Source/core/css/PropertyRegistration.cpp
diff --git a/third_party/WebKit/Source/core/css/PropertyRegistration.cpp b/third_party/WebKit/Source/core/css/PropertyRegistration.cpp
index f7723db0ea2010a640cd791d3b25e0017872c474..bb319f72c687d4232acec0fbe469456887fa35a5 100644
--- a/third_party/WebKit/Source/core/css/PropertyRegistration.cpp
+++ b/third_party/WebKit/Source/core/css/PropertyRegistration.cpp
@@ -5,6 +5,7 @@
 #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"
@@ -15,9 +16,36 @@
 
 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;
 }
 
@@ -53,8 +81,8 @@ void PropertyRegistration::registerProperty(ExecutionContext* executionContext,
             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);