Bug 1098343 (part 1) - support 'sticky' preferences, meaning a user value is retained even when it matches the default. r=bsmedberg
authorMark Hammond <mhammond@skippinet.com.au>
Thu, 30 Apr 2015 17:13:40 +1000
changeset 260505 99afe078d60228a2d660963428d6bf4bd7e6d358
parent 260504 461b2cd49f3c7701c3186fa42b2a9d44194b6d60
child 260506 c20dd193b5df475e3ec215217909e465b41dd60a
push id1812
push userpbrosset@mozilla.com
push dateThu, 30 Apr 2015 08:46:14 +0000
reviewersbsmedberg
bugs1098343
milestone40.0a1
Bug 1098343 (part 1) - support 'sticky' preferences, meaning a user value is retained even when it matches the default. r=bsmedberg
modules/libpref/prefapi.cpp
modules/libpref/prefapi.h
modules/libpref/prefread.cpp
modules/libpref/prefread.h
modules/libpref/test/unit/data/testPrefSticky.js
modules/libpref/test/unit/data/testPrefStickyUser.js
modules/libpref/test/unit/test_stickyprefs.js
modules/libpref/test/unit/xpcshell.ini
--- a/modules/libpref/prefapi.cpp
+++ b/modules/libpref/prefapi.cpp
@@ -135,17 +135,18 @@ struct CallbackNode {
     struct CallbackNode*    next;
 };
 
 /* -- Prototypes */
 static nsresult pref_DoCallback(const char* changed_pref);
 
 enum {
     kPrefSetDefault = 1,
-    kPrefForceSet = 2
+    kPrefForceSet = 2,
+    kPrefStickyDefault = 4,
 };
 static nsresult pref_HashPref(const char *key, PrefValue value, PrefType type, uint32_t flags);
 
 #define PREF_HASHTABLE_INITIAL_LENGTH   1024
 
 nsresult PREF_Init()
 {
     if (!gHashTable.IsInitialized()) {
@@ -335,17 +336,18 @@ pref_savePref(PLDHashTable *table, PLDHa
 
     // where we're getting our pref from
     PrefValue* sourcePref;
 
     if (PREF_HAS_USER_VALUE(pref) &&
         (pref_ValueChanged(pref->defaultPref,
                            pref->userPref,
                            (PrefType) PREF_TYPE(pref)) ||
-         !(pref->flags & PREF_HAS_DEFAULT))) {
+         !(pref->flags & PREF_HAS_DEFAULT) ||
+         pref->flags & PREF_STICKY_DEFAULT)) {
         sourcePref = &pref->userPref;
     } else {
         if (argData->saveTypes == SAVE_ALL_AND_DEFAULTS) {
             prefPrefix.AssignLiteral("pref(\"");
             sourcePref = &pref->defaultPref;
         }
         else
             // do not save default prefs that haven't changed
@@ -773,28 +775,32 @@ nsresult pref_HashPref(const char *key, 
     {
         if (!PREF_IS_LOCKED(pref))
         {       /* ?? change of semantics? */
             if (pref_ValueChanged(pref->defaultPref, value, type) ||
                 !(pref->flags & PREF_HAS_DEFAULT))
             {
                 pref_SetValue(&pref->defaultPref, &pref->flags, value, type);
                 pref->flags |= PREF_HAS_DEFAULT;
+                if (flags & kPrefStickyDefault)
+                    pref->flags |= PREF_STICKY_DEFAULT;
                 if (!PREF_HAS_USER_VALUE(pref))
                     valueChanged = true;
             }
             // What if we change the default to be the same as the user value?
             // Should we clear the user value?
         }
     }
     else
     {
-        /* If new value is same as the default value, then un-set the user value.
+        /* If new value is same as the default value and it's not a "sticky"
+           pref, then un-set the user value.
            Otherwise, set the user value only if it has changed */
         if ((pref->flags & PREF_HAS_DEFAULT) &&
+            !(pref->flags & PREF_STICKY_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)) {
@@ -997,12 +1003,17 @@ static nsresult pref_DoCallback(const ch
 
     return rv;
 }
 
 void PREF_ReaderCallback(void       *closure,
                          const char *pref,
                          PrefValue   value,
                          PrefType    type,
-                         bool        isDefault)
+                         bool        isDefault,
+                         bool        isStickyDefault)
 {
-    pref_HashPref(pref, value, type, isDefault ? kPrefSetDefault : kPrefForceSet);
+    uint32_t flags = isDefault ? kPrefSetDefault : kPrefForceSet;
+    if (isDefault && isStickyDefault) {
+        flags |= kPrefStickyDefault;
+    }
+    pref_HashPref(pref, value, type, flags);
 }
--- a/modules/libpref/prefapi.h
+++ b/modules/libpref/prefapi.h
@@ -56,16 +56,18 @@ void        PREF_CleanupPrefs();
 // Preference flags, including the native type of the preference
 // </font>
 */
 
 typedef enum { PREF_INVALID = 0,
                PREF_LOCKED = 1, PREF_USERSET = 2, PREF_CONFIG = 4, PREF_REMOTE = 8,
                PREF_LILOCAL = 16, PREF_STRING = 32, PREF_INT = 64, PREF_BOOL = 128,
                PREF_HAS_DEFAULT = 256,
+               // pref is default pref with "sticky" semantics
+               PREF_STICKY_DEFAULT = 512,
                PREF_VALUETYPE_MASK = (PREF_STRING | PREF_INT | PREF_BOOL)
              } PrefType;
 
 /*
 // <font color=blue>
 // Set the various types of preferences.  These functions take a dotted
 // notation of the preference name (e.g. "browser.startup.homepage").  
 // Note that this will cause the preference to be saved to the file if
@@ -178,14 +180,15 @@ nsresult PREF_UnregisterCallback( const 
 
 /*
  * Used by nsPrefService as the callback function of the 'pref' parser
  */
 void PREF_ReaderCallback( void *closure,
                           const char *pref,
                           PrefValue   value,
                           PrefType    type,
-                          bool        isDefault);
+                          bool        isDefault,
+                          bool        isStickyDefault);
 
 #ifdef __cplusplus
 }
 #endif
 #endif
--- a/modules/libpref/prefread.cpp
+++ b/modules/libpref/prefread.cpp
@@ -39,16 +39,17 @@ enum {
 };
 
 #define UTF16_ESC_NUM_DIGITS    4
 #define HEX_ESC_NUM_DIGITS      2
 #define BITS_PER_HEX_DIGIT      4
 
 static const char kUserPref[] = "user_pref";
 static const char kPref[] = "pref";
+static const char kPrefSticky[] = "sticky_pref";
 static const char kTrue[] = "true";
 static const char kFalse[] = "false";
 
 /**
  * pref_GrowBuf
  * 
  * this function will increase the size of the buffer owned
  * by the given pref parse state.  We currently use a simple
@@ -124,17 +125,18 @@ pref_DoCallback(PrefParseState *ps)
         value.intVal = atoi(ps->vb);
         break;
     case PREF_BOOL:
         value.boolVal = (ps->vb == kTrue);
         break;
     default:
         break;
     }
-    (*ps->reader)(ps->closure, ps->lb, value, ps->vtype, ps->fdefault);
+    (*ps->reader)(ps->closure, ps->lb, value, ps->vtype, ps->fdefault,
+                  ps->fstickydefault);
     return true;
 }
 
 void
 PREF_InitParseState(PrefParseState *ps, PrefReader reader, void *closure)
 {
     memset(ps, 0, sizeof(*ps));
     ps->reader = reader;
@@ -183,27 +185,30 @@ PREF_ParseBuf(PrefParseState *ps, const 
         switch (state) {
         /* initial state */
         case PREF_PARSE_INIT:
             if (ps->lbcur != ps->lb) { /* reset state */
                 ps->lbcur = ps->lb;
                 ps->vb    = nullptr;
                 ps->vtype = PREF_INVALID;
                 ps->fdefault = false;
+                ps->fstickydefault = false;
             }
             switch (c) {
             case '/':       /* begin comment block or line? */
                 state = PREF_PARSE_COMMENT_MAYBE_START;
                 break;
             case '#':       /* accept shell style comments */
                 state = PREF_PARSE_UNTIL_EOL;
                 break;
             case 'u':       /* indicating user_pref */
             case 'p':       /* indicating pref */
-                ps->smatch = (c == 'u' ? kUserPref : kPref);
+            case 's':       /* indicating sticky_pref */
+                ps->smatch = (c == 'u' ? kUserPref :
+                             (c == 's' ? kPrefSticky : kPref));
                 ps->sindex = 1;
                 ps->nextstate = PREF_PARSE_UNTIL_OPEN_PAREN;
                 state = PREF_PARSE_MATCH_STRING;
                 break;
             /* else skip char */
             }
             break;
 
@@ -237,17 +242,18 @@ PREF_ParseBuf(PrefParseState *ps, const 
             }
             else
                 *ps->lbcur++ = c;
             break;
 
         /* name parsing */
         case PREF_PARSE_UNTIL_NAME:
             if (c == '\"' || c == '\'') {
-                ps->fdefault = (ps->smatch == kPref);
+                ps->fdefault = (ps->smatch == kPref || ps->smatch == kPrefSticky);
+                ps->fstickydefault = (ps->smatch == kPrefSticky);
                 ps->quotechar = c;
                 ps->nextstate = PREF_PARSE_UNTIL_COMMA; /* return here when done */
                 state = PREF_PARSE_QUOTED_STRING;
             }
             else if (c == '/') {       /* allow embedded comment */
                 ps->nextstate = state; /* return here when done with comment */
                 state = PREF_PARSE_COMMENT_MAYBE_START;
             }
--- a/modules/libpref/prefread.h
+++ b/modules/libpref/prefread.h
@@ -21,22 +21,25 @@ extern "C" {
  * @param pref
  *        preference name
  * @param val
  *        preference value
  * @param type
  *        preference type (PREF_STRING, PREF_INT, or PREF_BOOL)
  * @param defPref
  *        preference type (true: default, false: user preference)
+ * @param stickyPref
+ *        default preference marked as a "sticky" pref
  */
 typedef void (*PrefReader)(void       *closure,
                            const char *pref,
                            PrefValue   val,
                            PrefType    type,
-                           bool        defPref);
+                           bool        defPref,
+                           bool        stickyPref);
 
 /* structure fields are private */
 typedef struct PrefParseState {
     PrefReader  reader;
     void       *closure;
     int         state;      /* PREF_PARSE_...                */
     int         nextstate;  /* sometimes used...             */
     const char *smatch;     /* string to match               */
@@ -47,16 +50,17 @@ typedef struct PrefParseState {
     char        esctmp[6];  /* raw escape to put back if err */
     char        quotechar;  /* char delimiter for quotations */
     char       *lb;         /* line buffer (only allocation) */
     char       *lbcur;      /* line buffer cursor            */
     char       *lbend;      /* line buffer end               */
     char       *vb;         /* value buffer (ptr into lb)    */
     PrefType    vtype;      /* PREF_STRING,INT,BOOL          */
     bool        fdefault;   /* true if (default) pref     */
+    bool        fstickydefault; /* true if (sticky) pref     */
 } PrefParseState;
 
 /**
  * PREF_InitParseState
  *
  * Called to initialize a PrefParseState instance.
  * 
  * @param ps
new file mode 100644
--- /dev/null
+++ b/modules/libpref/test/unit/data/testPrefSticky.js
@@ -0,0 +1,2 @@
+pref("testPref.unsticky.bool", true);
+sticky_pref("testPref.sticky.bool", false);
new file mode 100644
--- /dev/null
+++ b/modules/libpref/test/unit/data/testPrefStickyUser.js
@@ -0,0 +1,5 @@
+// testPrefSticky.js defined this pref as a sticky_pref(). Once a sticky
+// pref has been changed, it's written as a user_pref().
+// So this test file reflects that scenario.
+// Note the default in testPrefSticky.js is also false.
+user_pref("testPref.sticky.bool", false);
new file mode 100644
--- /dev/null
+++ b/modules/libpref/test/unit/test_stickyprefs.js
@@ -0,0 +1,170 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/  */
+
+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
+Components.utils.import("resource://gre/modules/Services.jsm");
+
+const ps = Services.prefs;
+
+// Once we fetch the profile directory the xpcshell test harness will send
+// a profile-before-change notification at shutdown. This causes the prefs
+// service to flush the prefs file - and the prefs file it uses ends up being
+// testPrefSticky*.js in the test dir. This upsets things in confusing ways :)
+// We avoid this by ensuring our "temp" prefs.js is the current prefs file.
+do_get_profile();
+do_register_cleanup(saveAndReload);
+
+// A little helper to reset the service and load some pref files
+function resetAndLoad(filenames) {
+  ps.resetPrefs();
+  for (let filename of filenames) {
+    ps.readUserPrefs(do_get_file(filename));
+  }
+}
+
+// A little helper that saves the current state to a file in the profile
+// dir, then resets the service and re-reads the file it just saved.
+// Used to test what gets actually written - things the pref service decided
+// not to write don't exist at all after this call.
+function saveAndReload() {
+  let file = do_get_profile();
+  file.append("prefs.js");
+  ps.savePrefFile(file);
+
+  // Now reset the pref service and re-read what we saved.
+  ps.resetPrefs();
+  ps.readUserPrefs(file);
+}
+
+function run_test() {
+  run_next_test();
+}
+
+// A sticky pref should not be written if the value is unchanged.
+add_test(function notWrittenWhenUnchanged() {
+  resetAndLoad(["data/testPrefSticky.js"]);
+  Assert.strictEqual(ps.getBoolPref("testPref.unsticky.bool"), true);
+  Assert.strictEqual(ps.getBoolPref("testPref.sticky.bool"), false);
+
+  // write prefs - but we haven't changed the sticky one, so it shouldn't be written.
+  saveAndReload();
+  // sticky should not have been written to the new file.
+  try {
+    ps.getBoolPref("testPref.sticky.bool");
+    Assert.ok(false, "expected failure reading this pref");
+  } catch (ex) {
+    Assert.ok(ex, "exception reading regular pref");
+  }
+  run_next_test();
+});
+
+// Loading a sticky_pref then a user_pref for the same pref means it should
+// always be written.
+add_test(function writtenOnceLoadedWithoutChange() {
+  // Load the same pref file *as well as* a pref file that has a user_pref for
+  // our sticky with the default value. It should be re-written without us
+  // touching it.
+  resetAndLoad(["data/testPrefSticky.js", "data/testPrefStickyUser.js"]);
+  // reset and re-read what we just wrote - it should be written.
+  saveAndReload();
+  Assert.strictEqual(ps.getBoolPref("testPref.sticky.bool"), false,
+                     "user_pref was written with default value");
+  run_next_test();
+});
+
+// If a sticky pref is explicicitly changed, even to the default, it is written.
+add_test(function writtenOnceLoadedWithChangeNonDefault() {
+  // Load the same pref file *as well as* a pref file that has a user_pref for
+  // our sticky - then change the pref. It should be written.
+  resetAndLoad(["data/testPrefSticky.js", "data/testPrefStickyUser.js"]);
+  // Set a new val and check we wrote it.
+  ps.setBoolPref("testPref.sticky.bool", false);
+  saveAndReload();
+  Assert.strictEqual(ps.getBoolPref("testPref.sticky.bool"), false,
+                     "user_pref was written with custom value");
+  run_next_test();
+});
+
+// If a sticky pref is changed to the non-default value, it is written.
+add_test(function writtenOnceLoadedWithChangeNonDefault() {
+  // Load the same pref file *as well as* a pref file that has a user_pref for
+  // our sticky - then change the pref. It should be written.
+  resetAndLoad(["data/testPrefSticky.js", "data/testPrefStickyUser.js"]);
+  // Set a new val and check we wrote it.
+  ps.setBoolPref("testPref.sticky.bool", true);
+  saveAndReload();
+  Assert.strictEqual(ps.getBoolPref("testPref.sticky.bool"), true,
+                     "user_pref was written with custom value");
+  run_next_test();
+});
+
+// Test that prefHasUserValue always returns true whenever there is a sticky
+// value, even when that value matches the default. This is mainly for
+// about:config semantics - prefs with a sticky value always remain bold and
+// always offer "reset" (which fully resets and drops the sticky value as if
+// the pref had never changed.)
+add_test(function hasUserValue() {
+  // sticky pref without user value.
+  resetAndLoad(["data/testPrefSticky.js"]);
+  Assert.strictEqual(ps.getBoolPref("testPref.sticky.bool"), false);
+  Assert.ok(!ps.prefHasUserValue("testPref.sticky.bool"),
+            "should not initially reflect a user value");
+
+  ps.setBoolPref("testPref.sticky.bool", false);
+  Assert.ok(ps.prefHasUserValue("testPref.sticky.bool"),
+            "should reflect a user value after set to default");
+
+  ps.setBoolPref("testPref.sticky.bool", true);
+  Assert.ok(ps.prefHasUserValue("testPref.sticky.bool"),
+            "should reflect a user value after change to non-default");
+
+  ps.clearUserPref("testPref.sticky.bool");
+  Assert.ok(!ps.prefHasUserValue("testPref.sticky.bool"),
+            "should reset to no user value");
+  ps.setBoolPref("testPref.sticky.bool", false, "expected default");
+
+  // And make sure the pref immediately reflects a user value after load.
+  resetAndLoad(["data/testPrefSticky.js", "data/testPrefStickyUser.js"]);
+  Assert.strictEqual(ps.getBoolPref("testPref.sticky.bool"), false);
+  Assert.ok(ps.prefHasUserValue("testPref.sticky.bool"),
+            "should have a user value when loaded value is the default");
+  run_next_test();
+});
+
+// Test that clearUserPref removes the "sticky" value.
+add_test(function clearUserPref() {
+  // load things such that we have a sticky value which is the same as the
+  // default.
+  resetAndLoad(["data/testPrefSticky.js", "data/testPrefStickyUser.js"]);
+  ps.clearUserPref("testPref.sticky.bool");
+
+  // Once we save prefs the sticky pref should no longer be written.
+  saveAndReload();
+  try {
+    ps.getBoolPref("testPref.sticky.bool");
+    Assert.ok(false, "expected failure reading this pref");
+  } catch (ex) {
+    Assert.ok(ex, "pref doesn't have a sticky value");
+  }
+  run_next_test();
+});
+
+// Test that a pref observer gets a notification fired when a sticky pref
+// has it's value changed to the same value as the default. The reason for
+// this behaviour is that later we might have other code that cares about a
+// pref being sticky (IOW, we notify due to the "state" of the pref changing
+// even if the value has not)
+add_test(function observerFires() {
+  // load things so there's no sticky value.
+  resetAndLoad(["data/testPrefSticky.js"]);
+
+  function observe(subject, topic, data) {
+    Assert.equal(data, "testPref.sticky.bool");
+    ps.removeObserver("testPref.sticky.bool", observe);
+    run_next_test();
+  }
+  ps.addObserver("testPref.sticky.bool", observe, false);
+
+  ps.setBoolPref("testPref.sticky.bool", ps.getBoolPref("testPref.sticky.bool"));
+  // and the observer will fire triggering the next text.
+});
--- a/modules/libpref/test/unit/xpcshell.ini
+++ b/modules/libpref/test/unit/xpcshell.ini
@@ -6,12 +6,14 @@ support-files =
   data/testPref.js
   extdata/testExt.js
 
 [test_warnings.js]
 [test_bug345529.js]
 [test_bug506224.js]
 [test_bug577950.js]
 [test_bug790374.js]
+[test_stickyprefs.js]
+support-files = data/testPrefSticky.js data/testPrefStickyUser.js
 [test_changeType.js]
 [test_dirtyPrefs.js]
 [test_extprefs.js]
 [test_libPrefs.js]