Bug 158114 – WebRTC: Update RTCPeerConnection overloaded legacy operations to return a Promise

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Adam Bergkvist]

The WebRTC 1.0 specification has been updated so that the overloaded legacy operations [1] (using callbacks) return a Promise. I.e. no errors will be throw. [2] links to the WG discussion and [3] is the specific change.

[1] https://w3c.github.io/webrtc-pc/archives/20160513/webrtc.html#legacy-interface-extensions
[2] https://github.com/w3c/webrtc-pc/issues/615
[3] https://github.com/w3c/webrtc-pc/pull/618

[Adam Bergkvist]

Created attachment 279879 [details]
Proposed patch

[Build Bot]

Comment on attachment 279879 [details]
Proposed patch

Attachment 279879 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1385627

New failing tests:
ietestcenter/css3/grid/grid-items-005.htm

[Build Bot]

Created attachment 279880 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Eric Carlson]

Comment on attachment 279879 [details]
Proposed patch

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

> LayoutTests/fast/mediastream/resources/promise-utils.js:18
> +function promiseShouldResolve(expr) {

This function is unused. Did you mean to use it, or is it for future use?

> Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:98
> +function objectAndCallbacksOverload(args, functionName, objectConstructor, objectOptions, promiseMode, legacyMode)
>  {
>      "use strict";
>  
> -    var options = {};
> +    let argsCount = Math.min(3, args.length);
> +    let objectArg = args[0];
> +    let objectArgOk = false;
> +
> +    if (argsCount == 0 && objectOptions.optionalAndNullable) {
> +        objectArg = null;
> +        objectArgOk = true;
> +        argsCount = 1;
> +    } else {
> +        const hasMatchingType = objectArg instanceof objectConstructor;
> +        objectArgOk = objectOptions.optionalAndNullable ? (objectArg === null || typeof objectArg === "undefined" || hasMatchingType) : hasMatchingType;
> +    }
>  
> -    if (args.length <= 1) {
> -        // Promise mode
> -        if (args.length && @isDictionary(args[0]))
> -            options = args[0]
> +    if (argsCount == 1 && objectArgOk)
> +        return promiseMode(objectArg);
>  
> -        return @enqueueOperation(peerConnection, function () {
> -            return targetFunction.@call(peerConnection, options);
> -        });
> -    }
> +    const successCallback = args[1];
> +    const errorCallback = args[2];
> +    if (argsCount == 3 && objectArgOk
> +        && (successCallback == null || typeof successCallback === "function")
> +        && (errorCallback == null || typeof errorCallback === "function")) {
> +        if (typeof successCallback !== "function")
> +            return @Promise.@reject(new @TypeError(`Argument 2 ('successCallback') to RTCPeerConnection.${functionName} must be a function`));
> +
> +        if (typeof errorCallback !== "function")
> +            return @Promise.@reject(new @TypeError(`Argument 3 ('errorCallback') to RTCPeerConnection.${functionName} must be a function`));
>  
> -    // Legacy callbacks mode (2 or 3 arguments)
> -    var successCallback = @extractCallbackArg(args, 0, "successCallback", functionName);
> -    var errorCallback = @extractCallbackArg(args, 1, "errorCallback", functionName);
> +        return legacyMode(objectArg, successCallback, errorCallback);
> +    }
>  
> -    if (args.length > 2 && @isDictionary(args[2]))
> -        options = args[2];
> +    if (argsCount < 1)
> +        return @Promise.@reject(new @TypeError("Not enough arguments"));
>  
> -    @enqueueOperation(peerConnection, function () {
> -        return targetFunction.@call(peerConnection, options).then(successCallback, errorCallback);
> -    });
> +    return @Promise.@reject(new @TypeError("Type error"));
>  }

I had a hard time figuring out this logic. I think it will be clearer if you restructure it to return early as soon as you know the arguments are valid or illegal. For example (untested so this may not be exactly correct):


function objectAndCallbacksOverload(args, functionName, objectConstructor, objectOptions, promiseMode, legacyMode)
{
    "use strict";

    let argsCount = Math.min(3, args.length);
    let objectArg = args[0];
    let objectArgOk = false;

    if (!argsCount && objectOptions.optionalAndNullable) {
        objectArg = null;
        objectArgOk = true;
        argsCount = 1;
    } else {
        const hasMatchingType = objectArg instanceof objectConstructor;
        objectArgOk = objectOptions.optionalAndNullable ? (objectArg === null || typeof objectArg === "undefined" || hasMatchingType) : hasMatchingType;
    }

    if (argsCount < 1)
        return @Promise.@reject(new @TypeError("Not enough arguments"));

    if (argsCount == 1 && objectArgOk)
        return promiseMode(objectArg);

    if (argsCount != 3 || !objectArgOk)
        return @Promise.@reject(new @TypeError("Type error"));

    const successCallback = args[1];
    if (successCallback != null && typeof errorCallback !== "function")
        return @Promise.@reject(new @TypeError(`Argument 2 ('successCallback') to RTCPeerConnection.${functionName} must be a function`));

    const errorCallback = args[2];
    if (errorCallback != null && typeof errorCallback !== "function"))
        return @Promise.@reject(new @TypeError(`Argument 3 ('errorCallback') to RTCPeerConnection.${functionName} must be a function`));

    return legacyMode(objectArg, successCallback, errorCallback);
}

> Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:123
> +    const argsCount = Math.min(3, args.length);
>  
> -    // Legacy callbacks mode (3 arguments)
> -    if (args.length < 3)
> -        throw new @TypeError("Not enough arguments");
> +    if (argsCount == 0 || argsCount == 1)
> +        return promiseMode(args[0]);
>  
> -    var successCallback = @extractCallbackArg(args, 1, "successCallback", functionName);
> -    var errorCallback = @extractCallbackArg(args, 2, "errorCallback", functionName);
> -
> -    @enqueueOperation(peerConnection, function () {
> -        return targetFunction.@call(peerConnection, description).then(successCallback, errorCallback);
> -    });
> -}
> +    const successCallback = args[0];
> +    const errorCallback = args[1];
> +    if ((argsCount == 2 || argsCount == 3)
> +        && (successCallback == null || typeof successCallback === "function")
> +        && (errorCallback == null || typeof errorCallback === "function")) {
> +        if (typeof successCallback !== "function")
> +            return @Promise.@reject(new @TypeError(`Argument 1 ('successCallback') to RTCPeerConnection.${functionName} must be a function`));
>  
> -function extractCallbackArg(args, index, name, parentFunctionName)
> -{
> -    "use strict";
> +        if (typeof errorCallback !== "function")
> +            return @Promise.@reject(new @TypeError(`Argument 2 ('errorCallback') to RTCPeerConnection.${functionName} must be a function`));
>  
> -    var callback = args[index];
> -    if (typeof callback !== "function")
> -        throw new @TypeError("Argument " + (index + 1) + " ('" + name + "') to RTCPeerConnection." + parentFunctionName + " must be a Function");
> +        return legacyMode(successCallback, errorCallback, args[2]);
> +    }
>  
> -    return callback;
> +    return @Promise.@reject(new @TypeError("Type error"));

I think this would be slightly clearer if you reject for an unexpected number of arguments immediately after getting the argument count.

[Adam Bergkvist]

Created attachment 279953 [details]
Updated patch

Addressed review comments

[Adam Bergkvist]

(In reply to comment #4)
> Comment on attachment 279879 [details]
> Proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279879&action=review
> 
> > LayoutTests/fast/mediastream/resources/promise-utils.js:18
> > +function promiseShouldResolve(expr) {
> 
> This function is unused. Did you mean to use it, or is it for future use?

It's used by other tests not yet upstreamed. I'll remove it and add it when it's needed.

[...]
> 
> I had a hard time figuring out this logic. I think it will be clearer if you
> restructure it to return early as soon as you know the arguments are valid
> or illegal. For example (untested so this may not be exactly correct):
> 
> 
> function objectAndCallbacksOverload(args, functionName, objectConstructor,
> objectOptions, promiseMode, legacyMode)
> {
>     "use strict";
> 
>     let argsCount = Math.min(3, args.length);
>     let objectArg = args[0];
>     let objectArgOk = false;
> 
>     if (!argsCount && objectOptions.optionalAndNullable) {
>         objectArg = null;
>         objectArgOk = true;
>         argsCount = 1;
>     } else {
>         const hasMatchingType = objectArg instanceof objectConstructor;
>         objectArgOk = objectOptions.optionalAndNullable ? (objectArg ===
> null || typeof objectArg === "undefined" || hasMatchingType) :
> hasMatchingType;
>     }
> 
>     if (argsCount < 1)
>         return @Promise.@reject(new @TypeError("Not enough arguments"));
> 
>     if (argsCount == 1 && objectArgOk)
>         return promiseMode(objectArg);
> 
>     if (argsCount != 3 || !objectArgOk)
>         return @Promise.@reject(new @TypeError("Type error"));
> 
>     const successCallback = args[1];
>     if (successCallback != null && typeof errorCallback !== "function")
>         return @Promise.@reject(new @TypeError(`Argument 2
> ('successCallback') to RTCPeerConnection.${functionName} must be a
> function`));
> 
>     const errorCallback = args[2];
>     if (errorCallback != null && typeof errorCallback !== "function"))
>         return @Promise.@reject(new @TypeError(`Argument 3 ('errorCallback')
> to RTCPeerConnection.${functionName} must be a function`));
> 
>     return legacyMode(objectArg, successCallback, errorCallback);
> }

I've imitated the structure of generated bindings and I agree with you that it's not the most obvious code flow for readability. :) I've restructured the bindings in the updated patch, as you proposed. The new version is also more specific on error messages (no generic "Type error" messages).

[...]
> 
> I think this would be slightly clearer if you reject for an unexpected
> number of arguments immediately after getting the argument count.

Se previous comment.

[Adam Bergkvist]

Comment on attachment 279953 [details]
Updated patch

Thanks for reviewing

[WebKit Commit Bot]

Comment on attachment 279953 [details]
Updated patch

Clearing flags on attachment: 279953

Committed r201455: <http://trac.webkit.org/changeset/201455>