Prefs backend needs some (more) cleanup

VERIFIED FIXED in mozilla1.0.1

Status

()

Core
Preferences: Backend
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: Brian Nesse (gone), Assigned: Brian Nesse (gone))

Tracking

({topembed-})

Trunk
mozilla1.0.1
topembed-
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

16 years ago
There is a bunch of crap still hanging around in the preferences library which
is standing in the way of doing a number of cleanup related items. These
include, but are not limited to removal of JS from the Preferences back end and
removal of nsIFileSpec from mozilla.
(Assignee)

Comment 1

16 years ago
Setting fields, adding bugs affected by this, cc'ing people who might
potentially be interested in this.
Status: NEW → ASSIGNED
Keywords: topembed
Target Milestone: --- → mozilla1.0

Comment 2

16 years ago
oh baby. I would say the JS removal is the most important thing here, though I
think its too risky for 1.0
(Assignee)

Comment 3

16 years ago
Created attachment 75057 [details] [diff] [review]
Patch to clean up numerous prefs issues.

This patch cleans up and/or fixes a number of things including:

 - removing the use of nsIFileSpec, nsOutputFileStream, and the functions
wrapping their use.
 - JS functionality is moved into autoconfig, where it belongs.
 - fixed "platform" definition so it is initialized properly on all platforms.
 - initpref.js and debug-developer.js are removed entirely.
 - unused mime_type declarations in macprefs removed.
 - reduced supported JS entry points to pref and user_pref to facilitate
eventual removal.
 - Added Mac support to unix build environment for mach-o build.
 - removed leftover windows .rc stuff.
(Assignee)

Comment 4

16 years ago
Created attachment 75058 [details] [diff] [review]
One line commercial patch.

The one change to the commercial tree.
(Assignee)

Comment 5

16 years ago
Alec, I agree. I'm not shooting to pull JS by 1.0... just attempting to lay the
groundwork.

Comment 6

16 years ago
Comment on attachment 75058 [details] [diff] [review]
One line commercial patch.

oh no! commercial code! commercial code! :)

sr=alecf just to get that out of the way.
Attachment #75058 - Flags: superreview+
Comment on attachment 75058 [details] [diff] [review]
One line commercial patch.

r=dveditz if we're removing the distinction between config and pref items,
which I guess we are :-)
Attachment #75058 - Flags: review+
Comment on attachment 75058 [details] [diff] [review]
One line commercial patch.

Actually, no. Remove this file entirely and put this in all-ns.js

There's no point in spending startup time opening a one-line file

In order to really remove it for people who upgrade we'll have to add a
"deleteThisFile" entry to the *ns* windows browser.jst file. Either file a
separate bug or better, just include that with this patch.
Attachment #75058 - Flags: review+ → needs-work+
Comment on attachment 75057 [details] [diff] [review]
Patch to clean up numerous prefs issues.

Index: Makefile.in
>===================================================================
>+ifeq ($(MOZ_WIDGET_TOOLKIT), windows)
>+AUTOCFG_JS_EXPORTS += $(srcdir)/win/platform_defs.js

>Index: makefile.win
>===================================================================
>     $(MAKE_INSTALL) .\prefcalls.js $(DIST)\bin\defaults\autoconfig
>+    $(MAKE_INSTALL) .\win\winpref.js $(DIST)\bin\defaults\autoconfig

The names don't match here ... which one is it?

>Index: nsReadConfig.cpp
>===================================================================
>+        // Evaluate platform specific directives
>+        rv = openAndEvaluateJSFile("platformDefs.js", PR_FALSE, PR_FALSE);
>         if (NS_FAILED(rv)) 

And here's a third version of the name :-)

>Index: beos/platformDefs.js

Looks like nsReadConfig.cpp was right, but up to now our pref files
have consistently been in all lower case and not CamelCase, so
platform-defs.js was a better name (or even just platform.js).


