Use more smart pointers in XPConnect

RESOLVED FIXED in Firefox 42

Status

()

Core
XPConnect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8642554 [details] [diff] [review]
part 1 - Use more smart pointers in XPConnect.

There's no hurry on reviewing this, though the patch isn't very tricky, just a little monotonous.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc7cf4cb9333
Attachment #8642554 - Flags: review?(gkrizsanits)
(Assignee)

Comment 2

3 years ago
Created attachment 8642555 [details] [diff] [review]
part 2 - Use an early return in XPCConvert::JSObject2NativeInterface.
Attachment #8642555 - Flags: review?(gkrizsanits)
Comment on attachment 8642554 [details] [diff] [review]
part 1 - Use more smart pointers in XPConnect.

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +65,5 @@
>  
>      nsXPConnect* xpc = nsXPConnect::XPConnect();
>      RootedObject obj(cx, &v.toObject());
> +    return NS_SUCCEEDED(xpc->GetWrappedNativeOfJSObject(cx, obj, getter_AddRefs(wn))) && wn &&
> +        NS_SUCCEEDED(wn->Native()->QueryInterface(iid, getter_AddRefs(iface))) && iface;

I find this line a bit difficult to read. I would prefer something like:

nsresult rv = xpc->GetWrappedNativeOfJSObject(cx, obj, getter_AddRefs(wn));
if (NS_FAILED(rv) || !wn)
  return false

rv = wn->Native()->QueryInterface(iid, getter_AddRefs(iface));
return NS_SUCCEDED(rv) && iface;


But I'm fine with any slightly different variants too as long as important calls are not inside NS macros. (Something Bobby asked me to avoid)
Attachment #8642554 - Flags: review?(gkrizsanits) → review+
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> But I'm fine with any slightly different variants too as long as important
> calls are not inside NS macros. (Something Bobby asked me to avoid)

Actually since this whole file is full of this practice, if it's too much of a hassle, I'm fine with landing it as it is, this patch is already a great improvement :)
Attachment #8642555 - Flags: review?(gkrizsanits) → review+
(Assignee)

Comment 5

3 years ago
Yeah, there's some pretty ugly code in here. :) Thanks for the reviews.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/318d42434ec3
https://hg.mozilla.org/mozilla-central/rev/9e2da5650273
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42

Updated

2 years ago
Depends on: 1198058
(Assignee)

Updated

2 years ago
No longer depends on: 1198058
You need to log in before you can comment on or make changes to this bug.