Bug 179065 – [Web Animations] Distinguish between an omitted and a null timeline argument to the Animation constructor

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

The Web Animations spec says that the Animation constructor should distinguish between an omitted and a null timeline argument. In https://w3c.github.io/web-animations/#dom-animation-animation:

"Run the procedure to set the timeline of an animation on animation passing timeline as the new timeline or, if a timeline argument is not provided, passing the default document timeline of the Document associated with the Window that is the current global object."

[Radar WebKit Bug Importer]

<rdar://problem/36869046>

[Antoine Quint]

Created attachment 332392 [details]
Patch

[Antoine Quint]

Created attachment 332393 [details]
Patch

[Chris Dumez]

Comment on attachment 332393 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332393&action=review

Custom constructor looks good but I have a couple of suggestions.

> Source/WebCore/bindings/js/JSWebAnimationCustom.cpp:47
> +    ASSERT(context->isDocument());

We prefer is<Document>(*context).

> Source/WebCore/bindings/js/JSWebAnimationCustom.cpp:59
> +    auto timeline = convert<IDLNullable<IDLInterface<AnimationTimeline>>>(state, state.argument(1), [](ExecState& state, ThrowScope& scope) {

Could use uncheckedArgument(1) for performance. If there was no second argument, you would have return early.

[Chris Dumez]

Comment on attachment 332393 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332393&action=review

>> Source/WebCore/bindings/js/JSWebAnimationCustom.cpp:47
>> +    ASSERT(context->isDocument());
> 
> We prefer is<Document>(*context).

Actually, just drop this assertion, it is already inside the downcast<Document>() call below.

[Antoine Quint]

Committed r227714: <https://trac.webkit.org/changeset/227714>