Closed Bug 1289137 Opened 3 years ago Closed 3 years ago

Make XPCNativeSet::NewInstance() take an nsTArray argument

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(1 file, 2 obsolete files)

Right now, it takes an XPCNativeInterface** argument which requires manual cleanup in the caller, and is ugly to iterate over.
This code is mostly untouched since the initial XPCDOM landing in 2001.
I could clean up ArrayAutoMarkingPtr more, but it is going to be removed entirely in bug 1288870.
Bobby, are you okay with billm reviewing this? I think the only tricky non-generic part is understanding ArrayAutoMarkingPtr and Bill is probably most familiar with it.
Flags: needinfo?(bobbyholley)
I suppose this request is redundant with my one in bug 1288817, so I'll just clear it and ask Bill for review. Feel free to register an objection. :)
Flags: needinfo?(bobbyholley)
Comment on attachment 8774380 [details] [diff] [review]
Make XPCNativeSet::NewInstance() take an nsTArray argument.

Review of attachment 8774380 [details] [diff] [review]:
-----------------------------------------------------------------

Setting r?
Attachment #8774380 - Flags: review?(wmccloskey)
Oops, thanks.
Minor rebasing.
Attachment #8776169 - Flags: review?(wmccloskey)
Attachment #8774380 - Attachment is obsolete: true
Attachment #8774380 - Flags: review?(wmccloskey)
I could clean up ArrayAutoMarkingPtr more, but it is going to be
removed entirely in bug 1288870.

Review commit: https://reviewboard.mozilla.org/r/69788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69788/
Attachment #8778485 - Flags: review?(wmccloskey)
Attachment #8776169 - Attachment is obsolete: true
Attachment #8776169 - Flags: review?(wmccloskey)
Comment on attachment 8778485 [details]
Bug 1289137 - Make XPCNativeSet::NewInstance() take an nsTArray argument.

https://reviewboard.mozilla.org/r/69788/#review67442

::: js/xpconnect/src/XPCWrappedNativeInfo.cpp:490
(Diff revision 1)
> -    // hacky way to get a XPCNativeInterface** using the AutoPtr
> -    XPCNativeInterface* temp[] = {iface};
> -    set = NewInstance(temp, 1);
> +    AutoTArray<XPCNativeInterface*, 1> temp;
> +    temp.AppendElement(iface);
> +    set = NewInstance(temp);

I would recommed passing the nsTArray to NewInstance as an nsTArray&&. Then you could call:
  set = NewInstance(AutoTArray<XPCNI*, 1>(iface));
I think this should work and be efficient.

::: js/xpconnect/src/XPCWrappedNativeInfo.cpp:567
(Diff revision 1)
> -            *(currentInterface++) = iface;
> +            interfaceArray.AppendElement(iface);
> -            interfaceCount++;
>          }
>  
> -        if (interfaceCount) {
> -            set = NewInstance(interfaceArray, interfaceCount);
> +        if (interfaceArray.Length() > 0) {
> +            set = NewInstance(interfaceArray);

You would need to use Move here if you take my suggestion.

::: js/xpconnect/src/XPCWrappedNativeInfo.cpp:641
(Diff revision 1)
> -        set = NewInstance(&newInterface, 1);
> +        AutoTArray<XPCNativeInterface*, 1> temp;
> +        temp.AppendElement(newInterface);
> +        set = NewInstance(temp);

Can use the same pattern here as above.
Attachment #8778485 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #10)
> I would recommed passing the nsTArray to NewInstance as an nsTArray&&.

That's pretty nice. Unfortunately, that doesn't work, because you can't pass in a single argument to the ctor. I'd noticed that before, which is why there was the AppendElement() on a second line. It looks like AutoTArray is just missing a ctor with initializer_list like the other array types, so I'll file a bug for that and then make the changes you suggest, assuming there are no issues there.
Oops. I read over that code and thought I saw it. The initializer_list thing sounds good.
Thinking about this some more, I might as well just use nsTArray in the caller: moving from an AutoTArray to an nsTArray is surely going to cause a heap allocation, so I might as well not bother with the auto array. This is technically less efficient, but I'm sure it doesn't matter.

And I can just do this, which is slick:
  set = NewInstance({newInterface});
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a7a5fd1b36e5
Make XPCNativeSet::NewInstance() take an nsTArray argument. r=billm
Blocks: 1290614
(In reply to Andrew McCreight [:mccr8] from comment #13)
> Thinking about this some more, I might as well just use nsTArray in the
> caller: moving from an AutoTArray to an nsTArray is surely going to cause a
> heap allocation, so I might as well not bother with the auto array. This is
> technically less efficient, but I'm sure it doesn't matter.
> 
> And I can just do this, which is slick:
>   set = NewInstance({newInterface});

We're not actually moving (as in invoking a move constructor), we're just passing a reference. So I think it would have stayed an AutoTArray the whole time. But yeah, it doesn't really matter.
https://hg.mozilla.org/mozilla-central/rev/a7a5fd1b36e5
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.