175197 - dom/ should make better use of its prefs

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

nsGlobalWindow.cpp should use a static global var for its prefs since it uses
prefs many times in two classes and currently gets a new service each time
through.  Additionally, that file and nsJSEnvironment need to use the new pref APIs.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v1.0

Also fixes some style nits in the general area of what I was touching.

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 103296 [details] [diff] [review]
Patch v1.0

Actually, I found a few issues with this.  new patch coming up soon...

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: patch v1.1 - just to nsGlobalWindow

Okay, I just can't seem to get nsJSEnvironment to work right with the new pref
APIs....  I keep crashing on startup no matter what I do.

Anyway, that should not keep this fix from happening.  It should have some
positive impact on webpages, though.  Right now (by using nsIPref everywhere),
we get a pref service, QI it to nsIPref, and by using nsIPref, every single
call does a QI to nsIPrefBranch.  By storing a static global, I cut out many
many getService calls, as well as cutting down on an extra QI for each usage of
prefs.	We do this a lot in places like window.status, window.open(),
window.dump(), etc.  So we will definitely save time in some of our tests. 
Whether or not it is noticable remains to be seen, but every bit helps :-)

Comment 4

[Daniel Bratell]

Can any of the prefs you're saving change during runtime? In that case you have
to be notified of that change and change the saved values too.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 107136 [details] [diff] [review]
patch v1.1 - just to nsGlobalWindow

Why did CanSetProperty() move into GlobalWindowImpl? If you want to do that, at
least make it static (and add a "// static" comment above the method
implementation).

- In nsDOMWindowController::nsDOMWindowController():

+  if (gPrefBranch) {
+    NS_ADDREF(gPrefBranch);
+  }
+  else {
+    nsCOMPtr<nsIPrefBranch> prefBranch =
+      do_GetService(NS_PREFSERVICE_CONTRACTID);
...

I don't like the idea of having multiple instances of the code that initializes
this global pref branch, I'd say put a global function in nsGlobalWindow.cpp
and call that from multiple places if you need to, but don't spread the code
for initializing this global around...

- In nsDOMWindowController::Observe()

+  NS_ASSERTION(nsDependentString(aData) ==
+		    NS_ConvertASCIItoUCS2("accessibility.browsewithcaret"),
+		"Wrong pref");

I know this is debug-only code, but you should still use NS_LITERLA_STRING
here...

r/sr=jst

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Updated patch

Updated per jst with a few tweaks.  I decided to only keep the one
instantiation of the static nsIPrefBranch in the GlobalWindowImpl constructor,
but I did some shifting around of where it frees it so that the
nsDOMWindowController has access to it if the GlobalWindowImpl does (it gets
created and stored within the mControllers member of GlobalWindowImpl so it
should only live if GlobalWindowImpl does, and we should always have a
prefbranch provided our earlier call to GetService worked....

Daniel, to answer your earlier question, yes the prefs can change in between
calls.	That is why we check the value each time (either explicitly by a
gPrefBranch->GetBoolPref() or implicitly by calling CanSetProperty() which does
similar calls).  Storing a static pref branch does not mean that all the pref
values are stored...

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch per sicking's comments on IRC

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 107351 [details] [diff] [review]
Patch per sicking's comments on IRC

Johnny said r/sr=jst.  Using his review as an sr this time.

Comment 9

[Alec Flett]

Comment on attachment 107136 [details] [diff] [review]
patch v1.1 - just to nsGlobalWindow

sr=alecf

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

Checked in.