507288 - move greprefs/*js into greprefs.js

Status: RESOLVED FIXED

(Toolkit :: Preferences, defect)

Comment 1

[(dormant account)]

Note to self:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#342

Comment 2

[Benjamin Smedberg]

Note: these are not JS files, even though they appear to be. They are read by the prefservice using file I/O and a directory enumerator.

You can't do this without adding some serious logic to prefservice to read prefs from JARs or channels.

Comment 3

[(dormant account)]

It seems that all the code does is read the files in and combine them on startup. Would it not make sense to do the combining step at build-time?

Then we could move all.js(including the other files) into res/ and get rid of that readdir.

Comment 4

[Benjamin Smedberg]

That would work for xulrunner, yeah. For the app prefs you'd either need a well-known filename or some other way of knowing where the prefs are...

Comment 5

[(dormant account)]

So to clarify, we can move app prefs into a single .js file, into jar.

Comment 6

[(dormant account)]

Attachment: load stuff via a single file + res url

this runs ~2-10x faster

Comment 7

[(dormant account)]

Attachment: greprefs.js

Comment 8

[Benjamin Smedberg]

Comment on attachment 398035 [details] [diff] [review]
greprefs.js

>diff --git a/modules/libpref/src/nsPrefService.cpp b/modules/libpref/src/nsPrefService.cpp

>+static nsresult openPrefInputStream(nsIInputStream *aInStr) {

style:
static nsresult
openPrefInputStream(nsIInputStream* aInStr)
{

same thing for openPrefFile below.

At first glance it appears that security-prefs.js and xpinstall.js are just being skipped altogether. What's up with that?

Comment 9

[(dormant account)]

Attachment: use preprocessor.py for creating greprefs.js

the preprocessor solution is much nicer, thanks for suggesting that on irc.

Comment 10

[(dormant account)]

Attachment: use preprocessor.py for creating greprefs.js(without slashslash filter)

slashslash busts strings with // in them. This is ready to checkin

Comment 11

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/40811b076d51

Comment 12

[Dão Gottwald [:dao]]

backed out, since this caused new orange, e.g. http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1252853301.1252854535.8760.gz

Comment 13

[dickvl]

What is the impact of this change on locking prefs?
See http://kb.mozillazine.org/Locking_preferences

A long time ago it was advised to modify all.js , but that would cause problems with updates.
So currently it is advised to create an extra file local-settings.js in the greprefs folder to store the pref: 
pref("general.config.filename", "mozilla.cfg");

Will *.js files in greprefs still be read or is there another solution possible that won't require to modify Firefox files?

Comment 14

[(dormant account)]

Looks like we got a chicken/egg problem. We need to access greprefs.js via a url, but in order to even make a url, we need nsIOService, which needs prefs.....which is bad.

Unless someone has a bright idea on how to read stuff out of a jar without chicken/egg issues(and work with flat chrome), I'm all ears.

Comment 15

[(dormant account)]

Attachment: attempting to use a concatenated pref file :(

Doing stuff with files is much uglier than with stuff in jar.mn. 

Benjamin or Ted, for some reason $(DESTDIR) is not is not being set in my makefile code.

I get echo "" out of that echo line, what am I doing wrong?

Comment 16

[(dormant account)]

Attachment: bin/greprefs.js

Comment 18

[(dormant account)]

Attachment: bin/greprefs.js

forgot to preprocess the prefs in last rev.

Comment 19

[Nick Thomas [:nthomas] (UTC+12)]

You'll need to clean up the three deprecated files in removed-files.in won't you ?

Comment 20

[Benjamin Smedberg]

Comment on attachment 401316 [details] [diff] [review]
bin/greprefs.js

>diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in
>--- a/browser/installer/package-manifest.in
>+++ b/browser/installer/package-manifest.in
>@@ -381,9 +381,7 @@
> @BINPATH@/@PREF_DIR@/firefox.js
> @BINPATH@/@PREF_DIR@/firefox-branding.js
> @BINPATH@/@PREF_DIR@/channel-prefs.js
>-@BINPATH@/greprefs/all.js
>-@BINPATH@/greprefs/security-prefs.js
>-@BINPATH@/greprefs/xpinstall.js
>+@BINPATH@/greprefs.js

You made changes here but didn't add anything to removed-files...

>diff --git a/modules/libpref/src/Makefile.in b/modules/libpref/src/Makefile.in

>+greprefs.js: $(topsrcdir)/xpinstall/public/xpinstall.js $(topsrcdir)/netwerk/base/public/security-prefs.js $(srcdir)/init/all.js

Can you make a GREPREF_FILES variable for this? I'd really like to split all.js into more pieces across the tree.

r=me with nits fixed

Comment 21

[(dormant account)]

Attachment: addressed review comments

Fixed nits. carrying over Benjamins review

Comment 22

[(dormant account)]

Attachment: addressed review comments

This is the patch i meant to upload

Comment 23

[(dormant account)]

Attachment: addressed review comments

Accidentally attached wrong file above[again]

Comment 24

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/0e8fae2dd64c

Comment 25

[Murali Nandigama [:murali]]

This bug has screwed up the javascript instrumentation logic for nightly instrumented builds.

Please notify catlee and murali whenever *.js files from defaults and greprefs are moved to other locations.

Comment 26

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: Follwup fix to removed-files.in

Attachment 403899 [details] [diff] added the three old greprefs files to removed-files.in but in a Mac specific section. This just moves to affect all platforms.

Comment 27

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 448314 [details] [diff] [review]
Follwup fix to removed-files.in

http://hg.mozilla.org/mozilla-central/rev/02dfc932abde