New issue
Advanced search Search tips

Issue 818103 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 430155
issue 674593



Sign in to add a comment

AnimationWorkletGlobalScope.registerAnimator takes VoidFunction argument

Project Member Reported by peria@chromium.org, Mar 2 2018

Issue description

The second argument of AnimationWokletGlobalScope.registerAnimator operation is defined as a VoidFunction in spec.
https://wicg.github.io/animation-worklet/https://wicg.github.io/animation-worklet/#animation-worklet-desc

and we implement it as a Function.
 
Cc: majidvp@chromium.org
Labels: Hotlist-Interop
Blocking: 430155
Labels: -Hotlist-Interop Hotlist-Experimental
Cc: smcgruer@chromium.org
I threw together a CL to 'fix' this last night as it looked trivial to me, and asked Yuki for comment. He had some great comments:

"""
Animation Worklet is standardized in the mostly same way as Custom Elements, and I'd recommend to go with the same way as custom elements.  I think that what you'd like to do is something like the following patch:
https://chromium-review.googlesource.com/c/chromium/src/+/1242784

    // core/html/custom/custom_element_registry.idl
    callback CustomElementConstructor = any ();
makes the bindings generator generate V8CustomElementConstructor, and V8CustomElementConstructor::Construct is used to create an instance.
https://cs.chromium.org/chromium/src/third_party/blink/renderer/bindings/core/v8/script_custom_element_definition.cc?rcl=523efae2ae9590d0bc4c0229277020db49fb5438&l=259

CallbackMethodRetriever is used to extract methods from the prototype chain.
https://cs.chromium.org/chromium/src/third_party/blink/renderer/bindings/core/v8/script_custom_element_definition_builder.cc?rcl=523efae2ae9590d0bc4c0229277020db49fb5438&l=102

In case of WorkletGlobalScope, it's good to use Member<V8SomethingCallbackFunction> to hold callback functions.

However, I think that it's wrong that Animation Worklet standard uses VoidFunction for constructors.  It conflicts with Web IDL.  Return type of void means that you discard the constructed object.
https://heycam.github.io/webidl/#construct-a-callback-function
Like custom elements, the return type should be type of |any| (if you don't need any type check in the above phase) or a specific type you want (if you want "construct" algorithm to check the type and throw a TypeError).

    https://wicg.github.io/animation-worklet/#create-a-new-animator-instance
    "create a new animator instance" algorithm
    step 6. Let animatorInstance be the result of constructing animatorCtor with [options, state as args.
Given that |animatorCtor| is VoidFunction, the result is void (animatorInstance = void???, which doesn't make sense).
"""

Majid - WDYT of changing the spec to replace VoidFunction with AnimatorInstanceConstructor, with:

callback AnimatorInstanceConstructor = any ();

?
Re #4: Make sense to me to switch using callback AnimatorInstanceConstructor = any (). 

I had no idea that the construction algorithm actually cares about the callback return type [1]. I can make the changes to the spec.


[1] Step 12 in https://heycam.github.io/webidl/#construct-a-callback-function converts the output to the return value. 

Set completion to the result of converting callResult.[[Value]] to an IDL value of the same type as the operation’s return type.



Cc: ikilpatrick@chromium.org
I believe this applies to PaintWorklet as well since it uses VoidFunction too.

Sign in to add a comment