Bug 1394578 - Move more operations into PrefValue. r=glandium draft
authorNicholas Nethercote <nnethercote@mozilla.com>
Thu, 23 Nov 2017 16:34:42 +1100
changeset 702909 6da6f38221e1718130eaa0449b19d6cff3ed8cfc
parent 702908 87279e3af6e86626a195f14190151671853ee740
child 702910 624cc4ce298e9311b4580307543f86b2465fea54
push id90624
push usernnethercote@mozilla.com
push dateThu, 23 Nov 2017 23:13:32 +0000
reviewersglandium
bugs1394578
milestone59.0a1
Bug 1394578 - Move more operations into PrefValue. r=glandium The nice thing about this is that the memory management of the strings (moz_xstrdup() and free()) is now entirely within PrefValue. MozReview-Commit-ID: KJjnURpmgfE
modules/libpref/Preferences.cpp
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -151,16 +151,39 @@ union PrefValue {
 
       case PrefType::Bool:
         return mBoolVal == aValue.mBoolVal;
 
       default:
         MOZ_CRASH("Unhandled enum value");
     }
   }
+
+  void Init(PrefType aNewType, PrefValue aNewValue)
+  {
+    if (aNewType == PrefType::String) {
+      MOZ_ASSERT(aNewValue.mStringVal);
+      aNewValue.mStringVal = moz_xstrdup(aNewValue.mStringVal);
+    }
+    *this = aNewValue;
+  }
+
+  void Clear(PrefType aType)
+  {
+    if (aType == PrefType::String) {
+      free(const_cast<char*>(mStringVal));
+      mStringVal = nullptr;
+    }
+  }
+
+  void Replace(PrefType aOldType, PrefType aNewType, PrefValue aNewValue)
+  {
+    Clear(aOldType);
+    Init(aNewType, aNewValue);
+  }
 };
 
 #ifdef DEBUG
 const char*
 PrefTypeToString(PrefType aType)
 {
   switch (aType) {
     case PrefType::String:
@@ -239,20 +262,18 @@ public:
     // entries.
   }
 
   ~Pref()
   {
     // There's no need to free mName because it's allocated in memory owned by
     // gPrefNameArena.
 
-    if (IsTypeString()) {
-      free(const_cast<char*>(mDefaultValue.mStringVal));
-      free(const_cast<char*>(mUserValue.mStringVal));
-    }
+    mDefaultValue.Clear(Type());
+    mUserValue.Clear(Type());
   }
 
   const char* Name() { return mName; }
 
   // Types.
 
   PrefType Type() const { return static_cast<PrefType>(mType); }
   void SetType(PrefType aType) { mType = static_cast<uint32_t>(aType); }
@@ -429,61 +450,29 @@ public:
         return false;
       }
     }
 
     return true;
   }
 
 private:
-  // 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.
-  void ReplaceValue(PrefValueKind aKind, PrefType aNewType, PrefValue aNewValue)
-  {
-    PrefValue* value =
-      aKind == PrefValueKind::Default ? &mDefaultValue : &mUserValue;
-
-    if (Type() == PrefType::String) {
-      free(const_cast<char*>(value->mStringVal));
-    }
-
-    SetType(aNewType);
-
-    if (aNewType == PrefType::String) {
-      MOZ_ASSERT(aNewValue.mStringVal);
-      value->mStringVal = moz_xstrdup(aNewValue.mStringVal);
-    } else {
-      *value = aNewValue;
-    }
-
-    if (aKind == PrefValueKind::Default) {
-      mHasDefaultValue = true;
-    } else {
-      mHasUserValue = true;
-    }
-  }
-
   bool ValueMatches(PrefValueKind aKind, PrefType aType, PrefValue aValue)
   {
     return (aKind == PrefValueKind::Default)
              ? IsType(aType) && mHasDefaultValue &&
                  mDefaultValue.Equals(aType, aValue)
              : IsType(aType) && mHasUserValue &&
                  mUserValue.Equals(aType, aValue);
   }
 
 public:
   void ClearUserValue()
   {
-    if (Type() == PrefType::String) {
-      free(const_cast<char*>(mUserValue.mStringVal));
-      mUserValue.mStringVal = nullptr;
-    }
-
+    mUserValue.Clear(Type());
     mHasUserValue = false;
   }
 
   nsresult SetValue(PrefType aType,
                     PrefValueKind aKind,
                     PrefValue aValue,
                     bool aIsSticky,
                     bool aForceSet,
@@ -494,17 +483,18 @@ public:
       // 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)) {
-        ReplaceValue(PrefValueKind::Default, aType, aValue);
+        mDefaultValue.Replace(Type(), aType, aValue);
+        mHasDefaultValue = true;
         if (aIsSticky) {
           mIsSticky = true;
         }
         if (!mHasUserValue) {
           *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.
@@ -527,17 +517,19 @@ public:
             *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)) {
-        ReplaceValue(PrefValueKind::User, aType, aValue);
+        mUserValue.Replace(Type(), aType, aValue);
+        SetType(aType);
+        mHasUserValue = true;
         if (!IsLocked()) {
           *aDirty = true;
           *aValueChanged = true;
         }
       }
     }
     return NS_OK;
   }