Remove the mPrefs member from nsDocShell

RESOLVED FIXED in mozilla7

Status

()

Core
Document Navigation
--
minor
RESOLVED FIXED
13 years ago
4 years ago

People

(Reporter: Vidar Haarr (not reading bugmail), Assigned: masayuki)

Tracking

Trunk
mozilla7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

13 years ago
DocShell is a bit static when it comes to prefs, lets fix it up with pref observers.
(Reporter)

Comment 1

13 years ago
Created attachment 197108 [details] [diff] [review]
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?
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.
(Reporter)

Comment 5

13 years ago
(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

Comment 8

4 years ago
This was fixed/removed by Bug 666901:
https://hg.mozilla.org/mozilla-central/rev/9c2e57bef564
Assignee: vhaarr+bmo → masayuki
Status: NEW → RESOLVED
Last Resolved: 4 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.