Redesign RTCIdentityProvider to not be a callback interface

[annevk]

We only have 3 callback interfaces in the platform and all of them are historical mistakes. Let's not introduce a fresh one.

See also whatwg/webidl#100.

[foolip]

@martinthomson who worked on this in https://bugzilla.mozilla.org/show_bug.cgi?id=975144

A change that would very likely be safe would be to make it a dictionary with two callback function members instead.

[martinthomson]

I think that I will need a little more information than just "dat bad".

Here's my put: I want the browser to invoke code implemented by the page. Should the browser instead expose event handlers and expect that the JS will later invoke methods on the event arguments, such as:

global.onassertionrequest = e => {
  getIdentityAssertion().then(assertion => e.provideAssertion(assertion));
}

Would that be a better model to follow? I can see a way that this might avoid certain problems.

[annevk]

There's three ways to get browsers to invoke code:

• Event listeners
• Callbacks
• Callback interfaces

You picked the one that is obsolete. As @foolip said, you could make this

dictionary RTCIdentityProvider {
  GenerateAssertionCallback generateAssertion;
  ValidateAssertionCallback validateAssertion;
};
callback GenerateAssertionCallback = Promise<RTCIdentityAssertionResult> (DOMString contents, DOMString origin, optional DOMString usernameHint);
callback ValidateAssertionCallback = Promise<RTCIdentityValidationResult> (DOMString assertion, DOMString origin);

or you could do something event-based.

[martinthomson]

I will cop to not understanding the relevance of a change from interface to dictionary, but will happily make such a minor change.

[annevk]

It will mostly affect the "this value" of the callbacks in practice (making them consistent with other callbacks in the platform), and going forward we might provide utilities around dictionaries that wouldn't necessarily work for legacy callback interfaces.

[martinthomson]

WFM. I certainly see value in having fewer ways to do things. I don't think that the "this" value is going to be much of a concern here, though it might in my tests, but we shall see, I guess.

[foolip]

The thing that I find most strange about callback interfaces is how the generateAssertion and validateAssertion would be looked up on the object when it's time to invoke the callbacks, which means they can change in the interim, and register({}) cannot throw. With a dictionary RTCIdentityProvider you can make the callbacks required or optional as makes sense and it'll be handled when register() is called.

[jan-ivar]

So we're OK with people having to use bind?

-    global.rtcIdentityProvider.register(new IDPJS());
+    var idp = new IDPJS();
+    global.rtcIdentityProvider.register({
+      generateAssertion: idp.generateAssertion.bind(idp),
+      validateAssertion: idp.validateAssertion.bind(idp)
+    });

[phistuck]

But global.rtcIdentityProvider.register(new IDPJS()); should work as well, I believe.

[annevk]

@jan-ivar for the case where you have a builtin, you probably want to take either the builtin or the dictionary, and not invoke the methods from the builtin directly (which could have been modified through script).

[jan-ivar]

@phistuck but it wont, because that will fail to bind the methods to the object, right?

[phistuck]

@jan-ivar - I wanted to test it, but I do not think there are similar cases in the web platform today that are implemented in Chrome...

[jan-ivar]

@phistuck it doesn't work in JS https://jsfiddle.net/5rucx7rs/ so unless WebIDL adds some magic, I doubt it would. - In other words, dictionaries have copy semantics, not reference semantics, so no this.

[jan-ivar]

@annevk IDPJS isn't a builtin. http://mxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/identity/idp.js#14

[annevk]

@jan-ivar I see, in that case yeah, you'd have to use bind(), just like you have to elsewhere.

[jan-ivar]

Seems a bit of a liability with this callbacks-in-dictionaries approach IMHO.

[phistuck]

@jan-ivar - how is you fiddle demonstrating this? You are sending methods directly as parameters, not the new Foo() object itself...

[jan-ivar]

@phistuck dictionaries aren't passed by reference like objects are, they're passed by copying (webidl binding code enforces this). Dictionary members are therefore on their own, a collection of individual parameters.

[jan-ivar]

The fact that this is less than obvious is what worries me here.

[jan-ivar]

@foolip JS code can always change in the interim. I don't see what limiting the API to callback functions prohibits.

[foolip]

@jan-ivar, see WICG/EventListenerOptions#12 (comment) for what I mean in the case of EventListener. One can of course do this kind of thing with callback functions as well by dispatching to something that can change, but then it's not part of the model.

In this specific case, are the callbacks expected to usually need some context beyond the arguments?

[jan-ivar]

Yes, this is inherently an interface. See http://mxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/identity/idp.js#14

EventListenerObject is a different topic imho.

[foolip]

EventListener is a different topic, it's just that I already had code written for the "they can change in the interim" point. That behavior is worth avoiding, IMHO.

To say that RTCIdentityProvider is inherently an interface doesn't really say whether developers will usually need to bind it to something or not. The example uses this.id, but is this much different from all of the events and callbacks on the platform where this is some platform object? Perhaps the spec could say that this is the RTCPeerConnection instance? Since that is an EventTarget, presumably people already have a way to get from it to the extra state they need.

[martinthomson]

I'm currently passing an undefined this. I think that's fine and people who need context can learn not to depend on the browser providing any. BTW, in this context, RTCPeerConnection doesn't exist.

[foolip]

OK, looks like there isn't any object which would make a lot of sense to pass as as this.

BTW, how is the returned promise supposed to be be used internally? The spec now just says "The RTCPeerConnection invokes the generateAssertion method on the RTCIdentityProvider instance registered by the IdP." and something similar for validateAssertion. It doesn't seem to say what happens if a promise isn't returned, or if it's resolved with the wrong type. Are there other callbacks on the web platform that should return promises? It's unfamiliar to me, but it's a big platform.

[martinthomson]

The promises are used in the usual fashion: if they resolve, the value is good and usable; if they reject (or don't resolve in a sensible amount of time), then they are no good. Adding language to the spec describing what to do with rejections, errors, is probably not a bad thing. I'll open something.

[annevk]

@foolip service workers has some event-based APIs that take promises, which is roughly the same thing. The IDL specification does account for the promise not resolving correctly so the specification probably does not need to deal with all the possibilities.

[foolip]

@annevk where does that happen? I kind of assumed that WebIDL didn't check the return values of callbacks at all, but don't know what string to search for.

[annevk]

@foolip it's not a callback, it's respondWith() which takes a promise. Anyway, https://heycam.github.io/webidl/#es-invoking-callback-functions 1.7 does the return value conversion for callbacks.

[foolip]

OK, I was still talking about the generateAssertion callback which returns a Promise<RTCIdentityAssertionResult>. respondWith() is https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#fetch-event-respondwith for anyone following along.