Closed Bug 1444524 Opened 6 years ago Closed 6 years ago

Improve XPIDL array ergonomics

Categories

(Core :: XPConnect, enhancement, P2)

enhancement

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: bholley, Unassigned)

References

(Blocks 1 open bug)

Details

Flags: needinfo?(MattN+bmo)
(In reply to Nathan Froyd [:froydnj] from bug 1444515 comment #2)
> Is marking parameters as [array] in XPIDL not suitable for this case?  Can
> you provide an example of what the interfaces involved look like for people
> unfamiliar with web payments?

In general the ergonomics around [array] are awful - manual memory allocation, passing around explicit sizes, etc.

We could add support for passing things via nsTArray, like WebIDL does, both as in and out params (via the dipper mechanism we use for nsA{,C}String).
[1] is where we have a helper in JS to convert 5 different nsIArray attributes implementing 4 different interfaces to regular JS arrays so we can then serialize them as a JSON string and send them to the content process.

[2] is where the attributes are defined on the interfaces.

[1] https://dxr.mozilla.org/mozilla-central/rev/f1965cf7425fe422c9e9c78018f11b97e0a0f229/toolkit/components/payments/content/paymentDialogWrapper.js#322-352
[2] https://dxr.mozilla.org/mozilla-central/source/dom/interfaces/payments/nsIPaymentRequest.idl#41,60-62,84
Flags: needinfo?(MattN+bmo)
See Also: → ArrayConverter, 333459
Yeah, nsIArray sucks.

I wonder if it would make sense to introduce a WebIDL XPCOMArray type. It could behave nicely like an array using all the WebIDL stuff, and the individual entries would just be nsISupports (types could implement nsIClassInfo if they wanted to skip the QI from JS). We could parent the thing into the shared JSM global or something. And it could implement a dummy XPCOM interface so that we could declare it in XPIDL.

Thoughts?
So here's my devil's advocate position.  For array return values, why can we not just return "jsval" and use ToJSValue on the C++ side to create the JS array from the nsTArray the underlying impl presumably has?

If we want to return arrays by reference, note that's something webidl doesn't really support unless you go off into "object" weeds (or FrozenArray, I guess).

For arguments array, we should think a bit.  Having to |new XPCOMArray| to pass an array argument is pretty annoying to start with...
That's a good point. Matt, want to try that out and let us know how it goes?
Flags: needinfo?(MattN+bmo)
Another design pattern for avoiding dealing with arrays in XPIDL is what I did in mozISandboxReporter[1]: expose objects with a length property and a “get nth element” method, and have a layer of JS[2] that builds regular JS values.  This seems less than ideal, but it also seemed simpler than jsval and using the JS API directly in that case.

[1] https://searchfox.org/mozilla-central/rev/8fa0b32c84f9/security/sandbox/linux/interfaces/mozISandboxReporter.idl
[2] https://searchfox.org/mozilla-central/rev/8fa0b32c84f9/toolkit/modules/Troubleshoot.jsm#722-735
(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #6)
> Another design pattern for avoiding dealing with arrays in XPIDL is what I
> did in mozISandboxReporter[1]: expose objects with a length property and a
> “get nth element” method, and have a layer of JS[2] that builds regular JS
> values.

That seems roughly analogous to what Matt is doing with nsIArray. Workable, but less than ideal.

> This seems less than ideal, but it also seemed simpler than jsval
> and using the JS API directly in that case.

Even with ToJSVal being pretty point-and-shoot these days?
Eden, is comment 4 something you could try so we can delete the code in [1] of comment 2?
Flags: needinfo?(MattN+bmo) → needinfo?(chenyu.chuang)
Notice that, I will try the suggestion in comment 4 after bug 1408234 is fixed. Thanks.
Flags: needinfo?(chenyu.chuang)
Priority: -- → P2
Nika has a proposed solution in bug 1474369.
Depends on: 1474369
Is this complete now with bug 1474369 landing?
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #11)
> Is this complete now with bug 1474369 landing?

Probably. Nika, do you agree? If so please resolve WFM.
Flags: needinfo?(nika)
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(nika)
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.