Web Animations: Fix bugs in procedure to process a keyframes argument #10399

chromium-wpt-export-bot · 2018-04-10T14:51:52Z

There were three minor bugs left in the implementation:

We threw on lists-in-custom-iterators instead of just ignoring them.
We returned all properties on the keyframe rather than just those
defined on the keyframe itself (e.g. we would include prototype
properties, against spec).
We didn't access the properties in ascending unicode order.

Bug: 827573
Change-Id: I213ae5b24e1f35d7f28d16625025122950a6ba88
Reviewed-on: https://chromium-review.googlesource.com/989261
Reviewed-by: Kentaro Hara haraken@chromium.org
Reviewed-by: Yuki Shiino yukishiino@chromium.org
Reviewed-by: Robert Flack flackr@chromium.org
Commit-Queue: Stephen McGruer smcgruer@chromium.org
Cr-Commit-Position: refs/heads/master@{#550641}

wpt-pr-bot

Already reviewed downstream.

stephenmcgruer · 2018-04-10T18:58:24Z

@birtles FYI this is an upstream (Chromium) change, but wanted your input on the test changes I am making.

birtles · 2018-04-11T07:23:00Z

web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-001.html

+  const test_error = { name: 'test' };
+  const bad_keyframe = { get left() { throw test_error; } };
+  assert_throws(bad_keyframe, () => {
+    new KeyframeEffect(null, createIteratble([


s/createIteratble/createIterable/

birtles · 2018-04-11T07:27:04Z

web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-001.html

+      return 42;  // Not an object.
+    },
+  };
+  assert_throws(new TypeError(), () => {


I think this case be just { name: 'TypeError' } right? For consistency.

birtles · 2018-04-11T07:41:40Z

web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-001.html

+test(() => {
+  const test_error = { name: 'test' };
+  const bad_keyframe = { get left() { throw test_error; } };
+  assert_throws(bad_keyframe, () => {


s/bad_keyframe/test_error/

birtles · 2018-04-11T07:52:41Z

I'm not sure if we should throw when the iterator returns null or undefined. As the spec currently reads we should, but it might be more consistent with regular WebIDL behavior to treat null or undefined as the default {} dictionary value. That's what Firefox does.

@heycam Do you happen to know if, from a WebIDL point of view, if we're iterating over a sequence of dictionary objects and the iterator returns null / undefined we should treat it as the default {} dictionary? (I think you wrote this code and I later moved it.)

birtles · 2018-04-11T07:56:08Z

Yeah, it looks like we shouldn't throw on null / undefined.

In WebIDL when we create a sequence from an iterable we have the step:

Initialize Si to the result of converting nextItem to an IDL value of type T.

For converting dictionary types we have:

If Type(V) is not Undefined, Null or Object, then throw a TypeError.

So we should update the Web Animations spec to match this and add a test for this case that we don't throw.

stephenmcgruer

Ack, filed w3c/csswg-drafts#2533.

stephenmcgruer · 2018-04-11T11:34:00Z

web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-001.html

+test(() => {
+  const test_error = { name: 'test' };
+  const bad_keyframe = { get left() { throw test_error; } };
+  assert_throws(bad_keyframe, () => {


stephenmcgruer · 2018-04-11T11:34:05Z

web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-001.html

+  const test_error = { name: 'test' };
+  const bad_keyframe = { get left() { throw test_error; } };
+  assert_throws(bad_keyframe, () => {
+    new KeyframeEffect(null, createIteratble([


stephenmcgruer · 2018-04-11T11:34:37Z

web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-001.html

+      return 42;  // Not an object.
+    },
+  };
+  assert_throws(new TypeError(), () => {


birtles · 2018-04-12T07:41:06Z

I've updated the spec now so we can drop the comments from this PR.

w3c/csswg-drafts@42dc347

birtles · 2018-04-12T07:41:39Z

web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-001.html

@@ -319,6 +333,39 @@
 }, 'Reading from a custom iterator that returns a non-object keyframe'
   + ' should throw');

+// The following two tests are currently against spec, but instead match what
+// we intend the spec to be; see https://github.com/w3c/csswg-drafts/issues/2533


Specifically, this comment is no longer needed since that issue has been addressed.

There were three minor bugs left in the implementation: - We threw on lists-in-custom-iterators instead of just ignoring them. - We returned all properties on the keyframe rather than just those defined on the keyframe itself (e.g. we would include prototype properties, against spec). - We didn't access the properties in ascending unicode order. Bug: 827573 Change-Id: I213ae5b24e1f35d7f28d16625025122950a6ba88 Reviewed-on: https://chromium-review.googlesource.com/989261 Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Robert Flack <flackr@chromium.org> Commit-Queue: Stephen McGruer <smcgruer@chromium.org> Cr-Commit-Position: refs/heads/master@{#550641}

chromium-wpt-export-bot added chromium-export do not merge yet labels Apr 10, 2018

wpt-pr-bot added web-animations wg-css wg-svg labels Apr 10, 2018

wpt-pr-bot approved these changes Apr 10, 2018

View reviewed changes

chromium-wpt-export-bot force-pushed the chromium-export-cl-989261 branch from d022009 to a63ba84 Compare April 10, 2018 17:53

stephenmcgruer requested a review from birtles April 10, 2018 17:54

birtles requested changes Apr 11, 2018

View reviewed changes

stephenmcgruer mentioned this pull request Apr 11, 2018

[web-animations-1] Don't throw for custom iterator that return null/undefined w3c/csswg-drafts#2533

Closed

chromium-wpt-export-bot force-pushed the chromium-export-cl-989261 branch from a63ba84 to 5ded871 Compare April 11, 2018 17:14

stephenmcgruer reviewed Apr 11, 2018

View reviewed changes

birtles approved these changes Apr 11, 2018

View reviewed changes

chromium-wpt-export-bot force-pushed the chromium-export-cl-989261 branch from 5ded871 to 04e1278 Compare April 11, 2018 18:23

birtles reviewed Apr 12, 2018

View reviewed changes

chromium-wpt-export-bot force-pushed the chromium-export-cl-989261 branch 3 times, most recently from 8b685d0 to 0891c1f Compare April 12, 2018 23:47

chromium-wpt-export-bot force-pushed the chromium-export-cl-989261 branch from 0891c1f to f048355 Compare April 13, 2018 16:52

chromium-wpt-export-bot removed the do not merge yet label Apr 13, 2018

chromium-wpt-export-bot merged commit 2707869 into master Apr 13, 2018

chromium-wpt-export-bot deleted the chromium-export-cl-989261 branch April 13, 2018 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Web Animations: Fix bugs in procedure to process a keyframes argument #10399

Web Animations: Fix bugs in procedure to process a keyframes argument #10399

chromium-wpt-export-bot commented Apr 10, 2018 •

edited by wpt-pr-bot

wpt-pr-bot left a comment

stephenmcgruer commented Apr 10, 2018

birtles Apr 11, 2018

stephenmcgruer Apr 11, 2018

birtles Apr 11, 2018

stephenmcgruer Apr 11, 2018

birtles Apr 11, 2018

stephenmcgruer Apr 11, 2018

birtles commented Apr 11, 2018

birtles commented Apr 11, 2018

stephenmcgruer left a comment

stephenmcgruer Apr 11, 2018

stephenmcgruer Apr 11, 2018

stephenmcgruer Apr 11, 2018

birtles commented Apr 12, 2018

birtles Apr 12, 2018

stephenmcgruer Apr 12, 2018

Web Animations: Fix bugs in procedure to process a keyframes argument #10399

Web Animations: Fix bugs in procedure to process a keyframes argument #10399

Conversation

chromium-wpt-export-bot commented Apr 10, 2018 • edited by wpt-pr-bot

wpt-pr-bot left a comment

Choose a reason for hiding this comment

stephenmcgruer commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

birtles commented Apr 11, 2018

birtles commented Apr 11, 2018

stephenmcgruer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

birtles commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chromium-wpt-export-bot commented Apr 10, 2018 •

edited by wpt-pr-bot