Closed Bug 523817 Opened 10 years ago Closed 10 years ago
Make optional out params actually work and allow [optional] before [retval]
Two proposed changes: 1) [optional] out params almost work. The XPCWrappedNative::CallMethod machinery deals with the part after the call fine (converts the out result to a jsval and if the arg wasn't passed just drops the jsval on the floor so it'll get gced later. But the checks before the call require an object for out params, even optional ones. Loosening up the checks is pretty straightforward. 2) xpidl enforces that only [optional] arguments come after an [optional] argument. I'd like to relax that to allow a [retval] as well. The net result is that I can then write: void getEventTargetChainFor(in nsIDOMEventTarget aEventTarget, [optional] out unsigned long aCount, [retval, array, size_is(aCount)] out nsIDOMEventTarget aOutArray); and from JS call: getEventTargetChainFor(foo); and have it work.
Mike, would you do the honors?
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #407748 - Flags: review?(shaver)
Oh, and if someone has bright ideas for how to test short of changing some of our existing tests to exercise this code, that'd be nice.
(In reply to comment #2) > Oh, and if someone has bright ideas for how to test short of changing some of > our existing tests to exercise this code, that'd be nice. We'd have to change some interface to make it work - I presume you'll have a test in 507488 which should be good enough, right?
Attachment #407748 - Flags: review?(shaver) → review+
Comment on attachment 407748 [details] [diff] [review] Like so r=shaver, given the referenced tests.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment on attachment 407748 [details] [diff] [review] Like so We should take this on 1.9.2. It's completely compatible with both js and c++ callers and callees.
Yay! If the interface's implementation is in JS, does it still need to set the now-optional size_is argument? Or does xpconnect magically pull .length from the JS array? Also, since you didn't bump the IID in the idl, I assume it's safe to go ahead and add the [optional] on existing interfaces?
(In reply to comment #9) > Also, since you didn't bump the IID in the idl, I assume it's safe to go ahead > and add the [optional] on existing interfaces? Yeah, it's a completely compatible interface change. Yay!
Justin, this doesn't affect callees, just like [optional] in general. So the JS impl still needs to set the size_is argument. And yes, this is a completely compatible change, so you can sprinkle the [optional] around.
This is likely to be needed for firebug.
Thus, blocking. Please land away on 1.9.2.
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Bug 399537 covers the need to update and migrate the XPIDL docs, removing the doc needed keyword here.
You need to log in before you can comment on or make changes to this bug.