There is a mTouchActionPropertyEnabled flag in AsyncPanZoomController that is basically reflecting the value of gfxPrefs::TouchActionEnabled(). The only reason it's a separate flag is so that we can toggle it during gtests and exercise the code that is normally disabled by default. While working on bug 1009733 I found this code somewhat problematic and would like to remove it. Instead, I think we should toggle the pref directly in the gtest. I tried using the newly-added TestScopedBoolPref to accomplish this, but because the TouchActionEnabled pref is a "Once" pref, flipping it using the TestScopedBoolPref doesn't have any effect. Because of the changes in bug 1030090 I also can't just destroy and re-create the gfxPrefs object per test, because that causes leakage and random memory corruption. Instead, I went with an approach to force-update "Once" prefs and used that. Patches incoming. In the end I'm not sure this ends up being cleaner than just leaving the code as-is and modifying my approach in bug 1009733 but since I have the patches done now I figured I might as well post them and see what people think.

Comment on attachment 8453863 [details] [diff] [review] Part 2 - Remove the flag from APZC Review of attachment 8453863 [details] [diff] [review]: ----------------------------------------------------------------- I feel like in the future, if we add more prefs that we want different tests to use different values for, we may want to change the design in TestAsyncPanZoomController.cpp, so that we don't need a different Tester class for every combination - but for now I think this is fine.

Comment on attachment 8453862 [details] [diff] [review] Part 1 - Add a force-update method to gfxPrefs Review of attachment 8453862 [details] [diff] [review]: ----------------------------------------------------------------- This is a good start. We talked about having a "set value" type of method instead (that updates both the preference and the cached value), and I think there may be more cleanup that can be done once this and perhaps set show up, but that would belong in a different bug. Not a peer, but I'll give you an r+ anyway, I'm pretty comfortable in this code.

The setter approach is a lot cleaner and feels less like a test-only hack. Updating patches to use that approach instead.

Minor change to call gfxPrefs:SetTouchActionEnabled in the TouchActionEnabledTester SetUp/TearDown functions

Comment on attachment 8453936 [details] [diff] [review] Part 1 - Add pref setter functions to gfxPrefs (v2) Review of attachment 8453936 [details] [diff] [review]: ----------------------------------------------------------------- I'm also thinking that Register method can very easily (with a mIsRegistered bool class member) become Evaluate() and behave like the ForceUpdate you had before, if we still want it. ::: gfx/thebes/gfxPrefs.cpp @@ +109,5 @@ > +} > + > +void gfxPrefs::PrefSet(const char* aPref, float aValue) > +{ > + Preferences::SetFloat(aPref, aValue); Pretty sure we don't have this method. ::: gfx/thebes/gfxPrefs.h @@ +60,5 @@ > #define DECL_GFX_PREF(Update, Pref, Name, Type, Default) \ > public: \ > static Type Name() { MOZ_ASSERT(SingletonExists()); return GetSingleton().mPref##Name.mValue; } \ > +static void Set##Name(Type aVal) { MOZ_ASSERT(SingletonExists()); AssertMainThread(); \ > + GetSingleton().mPref##Name.Set(UpdatePolicy::Update, Get##Name##PrefName, aVal); } \ Is it more consistent/appropriate to use Get##Name##PrefName(), and then have the member Set method below take char*, rather than char* (*f)? @@ +104,5 @@ > break; > } > } > + void Set(UpdatePolicy aUpdate, const char* aPref(void), T aValue) > + { What if we put AssertMainThread() here, rather than in the static above? Probably worth then putting inside of Register() method as well. @@ +285,5 @@ > static float PrefGet(const char*, float); > + static void PrefSet(const char* aPref, bool aValue); > + static void PrefSet(const char* aPref, int32_t aValue); > + static void PrefSet(const char* aPref, uint32_t aValue); > + static void PrefSet(const char* aPref, float aValue); I don't think we allow float, because it isn't atomically settable, or something like that. I think it's OK to just not cover the float scenario. @@ +287,5 @@ > + static void PrefSet(const char* aPref, int32_t aValue); > + static void PrefSet(const char* aPref, uint32_t aValue); > + static void PrefSet(const char* aPref, float aValue); > + > + static void AssertMainThread(); I like this.

(In reply to Milan Sreckovic [:milan] from comment #7) > Comment on attachment 8453936 [details] [diff] [review] > Part 1 - Add pref setter functions to gfxPrefs (v2) > > Review of attachment 8453936 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm also thinking that Register method can very easily (with a mIsRegistered > bool class member) become Evaluate() and behave like the ForceUpdate you had > before, if we still want it. Ah, neat. That could be handy in the future although I think with the Set approach we don't need it at the moment. > > +{ > > + Preferences::SetFloat(aPref, aValue); > > Pretty sure we don't have this method. I saw http://mxr.mozilla.org/mozilla-central/source/modules/libpref/public/Preferences.h?rev=f537dd9a7986#352 and assumed it existed. But it looks like that function isn't implemented anywhere.. and hasn't been since the original patch landed in bug 826586! > ::: gfx/thebes/gfxPrefs.h > @@ +60,5 @@ > > #define DECL_GFX_PREF(Update, Pref, Name, Type, Default) \ > > public: \ > > static Type Name() { MOZ_ASSERT(SingletonExists()); return GetSingleton().mPref##Name.mValue; } \ > > +static void Set##Name(Type aVal) { MOZ_ASSERT(SingletonExists()); AssertMainThread(); \ > > + GetSingleton().mPref##Name.Set(UpdatePolicy::Update, Get##Name##PrefName, aVal); } \ > > Is it more consistent/appropriate to use Get##Name##PrefName(), and then > have the member Set method below take char*, rather than char* (*f)? Good point, I'll change that. > @@ +104,5 @@ > > break; > > } > > } > > + void Set(UpdatePolicy aUpdate, const char* aPref(void), T aValue) > > + { > > What if we put AssertMainThread() here, rather than in the static above? > Probably worth then putting inside of Register() method as well. Sure, it'll make things a bit more readable. > @@ +285,5 @@ > > static float PrefGet(const char*, float); > > + static void PrefSet(const char* aPref, bool aValue); > > + static void PrefSet(const char* aPref, int32_t aValue); > > + static void PrefSet(const char* aPref, uint32_t aValue); > > + static void PrefSet(const char* aPref, float aValue); > > I don't think we allow float, because it isn't atomically settable, or > something like that. I think it's OK to just not cover the float scenario. Will get rid of it. And will file a follow-up bug to get rid of the spurious declaration in Preferences.h.

Comment on attachment 8453989 [details] [diff] [review] The "Evaluate" method I mentioned in the review comments... Review of attachment 8453989 [details] [diff] [review]: ----------------------------------------------------------------- Looks reasonable. Do you want me to add it?

So actually it doesn't compile without the float version of PrefSet. I might just have to implement Preferences::SetFloat instead.

Updated to address review comments. I left the float version of the setter in because otherwise things wouldn't compile. I also added an implementation for it in bug 1037164. Don't ask me why this code would link and run fine without that implementation. :) Left out the evaluate method for now, since I don't think it's necessary.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c109e4bd68ac https://hg.mozilla.org/integration/mozilla-inbound/rev/9c78e399333b

https://hg.mozilla.org/mozilla-central/rev/c109e4bd68ac https://hg.mozilla.org/mozilla-central/rev/9c78e399333b