Bug 985998 - Allow set*Pref() to override type of prefs with no default value. r=bsmedberg,khuey
authorIrving Reid <irving@mozilla.com>
Wed, 23 Apr 2014 09:48:24 -0400
changeset 179828 9fab567879bc
parent 179827 0096df4879f7
child 179829 d36139793476
push id6475
push usercbook@mozilla.com
push dateThu, 24 Apr 2014 08:37:46 +0000
treeherderfx-team@9fab567879bc [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbsmedberg, khuey
bugs985998
milestone31.0a1
Bug 985998 - Allow set*Pref() to override type of prefs with no default value. r=bsmedberg,khuey
modules/libpref/public/nsIPrefBranch.idl
modules/libpref/src/prefapi.cpp
modules/libpref/test/unit/test_changeType.js
modules/libpref/test/unit/xpcshell.ini
--- a/modules/libpref/public/nsIPrefBranch.idl
+++ b/modules/libpref/public/nsIPrefBranch.idl
@@ -65,17 +65,18 @@ interface nsIPrefBranch : nsISupports
   boolean getBoolPref(in string aPrefName);
 
   /**
    * Called to set the state of an individual boolean preference.
    *
    * @param aPrefName The boolean preference to set the state of.
    * @param aValue    The boolean value to set the preference to.
    *
-   * @throws Error if setting failed or the value is the wrong type.
+   * @throws Error if setting failed or the preference has a default
+             value of a type other than boolean.
    *
    * @see getBoolPref
    */
   void setBoolPref(in string aPrefName, in boolean aValue);
 
   /**
    * Called to get the state of an individual floating-point preference.
    * "Floating point" preferences are really string preferences that
@@ -101,17 +102,18 @@ interface nsIPrefBranch : nsISupports
   string getCharPref(in string aPrefName);
 
   /**
    * Called to set the state of an individual string preference.
    *
    * @param aPrefName The string preference to set.
    * @param aValue    The string value to set the preference to.
    *
-   * @throws Error if setting failed or the value is the wrong type.
+   * @throws Error if setting failed or the preference has a default
+             value of a type other than string.
    *
    * @see getCharPref
    */
   void setCharPref(in string aPrefName, in string aValue);
 
   /**
    * Called to get the state of an individual integer preference.
    *
@@ -124,17 +126,18 @@ interface nsIPrefBranch : nsISupports
   long getIntPref(in string aPrefName);
 
   /**
    * Called to set the state of an individual integer preference.
    *
    * @param aPrefName The integer preference to set the value of.
    * @param aValue    The integer value to set the preference to.
    *
-   * @throws Error if setting failed or the value is the wrong type.
+   * @throws Error if setting failed or the preference has a default
+             value of a type other than integer.
    *
    * @see getIntPref
    */
   void setIntPref(in string aPrefName, in long aValue);
 
   /**
    * Called to get the state of an individual complex preference. A complex
    * preference is a preference which represents an XPCOM object that can not
--- a/modules/libpref/src/prefapi.cpp
+++ b/modules/libpref/src/prefapi.cpp
@@ -701,29 +701,34 @@ static bool pref_ValueChanged(PrefValue 
     }
     else if (type & PREF_INT)
         changed = oldValue.intVal != newValue.intVal;
     else if (type & PREF_BOOL)
         changed = oldValue.boolVal != newValue.boolVal;
     return changed;
 }
 
-static void pref_SetValue(PrefValue* oldValue, PrefValue newValue, PrefType type)
+/*
+ * Overwrite the type and value of an existing preference. Caller must
+ * ensure that they are not changing the type of a preference that has
+ * a default value.
+ */
+static void pref_SetValue(PrefValue* existingValue, uint16_t *existingFlags,
+                          PrefValue newValue, PrefType newType)
 {
-    switch (type & PREF_VALUETYPE_MASK)
-    {
-        case PREF_STRING:
-            PR_ASSERT(newValue.stringVal);
-            if (oldValue->stringVal)
-                PL_strfree(oldValue->stringVal);
-            oldValue->stringVal = newValue.stringVal ? PL_strdup(newValue.stringVal) : nullptr;
-            break;
-
-        default:
-            *oldValue = newValue;
+    if ((*existingFlags & PREF_STRING) && existingValue->stringVal) {
+        PL_strfree(existingValue->stringVal);
+    }
+    *existingFlags = (*existingFlags & ~PREF_VALUETYPE_MASK) | newType;
+    if (newType & PREF_STRING) {
+        PR_ASSERT(newValue.stringVal);
+        existingValue->stringVal = newValue.stringVal ? PL_strdup(newValue.stringVal) : nullptr;
+    }
+    else {
+        *existingValue = newValue;
     }
     gDirty = true;
 }
 
 PrefHashEntry* pref_HashTableLookup(const void *key)
 {
 #ifndef MOZ_B2G
     MOZ_ASSERT(NS_IsMainThread());
@@ -747,66 +752,67 @@ nsresult pref_HashPref(const char *key, 
     if (!gHashTable.ops)
         return NS_ERROR_OUT_OF_MEMORY;
 
     PrefHashEntry* pref = static_cast<PrefHashEntry*>(PL_DHashTableOperate(&gHashTable, key, PL_DHASH_ADD));
 
     if (!pref)
         return NS_ERROR_OUT_OF_MEMORY;
 
-    // new entry, better intialize
+    // new entry, better initialize
     if (!pref->key) {
 
         // initialize the pref entry
         pref->flags = type;
         pref->key = ArenaStrDup(key, &gPrefNameArena);
         memset(&pref->defaultPref, 0, sizeof(pref->defaultPref));
         memset(&pref->userPref, 0, sizeof(pref->userPref));
     }
-    else if ((((PrefType)(pref->flags)) & PREF_VALUETYPE_MASK) !=
-                 (type & PREF_VALUETYPE_MASK))
+    else if ((pref->flags & PREF_HAS_DEFAULT) && PREF_TYPE(pref) != type)
     {
-        NS_WARNING(nsPrintfCString("Trying to set pref %s to with the wrong type!", key).get());
+        NS_WARNING(nsPrintfCString("Trying to overwrite value of default pref %s with the wrong type!", key).get());
         return NS_ERROR_UNEXPECTED;
     }
 
     bool valueChanged = false;
     if (flags & kPrefSetDefault)
     {
         if (!PREF_IS_LOCKED(pref))
         {       /* ?? change of semantics? */
             if (pref_ValueChanged(pref->defaultPref, value, type) ||
                 !(pref->flags & PREF_HAS_DEFAULT))
             {
-                pref_SetValue(&pref->defaultPref, value, type);
+                pref_SetValue(&pref->defaultPref, &pref->flags, value, type);
                 pref->flags |= PREF_HAS_DEFAULT;
                 if (!PREF_HAS_USER_VALUE(pref))
                     valueChanged = true;
             }
         }
     }
     else
     {
         /* If new value is same as the default value, then un-set the user value.
            Otherwise, set the user value only if it has changed */
-        if (!pref_ValueChanged(pref->defaultPref, value, type) &&
-            (pref->flags & PREF_HAS_DEFAULT) &&
+        if ((pref->flags & PREF_HAS_DEFAULT) &&
+            !pref_ValueChanged(pref->defaultPref, value, type) &&
             !(flags & kPrefForceSet))
         {
             if (PREF_HAS_USER_VALUE(pref))
             {
+                /* XXX should we free a user-set string value if there is one? */
                 pref->flags &= ~PREF_USERSET;
                 if (!PREF_IS_LOCKED(pref))
                     valueChanged = true;
             }
         }
-        else if ( !PREF_HAS_USER_VALUE(pref) ||
-                   pref_ValueChanged(pref->userPref, value, type) )
+        else if (!PREF_HAS_USER_VALUE(pref) ||
+                 PREF_TYPE(pref) != type ||
+                 pref_ValueChanged(pref->userPref, value, type) )
         {
-            pref_SetValue(&pref->userPref, value, type);
+            pref_SetValue(&pref->userPref, &pref->flags, value, type);
             pref->flags |= PREF_USERSET;
             if (!PREF_IS_LOCKED(pref))
                 valueChanged = true;
         }
     }
 
     nsresult rv = NS_OK;
     if (valueChanged) {
new file mode 100644
--- /dev/null
+++ b/modules/libpref/test/unit/test_changeType.js
@@ -0,0 +1,63 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+/* Tests for changing the type of a preference (bug 985998) */
+
+const PREF_INVALID = 0;
+const PREF_BOOL    = 128;
+const PREF_INT     = 64;
+const PREF_STRING  = 32;
+
+function run_test() {
+
+  var ps = Cc["@mozilla.org/preferences-service;1"].
+            getService(Ci.nsIPrefService);
+
+  let defaultBranch = ps.getDefaultBranch("");
+  let userBranch = ps.getBranch("");
+
+  //**************************************************************************//
+  // Can't change the type of prefs that have default values
+
+  defaultBranch.setBoolPref("TypeTest.existing.bool", true);
+  defaultBranch.setIntPref("TypeTest.existing.int", 23);
+  defaultBranch.setCharPref("TypeTest.existing.char", "hey");
+
+  // The user branch reads back the expected default
+  do_check_eq(userBranch.getBoolPref("TypeTest.existing.bool"), true);
+  do_check_eq(userBranch.getIntPref("TypeTest.existing.int"), 23);
+  do_check_eq(userBranch.getCharPref("TypeTest.existing.char"), "hey");
+
+  // All the combinations of attempted type changes
+  do_check_throws(function() {
+    userBranch.setCharPref("TypeTest.existing.bool", "boo"); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    userBranch.setIntPref("TypeTest.existing.bool", 5); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    userBranch.setCharPref("TypeTest.existing.int", "boo"); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    userBranch.setBoolPref("TypeTest.existing.int", true); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    userBranch.setBoolPref("TypeTest.existing.char", true); }, Cr.NS_ERROR_UNEXPECTED);
+  do_check_throws(function() {
+    userBranch.setIntPref("TypeTest.existing.char", 6); }, Cr.NS_ERROR_UNEXPECTED);
+
+
+  //**************************************************************************//
+  // Prefs that don't have default values can mutate
+  let pref = "TypeTest.user";
+  userBranch.setBoolPref(pref, true);
+  userBranch.setCharPref(pref, "yay");
+  do_check_eq(userBranch.getCharPref(pref), "yay");
+  userBranch.setIntPref(pref, 7);
+  do_check_eq(userBranch.getIntPref(pref), 7);
+  userBranch.setBoolPref(pref, false);
+  do_check_eq(userBranch.getBoolPref(pref), false);
+  userBranch.setIntPref(pref, 8);
+  do_check_eq(userBranch.getIntPref(pref), 8);
+  userBranch.setCharPref(pref, "whee");
+  do_check_eq(userBranch.getCharPref(pref), "whee");
+  userBranch.setBoolPref(pref, true);
+  do_check_eq(userBranch.getBoolPref(pref), true);
+}
--- a/modules/libpref/test/unit/xpcshell.ini
+++ b/modules/libpref/test/unit/xpcshell.ini
@@ -5,10 +5,11 @@ support-files =
   data/testPref.js
   extdata/testExt.js
 
 [test_warnings.js]
 [test_bug345529.js]
 [test_bug506224.js]
 [test_bug577950.js]
 [test_bug790374.js]
+[test_changeType.js]
 [test_extprefs.js]
 [test_libPrefs.js]