[Webkit-unassigned] [Bug 158114] WebRTC: Update RTCPeerConnection overloaded legacy operations to return a Promise

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=158114

Eric Carlson <eric.carlson at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #279879|                            |review+
              Flags|                            |

--- Comment #4 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 279879
  --> https://bugs.webkit.org/attachment.cgi?id=279879
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. at 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. at reject(new @TypeError(`Argument 2 ('successCallback') to RTCPeerConnection.${functionName} must be a function`));
> +
> +        if (typeof errorCallback !== "function")
> +            return @Promise. at 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. at reject(new @TypeError("Not enough arguments"));
>  
> -    @enqueueOperation(peerConnection, function () {
> -        return targetFunction. at call(peerConnection, options).then(successCallback, errorCallback);
> -    });
> +    return @Promise. at 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. at reject(new @TypeError("Not enough arguments"));

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

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

    const successCallback = args[1];
    if (successCallback != null && typeof errorCallback !== "function")
        return @Promise. at 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. at 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. at 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. at 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. at 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. at 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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160526/ad1cf04d/attachment-0001.html>