Closed Bug 523817 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: XPConnect, defect, P2)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta2-fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

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.
Attached patch Like soSplinter Review
Mike, would you do the honors?
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #407748 - Flags: review?(shaver)
Blocks: 507448
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?
Yeah, absolutely.
Comment on attachment 407748 [details] [diff] [review]
Like so

r=shaver, given the referenced tests.
Pushed http://hg.mozilla.org/mozilla-central/rev/6fa54de724ce
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Attachment #407748 - Flags: approval1.9.2?
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.
Blocks: 524270
Keywords: dev-doc-needed
Duplicate of this bug: 380597
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.
Blocks: 524533
This is likely to be needed for firebug.
Flags: blocking1.9.2?
Thus, blocking. Please land away on 1.9.2.
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Attachment #407748 - Flags: approval1.9.2?
Blocks: 525710
Bug 399537 covers the need to update and migrate the XPIDL docs, removing the doc needed keyword here.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.