>+/* -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1

We've been asked to make new files MPL instead of NPL since the
special provisions of the NPL have mostly expired. The exception
would be if this file were derived from an already NPL file

Not sure we want these headers in the actual default pref files,
though, it just chews up memory and slows down the processing,
and there's really not much "expression" to copyright here.

>+platform.beos = true;

For one line it's too bad the build system can't just slap this
on the end of config.js or all.js or something.

>+ * Portions created by the Initial Developer are Copyright (C) 1998

I don't think we want the license block, but if it turns out we
do was this created in 1998?

>+platform.unix = true;

Do we need to stick with this? We end up creating a new property
each time we distinguish a platform type. Maybe we can keep this
for backward compatibility if we need it, and switch to a more
flexible platform.value = "string"


>Index: Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/modules/libpref/src/Makefile.in,v
>+ifneq (,filter mac cocoa,$(MOZ_WIDGET_TOOLKIT))
>+PREF_JS_EXPORTS += $(srcdir)/mac/macpref.js
>+else
> ifeq ($(MOZ_WIDGET_TOOLKIT),windows)
> PREF_JS_EXPORTS += $(srcdir)/win/winpref.js

how do these macpref.js and winpref.js relate to the autoconf platformDefs.js
above?

>Index: makefile.win
>===================================================================
>RCS file: /cvsroot/mozilla/modules/libpref/src/makefile.win,v
>retrieving revision 3.39
>diff -u -2 -r3.39 makefile.win
>--- makefile.win	19 Feb 2002 06:29:14 -0000	3.39
>+++ makefile.win	19 Mar 2002 21:14:40 -0000
>@@ -88,5 +88,4 @@
> 
> libs::
>-    $(MAKE_INSTALL) .\initpref.js $(DIST)\bin\defaults\pref
>     $(MAKE_INSTALL) .\init\all.js $(DIST)\bin\defaults\pref
>     $(MAKE_INSTALL) .\init\mailnews.js $(DIST)\bin\defaults\pref
>@@ -94,5 +93,2 @@
>     $(MAKE_INSTALL) .\init\config.js $(DIST)\bin\defaults\pref
>     $(MAKE_INSTALL) .\win\winpref.js $(DIST)\bin\defaults\pref
>-!ifdef MOZ_DEBUG
>-    $(MAKE_INSTALL) .\debug-developer.js $(DIST)\bin\defaults\pref
>-!endif

So it looks like we'll already have to alter the windows installer
to delete old copies of initpref.js on upgrades

have a meeting now, will look at the rest later
Attachment #75057 - Flags: needs-work+
(Assignee)

Comment 10

16 years ago
Dan,

Actaully, the code for distingushing config from pref is still there (in the
form of the bit flags.) The distinction will have to come from the autoconfig
module however because that's where "config" prefs will be read in... I just
removed the JS portion, which isn't being used anyway.

The reason I left config-ns.js (and config.js) alone was because I figured they
contain the one preference that vendors are likely to change... In retrospect, I
guess it's the value in the properties file they are likely to change, not the
actual preference value.

>+AUTOCFG_JS_EXPORTS += $(srcdir)/win/platform_defs.js
>+    $(MAKE_INSTALL) .\win\winpref.js $(DIST)\bin\defaults\autoconfig
>+        rv = openAndEvaluateJSFile("platformDefs.js", PR_FALSE, PR_FALSE);

Doh! I am an idiot. Nice catch. I guess I couldn't make up my mind... :)

>Looks like nsReadConfig.cpp was right, but up to now our pref files
>have consistently been in all lower case and not CamelCase, so
>platform-defs.js was a better name (or even just platform.js).

I'm fine with that.

>Not sure we want these headers in the actual default pref files,
>though, it just chews up memory and slows down the processing,
>and there's really not much "expression" to copyright here.

Good point. It's a data file, not a source file. I'll pull it.

>Do we need to stick with this? We end up creating a new property
>each time we distinguish a platform type. Maybe we can keep this
>for backward compatibility if we need it, and switch to a more

The is no backward compatibility, per se, as we specifically state that 4.x
scripts are not compatible, and it doesn't work at all right now because it's in
the wrong JS context. If we are going to change it, now is the time... If we can
do it without the extra files... all the better.

>how do these macpref.js and winpref.js relate to the autoconf platformDefs.js
>above?

The platformDefs.js file contains the JS specific stuff that was in macpref,
winpref, etc. Essentially the platform.mac <windows><unix> declaration.
(Assignee)

Comment 11

16 years ago
Created attachment 75690 [details] [diff] [review]
New patch based on Dan's comments
Attachment #75057 - Attachment is obsolete: true
(Assignee)

Comment 12

16 years ago
Created attachment 75691 [details] [diff] [review]
New Commercial patch based on Dan's comments
Attachment #75058 - Attachment is obsolete: true
(Assignee)

Comment 13

16 years ago
Created attachment 75997 [details] [diff] [review]
Patch which doesn't remove winpref.js

Opps. I got overzealous in my cleaning up and removed the installation of
winpref.js. Corrected.
Attachment #75690 - Attachment is obsolete: true
embed triage: topembed-, not critical for immediate embedding needs
Keywords: topembed → topembed-

Comment 15

16 years ago
Comment on attachment 75997 [details] [diff] [review]
Patch which doesn't remove winpref.js

cool. It looks good. I preferred "notify" (or "Notify") to "Inform" because
that's the word we use elsewhere in the codebase to mean "tell a bunch of
observers something)

looking over this, I also think we could be a whole lot less memory-intensive
if we changed pref_savePref() to append to an existing nsCString of some kind -
since nsCStrings double when they grow, we'd only re-alloc a few times, instead
of once for every pref name. I guess that would make sorting kind of hard

anyway, enough rambling. sr=alecf
Attachment #75997 - Flags: superreview+
(Assignee)

Comment 16

16 years ago
>I preferred "notify" (or "Notify") to "Inform" because that's the word we use
>elsewhere in the codebase to mean "tell a bunch of observers something)

And that was exactly why I renamed the function. :) NotifyObservers() conflicted
with the ObserverService funtion of the same name (with different parameters.) I
didn't want there to be any confusion about what was going on there, so I
decided that renaming the function was a prudent thing to do.
Why not preserve the canonical verb (notify) and use some other object-noun than
Observers, or no object-part at all?

/be
(Assignee)

Comment 18

16 years ago
Sure, that seems reasonable. I've changed it to "NotifyServiceObservers". Both
descriptive and distinctive.
Comment on attachment 75997 [details] [diff] [review]
Patch which doesn't remove winpref.js

>+ifneq (,filter mac cocoa,$(MOZ_WIDGET_TOOLKIT))

using $(OS_ARCH) might have been better since this doesn't
have anything to do with the toolkit. Next time...

>+// set on a platform specific basis in platform.js
>+function plat() {
>+    this.value = "";
>+};
>+
>+platform = new plat();

this could have been simplified to
   platform = { value: "" };

>+// OS2 specific auto configuration preference defaults
>+platform.value = "windows";

Double-checking that the os/2 file should say "windows"

>+  deleteThisFile("Program",    "defaults/pref/config.js");
>+  deleteThisFile("Program",    "defaults/pref/initpref.js");

The commercial tree has separate scripts, please also fix up
the browser.jst files under ns/xpinstall/packager/etc.

r=dveditz
Attachment #75997 - Flags: review+
(Assignee)

Comment 20

16 years ago
>using $(OS_ARCH) might have been better...
I agree, and alecf suggested the same when I started this patch. Unfortunately,
when I started looking at the various OS_ARCH defines, I just came away more
confused then I already was by the MOZ_WIDGET_TOOLKIT defines :)

Since this isn't likely to go into 1.0 at this point anyway... If someone wants
to help me sort through all the various architectures, I'll change it now,
otherwise once this lands I will file a bug to switch both prefs and autoconfig
over to using OS_ARCH.

>platform = { value: "" };
I can do that...

>Double-checking that the os/2 file should say "windows"
This is what it was doing before... I'm making an assumption that the previous
implementation was correct.

>The commercial tree has separate scripts...
Would those be the scripts in attachment 75691 [details] [diff] [review] (the revised commercial patch)?
Target Milestone: mozilla1.0 → mozilla1.0.1
(Assignee)

Comment 21

16 years ago
Created attachment 77689 [details] [diff] [review]
Updated commercial patch.

New commercial patch updated to reflect the fact that dveditz removed
config-ns.js and editor-ns.js from the commercial build over the weekend. Also
adds these files to the browser.jst scripts and cvs removes them.
Attachment #75691 - Attachment is obsolete: true

Comment 22

16 years ago
Comment on attachment 77689 [details] [diff] [review]
Updated commercial patch.

sr=alecf
Attachment #77689 - Flags: superreview+
Comment on attachment 77689 [details] [diff] [review]
Updated commercial patch.

r=dveditz -- Thanks!
Attachment #77689 - Flags: review+
(Assignee)

Comment 24

16 years ago
Brenden, Shaver, does this seem like something I should push to get landed for
1.0? It has the potential for improving startup times, but probably not
greatly... Other than that I doubt there's much here that will interest either
mozilla.org or ADT.
(Assignee)

Comment 25

16 years ago
Patches checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
rs vrfy
Status: RESOLVED → VERIFIED
I am getting a bunch of dialogs ("Configuration Warning", "An error occurred
reading the startup configuration file. Please contact your administrator. line
40: ReferenceError: config is not defined") and assertions ("Config file not
parsed successfully"), same for "defaultPref" during startup now, and I suspect
this is the reason. jst is seeing them in release builds as well (I haven't
tried). Removing initpref.js from my <bin> dir did not help.

Comment 28

16 years ago
also delete config.js

the best thing is to nuke your whole dist/bin/defaults/pref directory, and then
make export from the top

(if you're feeling daring you can just make export from each of the following
directories:
modules/libpref/src
netwerk/base/public
mailnews/extensions
extensions/inspector
xpinstall/public
)
You need to log in before you can comment on or make changes to this bug.