Things like: LogTestDataForPaint<..> std::osteram::operator<<(float) It would be better if it didn't.

These log statements were introduced in bug 961289. I've been meaning to put these log statements under a pref, as Kats suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=961289#c78. I'll use this bug for that.

This patch puts the logging of APZ test data behind a pref which is false by default, and turns on the pref while running tests.

Comment on attachment 8430893 [details] [diff] [review] bug1016573.patch Review of attachment 8430893 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.h @@ +26,5 @@ > #include "imgIContainer.h" > #include "mozilla/gfx/2D.h" > #include "Units.h" > #include "mozilla/ToString.h" > +#include "gfxPrefs.h" I could avoid adding this include here by introducing a private static method nsLayoutUtils::IsAPZTestLoggingEnabled() and having its implementation in nsLayoutUtils.cpp call gfxPrefs::APZTestLoggingEnabled(). Let me know if you think that's worth doing.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=284f6cc81028

Comment on attachment 8430893 [details] [diff] [review] bug1016573.patch Review of attachment 8430893 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/app/b2g.js @@ +951,5 @@ > pref("apz.overscroll.snap_back_accel", "0.003"); > pref("apz.overscroll.snap_back_init_vel", "1"); > > +// APZ testing (bug 961289) > +pref("apz.test.logging_enabled", false); I'd prefer putting this in all.js so it's on all platforms. I've run into a couple of problems so far with prefs not being available on particular platforms when JS code tries to read them and barfs. This pref seems like it might want to be read from similar places eventually. ::: layout/base/nsLayoutUtils.h @@ +26,5 @@ > #include "imgIContainer.h" > #include "mozilla/gfx/2D.h" > #include "Units.h" > #include "mozilla/ToString.h" > +#include "gfxPrefs.h" Can we instead just move the implementations of LogTestDataForPaint into the .cpp file? That would also allow moving the #include into the .cpp, and I think that would be preferable.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5) > Comment on attachment 8430893 [details] [diff] [review] > bug1016573.patch > > Review of attachment 8430893 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: b2g/app/b2g.js > @@ +951,5 @@ > > pref("apz.overscroll.snap_back_accel", "0.003"); > > pref("apz.overscroll.snap_back_init_vel", "1"); > > > > +// APZ testing (bug 961289) > > +pref("apz.test.logging_enabled", false); > > I'd prefer putting this in all.js so it's on all platforms. I've run into a > couple of problems so far with prefs not being available on particular > platforms when JS code tries to read them and barfs. This pref seems like it > might want to be read from similar places eventually. Makes sense. I'll do that. > ::: layout/base/nsLayoutUtils.h > @@ +26,5 @@ > > #include "imgIContainer.h" > > #include "mozilla/gfx/2D.h" > > #include "Units.h" > > #include "mozilla/ToString.h" > > +#include "gfxPrefs.h" > > Can we instead just move the implementations of LogTestDataForPaint into the > .cpp file? That would also allow moving the #include into the .cpp, and I > think that would be preferable. Not the template one.

(In reply to Botond Ballo [:botond] from comment #6) > > Can we instead just move the implementations of LogTestDataForPaint into the > > .cpp file? That would also allow moving the #include into the .cpp, and I > > think that would be preferable. > > Not the template one. Ah true. The patch is fine as-is then.

Updated patch to move the pref from b2g.js to all.js.

(In reply to Botond Ballo [:botond] from comment #3) > Comment on attachment 8430893 [details] [diff] [review] > bug1016573.patch > > Review of attachment 8430893 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/nsLayoutUtils.h > @@ +26,5 @@ > > #include "imgIContainer.h" > > #include "mozilla/gfx/2D.h" > > #include "Units.h" > > #include "mozilla/ToString.h" > > +#include "gfxPrefs.h" > > I could avoid adding this include here by introducing a private static > method nsLayoutUtils::IsAPZTestLoggingEnabled() and having its > implementation in nsLayoutUtils.cpp call gfxPrefs::APZTestLoggingEnabled(). > Let me know if you think that's worth doing. Having nsLayoutUtils.h include gfxPrefs.h is causing me very long recompiles for simple changes in gfxPrefs.h. I'd like to avoid this include after all. Filed bug 1019766.