Use nsRefPtr in nsIDocument

RESOLVED FIXED in mozilla17

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 643037 [details] [diff] [review]
Patch

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)

Comment 1

5 years ago
Why does an embedder need to include Loader.h already?
(Assignee)

Comment 2

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

Comment 3

5 years ago
Hmm, ok.  Then why not do the same thing with nsNodeInfoManager?
(Assignee)

Comment 4

5 years ago
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

5 years ago
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+
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2e22d568f82
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/f2e22d568f82
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.