Bug 1473631: Part 0b - Add helper for registering instance methods as pref callbacks. r?njn draft
authorKris Maglione <maglione.k@gmail.com>
Fri, 13 Jul 2018 18:54:11 -0700
changeset 818472 c0880fafb7ba06c02b501b14a896f393eb6818e5
parent 818471 9f9fc4ec95cfdb86d7b038188caaeceed01da1c6
child 818473 1e8404ea2dbd50a05258589a005f39ccddbc9a89
push id116269
push usermaglione.k@gmail.com
push dateSat, 14 Jul 2018 02:18:47 +0000
reviewersnjn
bugs1473631
milestone63.0a1
Bug 1473631: Part 0b - Add helper for registering instance methods as pref callbacks. r?njn I also tried to avoid this change but, again, given the number of times I was repeating the same pattern of defining a static method just to forward a callback to an instance method, decided it was probably necessary. Without an easy way to do this, people are more likely to register observers rather than callbacks, for which we'll wind up paying a continued memory and performance penalty. This patch adds a helper which creates a type-safe preference callback function which forwards calls to an instance method on its closure object. The implementation is somewhat complicated, mainly due to the constraint that unregistering a callback requires passing the exact same function pointer that was used to register it. The patch achieves this by creating the callback function as a template, with the method pointer as a template parameter. As long as the Register and Unregister calls happen in the same translation unit, the same template instance is guaranteed to be used for both. The main difficulty is that, until C++ 17, there's no way match a value as a template parameter unless you know its complete type, or can at least compute its complete type based on earlier template parameters. That means that we need a macro to extract the type of the method, and construct the template with the full set of explicit parameters. MozReview-Commit-ID: 10N3R2SRtPc
dom/base/nsDocument.cpp
layout/base/nsPresContext.cpp
layout/base/nsPresContext.h
modules/libpref/Preferences.h
--- a/dom/base/nsDocument.cpp
+++ b/dom/base/nsDocument.cpp
@@ -12575,41 +12575,40 @@ struct PrefStore
     : mFlashBlockEnabled(false)
     , mPluginsHttpOnly(false)
   {
     Preferences::AddBoolVarCache(&mFlashBlockEnabled,
                                  "plugins.flashBlock.enabled");
     Preferences::AddBoolVarCache(&mPluginsHttpOnly,
                                  "plugins.http_https_only");
 
-    Preferences::RegisterCallbacks(UpdateStringPrefs, gCallbackPrefs, this);
+    Preferences::RegisterCallbacks(
+      PREF_CHANGE_METHOD(PrefStore::UpdateStringPrefs),
+      gCallbackPrefs, this);
 
     UpdateStringPrefs();
   }
 
   ~PrefStore()
   {
-    Preferences::UnregisterCallbacks(UpdateStringPrefs, gCallbackPrefs, this);
-  }
-
-  void UpdateStringPrefs()
+    Preferences::UnregisterCallbacks(
+      PREF_CHANGE_METHOD(PrefStore::UpdateStringPrefs),
+      gCallbackPrefs, this);
+  }
+
+  void UpdateStringPrefs(const char* aPref = nullptr)
   {
     Preferences::GetCString("urlclassifier.flashAllowTable", mAllowTables);
     Preferences::GetCString("urlclassifier.flashAllowExceptTable", mAllowExceptionsTables);
     Preferences::GetCString("urlclassifier.flashTable", mDenyTables);
     Preferences::GetCString("urlclassifier.flashExceptTable", mDenyExceptionsTables);
     Preferences::GetCString("urlclassifier.flashSubDocTable", mSubDocDenyTables);
     Preferences::GetCString("urlclassifier.flashSubDocExceptTable", mSubDocDenyExceptionsTables);
   }
 
-  static void UpdateStringPrefs(const char*, PrefStore* aSelf)
-  {
-    aSelf->UpdateStringPrefs();
-  }
-
   bool mFlashBlockEnabled;
   bool mPluginsHttpOnly;
 
   nsCString mAllowTables;
   nsCString mAllowExceptionsTables;
   nsCString mDenyTables;
   nsCString mDenyExceptionsTables;
   nsCString mSubDocDenyTables;
--- a/layout/base/nsPresContext.cpp
+++ b/layout/base/nsPresContext.cpp
@@ -145,23 +145,16 @@ nsPresContext::IsDOMPaintEventPending()
     // invalidation doesn't invalidate anything further.
     NotifyInvalidation(drpc->mRefreshDriver->LastTransactionId().Next(), nsRect(0, 0, 0, 0));
     return true;
   }
   return false;
 }
 
 void
-nsPresContext::PrefChangedCallback(const char* aPrefName, nsPresContext* aPresContext)
-{
-  NS_ASSERTION(aPresContext, "bad instance data");
-  aPresContext->PreferenceChanged(aPrefName);
-}
-
-void
 nsPresContext::ForceReflowForFontInfoUpdate()
 {
   // We can trigger reflow by pretending a font.* preference has changed;
   // this is the same mechanism as gfxPlatform::ForceGlobalReflow() uses
   // if new fonts are installed during the session, for example.
   PreferenceChanged("font.internaluseonly.changed");
 }
 
