212222 - need way to specify "default pref files" to preservice

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Benjamin Smedberg]

I need a way to specify a *default* pref file to the prefservice (at an
arbitrary location). Something like nsIPrefService2.readDefaultPrefs(in nsIFile)

In addition, I want to think about how "default pref files" stack on each
other... example:
I want to move some of the core network/layout prefs into the GRE... the load
order should be

GRE (default prefs)
Application (default prefs, override GRE prefs)
Extensions (default prefs, override application prefs)
Profile (user prefs)

Right now the prefservice loads all of bin/defaults/pref automatically... can we
have it auto-load $GREDIR$/prefs and then bin/default/prefs, and then my
extension manager can use readDefaultPrefs(...) method to load extension prefs

Comment 1

[Benjamin Smedberg]

alecf, I'm cc'ing you too, because I think you wrote a lot of the pref backend.

Comment 2

[Conrad Carlen (not reading bugmail)]

Yeah, we need this.

But, rather than something like nsIPrefService2.readDefaultPrefs(in nsIFile), I
like your stacking idea. The pref service should just be able to ask
nsIDirectoryService for a directory containing default prefs, for each domain
you gave:

1. GRE (default prefs)
2. Application (default prefs, override GRE prefs)
3. Extensions (default prefs, override application prefs)

Right now, it asks directory service for only 1 defaults directory. I think that
to do this, just define 2 more directory service keys, and then do the same for
each.

There is already a domain for profile prefs, and the way that's done now using
directory service is fine, I think.

But, this could be generally solved by a new, improved directory service - one
which took a domain param in addition to just the single property param that we
use now. Most components (not just prefs) need to be aware of at least "App" and
"GRE" domains.

Comment 3

[Benjamin Smedberg]

In my extensions proposal (see bug 209439) extensions can be dynamically
enabled/disabled, and each extension is installed in its own directory... so a
single "extensions default prefs directory" isn't sufficient for this bug, I
need a dynamic method of adding default prefs at runtime, from a single file
(not a directory). In my imagination, the (in nsIFile) param could be either a
regular file, which would be parsed directly, or a directory of preffiles.

--BDS

Comment 4

[Benjamin Smedberg]

I've got the code worked out...

Comment 5

[Benjamin Smedberg]

Attachment: Changes to the prefservice

This is *only* the C++ changes to the prefservice. It should work by itself,
although I haven't tested it thoroughly in that configuration.

Changes:
load default prefs from a directory gre_path/gredpref (loading
gre_path/gredpref/platform.js last)
load application default prefs as normal, except we don't need to do the
alpha-sort by filename

This patch does not cover the actual separation of prefs into "embedding core
prefs" that would go into the gre, and application prefs to be shipped with
applications. I've got some of that in my tree, but it's a bit complicated...

Comment 6

[Alec Flett]

Comment on attachment 134322 [details] [diff] [review]
Changes to the prefservice

looks great, only 
+  static const char* platformFile = "platform.js";

should be static const char platformFile[] = "platform.js";

(that makes the variable platformFile be readonly)

this may require pref_ParsePrefDir to take an extra const in the 2nd
parameter...

The only problem I see is that we are no longer sorting. In its current state,
embeddors can rely on overriding default prefs simply by naming their files
appropriately. This is what Netscape does - they supply a bunch of default pref
names, ns-all.js, and so forth. 

Is there a way to do this with the GRE work you've got in here? does ns-all.js
go into the application's default pref dir now?

If you were avoiding the sort because it seemed like a pain, don't forget we
now have nsCOMArray<T>::Sort()

Comment 7

[Benjamin Smedberg]

For the platformFile I was just taking the address of the pointer, but I can do
it as an array if that's better.

For the pref hierarchy, what I am thinking would look like is:

gre/gredpref/all.js
gre/gredpref/platform.js

application_pref_dir/browser.js (or whatever)
application_pref_dir/mailnews.js
application_pref_dir/ns_all.js (althought NS should really just preprocess
ns-all.js into browser.js, thus avoiding license issues).

So you know that all.js will be parsed before browser/mailnews/ns-all, but you
don't have a hierarchy between browser.js and ns-all.js or something like that
I can re-add the sort if you think that non-sorted behavior way will cause pain
for embedders.

Comment 8

[Darin Fisher]

>I can re-add the sort if you think that non-sorted behavior way will cause pain
>for embedders.

