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

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
6 years ago
5 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

unspecified
mozilla23
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Assignee: nobody → 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?
Flags: needinfo?(Ms2ger)
Flags: needinfo?(cpearce)
I don't know what that means, but I bet Roc does. Roc, can you address bz's question in comment 2 please?
Flags: needinfo?(cpearce) → needinfo?(roc)
The short story is that with this patch, HTMLVideoElement.NETWORK_EMPTY == 0, because HTMLVideoElement.__proto__ == HTMLMediaElement.
That sounds fine assuming the WebIDL spec agrees.
Flags: needinfo?(roc)
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.)
(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
Flags: needinfo?(Ms2ger)
It came up during web components discussion, but basically it's just the "we should treat webidl interfaces like es6 classes" thing.
Whiteboard: [need review]
Attachment #736399 - Attachment is obsolete: true
Attachment #736399 - Flags: review?(peterv)
No longer blocks: 861022
Depends on: 861022
Attachment #741161 - Attachment is obsolete: true
Attachment #741161 - Flags: review?(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.
Attachment #744503 - Flags: review?(peterv) → review+
> 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....
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c1074ad5172
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla23
https://hg.mozilla.org/mozilla-central/rev/4c1074ad5172
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.