Web Animations: Support iterator protocol in property indexed keyframe values

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

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 138 matching lines @@
     const Dictionary& dictionary = effectInput.getAsDictionary();
     DictionaryIterator iterator = dictionary.getIterator(executionContext);
     if (!iterator.isNull()) {
         // TODO(alancutter): Convert keyframes during iteration rather than after to match spec.
         Vector<Dictionary> keyframeDictionaries;
         if (exhaustDictionaryIterator(iterator, executionContext, exceptionState, keyframeDictionaries))
             return convertArrayForm(*element, keyframeDictionaries, exceptionState);
         return nullptr;
     }
 
-    return convertObjectForm(*element, dictionary, exceptionState);
+    return convertObjectForm(*element, dictionary, executionContext, exceptionState);
 }
 
 EffectModel* EffectInput::convertArrayForm(Element& element, const Vector<Dictionary>& keyframeDictionaries, ExceptionState& exceptionState)
 {
     StringKeyframeVector keyframes;
     double lastOffset = 0;
 
     for (const Dictionary& keyframeDictionary : keyframeDictionaries) {
         RefPtr<StringKeyframe> keyframe = StringKeyframe::create();
 
@@ skipping 43 matching lines @@
             setKeyframeValue(element, *keyframe.get(), property, value);
         }
         keyframes.append(keyframe);
     }
 
     DCHECK(!exceptionState.hadException());
 
     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;
 
     String timingFunctionString;
     RefPtr<TimingFunction> timingFunction = nullptr;
     if (DictionaryHelper::get(keyframeDictionary, "easing", timingFunctionString)) {
         timingFunction = AnimationInputHelpers::parseTimingFunction(timingFunctionString, &element.document(), exceptionState);
         if (!timingFunction)
             return nullptr;
     }
@@ skipping 10 matching lines @@
     Vector<String> keyframeProperties;
     keyframeDictionary.getPropertyNames(keyframeProperties);
     for (const auto& property : keyframeProperties) {
         if (property == "offset"
             || property == "composite"
             || property == "easing") {
             continue;
         }
 
         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) {
             RefPtr<StringKeyframe> keyframe = StringKeyframe::create();
 
             if (frameHasOffset)
                 keyframe->setOffset(offset);
             else if (numKeyframes == 1)
                 keyframe->setOffset(1.0);
             else
@@ skipping 12 matching lines @@
     }
 
     std::sort(keyframes.begin(), keyframes.end(), compareKeyframes);
 
     DCHECK(!exceptionState.hadException());
 
     return createEffectModelFromKeyframes(element, keyframes, exceptionState);
 }
 
 } // namespace blink