Bug 179065

Summary: [Web Animations] Distinguish between an omitted and a null timeline argument to the Animation constructor
Product: WebKit Reporter: Antoine Quint <graouts>
Component: AnimationsAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cmarcelo, dbates, dino, esprehn+autocc, ews-watchlist, kangil.han, kondapallykalyan, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=122912
Attachments:
Description Flags
Patch
none
Patch dino: review+

Description Antoine Quint 2017-10-31 12:11:18 PDT
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."
Comment 1 Radar WebKit Bug Importer 2018-01-25 08:13:33 PST
<rdar://problem/36869046>
Comment 2 Antoine Quint 2018-01-26 11:35:18 PST
Created attachment 332392 [details]
Patch
Comment 3 Antoine Quint 2018-01-26 11:41:30 PST
Created attachment 332393 [details]
Patch
Comment 4 Chris Dumez 2018-01-26 13:10:57 PST
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.
Comment 5 Chris Dumez 2018-01-26 13:17:46 PST
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.
Comment 6 Antoine Quint 2018-01-27 01:26:25 PST
Committed r227714: <https://trac.webkit.org/changeset/227714>