Closed
Bug 1444524
Opened 7 years ago
Closed 6 years ago
Improve XPIDL array ergonomics
Categories
(Core :: XPConnect, enhancement, P2)
Core
XPConnect
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: bholley, Unassigned)
References
(Blocks 1 open bug)
Details
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(MattN+bmo)
Reporter | ||
Comment 1•7 years ago
|
||
(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).
Comment 2•7 years ago
|
||
[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)
Reporter | ||
Comment 3•7 years ago
|
||
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?
Comment 4•7 years ago
|
||
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...
Reporter | ||
Comment 5•7 years ago
|
||
That's a good point. Matt, want to try that out and let us know how it goes?
Flags: needinfo?(MattN+bmo)
Comment 6•7 years ago
|
||
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
Reporter | ||
Comment 7•7 years ago
|
||
(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?
Comment 8•7 years ago
|
||
Flags: needinfo?(MattN+bmo) → needinfo?(chenyu.chuang)
Comment 9•7 years ago
|
||
Notice that, I will try the suggestion in comment 4 after bug 1408234 is fixed. Thanks.
Flags: needinfo?(chenyu.chuang)
Updated•7 years ago
|
Priority: -- → P2
Comment 11•6 years ago
|
||
Is this complete now with bug 1474369 landing?
Reporter | ||
Comment 12•6 years ago
|
||
(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)
Updated•6 years ago
|
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.
Description
•