Last Comment Bug 774751 - Use nsRefPtr in nsIDocument
: Use nsRefPtr in nsIDocument
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: David Zbarsky (:dzbarsky)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-17 11:01 PDT by David Zbarsky (:dzbarsky)
Modified: 2012-07-19 07:32 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (7.16 KB, patch)
2012-07-17 11:01 PDT, David Zbarsky (:dzbarsky)
bzbarsky: review+
Details | Diff | Splinter Review

Description David Zbarsky (:dzbarsky) 2012-07-17 11:01:14 PDT
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.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2012-07-17 11:50:36 PDT
Why does an embedder need to include Loader.h already?
Comment 2 David Zbarsky (:dzbarsky) 2012-07-17 11:57:08 PDT
Err they shouldn't, but it actually doesn't matter, because they won't instantiate the template.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2012-07-17 12:06:44 PDT
Hmm, ok.  Then why not do the same thing with nsNodeInfoManager?
Comment 4 David Zbarsky (:dzbarsky) 2012-07-17 12:28:04 PDT
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 [:bz] (still a bit busy) 2012-07-17 12:41:34 PDT
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) 2012-07-18 15:16:39 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2e22d568f82
Comment 7 Ed Morley [:emorley] 2012-07-19 07:32:48 PDT
https://hg.mozilla.org/mozilla-central/rev/f2e22d568f82

Note You need to log in before you can comment on or make changes to this bug.