774751 - Use nsRefPtr in nsIDocument

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[David Zbarsky (:dzbarsky)]

Attachment: Patch

I think this should be ok even when MOZILLA_INTERNAL_API is undefined because an embedders need to include Loader.h already.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Why does an embedder need to include Loader.h already?

Comment 2

[David Zbarsky (:dzbarsky)]

Err they shouldn't, but it actually doesn't matter, because they won't instantiate the template.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm, ok.  Then why not do the same thing with nsNodeInfoManager?

Comment 4

[David Zbarsky (:dzbarsky)]

I can, but nsIDocument doesn't actually addref nsNodeInfoManager, instead it holds onto nsNodeInfo which holds the nsNodeInfoManager.  I wasn't sure what to do about this, but we can definitely have nsNodeInfoManager be a nsRefPtr.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 643037 [details] [diff] [review]
Patch

I see.  Could you please fix the totally misleading comment on mNodeInfoManager to say that we hold a ref to it via mNodeInfo but not directly?

Also, no need for the #ifdef in nsDocument.cpp: that macro is so totally defined there, it's not even funny.  ;)

And finally, the NS_RELEASE(mCSSLoader) in ~nsDocument needs to go away, right?

r=me with that.  Especially that last part.

Comment 6

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2e22d568f82

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f2e22d568f82