yes, it will definitely break expected behavior.  the sorting is crucial to
embedders.

Comment 9

[Alec Flett]

actually the [] trick is what you should ALWAYS use for strings... 

i.e. instead of 
const char* kFooString = "foo";

which allows me later to say
kFooString = "bar";

you should say

const char kFooString[] = "foo";

this allows kFooString be completely readonly, and will cause a compile error
when I say
kFooString = "bar";

As for sorting, I feel like we should still continue it for now.. because
according to what you're saying, browser.js may be loaded before or after
ns_all.js, depending on how they exist in the directory on the particular
installation (i.e. we'll potentially get different default prefs on different
machines) - I'm pretty sure Netscape isn't the only embeddor that relies on this.

Comment 10

[Benjamin Smedberg]

Comment on attachment 134322 [details] [diff] [review]
Changes to the prefservice

The other way to do const string pointers is:

char const * const sP = "aString";
This allows you to take the address of the pointer (which you can't do if the
variables is a char const str[] = "myString";

Comment 11

[Benjamin Smedberg]

Attachment: update, does sorting

OK, this does sorting, and I made the platformFile constant.

Comment 12

[Benjamin Smedberg]

Comment on attachment 134331 [details] [diff] [review]
update, does sorting

I made the sort function for nsCOMArray take non-const interface pointers,
because constant interface pointers make no sense.

Comment 13

[Alec Flett]

Comment on attachment 134331 [details] [diff] [review]
update, does sorting

+	 for (PRUint32 i = 0; i < aSpecialFilesCount; ++i) {
+	   if (leafName.Equals(nsDependentCString(aSpecialFiles[i]))) {
+	     shouldParse = PR_FALSE;
+	     foundSpecialFiles.ReplaceObjectAt(prefFile, i);
+	   }
+	 }

I'm not sure I understand the motivation for using ReplaceObjectAt() here. Is
this to avoid parsing duplicates? If so, please add a comment.

+  for (i = 0; i < aSpecialFilesCount && i < arrayCount; ++i) {
+    if (nsIFile* file = foundSpecialFiles[i]) {

I guess so. Add a comment here indicating that foundSpecialFiles is sparsely
populated.

+  NS_ASSERTION(hasMoreElements, "Prefs directory is empty!");
+  if (!hasMoreElements)
+    return NS_SUCCESS_FILE_DIRECTORY_EMPTY;

I like the addition of NS_SUCCESS_FILE_DIRECTORY_EMPTY to nsError.h, but
instead of returning it in pref_ParsePrefDir(), what if
nsIFile::GetDirectoryEntries returned NS_SUCCESS_FILE_DIRECTORY_EMPTY (along
with a shared empty iterator)

Since it is a success code, you should add this to each platform's nsLocalFile
implementation as you get time, and consumers of GetDirectoryEntries could
start using it right away.

pref_ParsePrefDir would definitely be simplified:

+  // this may fail in "normal" cases, such as 
+  rv = aDir->GetDirectoryEntries(getter_AddRefs(dirIterator));
+  NS_ENSURE_SUCCESS(rv, rv);

(also, fix that comment that ends with 'such as' - such as what? the code?)

Comment 14

[Benjamin Smedberg]

Attachment: updated for alecf, don't use sparse array

The unnecessary sparse array was a relic of something I was doing earlier,
thanks for catching it. If you don't mind, I would like to do the
NS_SUCCESS_FILE_DIRECTORY_EMPTY changes in another bug... I think it's a good
idea, but I don't want to touch more than necessary for this bug.

Comment 15

[Benjamin Smedberg]

Comment on attachment 134336 [details] [diff] [review]
updated for alecf, don't use sparse array

let's let alecf review before I bug darin yet again... ;)

Comment 16

[Alec Flett]

Comment on attachment 134336 [details] [diff] [review]
updated for alecf, don't use sparse array

ok, looks good. Just two things:
- add some comments around the special files processing to indicate why you're
seperating them out (because the last-processed prefs override the
just-processed prefs, etc)

- file a bug about making nsIFile implementations handle
NS_SUCCESS_FILE_DIRECTORY_EMPTY
r=alecf

Comment 17

[Darin Fisher]

Comment on attachment 134336 [details] [diff] [review]
updated for alecf, don't use sparse array

Index: modules/libpref/src/nsPrefService.cpp

>+static int
>+pref_CompareFileNames(nsIFile* aFile1, nsIFile* aFile2, void* /*unused*/)
>+{
>+  nsCAutoString filename1, filename2;
>+  aFile1->GetNativeLeafName(filename1);
>+  aFile2->GetNativeLeafName(filename2);
>+
>+  return Compare(filename1, filename2);
>+}

it's a shame nsLocalFile doesn't implement an interface with a Compare method.

nsLocalFile should also provide a nsIFileEnumerator ;-)

>+static nsresult
>+pref_ParsePrefDir(nsIFile* aDir, char const *const * aSpecialFiles, PRUint32 aSpecialFilesCount)

i'm not crazy about this name.	ParsePrefDir doesn't sound like it is going
to load pref files in the directory?  Maybe pref_LoadPrefsInDir would be a
better name?

>+{
>+  nsresult rv, rv2;
>+  PRBool hasMoreElements;
>+
>+  nsCOMPtr<nsISimpleEnumerator> dirIterator;
>+
>+  // this may fail in some normal cases, such as embedders who do not use a GRE
>+  rv = aDir->GetDirectoryEntries(getter_AddRefs(dirIterator));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = dirIterator->HasMoreElements(&hasMoreElements);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  NS_ASSERTION(hasMoreElements, "Prefs directory is empty!");
>+  if (!hasMoreElements)
>+    return NS_SUCCESS_FILE_DIRECTORY_EMPTY;

so, you return NS_SUCCESS_FILE_DIRECTORY_EMPTY in this
case, but what if the directory contains some files, but
none that we care to load?  then wouldn't you logically
want that to correspond to the same thing?  seems to me
that you would rather go through the loop, and finally
return NS_SUCCESS_FILE_DIRECTORY_EMPTY if prefFiles is
empty.


>+
>+  nsCOMArray<nsIFile> prefFiles(INITIAL_PREF_FILES);
>+  nsCOMArray<nsIFile> specialFiles(aSpecialFilesCount);
>+  nsCOMPtr<nsIFile> prefFile;
>+  rv2 = NS_OK;

please comment on the significance of rv2 here.  why are you
setting it to NS_OK here?  it doesn't seem to be used inside
the loop... better to defer initialization until you will need
this variable.


>+  while (hasMoreElements && NS_SUCCEEDED(rv)) {
>+    PRBool shouldParse = PR_TRUE;
>+    nsCAutoString leafName;
>+
>+    rv = dirIterator->GetNext(getter_AddRefs(prefFile));
>+    rv |= prefFile->GetNativeLeafName(leafName);
>+
>+    NS_ASSERTION(NS_SUCCEEDED(rv), "Failure in default prefs directory enumerator.");
> 
>+      // Skip non-js files
>+      if (!StringEndsWith(leafName, NS_LITERAL_CSTRING(".js"))) {
>+        shouldParse = PR_FALSE;
>+      } else {
>+        // Skip special files
>+        for (PRUint32 i = 0; i < aSpecialFilesCount; ++i) {
>+          if (leafName.Equals(nsDependentCString(aSpecialFiles[i]))) {
>+            shouldParse = PR_FALSE;
>+            specialFiles.AppendObject(prefFile);
>+          }
>+        }
>+      }
>+
>+      if (shouldParse) {
>+        prefFiles.AppendObject(prefFile);
>       }

something like this might be clearer:

	NS_ASSERTION(!leafName.IsEmpty(), "Failure in...");
	shouldParse = StringEndsWith(leafName, NS_LITERAL_CSTRING(".js"));
	if (shouldParse) {
	  // Skip special files
	  ...
	  if (shouldParse) {
	    prefFiles.AppendObject(prefFile);
	  }
	}

also, i would replace the assertion with one that asserts that the
leafName is not empty.	then you can avoid saving |rv| and performing
the bitwise-OR in optimized builds :)


>+  PRUint32 arrayCount = prefFiles.Count();
>+  PRUint32 i;
>+  for (i = 0; i < arrayCount; ++i) {
>+    rv = openPrefFile(prefFiles[i]);
>+    NS_ASSERTION(NS_SUCCEEDED(rv), "Default pref file not parsed successfully.");
>+    if (NS_FAILED(rv))
>+      rv2 = rv;

use NS_ERROR instead of NS_ASSERTION.  that way you aren't repeating
the test expression twice... helps readability since there is just
less text:

      if (NS_FAILED(rv)) {
	NS_ERROR("Default pref file not parsed successfully.");
	rv2 = rv;
      }


>+  arrayCount = specialFiles.Count();
>+  for (i = 0; i < arrayCount; ++i) {
>+    rv = openPrefFile(specialFiles[i]);
>+    NS_ASSERTION(NS_SUCCEEDED(rv), "Special default pref file not parsed successfully.");
>+    if (NS_FAILED(rv))
>+      rv2 = rv;

same NS_ERROR nit here.


>+  return rv2;

footprint nit: if you set this up so you are always returning |rv|
then the compiler will be able to use the same return code for this
return as well as the early returns.  remember that each return 
statement carries with it destructor calls for any objects on the
stack.	though in this function, i think it only makes a difference
for the dirIterator.

finally, can you please add some documentation above this function
explaining what it does briefly, along with parameter descriptions.
some documentation would be helpful.


> static nsresult pref_InitInitialObjects()
> {
>   nsCOMPtr<nsIFile> aFile;
>   nsCOMPtr<nsIFile> defaultPrefDir;
>   nsresult          rv;
>+
>+  // first we parse the GRE default prefs. This also works if we're not using a GRE, 
>+
>+  rv = NS_GetSpecialDirectory(NS_GRE_DIR, getter_AddRefs(defaultPrefDir));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = defaultPrefDir->AppendNative(NS_LITERAL_CSTRING("gredprefs"));
>+  NS_ENSURE_SUCCESS(rv, rv);

"gredprefs"!?! ... why not "greprefs" or "gre-prefs" or something like that?
what does the "d" stand for?


>+  rv = pref_ParsePrefDir(defaultPrefDir, &platformFile, 1);
>+#ifdef DEBUG
>+  if (NS_FAILED(rv)) {
>+    NS_WARNING("Error parsing GRE default preferences. Is this an old-style embedding app?");
>+  }
>+#endif

the ifdef DEBUG is not needed.	it is just noise.  compilers will optimize
away the empty branch.


>+  NS_ASSERTION(rv != NS_SUCCESS_FILE_DIRECTORY_EMPTY, "GRE preferences directory is empty!");

why is this an assertion?  why not a warning instead?  the pref system
should work just fine without any actual pref files.  that should not
be considered an error.

>+  rv = pref_ParsePrefDir(defaultPrefDir, specialFiles, sizeof(specialFiles) / sizeof(char *));

nit: use NS_ARRAY_LENGTH instead of explict array length calc.


>+  NS_ASSERTION(rv != NS_SUCCESS_FILE_DIRECTORY_EMPTY, "Application preferences directory is empty!");

again, this last assertion should be at most a warning.


mostly a bunch of nits.  the patch is mostly fine... just address my nits
and sr=darin ;)

Comment 18

[Benjamin Smedberg]

Attachment: update darin's nits

Comment 19

[Benjamin Smedberg]

Comment on attachment 134625 [details] [diff] [review]
update darin's nits

While addressing darin's nits, I realized that I do need the sparse array for
the special files, and I don't need to specify platform.js as a special file...
I will just let the alpha-sort take care of that for me.

Comment 20

[Benjamin Smedberg]

(the reason I need a sparse array is because the special files need to be
processed in order, not in random/directory order)

Comment 21

[Darin Fisher]

Comment on attachment 134625 [details] [diff] [review]
update darin's nits

minusing based on comments over irc with bsmedberg.

Comment 22

[Benjamin Smedberg]

Attachment: update again

update from IRC log

Comment 23

[Benjamin Smedberg]

Checked in. Bug 224576 is the followup bug on the success code for directory
enumeration. Bug 224578 is the followup bug on actually separating the
preferences into the GRE/applications.

Comment 24

[Boris Zbarsky [:bzbarsky]]

This patch reversed the sort order for prefs files.. see the 

-        // we want it so foo.js will come before foo-<bar>.js
-        // "foo." is before "foo-", so we have to reverse the order to accomplish
-        sortResult = Compare(name2, name1); // XXX i18n

code that was removed and what replaced it.  Also see bug 224840 

Comment 25

[Mike Pinkerton (not reading bugmail)]

i'm now seeing that prefs set in all-chimera.js no longer stick. could this be
the cause?

Comment 26

[Mike Pinkerton (not reading bugmail)]

maybe this was fixed by 224840. i'll try again tomorrow and report back