Layout should use the newer nsIPrefService APIs instead of nsIPref

RESOLVED FIXED

Status

()

Core
Layout: Misc Code
P2
minor
RESOLVED FIXED
16 years ago
9 years ago

People

(Reporter: Christopher Aillon (sabbatical, not receiving bugmail), Assigned: Christopher Aillon (sabbatical, not receiving bugmail))

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patchlove])

Attachments

(3 attachments, 1 obsolete attachment)

Part of the ongoing work I'm doing to rid the source of nsIPref.
Created attachment 107779 [details] [diff] [review]
Proposed patch v1

Of note:

- I moved the pref listening stuff for blink out of nsHTMLReflowState into
nsPresContext.	I stored a static global there and nsHTMLReflowState caches a
pointer to that static.  nsPresContext then listens for changes to the pref and
updates the global so that nsHTMLReflowState gets the updates.

- We probably should clean up nsPresContext::PreferenceChanged() so that for
every pref that changes, we don't re-get every pref value.  I opted to not do
that in this patch.
Attachment #107779 - Flags: review?(dbaron)
Comment on attachment 107779 [details] [diff] [review]
Proposed patch v1

New patch to follow in the next day or so.
Attachment #107779 - Attachment is obsolete: true
Attachment #107779 - Flags: review?(dbaron)
Created attachment 108643 [details] [diff] [review]
Better patch
[Checkin: Comment 6]

Here we go.
Attachment #108643 - Flags: review?(roc+moz)
+#if defined(XP_MAC) || defined(XP_MACOSX)
+#include "nsIPrefBranch.h"
+#include "nsIPrefService.h"
+#endif

Just include them unconditionally. #ifdefs suck.

Other than that, looks good.
Comment on attachment 108643 [details] [diff] [review]
Better patch
[Checkin: Comment 6]

r=roc+moz AND sr=roc+moz
Yes, I can do this. Ask brendan if you don't believe me :-).
Attachment #108643 - Flags: superreview+
Attachment #108643 - Flags: review?(roc+moz)
Attachment #108643 - Flags: review+
Landed
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Reopening since caillon backed this out due to a Txul regression.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 108643 [details] [diff] [review]
Better patch
[Checkin: Comment 6]

