181975 - Layout should use the newer nsIPrefService APIs instead of nsIPref

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

Part of the ongoing work I'm doing to rid the source of nsIPref.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 107779 [details] [diff] [review]
Proposed patch v1

New patch to follow in the next day or so.

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Better patch [Checkin: Comment 6]

Here we go.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+#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 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

Landed

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Reopening since caillon backed this out due to a Txul regression.

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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?

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

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.

Comment 10

[Simon Paquet [:sipaq]]

Caillon, any news on this?

Comment 11

[Vidar Haarr (not reading bugmail)]

Attachment: 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?

Comment 12

[Vidar Haarr (not reading bugmail)]

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.

Comment 13

[Vidar Haarr (not reading bugmail)]

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

Comment 14

[Boris Zbarsky [:bzbarsky]]

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.

Comment 15

[Vidar Haarr (not reading bugmail)]

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

Comment 16

[Vidar Haarr (not reading bugmail)]

Or do you mean in nsLayoutModule::Shutdown()? How would we enumerate all the
nsPresContexts there?

Comment 17

[Boris Zbarsky [:bzbarsky]]

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 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 195098 [details] [diff] [review]
nsContentUtils::RegisterPrefCallback cleanup

clearing request based on comments

Comment 19

[Simon Paquet [:sipaq]]

caillon, are you still working on this bug? The bug hasn't seen any significant activity for over 18 months now.

Comment 20

[Peter Weilbacher]

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

Comment 21

[Peter Weilbacher]

OK, seems that nobody disagrees --> marking fixed.

Comment 22

[[e-mail]]

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

Comment 23

[Steffen Wilberg]

nsIPref has been removed by bug 524449, marking fixed.