Bug 1421541 - Split Pref::SetValue() in two. r=glandium draft
authorNicholas Nethercote <nnethercote@mozilla.com>
Wed, 29 Nov 2017 18:57:06 +1100
changeset 704983 b89408f0e4c63c2186bbf1b4abbeef1a28b9143c
parent 704982 0e074f279b9002d42ecdf64aa0127415adfaa241
child 704984 84f8de086fd4d2ab676a7168387e90845820d3c3
push id91299
push usernnethercote@mozilla.com
push dateWed, 29 Nov 2017 08:10:36 +0000
reviewersglandium
bugs1421541
milestone59.0a1
Bug 1421541 - Split Pref::SetValue() in two. r=glandium The default path and the user path are entirely disjoint, and some of the arguments only apply to one of the paths, so splitting it into two functions makes things a bit clearer. The aForceSet arg is also renamed aFromFile. MozReview-Commit-ID: LYtrwz5JHiN
modules/libpref/Preferences.cpp
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -526,74 +526,74 @@ public:
     if (Type() == PrefType::String) {
       free(const_cast<char*>(mUserValue.mStringVal));
       mUserValue.mStringVal = nullptr;
     }
 
     mHasUserValue = false;
   }
 
-  nsresult SetValue(PrefType aType,
-                    PrefValueKind aKind,
-                    PrefValue aValue,
-                    bool aIsSticky,
-                    bool aForceSet,
-                    bool* aValueChanged,
-                    bool* aDirty)
+  nsresult SetDefaultValue(PrefType aType,
+                           PrefValue aValue,
+                           bool aIsSticky,
+                           bool* aValueChanged)
   {
-    if (aKind == PrefValueKind::Default) {
-      // Types must always match when setting the default value.
-      if (!IsType(aType)) {
-        return NS_ERROR_UNEXPECTED;
+    // Types must always match when setting the default value.
+    if (!IsType(aType)) {
+      return NS_ERROR_UNEXPECTED;
+    }
+
+    // Should we set the default value? Only if the pref is not locked, and
+    // doing so would change the default value.
+    if (!IsLocked() && !ValueMatches(PrefValueKind::Default, aType, aValue)) {
+      mDefaultValue.Replace(Type(), aType, aValue);
+      mHasDefaultValue = true;
+      if (aIsSticky) {
+        mIsSticky = true;
+      }
+      if (!mHasUserValue) {
+        *aValueChanged = true;
       }
-
-      // Should we set the default value? Only if the pref is not locked, and
-      // doing so would change the default value.
-      if (!IsLocked() && !ValueMatches(PrefValueKind::Default, aType, aValue)) {
-        mDefaultValue.Replace(Type(), aType, aValue);
-        mHasDefaultValue = true;
-        if (aIsSticky) {
-          mIsSticky = true;
-        }
-        if (!mHasUserValue) {
+      // What if we change the default to be the same as the user value?
+      // Should we clear the user value? Currently we don't.
+    }
+    return NS_OK;
+  }
+
+  nsresult SetUserValue(PrefType aType,
+                        PrefValue aValue,
+                        bool aFromFile,
+                        bool* aValueChanged)
+  {
+    // If we have a default value, types must match when setting the user
+    // value.
+    if (mHasDefaultValue && !IsType(aType)) {
+      return NS_ERROR_UNEXPECTED;
+    }
+
+    // Should we clear the user value, if present? Only if the new user value
+    // matches the default value, and the pref isn't sticky, and we aren't
+    // force-setting it.
+    if (ValueMatches(PrefValueKind::Default, aType, aValue) && !mIsSticky &&
+        !aFromFile) {
+      if (mHasUserValue) {
+        ClearUserValue();
+        if (!IsLocked()) {
           *aValueChanged = true;
         }
-        // What if we change the default to be the same as the user value?
-        // Should we clear the user value? Currently we don't.
       }
-    } else {
-      // If we have a default value, types must match when setting the user
-      // value.
-      if (mHasDefaultValue && !IsType(aType)) {
-        return NS_ERROR_UNEXPECTED;
-      }
-
-      // Should we clear the user value, if present? Only if the new user value
-      // matches the default value, and the pref isn't sticky, and we aren't
-      // force-setting it.
-      if (ValueMatches(PrefValueKind::Default, aType, aValue) && !mIsSticky &&
-          !aForceSet) {
-        if (mHasUserValue) {
-          ClearUserValue();
-          if (!IsLocked()) {
-            *aDirty = true;
-            *aValueChanged = true;
-          }
-        }
-
-        // Otherwise, should we set the user value? Only if doing so would
-        // change the user value.
-      } else if (!ValueMatches(PrefValueKind::User, aType, aValue)) {
-        mUserValue.Replace(Type(), aType, aValue);
-        SetType(aType);   // needed because we may have changed the type
-        mHasUserValue = true;
-        if (!IsLocked()) {
-          *aDirty = true;
-          *aValueChanged = true;
-        }
+
+      // Otherwise, should we set the user value? Only if doing so would
+      // change the user value.
+    } else if (!ValueMatches(PrefValueKind::User, aType, aValue)) {
+      mUserValue.Replace(Type(), aType, aValue);
+      SetType(aType);   // needed because we may have changed the type
+      mHasUserValue = true;
+      if (!IsLocked()) {
+        *aValueChanged = true;
       }
     }
     return NS_OK;
   }
 
   // Returns false if this pref doesn't have a user value worth saving.
   bool UserValueToStringForSaving(nsCString& aStr)
   {
@@ -800,17 +800,17 @@ pref_HashTableLookup(const char* aKey)
 }
 
 static nsresult
 pref_SetPref(const char* aPrefName,
              PrefType aType,
              PrefValueKind aKind,
              PrefValue aValue,
              bool aIsSticky,
-             bool aForceSet)
+             bool aFromFile)
 {
   MOZ_ASSERT(NS_IsMainThread());
 
   if (!gHashTable) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
 
   auto pref = static_cast<Pref*>(gHashTable->Add(aPrefName, fallible));
@@ -819,36 +819,40 @@ pref_SetPref(const char* aPrefName,
   }
 
   if (!pref->Name()) {
     // New (zeroed) entry. Partially initialize it.
     new (pref) Pref(aPrefName);
     pref->SetType(aType);
   }
 
-  bool valueChanged = false, handleDirty = false;
-  nsresult rv = pref->SetValue(
-    aType, aKind, aValue, aIsSticky, aForceSet, &valueChanged, &handleDirty);
+  bool valueChanged = false;
+  nsresult rv;
+  if (aKind == PrefValueKind::Default) {
+    rv = pref->SetDefaultValue(aType, aValue, aIsSticky, &valueChanged);
+  } else {
+    rv = pref->SetUserValue(aType, aValue, aFromFile, &valueChanged);
+  }
   if (NS_FAILED(rv)) {
     NS_WARNING(
       nsPrintfCString(
         "Rejected attempt to change type of pref %s's %s value from %s to %s",
         aPrefName,
         (aKind == PrefValueKind::Default) ? "default" : "user",
         PrefTypeToString(pref->Type()),
         PrefTypeToString(aType))
         .get());
 
     return rv;
   }
 
-  if (handleDirty && XRE_IsParentProcess()) {
-    Preferences::HandleDirty();
-  }
   if (valueChanged) {
+    if (aKind == PrefValueKind::User && XRE_IsParentProcess()) {
+      Preferences::HandleDirty();
+    }
     NotifyCallbacks(aPrefName);
   }
 
   return NS_OK;
 }
 
 // Removes |node| from callback list. Returns the node after the deleted one.
 static CallbackNode*
@@ -1048,26 +1052,19 @@ Parser::GrowBuf()
 
 void
 Parser::HandleValue(const char* aPrefName,
                     PrefType aType,
                     PrefValue aValue,
                     bool aIsDefault,
                     bool aIsSticky)
 {
-  PrefValueKind kind;
-  bool forceSet;
-  if (aIsDefault) {
-    kind = PrefValueKind::Default;
-    forceSet = false;
-  } else {
-    kind = PrefValueKind::User;
-    forceSet = true;
-  }
-  pref_SetPref(aPrefName, aType, kind, aValue, aIsSticky, forceSet);
+  PrefValueKind kind =
+    aIsDefault ? PrefValueKind::Default : PrefValueKind::User;
+  pref_SetPref(aPrefName, aType, kind, aValue, aIsSticky, /* fromFile */ true);
 }
 
 // Report an error or a warning. If not specified, just dump to stderr.
 void
 Parser::ReportProblem(const char* aMessage, int aLine, bool aError)
 {
   nsPrintfCString message("** Preference parsing %s (line %d) = %s **\n",
                           (aError ? "error" : "warning"),
@@ -4413,17 +4410,17 @@ Preferences::SetCStringInAnyProcess(cons
   PrefValue prefValue;
   const nsCString& flat = PromiseFlatCString(aValue);
   prefValue.mStringVal = flat.get();
   return pref_SetPref(aPrefName,
                       PrefType::String,
                       aKind,
                       prefValue,
                       /* isSticky */ false,
-                      /* forceSet */ false);
+                      /* fromFile */ false);
 }
 
 /* static */ nsresult
 Preferences::SetCString(const char* aPrefName,
                         const nsACString& aValue,
                         PrefValueKind aKind)
 {
   ENSURE_PARENT_PROCESS("SetCString", aPrefName);
@@ -4439,17 +4436,17 @@ Preferences::SetBoolInAnyProcess(const c
 
   PrefValue prefValue;
   prefValue.mBoolVal = aValue;
   return pref_SetPref(aPrefName,
                       PrefType::Bool,
                       aKind,
                       prefValue,
                       /* isSticky */ false,
-                      /* forceSet */ false);
+                      /* fromFile */ false);
 }
 
 /* static */ nsresult
 Preferences::SetBool(const char* aPrefName, bool aValue, PrefValueKind aKind)
 {
   ENSURE_PARENT_PROCESS("SetBool", aPrefName);
   return SetBoolInAnyProcess(aPrefName, aValue, aKind);
 }
@@ -4463,17 +4460,17 @@ Preferences::SetIntInAnyProcess(const ch
 
   PrefValue prefValue;
   prefValue.mIntVal = aValue;
   return pref_SetPref(aPrefName,
                       PrefType::Int,
                       aKind,
                       prefValue,
                       /* isSticky */ false,
-                      /* forceSet */ false);
+                      /* fromFile */ false);
 }
 
 /* static */ nsresult
 Preferences::SetInt(const char* aPrefName, int32_t aValue, PrefValueKind aKind)
 {
   ENSURE_PARENT_PROCESS("SetInt", aPrefName);
   return SetIntInAnyProcess(aPrefName, aValue, aKind);
 }