Bug 1441754 - Privatize CallbackNode's field. r=glandium draft
authorNicholas Nethercote <nnethercote@mozilla.com>
Wed, 28 Feb 2018 16:10:11 +1100
changeset 760833 e5d911a69697461aa91914cd08c02a97ad625f4c
parent 760832 fe50a84a16bda1e82d29217be779e7395d34ebb8
child 760834 f56b4aab9dd31ab3f04459f1f134085df29a6fa4
push id100761
push usernnethercote@mozilla.com
push dateWed, 28 Feb 2018 07:48:23 +0000
reviewersglandium
bugs1441754
milestone60.0a1
Bug 1441754 - Privatize CallbackNode's field. r=glandium This isn't compelling on its own, but it's necessary for the next patch. MozReview-Commit-ID: CFON8DCdGoA
modules/libpref/Preferences.cpp
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -741,36 +741,52 @@ public:
   {
     auto entry = static_cast<PrefEntry*>(aEntry);
 
     delete entry->mPref;
     entry->mPref = nullptr;
   }
 };
 
-struct CallbackNode
+class CallbackNode
 {
+public:
   CallbackNode(const char* aDomain,
                PrefChangedFunc aFunc,
                void* aData,
                Preferences::MatchKind aMatchKind)
     : mDomain(moz_xstrdup(aDomain))
     , mFunc(aFunc)
     , mData(aData)
     , mMatchKind(aMatchKind)
     , mNext(nullptr)
   {
   }
 
+  // mDomain is a UniquePtr<>, so any uses of Domain() should only be temporary
+  // borrows.
+  const char* Domain() const { return mDomain.get(); }
+
+  PrefChangedFunc Func() const { return mFunc; }
+  void ClearFunc() { mFunc = nullptr; }
+
+  void* Data() const { return mData; }
+
+  Preferences::MatchKind MatchKind() const { return mMatchKind; }
+
+  CallbackNode* Next() const { return mNext; }
+  void SetNext(CallbackNode* aNext) { mNext = aNext; }
+
   void AddSizeOfIncludingThis(MallocSizeOf aMallocSizeOf, PrefsSizes& aSizes)
   {
     aSizes.mCallbacksObjects += aMallocSizeOf(this);
     aSizes.mCallbacksDomains += aMallocSizeOf(mDomain.get());
   }
 
+private:
   UniqueFreePtr<const char> mDomain;
 
   // If someone attempts to remove the node from the callback list while
   // NotifyCallbacks() is running, |func| is set to nullptr. Such nodes will
   // be removed at the end of NotifyCallbacks().
   PrefChangedFunc mFunc;
   void* mData;
   Preferences::MatchKind mMatchKind;
@@ -959,26 +975,26 @@ pref_SetPref(const char* aPrefName,
 
   return NS_OK;
 }
 
 // Removes |node| from callback list. Returns the node after the deleted one.
 static CallbackNode*
 pref_RemoveCallbackNode(CallbackNode* aNode, CallbackNode* aPrevNode)
 {
-  NS_PRECONDITION(!aPrevNode || aPrevNode->mNext == aNode, "invalid params");
+  NS_PRECONDITION(!aPrevNode || aPrevNode->Next() == aNode, "invalid params");
   NS_PRECONDITION(aPrevNode || gFirstCallback == aNode, "invalid params");
 
   NS_ASSERTION(
     !gCallbacksInProgress,
     "modifying the callback list while gCallbacksInProgress is true");
 
-  CallbackNode* next_node = aNode->mNext;
+  CallbackNode* next_node = aNode->Next();
   if (aPrevNode) {
-    aPrevNode->mNext = next_node;
+    aPrevNode->SetNext(next_node);
   } else {
     gFirstCallback = next_node;
   }
   if (gLastPriorityNode == aNode) {
     gLastPriorityNode = aPrevNode;
   }
   delete aNode;
   return next_node;
@@ -990,41 +1006,40 @@ NotifyCallbacks(const char* aPrefName)
   bool reentered = gCallbacksInProgress;
 
   // Nodes must not be deleted while gCallbacksInProgress is true.
   // Nodes that need to be deleted are marked for deletion by nulling
   // out the |func| pointer. We release them at the end of this function
   // if we haven't reentered.
   gCallbacksInProgress = true;
 
-  for (CallbackNode* node = gFirstCallback; node; node = node->mNext) {
-    if (node->mFunc) {
-      bool matches = node->mMatchKind == Preferences::ExactMatch
-                       ? strcmp(node->mDomain.get(), aPrefName) == 0
-                       : strncmp(node->mDomain.get(),
-                                 aPrefName,
-                                 strlen(node->mDomain.get())) == 0;
+  for (CallbackNode* node = gFirstCallback; node; node = node->Next()) {
+    if (node->Func()) {
+      bool matches =
+        node->MatchKind() == Preferences::ExactMatch
+          ? strcmp(node->Domain(), aPrefName) == 0
+          : strncmp(node->Domain(), aPrefName, strlen(node->Domain())) == 0;
       if (matches) {
-        (node->mFunc)(aPrefName, node->mData);
+        (node->Func())(aPrefName, node->Data());
       }
     }
   }
 
   gCallbacksInProgress = reentered;
 
   if (gShouldCleanupDeadNodes && !gCallbacksInProgress) {
     CallbackNode* prev_node = nullptr;
     CallbackNode* node = gFirstCallback;
 
     while (node) {
-      if (!node->mFunc) {
+      if (!node->Func()) {
         node = pref_RemoveCallbackNode(node, prev_node);
       } else {
         prev_node = node;
-        node = node->mNext;
+        node = node->Next();
       }
     }
     gShouldCleanupDeadNodes = false;
   }
 }
 
 //===========================================================================
 // Prefs parsing
@@ -2700,17 +2715,17 @@ PreferenceServiceReporter::CollectReport
     sizes.mCacheData += gCacheData->ShallowSizeOfIncludingThis(mallocSizeOf);
     for (uint32_t i = 0, count = gCacheData->Length(); i < count; ++i) {
       sizes.mCacheData += mallocSizeOf((*gCacheData)[i]);
     }
   }
 
   sizes.mPrefNameArena += gPrefNameArena.SizeOfExcludingThis(mallocSizeOf);
 
-  for (CallbackNode* node = gFirstCallback; node; node = node->mNext) {
+  for (CallbackNode* node = gFirstCallback; node; node = node->Next()) {
     node->AddSizeOfIncludingThis(mallocSizeOf, sizes);
   }
 
   MOZ_COLLECT_REPORT("explicit/preferences/hash-table",
                      KIND_HEAP,
                      UNITS_BYTES,
                      sizes.mHashTable,
                      "Memory used by libpref's hash table.");
@@ -3006,17 +3021,17 @@ Preferences::~Preferences()
   delete gCacheData;
   gCacheData = nullptr;
 
   NS_ASSERTION(!gCallbacksInProgress,
                "~Preferences was called while gCallbacksInProgress is true!");
 
   CallbackNode* node = gFirstCallback;
   while (node) {
-    CallbackNode* next_node = node->mNext;
+    CallbackNode* next_node = node->Next();
     delete node;
     node = next_node;
   }
   gLastPriorityNode = gFirstCallback = nullptr;
 
   delete gHashTable;
   gHashTable = nullptr;
 
@@ -4338,28 +4353,28 @@ Preferences::RegisterCallback(PrefChange
   NS_ENSURE_ARG(aCallback);
 
   NS_ENSURE_TRUE(InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
 
   auto node = new CallbackNode(aPrefNode, aCallback, aData, aMatchKind);
 
   if (aIsPriority) {
     // Add to the start of the list.
-    node->mNext = gFirstCallback;
+    node->SetNext(gFirstCallback);
     gFirstCallback = node;
     if (!gLastPriorityNode) {
       gLastPriorityNode = node;
     }
   } else {
     // Add to the start of the non-priority part of the list.
     if (gLastPriorityNode) {
-      node->mNext = gLastPriorityNode->mNext;
-      gLastPriorityNode->mNext = node;
+      node->SetNext(gLastPriorityNode->Next());
+      gLastPriorityNode->SetNext(node);
     } else {
-      node->mNext = gFirstCallback;
+      node->SetNext(gFirstCallback);
       gFirstCallback = node;
     }
   }
 
   return NS_OK;
 }
 
 /* static */ nsresult
@@ -4389,33 +4404,33 @@ Preferences::UnregisterCallback(PrefChan
   }
   NS_ENSURE_TRUE(sPreferences, NS_ERROR_NOT_AVAILABLE);
 
   nsresult rv = NS_ERROR_FAILURE;
   CallbackNode* node = gFirstCallback;
   CallbackNode* prev_node = nullptr;
 
   while (node) {
-    if (node->mFunc == aCallback && node->mData == aData &&
-        node->mMatchKind == aMatchKind &&
-        strcmp(node->mDomain.get(), aPrefNode) == 0) {
+    if (node->Func() == aCallback && node->Data() == aData &&
+        node->MatchKind() == aMatchKind &&
+        strcmp(node->Domain(), aPrefNode) == 0) {
       if (gCallbacksInProgress) {
-        // postpone the node removal until after
-        // callbacks enumeration is finished.
-        node->mFunc = nullptr;
+        // Postpone the node removal until after callbacks enumeration is
+        // finished.
+        node->ClearFunc();
         gShouldCleanupDeadNodes = true;
         prev_node = node;
-        node = node->mNext;
+        node = node->Next();
       } else {
         node = pref_RemoveCallbackNode(node, prev_node);
       }
       rv = NS_OK;
     } else {
       prev_node = node;
-      node = node->mNext;
+      node = node->Next();
     }
   }
   return rv;
 }
 
 static void
 CacheDataAppendElement(CacheData* aData)
 {