[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

--- Comment #6 from Adam Bergkvist <adam.bergkvist at ericsson.com> ---
(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. 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);
> }

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.

-- 
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/20160527/e2a6ed2b/attachment-0001.html>