Closed
Bug 965920
Opened 7 years ago
Closed 7 years ago
Smart pointerize getters in XPConnect
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: mccr8, Assigned: mccr8)
Details
Attachments
(3 files)
13.18 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
2.99 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
5.08 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
Instead of calling ADDREF, we can assign into a smart pointer, and return a forget. This is kind of silly, but it makes the remaining more complex uses of ADDREF stand out more, so it is kind of a code hygiene issue.
Assignee | ||
Comment 1•7 years ago
|
||
green L64 debug try run: https://tbpl.mozilla.org/?tree=Try&rev=c1238ed7dd0a
Attachment #8369442 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8369443 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 3•7 years ago
|
||
No hurry on any of these...
Attachment #8369444 -
Flags: review?(bobbyholley)
Comment 4•7 years ago
|
||
Comment on attachment 8369442 [details] [diff] [review] part 1 - Smart pointerize getters in XPConnect. Review of attachment 8369442 [details] [diff] [review]: ----------------------------------------------------------------- r=bholley with comments addressed. ::: js/xpconnect/src/XPCConvert.cpp @@ +770,5 @@ > CreateHolderIfNeeded(HandleObject obj, MutableHandleValue d, > nsIXPConnectJSObjectHolder** dest) > { > if (dest) { > + nsRefPtr<XPCJSObjectHolder> objHolder = XPCJSObjectHolder::newHolder(obj); This is infallible if obj is non-null, FWIW. ::: js/xpconnect/src/XPCWrappedJSClass.cpp @@ +420,5 @@ > /* readonly attribute nsIVariant value; */ > NS_IMETHODIMP xpcProperty::GetValue(nsIVariant * *aValue) > { > + nsCOMPtr<nsIVariant> rval = mValue; > + mValue.forget(aValue); I think you mean rval here. Presumably, we must not have great test coverage for this. ::: js/xpconnect/src/nsXPConnect.cpp @@ +665,3 @@ > } > > + nsRefPtr<nsIXPConnectWrappedNative> temp = XPCWrappedNative::Get(aJSObj); If we're using an nsRefPtr, we should use the concrete type.
Attachment #8369442 -
Flags: review?(bobbyholley) → review+
Updated•7 years ago
|
Attachment #8369443 -
Flags: review?(bobbyholley) → review+
Updated•7 years ago
|
Attachment #8369444 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 5•7 years ago
|
||
I filed bug 967562 on possibly getting rid of newHolder. > I think you mean rval here. Presumably, we must not have great test coverage for this. Oops, good catch! Yeah that's the problem with changing this old code.
Assignee | ||
Comment 6•7 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/55dcd2a5362e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ca0b05e7155f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0f26c4b3eab7
Assignee | ||
Comment 7•7 years ago
|
||
Followup to fix some magical bustage that appeared recently. https://hg.mozilla.org/integration/mozilla-inbound/rev/6c2bba06ed58
Comment 8•7 years ago
|
||
Sorry, backed 'em all out in https://hg.mozilla.org/integration/mozilla-inbound/rev/930f794bfba3 for the https://tbpl.mozilla.org/php/getParsedLog.php?id=34108227&tree=Mozilla-Inbound bustage on the followup.
Comment 9•7 years ago
|
||
Sorry again, you'll find that failure incomprehensible from any of your patches because it was from bustage that landed on your bustage busted on the same builds.
Comment 10•7 years ago
|
||
Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/e998c14ccd10 https://hg.mozilla.org/integration/mozilla-inbound/rev/36f567b48ce2 https://hg.mozilla.org/integration/mozilla-inbound/rev/1c9ccc6fd564 https://hg.mozilla.org/integration/mozilla-inbound/rev/1436b6638ff3 Watch out for mcMerge not commenting or closing the bug when this gets merged to m-c, not sure it copes with in-out-in in a single merge-wad.
Comment 11•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e998c14ccd10 https://hg.mozilla.org/mozilla-central/rev/36f567b48ce2 https://hg.mozilla.org/mozilla-central/rev/1c9ccc6fd564 https://hg.mozilla.org/mozilla-central/rev/1436b6638ff3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•