>Index: layout/base/src/nsPresContext.cpp
>@@ -328,13 +317,13 @@
>   // get attributes specific to each generic font
>   nsCAutoString generic_dot_langGroup;
>   for (PRInt32 eType = eDefaultFont_Variable; eType < eDefaultFont_COUNT; ++eType) {
>     generic_dot_langGroup.Assign(kGenericFont[eType]);
>     generic_dot_langGroup.Append(NS_ConvertUCS2toUTF8(langGroup));
> 
>-    nsFont* font;
>+    nsFont* font = nsnull;
>     switch (eType) {
>       case eDefaultFont_Variable:  font = &mDefaultVariableFont;  break;
>       case eDefaultFont_Fixed:     font = &mDefaultFixedFont;     break;
>       case eDefaultFont_Serif:     font = &mDefaultSerifFont;     break;
>       case eDefaultFont_SansSerif: font = &mDefaultSansSerifFont; break;
>       case eDefaultFont_Monospace: font = &mDefaultMonospaceFont; break;

I'm actually not a big fan of warning-silencing changes of this type, since
they can prevent purify from finding real bugs in case someone misses a case. 
Maybe it's better to put a default case with a NS_NOTREACHED and assignment to
null?
I only backed out of layout/base and layout/html, leaving the layout/svg and
layout/xul changes alone, based on feedback from myotonic which showed the
regression.

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Ports/1039563660.4137.gz&fulltext=1

I'll try and incrementally land these changes again tomorrow to see if I can
determine exactly where the spike is coming from.
Caillon, any news on this?
OS: Linux → All
Hardware: PC → All
Summary: Layout should be converted over to using the newer, frozen pref APIs → Layout should use the newer nsIPrefService APIs instead of nsIPref
Severity: normal → minor
Priority: -- → P2
Created attachment 195098 [details] [diff] [review]
nsContentUtils::RegisterPrefCallback cleanup

I figured that if we want to clean layout of nsIPref, we'll have to clean out
all the PrefCallback stuff and replace it with Observers, so here's Take #1.

There are several things I'm not sure about here ..
1. Currently in nsPresContext::Observe we just catch-all and then update all
preferences regardless of which one changed. I didn't fix this because it was
like that before I converted it - the various methods did take aPrefName (or
whatever) argument, but it was disregarded.
2. In nsFramesetFrame.cpp I'm not sure if the QueryInterface I have removed
(commented out in this patch) there in favor of NS_IMPL_ISUPPORTS is the same
thing at all ...
3. I've added several small helper classes like ns<Something>Observer. Think I
should do it some other way?
4. There might be some tab/space hiccups, because I'm testing out a new editor
at the moment.

Anyway .. Roc, think you can take a look at this?
Attachment #195098 - Flags: review?(roc)
Comment on attachment 195098 [details] [diff] [review]
nsContentUtils::RegisterPrefCallback cleanup

This patch makes us crash on shutdown when nsIPrefBranch2 tries to Release the
nsPresContext, at
<http://lxr.mozilla.org/mozilla/source/layout/base/nsPresContext.cpp#230>. I'm
not sure why ... I'll attach a stacktrace in a sec.
Created attachment 195817 [details]
stacktrace

Note that the nsPresContext line numbers in this stacktrace are off compared to
the ones in CVS. nsPresContext.cpp:234 is 230, like I said in my last comment.
Sounds to me like 

1206     nsIScriptSecurityManager *securityManager =
1207       nsContentUtils::SecurityManager();

in nsDocument.cpp returns null.  Which would happen if layout shutdown comes
before we shut down the observer service.

So we need to either have weak refs here (to the prescontexts) or better yet
remove all prescontexts from the observer service on layout shutdown.
(In reply to comment #14)
> So we need to either have weak refs here (to the prescontexts) or better yet
> remove all prescontexts from the observer service on layout shutdown.

So .. register nsPresContext for observing "xpcom-shutdown" and then remove the
pref observers there? I'm guessing not, because will we be guaranteed to have
them removed before the pref service gets "xpcom-shutdown" and starts releasing
its observers?
Or do you mean in nsLayoutModule::Shutdown()? How would we enumerate all the
nsPresContexts there?
I mean layout shutdown... and there is no good way to enumerate the prescontexts
there.  Sounds like holding weak refs to the prescontexts in the pref service is
the way to go, perhaps.  Or something.  I can't really think of an obviously
good solution right now.
Comment on attachment 195098 [details] [diff] [review]
nsContentUtils::RegisterPrefCallback cleanup

clearing request based on comments
Attachment #195098 - Flags: review?(roc)
caillon, are you still working on this bug? The bug hasn't seen any significant activity for over 18 months now.
QA Contact: nobody → layout.misc-code

Comment 20

11 years ago
I just searched for nsIPref in layout and only found nsIPrefService and nsIPrefBranch. So I think this bug can be resolved now.

Comment 21

11 years ago
OK, seems that nobody disagrees --> marking fixed.
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago11 years ago
Resolution: --- → FIXED

Comment 22

11 years ago
this isn't fixed. layout still uses nsContentUtils::RegisterPrefCallback and ::UnregisterPrefCallback, which depend on nsIPref. fixing those will likely require changes to layout.

http://mxr.mozilla.org/mozilla/ident?i=RegisterPrefCallback
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #108643 - Attachment description: Better patch → Better patch [Checkin: Comment 6]
Whiteboard: [patchlove]

Comment 23

9 years ago
nsIPref has been removed by bug 524449, marking fixed.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.