Closed Bug 1444991 Opened 2 years ago Closed 2 years ago

Add a cleaner mechanism for representing WebIDL interfaces in XPIDL

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: Nika, Assigned: Nika)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files)

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).
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 ^_^.
Assignee: nobody → nika
Depends on: 1438688
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.
(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.
Depends on: 1444745
This information is read in order to handle correctly selecting the native
type and header files for WebIDL types.
Attachment #8965867 - Flags: review?(continuation)
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.
Attachment #8965868 - Flags: review?(continuation)
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.
Attachment #8965869 - Flags: review?(continuation)
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.
Attachment #8965870 - Flags: review?(continuation)
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.
Attachment #8965871 - Flags: review?(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.
Attachment #8965871 - Flags: review?(bzbarsky) → review+
(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 >.<
Attachment #8965867 - Flags: review?(continuation) → review+
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.
Attachment #8965868 - Flags: review?(continuation) → review+
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.
Attachment #8965869 - Flags: review?(continuation) → review+
Attachment #8965870 - Flags: review?(continuation) → review+
Pushed by nika@thelayzells.com:
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
You need to log in before you can comment on or make changes to this bug.