Closed Bug 774751 Opened 10 years ago Closed 10 years ago

Use nsRefPtr in nsIDocument

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
I think this should be ok even when MOZILLA_INTERNAL_API is undefined because an embedders need to include Loader.h already.
Attachment #643037 - Flags: review?(bzbarsky)
Why does an embedder need to include Loader.h already?
Err they shouldn't, but it actually doesn't matter, because they won't instantiate the template.
Hmm, ok.  Then why not do the same thing with nsNodeInfoManager?
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 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.
Attachment #643037 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/f2e22d568f82
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.