523817 - Make optional out params actually work and allow [optional] before [retval]

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Mike, would you do the honors?

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Shawn Wilsher :sdwilsh]

(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?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, absolutely.

Comment 5

[Mike Shaver (:shaver emeritus)]

Comment on attachment 407748 [details] [diff] [review]
Like so

r=shaver, given the referenced tests.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/6fa54de724ce

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Justin Dolske [:Dolske]]

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?

Comment 10

[Shawn Wilsher :sdwilsh]

(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!

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

This is likely to be needed for firebug.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Thus, blocking. Please land away on 1.9.2.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/18a08c4957b4

Comment 15

[Eric Shepherd [:sheppy]]

Bug 399537 covers the need to update and migrate the XPIDL docs, removing the doc needed keyword here.