<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<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#c6">Comment # 6</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:adam.bergkvist@ericsson.com" title="Adam Bergkvist <adam.bergkvist@ericsson.com>"> <span class="fn">Adam Bergkvist</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=158114#c4">comment #4</a>)
<span class="quote">> 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>
>
> > 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?</span >
It's used by other tests not yet upstreamed. I'll remove it and add it when it's needed.
[...]
<span class="quote">>
> 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 >
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).
[...]
<span class="quote">>
> I think this would be slightly clearer if you reject for an unexpected
> number of arguments immediately after getting the argument count.</span >
Se previous comment.</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>