<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:eric.carlson@apple.com" title="Eric Carlson <eric.carlson@apple.com>"> <span class="fn">Eric Carlson</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - WebRTC: Update RTCPeerConnection overloaded legacy operations to return a Promise"
href="https://bugs.webkit.org/show_bug.cgi?id=158114">bug 158114</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #279879 Flags</td>
<td>
</td>
<td>review+
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - WebRTC: Update RTCPeerConnection overloaded legacy operations to return a Promise"
href="https://bugs.webkit.org/show_bug.cgi?id=158114#c4">Comment # 4</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - WebRTC: Update RTCPeerConnection overloaded legacy operations to return a Promise"
href="https://bugs.webkit.org/show_bug.cgi?id=158114">bug 158114</a>
from <span class="vcard"><a class="email" href="mailto:eric.carlson@apple.com" title="Eric Carlson <eric.carlson@apple.com>"> <span class="fn">Eric Carlson</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=279879&action=diff" name="attach_279879" title="Proposed patch">attachment 279879</a> <a href="attachment.cgi?id=279879&action=edit" title="Proposed patch">[details]</a></span>
Proposed patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=279879&action=review">https://bugs.webkit.org/attachment.cgi?id=279879&action=review</a>
<span class="quote">> LayoutTests/fast/mediastream/resources/promise-utils.js:18
> +function promiseShouldResolve(expr) {</span >
This function is unused. Did you mean to use it, or is it for future use?
<span class="quote">> 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"));
> }</span >
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);
}
<span class="quote">> 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"));</span >
I think this would be slightly clearer if you reject for an unexpected number of arguments immediately after getting the argument count.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>