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
Created attachment 279879 [details] Proposed patch
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
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
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.
Created attachment 279953 [details] Updated patch Addressed review comments
(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.
Comment on attachment 279953 [details] Updated patch Thanks for reviewing
Comment on attachment 279953 [details] Updated patch Clearing flags on attachment: 279953 Committed r201455: <http://trac.webkit.org/changeset/201455>