(dormant account) Assignee
	Description • 16 years ago

patch coming soon

	(dormant account) Assignee
	Comment 1 • 16 years ago

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?

	(dormant account) Assignee
	Comment 2 • 16 years ago

Would be landing with this as part of the bigger patch

	(dormant account) Assignee
	Comment 3 • 16 years ago

sorry, forgot to qrefresh

	Johnny Stenback (:jst)
	Comment 4 • 16 years ago

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!

	(dormant account) Assignee
	Comment 5 • 16 years ago

Jim, I need your emacs powers!

	(dormant account) Assignee
	Comment 6 • 16 years ago

another automatic rev

	(dormant account) Assignee
	Comment 7 • 16 years ago

I'm frustrated with the outparamdelled getter nsNodeInfoManager::GetNodeInfo() as I don't know how to make xpcom not leak in that case.

	Johnny Stenback (:jst)
	Comment 8 • 16 years ago

(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.

	(dormant account) Assignee
	Comment 9 • 16 years ago

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().

	Johnny Stenback (:jst)
	Comment 10 • 16 years ago

Comment on attachment 338370 [details] [diff] [review]
alreadyAddRefed patch

Looks good! r+sr=jst

	(dormant account) Assignee
	Comment 11 • 16 years ago

pushed http://hg.mozilla.org/mozilla-central/file/ec4733eb1bd8

	neil@parkwaycc.co.uk
	Comment 12 • 16 years ago

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...