Web Animations: Support iterator protocol in property indexed keyframe values

third_party/WebKit/Source/core/animation/EffectInput.cpp

Index: third_party/WebKit/Source/core/animation/EffectInput.cpp
diff --git a/third_party/WebKit/Source/core/animation/EffectInput.cpp b/third_party/WebKit/Source/core/animation/EffectInput.cpp
index 940dcd77a75365adc6f6af95fb36078cc84c6bbb..56e829418d1144381d2a733fc9d497d7feb8ee51 100644
--- a/third_party/WebKit/Source/core/animation/EffectInput.cpp
+++ b/third_party/WebKit/Source/core/animation/EffectInput.cpp
@@ -156,7 +156,7 @@ EffectModel* EffectInput::convert(Element* element, const DictionarySequenceOrDi
         return nullptr;
     }
 
-    return convertObjectForm(*element, dictionary, exceptionState);
+    return convertObjectForm(*element, dictionary, executionContext, exceptionState);
 }
 
 EffectModel* EffectInput::convertArrayForm(Element& element, const Vector<Dictionary>& keyframeDictionaries, ExceptionState& exceptionState)
@@ -220,7 +220,33 @@ EffectModel* EffectInput::convertArrayForm(Element& element, const Vector<Dictio
     return createEffectModelFromKeyframes(element, keyframes, exceptionState);
 }
 
-EffectModel* EffectInput::convertObjectForm(Element& element, const Dictionary& keyframeDictionary, ExceptionState& exceptionState)
+static bool getPropertyIndexedKeyframeValues(const Dictionary& keyframeDictionary, const String& property, ExecutionContext* executionContext, ExceptionState& exceptionState, Vector<String>& result)
+{
+    DCHECK(result.isEmpty());
+    if (DictionaryHelper::get(keyframeDictionary, property, result))
+        return true;
+
+    // The above get() only works for Array objects, we still need to handle objects that satisfy the ES6 iterator protocol.
+    Dictionary valuesDictionary;
+    bool success = keyframeDictionary.get(property, valuesDictionary);
+    DictionaryIterator iterator(success ? valuesDictionary.getIterator(executionContext) : nullptr);
+    if (iterator.isNull()) {

[suzyh_UTC10 (ex-contributor), 2016/09/13 08:21:21]

This code is difficult to read/understand. IIUC, it does this:
1. try to read keyframeDictionary[property] as a dictionary
2. if that succeeds, create an iterator over that dictionary
3. if either 1 or 2 failed, it's just a single value, so interpret it as a
string and exit early.

Why might 2 fail if 1 succeeded? That is, can we simplify by testing "if
(!success)" before creating the iterator?

If we can't simplify the code, please instead add a comment inside the "if
(iterator.isNull())" test to explain what this means.

[alancutter (OOO until 2018), 2016/09/13 09:31:28]

I was avoiding code duplication. Probably not worthwhile for the complicated
code that resulted. Rewrote with code duplication.

+        String value;
+        DictionaryHelper::get(keyframeDictionary, property, value);
+        result.append(value);
+        return true;
+    }
+
+    while (iterator.next(executionContext, exceptionState)) {
+        String value;
+        if (!iterator.valueAsString(value))
+            return false;
+        result.append(value);
+    }
+    return !exceptionState.hadException();
+}
+
+EffectModel* EffectInput::convertObjectForm(Element& element, const Dictionary& keyframeDictionary, ExecutionContext* executionContext, ExceptionState& exceptionState)
 {
     StringKeyframeVector keyframes;
 
@@ -251,12 +277,8 @@ EffectModel* EffectInput::convertObjectForm(Element& element, const Dictionary&
         }
 
         Vector<String> values;
-        bool isList = DictionaryHelper::get(keyframeDictionary, property, values);
-        if (!isList) {
-            String value;
-            DictionaryHelper::get(keyframeDictionary, property, value);
-            values.append(value);
-        }
+        if (!getPropertyIndexedKeyframeValues(keyframeDictionary, property, executionContext, exceptionState, values))
+            return nullptr;

[suzyh_UTC10 (ex-contributor), 2016/09/13 08:21:21]

In other places where we return nullptr, it happens after a call to
exceptionState.throwDOMException or exceptionState.throwTypeError. I'm not sure
if this is supposed to be an invariant, so just to check: should we be throwing
an error at this point? (AFAICT, we could get here without an error being thrown
if iterator.valueAsString returns false; not sure if there might be other such
cases.)

[alancutter (OOO until 2018), 2016/09/13 09:31:28]

Added throwTypeError() call and test case.

 
         size_t numKeyframes = values.size();
         for (size_t i = 0; i < numKeyframes; ++i) {