Closed
Bug 1190436
Opened 9 years ago
Closed 9 years ago
Use more smart pointers in XPConnect
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
Details
Attachments
(2 files)
8.32 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
2.96 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
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•9 years ago
|
||
Attachment #8642555 -
Flags: review?(gkrizsanits)
Comment 3•9 years ago
|
||
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+
Comment 4•9 years ago
|
||
(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 :)
Updated•9 years ago
|
Attachment #8642555 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 5•9 years ago
|
||
Yeah, there's some pretty ugly code in here. :) Thanks for the reviews.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/318d42434ec3 https://hg.mozilla.org/integration/mozilla-inbound/rev/9e2da5650273
Keywords: checkin-needed
Comment 7•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/318d42434ec3 https://hg.mozilla.org/mozilla-central/rev/9e2da5650273
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•