1444991 - Add a cleaner mechanism for representing WebIDL interfaces in XPIDL

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nika Layzell [:nika] (ni? for response)]

Currently the best option for XPIDL interfaces is to simply return an nsISupports and rely on the binding code grabbing the right cached wrapper. This is really gross in C++ code though, as you lose all of your type information.

We should add a typed mechanism for this to xpidl (even if it's only sugar).

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Taking this as I know how I want to implement it, but I'm going to wait for mccr8's work in bug 1438688 before making the changes, as I don't want to conflict with him, and his changes will actually make my life vastly easier ^_^.

Comment 2

[Andrew McCreight [:mccr8]]

I don't know what you have in mind exactly, but will this require generating XPT data for specific WebIDL interfaces? My concern with that is it will make the XPIDL build depend on on WebIDL, which might be tricky because of the way that XPIDL sort of exists outside of the regular build system, and the WebIDL build system is complicated.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

(In reply to Andrew McCreight [:mccr8] from comment #2)
> I don't know what you have in mind exactly, but will this require generating
> XPT data for specific WebIDL interfaces? My concern with that is it will
> make the XPIDL build depend on on WebIDL, which might be tricky because of
> the way that XPIDL sort of exists outside of the regular build system, and
> the WebIDL build system is complicated.

No, the way I'm implementing this will not have that requirement. I can get the information I need other ways.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Read webidl's Bindings.conf, and pass it into xpidl

This information is read in order to handle correctly selecting the native
type and header files for WebIDL types.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Parse webidl productions in xpidl

They are parsed into a WebIDL object, and lowered into C++, rust, and XPT.

For C++ code, we generate a correctly namespaced forward declaration. In rust,
the types are exposed as `*const c_void`, as we don't have WebIDL type
information there.

The XPT code generator needs to know the header filename in order to perform
correct codegen, so we also get that information.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Generate DOMObject info for xptinfo

Unlike the other lists in xptinfo, this list contains relocations. Each
DOMObject has 3 functions generated for it, `Wrap`, `Unwrap` and `Cleanup`,
which perform the necessary actions. These are stored as function pointers.

Wrap gets the DOMObject wrapper using the DOM binding code, Unwrap gets the
underlying C++ object, and addrefs it (as XPCOM methods return native types
via getter_AddRefs), and Cleanup releases a reference to the underlying
C++ object, for when the unwrapped object is used as a temporary during a call.

To generate the code, we need to have the declaration of the native C++ type
in scope, so we also emit #include-s for the headerFiles.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 4: Handle DOM Objects in XPConnect

This patch goes through the XPConnect conversion methods, and adds cases for
T_DOMOBJECT which call the Wrap, Unwrap, and Cleanup methods from the
nsXPTDOMObjectInfo objects created in the last part.

For consistency with normal interface pointers, and because it wasn't too
complex, I also added support for including T_DOMOBJECTs in XPCOM arrays.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 5: Make some XPCOM methods more strongly typed

This patch goes through and changes a bunch of places in our tree which mention
this bug to use the new feature, making the methods more strongly typed.

There are probably more places in tree which could be changed, but I didn't try
to find them.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8965871 [details] [diff] [review]
Part 5: Make some XPCOM methods more strongly typed

>+++ b/security/manager/ssl/nsCertTree.cpp
> NS_IMETHODIMP nsCertTree::Drop(int32_t row, int32_t orient,
>-			       nsISupports* aDataTransfer)
>+			                         mozilla::dom::DataTransfer* aDataTransfer)

Tabs?

r=me.  This looks awesome.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9)
> Tabs?
> 
> r=me.  This looks awesome.

Ugh

I didn't realize they were tabs & just aligned it locally >.<

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 8965868 [details] [diff] [review]
Part 2: Parse webidl productions in xpidl

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

Nit: "rust" to "Rust" in the commit message.

Comment 12

[Andrew McCreight [:mccr8]]

Comment on attachment 8965869 [details] [diff] [review]
Part 3: Generate DOMObject info for xptinfo

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

::: xpcom/reflect/xptinfo/xptcodegen.py
@@ +219,5 @@
>              d1 = type['iid_is']
>  
> +        elif tag == 'TD_DOMOBJECT':
> +            idx = lower_domobject(type)
> +            d1 = idx >> 8

Similar to the C++ file, having a little "split index" helper function here would be slick, if you think that makes sense.

::: xpcom/reflect/xptinfo/xptinfo.h
@@ +269,5 @@
>    }
>  
> +  const nsXPTDOMObjectInfo& GetDOMObjectInfo() const {
> +    MOZ_ASSERT(Tag() == TD_DOMOBJECT);
> +    uint16_t index = ((uint16_t)mData1 << 8) | mData2;

Maybe add a private helper that does this bit mashing and use it both here and GetInterface()? You could put the 16-bit alignment requirement comment on that.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40a027a1f2cf
Part 1: Read webidl's Bindings.conf, and pass it into xpidl, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/c59b5be67ba2
Part 2: Parse webidl productions in xpidl, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae4da56bcf71
Part 3: Generate DOMObject info for xptinfo, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/667b0dbdc190
Part 4: Handle DOM Objects in XPConnect, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/25b2e5c62dbf
Part 5: Make some XPCOM methods more strongly typed, r=bz

Comment 14

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/40a027a1f2cf
https://hg.mozilla.org/mozilla-central/rev/c59b5be67ba2
https://hg.mozilla.org/mozilla-central/rev/ae4da56bcf71
https://hg.mozilla.org/mozilla-central/rev/667b0dbdc190
https://hg.mozilla.org/mozilla-central/rev/25b2e5c62dbf