Closed Bug 1016573 Opened 6 years ago Closed 6 years ago

Put logging of APZ test data behind a pref

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: jrmuizel, Assigned: botond)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 961289
Summary: Logging shows up in profiles of painting → Put logging of APZ test data behind a pref
Attached patch bug1016573.patch (obsolete) — Splinter Review
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.
Attachment #8430893 - Flags: review?(bugmail.mozilla)
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 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.
Attachment #8430893 - Flags: review?(bugmail.mozilla) → review+
(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.
Attached patch bug1016573.patchSplinter Review
Updated patch to move the pref from b2g.js to all.js.
Attachment #8430893 - Attachment is obsolete: true
Attachment #8430973 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9164a6ab85b2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(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.
You need to log in before you can comment on or make changes to this bug.