309654 - Remove the mPrefs member from nsDocShell

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Vidar Haarr (not reading bugmail)]

DocShell is a bit static when it comes to prefs, lets fix it up with pref observers.

Comment 1

[Vidar Haarr (not reading bugmail)]

Attachment: version 0.1

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?

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

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...

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

cc'ing bz for the Init/Create question ;)

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 5

[Vidar Haarr (not reading bugmail)]

(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 (?).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yes, you're guaranteed that Init() will be called before anything else.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

(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.

Comment 8

[:Cykesiopka]

This was fixed/removed by Bug 666901:
https://hg.mozilla.org/mozilla-central/rev/9c2e57bef564