1016573 - Put logging of APZ test data behind a pref

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Things like:
  LogTestDataForPaint<..>
  std::osteram::operator<<(float)

It would be better if it didn't.

Comment 1

[Botond Ballo [:botond]]

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.

Comment 2

[Botond Ballo [:botond]]

Attachment: bug1016573.patch

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 3

[Botond Ballo [:botond]]

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.

Comment 4

[Botond Ballo [:botond]]

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

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 6

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 8

[Botond Ballo [:botond]]

Attachment: bug1016573.patch

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

Comment 9

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9164a6ab85b2

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9164a6ab85b2

Comment 11

[Botond Ballo [:botond]]

(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.