Reuben Morais [:reuben] Reporter
	Description • 11 years ago

We need inheritance for the Contacts API. This is an example of an use case:

interface ContactField {
  attribute DOMString type;
  attribute DOMString value;
};

interface ContactTelField : ContactField {
  attribute DOMString carrier;
};

ContactTelField objects should have .type and .value properties.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 2 • 11 years ago

Comment on attachment 728563 [details] [diff] [review]
part 1.  Don't mark JS-implemented interfaces with descendant interfaces as final.

Actually, Kyle, would you look over the parser change here?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 8 • 11 years ago

I have no idea where those other versions of it came from.  :(

	Andrew McCreight [:mccr8]
	Comment 11 • 11 years ago

Comment on attachment 728563 [details] [diff] [review]
part 1.  Don't mark JS-implemented interfaces with descendant interfaces as final.

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

::: dom/bindings/Codegen.py
@@ +5541,5 @@
>  
>      visibility determines the visibility of the destructor (public,
>      protected, private), defaults to private.
>  
>      body contains a string with the code for the destructor, defaults to None.

This comment on |body| needs to be updated.  Dropping the "defaults to None" is probably enough, as it doesn't seem that useful to me.  I'll also mention that there is no comment for "virtual", but assuming it means "this destructor should be virtual" no comment is required, so far as I believe.

	Andrew McCreight [:mccr8]
	Comment 12 • 11 years ago

Comment on attachment 728564 [details] [diff] [review]
part 2.  Add an infallibel constructor for CallbackObjects which are already in the right compartment and use this to simplify construction of the autogenerated implementation of a JS-implemented WebIDL binding.

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

::: dom/bindings/CallbackObject.h
@@ +67,5 @@
>    }
>  
> +  /*
> +   * Create a CallbackObject without any sort of interesting games
> +   * with compartments.  This constructor can never fail.

Maybe add a comment here about why you'd want to do this?  Because we're already in the right compartment, I assume.  Would it be useful to have assertions about being in the right compartment here, or are we going to touch JSAPI soon so we'd blow up quickly anyways?

@@ +109,5 @@
> +  {
> +    Init(aCallbackObject->mCallback);
> +  }
> +
> +  inline void Init(JSObject* aCallback)

Seems like Init could be private rather than protected, but it also seems like it doesn't really matter.

::: dom/bindings/Codegen.py
@@ +5453,5 @@
>  
>      baseConstructors is a list of strings containing calls to base constructors,
>      defaults to None.
>  
>      body contains a string with the code for the constructor, defaults to None.

The comment for |body| needs to be updated.

@@ +8084,5 @@
> +        constructor = ClassConstructor(
> +            [Argument("JSObject*", "aJSImplObject"),
> +             Argument("nsISupports*", "aParent")],
> +            visibility="public",
> +            baseConstructors=["mImpl(new %s(aJSImplObject))" %

Ah, yes, that's a lot nicer.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 13 • 11 years ago

Peter, would you look over the naming of stuff here?  I'm not entirely happy with it, but I didn't have better ideas...

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 11 years ago

> Dropping the "defaults to None" is probably enough

I changed it to "defaults to empty"

> but assuming it means "this destructor should be virtual"

It does.  I added a comment to that effect.

> Maybe add a comment here about why you'd want to do this? 

Added "for cases when you want to just use the existing object as-is".

> Would it be useful to have assertions about being in the right compartment here

There is no right or wrong compartment for callbacks, really.  The compartment munging we do right now was mostly for things like event listeners to be stored in the compartment of the thing they're attached to...

> Seems like Init could be private rather than protected,

Done.

> The comment for |body| needs to be updated.

Done.

	Andrew McCreight [:mccr8]
	Comment 15 • 11 years ago

Is this intended to work for JS-implemented interfaces inheriting from JS-implemented interfaces?  I notice the test examples only have JS-implemented inheriting from C++-implemented, but nothing in the code seems to check that.

If we inherit from another WebIDL class (JS-implemented or otherwise), is that class likely to have another parent, which will be the same parent we'll use?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 16 • 11 years ago

> Is this intended to work for JS-implemented interfaces inheriting from JS-implemented
> interfaces? 

Yes.

> I notice the test examples only have JS-implemented inheriting from C++-implemented

One of which inherits from JS-implemented, note.

We could add an explicit test for JS-implemented inheriting directly from JS-implemented; might be a good idea.

> If we inherit from another WebIDL class (JS-implemented or otherwise), is that class
> likely to have another parent, which will be the same parent we'll use?

It's possible, yes, if you mean GetParentObject.  But the whole parent thing is pretty silly for everything that's not a node: it doesn't much matter what it is as long as it leads to the right window.

	Andrew McCreight [:mccr8]
	Comment 17 • 11 years ago

(In reply to Boris Zbarsky (:bz) from comment #16)
> One of which inherits from JS-implemented, note.

Ah, ok.

> We could add an explicit test for JS-implemented inheriting directly from
> JS-implemented; might be a good idea.

Oops, I missed the part of the code where it generates calls to the parent constructor.  I see how that goes now.

> It's possible, yes, if you mean GetParentObject.  But the whole parent thing
> is pretty silly for everything that's not a node: it doesn't much matter
> what it is as long as it leads to the right window.

It seems a little odd to me that if you have a series of inherited classes, then each one will have its own mParent field that will get initialized and traced, so an instance of one of these objects will have a bunch of duplicated fields.

For mParent, it doesn't seem like a big deal, because I wouldn't expect inheritance to be very deep, and the cost of one extra pointer per parent seems fairly minor in the scheme of things.

What seems a little more concerning is that I think such an object will end up instantiating and owning a bunch of JS implementations, one for each parent class, even though it is only using one.  Am I right in thinking that?  Is it something we should worry about?

	Andrew McCreight [:mccr8]
	Comment 18 • 11 years ago

> one for each parent class
JS implemented parent class, that is

	Andrew McCreight [:mccr8]
	Comment 19 • 11 years ago

Hmm, I guess we'll get one C++-callback-wrapper-class instantiation per JS-implemented parent class, but they'll all share the same JS implementation, so that's not too bad.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 20 • 11 years ago

> I guess we'll get one C++-callback-wrapper-class instantiation per JS-implemented parent
> class, but they'll all share the same JS implementation

Yes, exactly.

We could try for something where we do some sort of inheritance on those callback wrapper objects, but I'm not sure it's worth it.

	Andrew McCreight [:mccr8]
	Comment 21 • 11 years ago

Comment on attachment 728570 [details] [diff] [review]
Part 3 for real real

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

> We could try for something where we do some sort of inheritance on those callback wrapper objects, but I'm not sure it's worth it.

Yeah, it would be neat, but it doesn't really seem worthwhile.

::: dom/bindings/Codegen.py
@@ +8062,5 @@
> +            ccDecl = ("NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(%s, %s)" %
> +                      (descriptor.name, parentClass))
> +            constructorBody = (
> +                "// Make sure we're an nsWrapperCache already\n"
> +                "MOZ_ASSERT(static_cast<nsWrapperCache*>(this));\n"

Is this essentially a static assert that you can do this cast at all?

Also, if our parent doesn't inherit from nsISupports everything will fail to compile hopefully?

	Andrew McCreight [:mccr8]
	Comment 22 • 11 years ago

Comment on attachment 728566 [details] [diff] [review]
part 4.  Fix forward-declarations and includes for JS-implemented interfaces.

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

::: dom/bindings/Codegen.py
@@ +7143,5 @@
>              ifaces.extend(getInterfacesFromCallback(callback))
>          for callback in workerCallbacks:
>              workerIfaces.extend(getInterfacesFromCallback(callback))
>  
> +        for callbackDescriptor in callbackDescriptors + jsImplemented:

What stuff is this generating?

	Andrew McCreight [:mccr8]
	Comment 23 • 11 years ago

How does the NativePropertyHooks stuff work here... it looks at the &TestCImplementedInterfaceBinding::sNativePropertyHooks if it fails to find it in sNativePropertyHooks?  I didn't see any code to generate that, so I assume it is something that Just Works?

	Andrew McCreight [:mccr8]
	Comment 24 • 11 years ago

Comment on attachment 728686 [details] [diff] [review]
part 6.  Handle cases when the C++ class we want to actually inherit from is not the one that the WebIDL interface is mapped to.

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

So this happens because nsDOMEventTargetHelper has a bunch of implementation machinery every event implementation will use, but is not web-exposed?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 25 • 11 years ago

> Is this essentially a static assert that you can do this cast at all?

Yes, but you can't use static_cast in static assertions.

> Also, if our parent doesn't inherit from nsISupports everything will fail to compile
> hopefully?

Yes, but I could add an explicit cast of "this" to nsISupports...  Might run afoul of multiple inheritance, though.  Maybe I can use ToSupports() for that.  Want me to?

> What stuff is this generating?

It's generating forward-declarations for interfaces that appear in the call signatures of callback interface descriptors (and with my change the js-implemented interface descriptors).  These are needed because the callback interface (or jsimpl) leads to a class inheriting from CallbackInterface in the header and that class has methods which take arguments of these interface types, so we need to forward-declare them in the header, or include the relevant headers, so the types will be defined.  We forward-declare to avoid over-inclusion and header loops.

> How does the NativePropertyHooks stuff work here...

XrayResolveNativeProperty walks the mProtoHooks chain.  And for example for TestJSImplInterface2Bindingw the generated code has:

  const NativePropertyHooks sNativePropertyHooks = {
    ...
    &TestCImplementedInterfaceBinding::sNativePropertyHooks
  };

and for TestCImplementedInterfaceBinding it has:

  const NativePropertyHooks sNativePropertyHooks = {
    ...
    &TestJSImplInterfaceBinding::sNativePropertyHooks
  };

so yeah, Just Works.  ;)

> So this happens because nsDOMEventTargetHelper has a bunch of implementation machinery
> every event implementation will use, but is not web-exposed?

This happens because nsDOMEventTargetHelper has a bunch of implementation machinery that an event implementation can use.

The only reason EventTarget doesn't map to nsDOMEventTargetHelper is that nodes and windows use a slightly different form of that machinery (in particular, they store it slightly out of band in the case of nodes to keep down object size).  So EventTarget maps to dom::EventTarget, which is an abstract class that nsGlobalWindow, nsINode, and nsDOMEventTargetHelper inherit from.  Then all non-window and non-node event targets inherit from nsDOMEventTargetHelper.  So we want to inherit from that, and pick up all the machinery automatically; inheriting from the abstract dom::EventTarget class wouldn't get us much (and not compile, too, since it has pure virtual methods).

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 26 • 11 years ago

And thanks for the reviews!

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 28 • 11 years ago

Comment on attachment 728686 [details] [diff] [review]
part 6.  Handle cases when the C++ class we want to actually inherit from is not the one that the WebIDL interface is mapped to.

Got some feedback to make this 'jsImplParent'

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 29 • 11 years ago

           https://hg.mozilla.org/integration/mozilla-inbound/rev/7d2c6a721c1a
           https://hg.mozilla.org/integration/mozilla-inbound/rev/33d93829b2fa
           https://hg.mozilla.org/integration/mozilla-inbound/rev/f2795dffde20
           https://hg.mozilla.org/integration/mozilla-inbound/rev/bc202c64b731
           https://hg.mozilla.org/integration/mozilla-inbound/rev/658ef1af4878
           https://hg.mozilla.org/integration/mozilla-inbound/rev/1d285beb5842

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/7d2c6a721c1a
https://hg.mozilla.org/mozilla-central/rev/33d93829b2fa
https://hg.mozilla.org/mozilla-central/rev/f2795dffde20
https://hg.mozilla.org/mozilla-central/rev/bc202c64b731
https://hg.mozilla.org/mozilla-central/rev/658ef1af4878
https://hg.mozilla.org/mozilla-central/rev/1d285beb5842