Closed Bug 309654 Opened 15 years ago Closed 6 years ago

Remove the mPrefs member from nsDocShell

Categories

(Core :: DOM: Navigation, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: vhaarr+bmo, Assigned: masayuki)

References

Details

Attachments

(1 file)

DocShell is a bit static when it comes to prefs, lets fix it up with pref observers.
Attached patch version 0.1Splinter Review
First stab.

Where can I find the docs/method signature for nsCAutoString::Find(const
char*)? Sounds pretty deprecated to me.

biesi: Are we guaranteed that ::Create will be called before anything else? Why
is there both a ::Init and a ::Create?
Attachment #197108 - Flags: review?(cbiesinger)
Comment on attachment 197108 [details] [diff] [review]
version 0.1

urg, you're bloating docshell...

http://lxr.mozilla.org/seamonkey/source/xpcom/string/public/nsTString.h#143
sounds like Find is indeed obsolete :)

I'd really rather not add all these members...
cc'ing bz for the Init/Create question ;)
> Are we guaranteed that ::Create will be called before anything else?

Which "anything else"?  But I suspect the answer here is "no".

> Why is there both a ::Init and a ::Create?

Init() is called at object creation time by the factory.  Create() is an
interface method on the nsIBaseWindow interface, which the docshell implements.
(In reply to comment #2)
> (From update of attachment 197108 [details] [diff] [review] [edit])
> urg, you're bloating docshell...

Indeed. I asked, and you replied in bug 282050 comment 14 that we should observe
prefs, so I made this patch :-)

In any case, taking a second look at the patch (in bug 282050), I see
AddObserver instructs the pref service to hold a strong ref to the DocShell. I
think this will cause us problems on shutdown (this patch makes it use weak refs).

So whether we'd like this patch or not, I don't know - but it does remove
"in-action" pref getting. If we want it, I think the initial pref getting should
be moved out of ::Create and into ::Init, since we're guaranteed that it is
invoked before any other methods (?).
Yes, you're guaranteed that Init() will be called before anything else.
(finally cc'ing myself. sorry for replying only now; didn't see the comments)

(In reply to comment #5)
> Indeed. I asked, and you replied in bug 282050 comment 14 that we should observe
> prefs, so I made this patch :-)

Oh. That was a misunderstanding, sorry about that.

The prefs that are gotten directly before they are used don't need to observe
anything, they are already live enough.

I replied there without reading docshell code. My comment was meant to only
refer to prefs that already are members of nsDocShell.cpp; I believe a few
exist. I wouldn't change the places that get the prefs in-place.

> In any case, taking a second look at the patch (in bug 282050), I see
> AddObserver instructs the pref service to hold a strong ref to the DocShell. I
> think this will cause us problems on shutdown (this patch makes it use weak refs).

That might be ok... but I'm not sure under which conditions Destroy is called
(which removes the observers).

> So whether we'd like this patch or not, I don't know - but it does remove
> "in-action" pref getting. If we want it, I think the initial pref getting should
> be moved out of ::Create and into ::Init, since we're guaranteed that it is
> invoked before any other methods (?).

yep.
Attachment #197108 - Flags: review?(cbiesinger) → review-
QA Contact: adamlock → docshell
This was fixed/removed by Bug 666901:
https://hg.mozilla.org/mozilla-central/rev/9c2e57bef564
Assignee: vhaarr+bmo → masayuki
Status: NEW → RESOLVED
Closed: 6 years ago
Depends on: 666901
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.