860841 - Hook up interface object proto chains as if they were ES6 classes

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

So the proto chain for HTMLElement, say would look like this:

  HTMLElement -> Element -> Node -> EventTarget -> Function.prototype

Note that my patch (coming up) skips [NoInterfaceObject] links in the chain.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Hook up the proto chain of a WebIDL interface object to the interface object of its nearest ancestor interface that has one, as if these were ES6 classes.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So this obviously fails a bunch of tests in dom/imptests/webapps/DOMCore/tests/approved/test_interfaces.html and dom/imptests/webapps/XMLHttpRequest/tests/submissions/Ms2ger/test_interfaces.html.  Ms2ger, should I just throw stuff in the json for now and file an upstream bug, if we take this patch?

It also fails a bunch of tests in content/media/test/test_constants.html because that's checking that the constants aren't on HTMLVideo/AudioElement when of course now they're on the proto chain of those interface objects.  Chris, is just adjusting the test ok?

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

I don't know what that means, but I bet Roc does. Roc, can you address bz's question in comment 2 please?

Comment 4

[Boris Zbarsky [:bzbarsky]]

The short story is that with this patch, HTMLVideoElement.NETWORK_EMPTY == 0, because HTMLVideoElement.__proto__ == HTMLMediaElement.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That sounds fine assuming the WebIDL spec agrees.

Comment 6

[Cameron McCormack (:heycam)]

This may be me not paying enough attention recently, but where does this requirement for interface objects to be class-like in their __proto__ come from?  (Not that I disagree, if that's what ES6 classes are like, since it probably makes sense to model Web IDL interfaces like ES6 classes.)

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Boris Zbarsky (:bz) from comment #2)
> So this obviously fails a bunch of tests in
> dom/imptests/webapps/DOMCore/tests/approved/test_interfaces.html and
> dom/imptests/webapps/XMLHttpRequest/tests/submissions/Ms2ger/test_interfaces.
> html.  Ms2ger, should I just throw stuff in the json for now and file an
> upstream bug, if we take this patch?

Yeah. I filed <https://github.com/w3c/testharness.js/issues/26>.

(In reply to Cameron McCormack (:heycam) from comment #6)
> This may be me not paying enough attention recently, but where does this
> requirement for interface objects to be class-like in their __proto__ come
> from?  (Not that I disagree, if that's what ES6 classes are like, since it
> probably makes sense to model Web IDL interfaces like ES6 classes.)

http://lists.w3.org/Archives/Public/public-script-coord/2013AprJun/0031.html

Comment 8

[Boris Zbarsky [:bzbarsky]]

It came up during web components discussion, but basically it's just the "we should treat webidl interfaces like es6 classes" thing.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: With the test suite bits adjusted

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Hook up the proto chain of a WebIDL interface object to the interface object of its nearest ancestor interface that has one, as if these were ES6 classes.

Comment 11

[Peter Van der Beken [:peterv]]

Comment on attachment 744503 [details] [diff] [review]
Hook up the proto chain of a WebIDL interface object to the interface object of its nearest ancestor interface that has one, as if these were ES6 classes.

Review of attachment 744503 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/test/test_constants.html
@@ +38,5 @@
>  is(HTMLMediaElement.MEDIA_ERR_ABORTED, undefined);
>  is(HTMLMediaElement.MEDIA_ERR_NETWORK, undefined);
>  is(HTMLMediaElement.MEDIA_ERR_DECODE, undefined);
>  is(HTMLMediaElement.MEDIA_ERR_SRC_NOT_SUPPORTED, undefined);
> +is(HTMLVideoElement.NETWORK_EMPTY, 0);

We could do:

is(HTMLVideoElement.NETWORK_EMPTY, HTMLMediaElement.NETWORK_EMPTY);

Up to you.

::: dom/bindings/BindingUtils.cpp
@@ -321,5 @@
>    bool isCallbackInterface = constructorClass == js::Jsvalify(&js::ObjectClass);
>    if (constructorClass) {
> -    JSObject* constructorProto;
> -    if (isCallbackInterface) {
> -      constructorProto = JS_GetObjectPrototype(cx, global);

Do we actually end up here anymore? AFAICT this changes the prototype for callback interface objects with no parent to be Function. I think that'll break toString on those interface objects (since we specifically don't override Function.prototype.toString for callback interface below). But I think we never end up here with constructorClass equal to ObjectClass anymore, we just kept the code because we wanted to switch back to ObjectClass at some point (if possible). You should either fix the toString problem or remove the isCallbackInterface stuff.

@@ +324,3 @@
>      if (!constructorProto) {
>        return NULL;
>      }

I think we should assert this and have the caller check for null (which you made it do).

@@ +500,5 @@
>  
>  void
>  CreateInterfaceObjects(JSContext* cx, JS::Handle<JSObject*> global,
>                         JS::Handle<JSObject*> protoProto,
> +                       JS::Handle<JSObject*> constructorProto,

Might want to group it with the rest of the constructor stuff (just before constructorClass?). The number of arguments is sadmaking.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> But I think we never end up here with constructorClass equal to ObjectClass anymore

Agreed.  Nixed the isCallbackInterface bits.

> I think we should assert this and have the caller check for null (which you made it do).

Done.

> just before constructorClass?

Done.

> The number of arguments is sadmaking.

Indeed.  We could probably kill "constructor", fwiw....

Comment 13

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c1074ad5172

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c1074ad5172