Web Animations: Support iterator protocol in property indexed keyframe values

Web Animations: Support iterator protocol in property indexed keyframe values

For element.animate() calls of the form
"element.animate({property: [a, b, c]}, timing);" we were not accepting
non-array iterables for the value list.

This patch extends support for generic iterable objects to property indexed
keyframe values.

This behaviour corresponds to step 6.3 of
http://w3c.github.io/web-animations/#process-a-keyframe-like-object
where we cast the values as a (DOMString or sequence<DOMString>) object.

BUG=645393
Committed: https://crrev.com/3abd8209bdc0a59fda5350b4e893c1eb0a28be88
Cr-Commit-Position: refs/heads/master@{#418507}

[alancutter (OOO until 2018), 2016-09-13 06:37:26]

Description was changed from

==========
Web Animations: Support iterator protocol in property indexed keyframe values

For element.animate() calls of the form
element.animate({
    property: [a, b, c],
}, timing);
we were not accepting non-array iterables for the value list.

This patch extends support for generic iterable objects to property indexed
keyframe values.

BUG=645393
==========

to

==========
Web Animations: Support iterator protocol in property indexed keyframe values

For element.animate() calls of the form
"element.animate({property: [a, b, c]}, timing);" we were not accepting
non-array iterables for the value list.

This patch extends support for generic iterable objects to property indexed
keyframe values.

BUG=645393
==========

[alancutter (OOO until 2018), 2016-09-13 06:38:02]

alancutter@chromium.org changed reviewers:
+ haraken@chromium.org, suzyh@chromium.org

[alancutter (OOO until 2018), 2016-09-13 06:38:03]

suzyh for core/animation.
haraken for bindings.

[haraken, 2016-09-13 06:42:05]

LGTM

[suzyh_UTC10 (ex-contributor), 2016-09-13 07:19:57]

Devil's advocate: Does the spec say we need to support iterators here, or is
there a discussion of this on a mailing list for reference?

[alancutter (OOO until 2018), 2016-09-13 07:29:36]

Description was changed from

==========
Web Animations: Support iterator protocol in property indexed keyframe values

For element.animate() calls of the form
"element.animate({property: [a, b, c]}, timing);" we were not accepting
non-array iterables for the value list.

This patch extends support for generic iterable objects to property indexed
keyframe values.

BUG=645393
==========

to

==========
Web Animations: Support iterator protocol in property indexed keyframe values

For element.animate() calls of the form
"element.animate({property: [a, b, c]}, timing);" we were not accepting
non-array iterables for the value list.

This patch extends support for generic iterable objects to property indexed
keyframe values.

This behaviour corresponds to step 6.3 of
http://w3c.github.io/web-animations/#process-a-keyframe-like-object
where we convert the values from a (DOMString or sequence<DOMString>) object.

BUG=645393
==========

[alancutter (OOO until 2018), 2016-09-13 07:29:51]

On 2016/09/13 at 07:19:57, suzyh wrote:
> Devil's advocate: Does the spec say we need to support iterators here, or is
there a discussion of this on a mailing list for reference?

Added spec link.

[alancutter (OOO until 2018), 2016-09-13 07:48:13]

Description was changed from

==========
Web Animations: Support iterator protocol in property indexed keyframe values

For element.animate() calls of the form
"element.animate({property: [a, b, c]}, timing);" we were not accepting
non-array iterables for the value list.

This patch extends support for generic iterable objects to property indexed
keyframe values.

This behaviour corresponds to step 6.3 of
http://w3c.github.io/web-animations/#process-a-keyframe-like-object
where we convert the values from a (DOMString or sequence<DOMString>) object.

BUG=645393
==========

to

==========
Web Animations: Support iterator protocol in property indexed keyframe values

For element.animate() calls of the form
"element.animate({property: [a, b, c]}, timing);" we were not accepting
non-array iterables for the value list.

This patch extends support for generic iterable objects to property indexed
keyframe values.

This behaviour corresponds to step 6.3 of
http://w3c.github.io/web-animations/#process-a-keyframe-like-object
where we cast the values as a (DOMString or sequence<DOMString>) object.

BUG=645393
==========

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

Ideally I'd like to separate code that needs to know about parsing stuff and
iterators from animations logic code, but I won't block this patch on it. If you
have any ideas for how to disentangle these different parts, please capture that
in a bug or our fledgling backlog doc.

https://codereview.chromium.org/2334123002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/EffectInput.cpp (right):

https://codereview.chromium.org/2334123002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/EffectInput.cpp:233: if
(iterator.isNull()) {
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.

https://codereview.chromium.org/2334123002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/EffectInput.cpp:281: return nullptr;
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]

Thanks for the thorough review.

https://codereview.chromium.org/2334123002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/EffectInput.cpp (right):

https://codereview.chromium.org/2334123002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/EffectInput.cpp:233: if
(iterator.isNull()) {
On 2016/09/13 at 08:21:21, suzyh wrote:
> 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.

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

https://codereview.chromium.org/2334123002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/EffectInput.cpp:281: return nullptr;
On 2016/09/13 at 08:21:21, suzyh wrote:
> 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.)

Added throwTypeError() call and test case.

[alancutter (OOO until 2018), 2016-09-13 09:43:30]

Heh, turns out the old code crashes on a slight variant of the new test case.
Added crash test.

[alancutter (OOO until 2018), 2016-09-13 09:53:12]

On 2016/09/13 at 09:43:30, alancutter wrote:
> Heh, turns out the old code crashes on a slight variant of the new test case.
> Added crash test.

BTW the crash appears to be a nullptr crash, no major security concern.

[suzyh_UTC10 (ex-contributor), 2016-09-14 01:07:53]

lgtm with nit

https://codereview.chromium.org/2334123002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/EffectInput.cpp (right):

https://codereview.chromium.org/2334123002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/EffectInput.cpp:229: // The above get()
only works for Array objects, we still need to handle objects that satisfy the
ES6 iterator protocol.
Nit: As discussed in person, remove this comment and instead preface each case
in this function with a comment about what kind of input it handles.

[alancutter (OOO until 2018), 2016-09-14 01:11:34]

On 2016/09/14 at 01:07:53, suzyh wrote:
> lgtm with nit
> 
>
https://codereview.chromium.org/2334123002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/animation/EffectInput.cpp (right):
> 
>
https://codereview.chromium.org/2334123002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/animation/EffectInput.cpp:229: // The above
get() only works for Array objects, we still need to handle objects that satisfy
the ES6 iterator protocol.
> Nit: As discussed in person, remove this comment and instead preface each case
in this function with a comment about what kind of input it handles.

Done.

[alancutter (OOO until 2018), 2016-09-14 01:11:41]

The CQ bit was checked by alancutter@chromium.org

[alancutter (OOO until 2018), 2016-09-14 01:11:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, suzyh@chromium.org
Link to the patchset: https://codereview.chromium.org/2334123002/#ps80001
(title: "moarcomntents.")

[commit-bot: I haz the power, 2016-09-14 01:12:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 03:46:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 03:46:10]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[alancutter (OOO until 2018), 2016-09-14 06:20:53]

The CQ bit was checked by alancutter@chromium.org

[alancutter (OOO until 2018), 2016-09-14 06:20:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, suzyh@chromium.org
Link to the patchset: https://codereview.chromium.org/2334123002/#ps100001
(title: "Add missing null check")

[commit-bot: I haz the power, 2016-09-14 06:21:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 07:59:30]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-14 08:01:45]

Description was changed from

==========
Web Animations: Support iterator protocol in property indexed keyframe values

For element.animate() calls of the form
"element.animate({property: [a, b, c]}, timing);" we were not accepting
non-array iterables for the value list.

This patch extends support for generic iterable objects to property indexed
keyframe values.

This behaviour corresponds to step 6.3 of
http://w3c.github.io/web-animations/#process-a-keyframe-like-object
where we cast the values as a (DOMString or sequence<DOMString>) object.

BUG=645393
==========

to

==========
Web Animations: Support iterator protocol in property indexed keyframe values

For element.animate() calls of the form
"element.animate({property: [a, b, c]}, timing);" we were not accepting
non-array iterables for the value list.

This patch extends support for generic iterable objects to property indexed
keyframe values.

This behaviour corresponds to step 6.3 of
http://w3c.github.io/web-animations/#process-a-keyframe-like-object
where we cast the values as a (DOMString or sequence<DOMString>) object.

BUG=645393

Committed: https://crrev.com/3abd8209bdc0a59fda5350b4e893c1eb0a28be88
Cr-Commit-Position: refs/heads/master@{#418507}
==========

[commit-bot: I haz the power, 2016-09-14 08:01:46]

Patchset 6 (id:??) landed as
https://crrev.com/3abd8209bdc0a59fda5350b4e893c1eb0a28be88
Cr-Commit-Position: refs/heads/master@{#418507}