Closed Bug 450777 Opened 12 years ago Closed 12 years ago

outparamdel nsNodeInfoManager::GetNodeInfo

Categories

(Firefox Build System :: Source Code Analysis, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: taras.mozilla, Assigned: taras.mozilla)

References

Details

Attachments

(1 file, 5 obsolete files)

patch coming soon
Attached patch automagical patch (obsolete) — Splinter Review
I'd like to land one of these patches. I've cleaned up the generated code a bit. Do you have any more suggestions as to what needs to be better(other than indentation) before this can land?
Assignee: nobody → tglek
Status: NEW → ASSIGNED
Attachment #334028 - Flags: review?(jst)
Attached patch labour-intensive patch (obsolete) — Splinter Review
Would be landing with this as part of the bigger patch
Attached patch manual patch (obsolete) — Splinter Review
sorry, forgot to qrefresh
Attachment #334031 - Attachment is obsolete: true
Comment on attachment 334028 [details] [diff] [review]
automagical patch

+  rv = nodeInfo ? NS_OK : NS_ERROR_FAILURE;
   NS_ENSURE_SUCCESS(rv, rv);

It'd be nice if this could be changed to NS_ENSURE_TRUE(nodeInfo, NS_ERROR_FAILURE), and extra bonus points for automatically fixing up indentation too! :)

r+sr=jst!
Attachment #334028 - Flags: superreview+
Attachment #334028 - Flags: review?(jst)
Attachment #334028 - Flags: review+
Jim, I need your emacs powers!
Attached patch automagic patch (obsolete) — Splinter Review
another automatic rev
Attachment #334028 - Attachment is obsolete: true
Attached patch full patch(auto + manual) (obsolete) — Splinter Review
I'm frustrated with the outparamdelled getter nsNodeInfoManager::GetNodeInfo() as I don't know how to make xpcom not leak in that case.
Attachment #334033 - Attachment is obsolete: true
Attachment #338196 - Attachment is obsolete: true
Attachment #338207 - Flags: review?(jst)
(In reply to comment #7)
> Created an attachment (id=338207) [details]
> full patch(auto + manual)
> 
> I'm frustrated with the outparamdelled getter nsNodeInfoManager::GetNodeInfo()
> as I don't know how to make xpcom not leak in that case.

I think we'll inevitably need to make this return an already_AddRefed<nsINodeInfo> and have the places that use raw pointers with this return value use .get() on the return, i.e. nsINodeInfo *foo = nsNodeInfoManager::GetNodeInfo(...).get(), or something along those lines.
so the logic here if someone called the getter with getter_AddRefs(), then it becomes
foo = getter() 
in other cases the form is
foo = getter().get()
I think this gets correct behavior for cases like nsNodeInfoManager::GetTextNodeInfo().
Attachment #338207 - Attachment is obsolete: true
Attachment #338370 - Flags: review?(jst)
Attachment #338207 - Flags: review?(jst)
Attachment #338370 - Flags: superreview+
Attachment #338370 - Flags: review?(jst)
Attachment #338370 - Flags: review+
Comment on attachment 338370 [details] [diff] [review]
alreadyAddRefed patch

Looks good! r+sr=jst
pushed http://hg.mozilla.org/mozilla-central/file/ec4733eb1bd8
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 455536
Comment on attachment 338370 [details] [diff] [review]
alreadyAddRefed patch

>+  NS_ENSURE_TRUE(newNodeInfo, NS_ERROR_FAILURE);
>   NS_ASSERTION(newNodeInfo, "GetNodeInfo lies");
This makes no sense...
Blocks: 488360
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.