@@ -316,20 +309,22 @@ nsPresContext::Destroy()
   if (mEventManager) {
     // unclear if these are needed, but can't hurt
     mEventManager->NotifyDestroyPresContext(this);
     mEventManager->SetPresContext(nullptr);
     mEventManager = nullptr;
   }
 
   // Unregister preference callbacks
-  Preferences::UnregisterPrefixCallbacks(nsPresContext::PrefChangedCallback,
-                                         gPrefixCallbackPrefs, this);
-  Preferences::UnregisterCallbacks(nsPresContext::PrefChangedCallback,
-                                   gExactCallbackPrefs, this);
+  Preferences::UnregisterPrefixCallbacks(
+    PREF_CHANGE_METHOD(nsPresContext::PreferenceChanged),
+    gPrefixCallbackPrefs, this);
+  Preferences::UnregisterCallbacks(
+    PREF_CHANGE_METHOD(nsPresContext::PreferenceChanged),
+    gExactCallbackPrefs, this);
 
   mRefreshDriver = nullptr;
 }
 
 nsPresContext::~nsPresContext()
 {
   MOZ_ASSERT(!mShell, "Presshell forgot to clear our mShell pointer");
   DetachShell();
@@ -853,20 +848,22 @@ nsPresContext::Init(nsDeviceContext* aDe
     }
 
     if (!mRefreshDriver) {
       mRefreshDriver = new nsRefreshDriver(this);
     }
   }
 
   // Register callbacks so we're notified when the preferences change
-  Preferences::RegisterPrefixCallbacks(nsPresContext::PrefChangedCallback,
-                                       gPrefixCallbackPrefs, this);
-  Preferences::RegisterCallbacks(nsPresContext::PrefChangedCallback,
-                                 gExactCallbackPrefs, this);
+  Preferences::RegisterPrefixCallbacks(
+    PREF_CHANGE_METHOD(nsPresContext::PreferenceChanged),
+    gPrefixCallbackPrefs, this);
+  Preferences::RegisterCallbacks(
+    PREF_CHANGE_METHOD(nsPresContext::PreferenceChanged),
+    gExactCallbackPrefs, this);
 
   nsresult rv = mEventManager->Init();
   NS_ENSURE_SUCCESS(rv, rv);
 
   mEventManager->SetPresContext(this);
 
 #ifdef DEBUG
   mInitialized = true;
--- a/layout/base/nsPresContext.h
+++ b/layout/base/nsPresContext.h
@@ -1193,17 +1193,16 @@ protected:
   UIResolutionChangedSubdocumentCallback(nsIDocument* aDocument, void* aData);
 
   void SetImgAnimations(nsIContent *aParent, uint16_t aMode);
   void SetSMILAnimations(nsIDocument *aDoc, uint16_t aNewMode,
                                      uint16_t aOldMode);
   void GetDocumentColorPreferences();
 
   void PreferenceChanged(const char* aPrefName);
-  static void PrefChangedCallback(const char*, nsPresContext*);
 
   void UpdateAfterPreferencesChanged();
   void DispatchPrefChangedRunnableIfNeeded();
 
   void GetUserPreferences();
 
   /**
    * Fetch the user's font preferences for the given aLanguage's
--- a/modules/libpref/Preferences.h
+++ b/modules/libpref/Preferences.h
@@ -62,16 +62,58 @@ struct TypedPrefChangeFunc
   operator PrefChangedFunc() const
   {
     return reinterpret_cast<PrefChangedFunc>(mCallback);
   }
 
   CallbackType mCallback;
 };
 
+// Similar to PrefChangedFunc, but for use with instance methods.
+//
+// Any instance method with this signature may be passed to the
+// PREF_CHANGE_METHOD macro, which will wrap it into a typesafe preference
+// callback function, which accepts a preference name as its first argument, and
+// an instance of the appropriate class as the second.
+//
+// When called, the wrapper will forward the call to the wrapped method on the
+// given instance, with the notified preference as its only argument.
+typedef void(PrefChangedMethod)(const char* aPref);
+
+namespace detail {
+// Helper to extract the instance type from any instance method. For an instance
+// method `Method = U T::*`, InstanceType<Method>::Type returns T.
+template<typename T>
+struct InstanceType;
+
+template<typename T, typename U>
+struct InstanceType<U T::*>
+{
+  using Type = T;
+};
+
+// A wrapper for a PrefChangeMethod instance method which forwards calls to the
+// wrapped method on the given instance.
+template<typename T, PrefChangedMethod T::*Method>
+void
+PrefChangeMethod(const char* aPref, T* aInst)
+{
+  ((*aInst).*Method)(aPref);
+}
+} // namespace detail
+
+// Creates a wrapper around an instance method, with the signature of
+// PrefChangedMethod, from an arbitrary class, so that it can be used as a
+// preference callback. The closure data passed to RegisterCallback must be an
+// instance of this class.
+#define PREF_CHANGE_METHOD(meth)                                               \
+  (&::mozilla::detail::PrefChangeMethod<                                       \
+    ::mozilla::detail::InstanceType<decltype(&meth)>::Type,                    \
+    &meth>)
+
 class PreferenceServiceReporter;
 
 namespace dom {
 class Pref;
 class PrefValue;
 } // namespace dom
 
 namespace ipc {