479248 - integrate new version of Preferences.js

Status: RESOLVED FIXED

(Cloud Services :: General, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch that integrates new version of module

I've pushed a new version of Preferences.js with a number of fixes, including two that were recently checked into Weave: trapping exceptions in the reset and resetBranch methods.

The resetBranch fix is a bit more complicated than the one in Weave, however, since it turns out that resetBranch isn't actually implemented in Mozilla's nsPrefService.cpp, which is why that method would always throw when you called it, even when the branch existed.

The new version of Preferences.js thus implements resetBranch itself.

The other fixes are:

1. it's now possible to call reset with an array of prefs to reset;

2. non-ASCII string values are now set and retrieved correctly;

3. setting a pref to a non-integer number reports a warning to the Error Console that doing so converts the number to an integer, dropping its fraction.

Weave should integrate the new version of the Preferences module, which is backwards-compatible with the old one for Weave use cases.  Here's the patch that does that.  Weave with this patch shows no change to test results.  I'll check this in.

Note: Weave calls |Preferences.reset("some pref"); Preferences.set("some pref", "some value")| in a few places, but it shouldn't be necessary to reset a pref right before you set it.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Fixed by changeset 11ac2f49aa0a.

Comment 2

[Dan Mills [:thunder]]

Fantastic.  Thanks, Myk!

Comment 3

[Dan Mills [:thunder]]

About to the 'reset; set' Weave does in a couple of places, it is because we've had a couple of prefs that have erroneously been strings when they should be integers instead.  That will cause an exception, which reset fixes.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #3)
> About to the 'reset; set' Weave does in a couple of places, it is because we've
> had a couple of prefs that have erroneously been strings when they should be
> integers instead.  That will cause an exception, which reset fixes.

Aha, I knew there was a reason! Hmm, that seems like something the module should handle automagically, no?

Comment 5

[Dan Mills [:thunder]]

Arguably, yes.  It's less clear to me whether that should throw or not.  I think that I would want to know if I was accidentally changing pref types.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #5)
> Arguably, yes.  It's less clear to me whether that should throw or not.  I
> think that I would want to know if I was accidentally changing pref types.

Indeed.  But it's not clear how likely it is for changing pref types to be accidental vs. intentional.  In your use case, for example, the change is intentional, even if the original use of strings wasn't.  And the original use wouldn't have been caught by this type checking.

By way of comparison, the content pref service doesn't throw in this case, nor does the SQLite database in which it stores its prefs, since that database uses manifest typing, which means you can store a value of any type in any field (regardless of the column's type affinity).

I'm not sure what the annotation service does.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Another option would be to warn when a pref is set to a value of a different type.

Comment 8

[Dan Mills [:thunder]]

Yes, a warning would be my preferred solution.

Ideally a warning would go out through log4moz, but to avoid a module dependency you could write straight to the error console.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #8)
> Yes, a warning would be my preferred solution.

Mine too, but it turns out to be more complicated than I thought, since the preferences service also throws if the datatype is different from the datatype of the default value of the preference, and we can't override that by resetting the preference, since that just resets it to its default value.

I suppose we could warn when a caller changes the datatype of a preference that doesn't have a default value while continuing to throw when a caller changes the datatype of a preference that does have a default value.  But that seems worse (because inconsistent) than simply throwing in both cases and requiring callers to handle this (admittedly rare) case.