Closed
Bug 1016573
Opened 11 years ago
Closed 11 years ago
Put logging of APZ test data behind a pref
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: jrmuizel, Assigned: botond)
References
Details
Attachments
(1 file, 1 obsolete file)
12.31 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
Things like:
LogTestDataForPaint<..>
std::osteram::operator<<(float)
It would be better if it didn't.
Assignee | ||
Comment 1•11 years ago
|
||
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
Assignee | ||
Comment 2•11 years ago
|
||
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)
Assignee | ||
Comment 3•11 years ago
|
||
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.
Assignee | ||
Comment 4•11 years ago
|
||
green try |
Comment 5•11 years ago
|
||
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+
Assignee | ||
Comment 6•11 years ago
|
||
(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.
Comment 7•11 years ago
|
||
(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.
Assignee | ||
Comment 8•11 years ago
|
||
Updated patch to move the pref from b2g.js to all.js.
Attachment #8430893 -
Attachment is obsolete: true
Attachment #8430973 -
Flags: review+
Assignee | ||
Comment 9•11 years ago
|
||
landing |
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Assignee | ||
Comment 11•11 years ago
|
||
(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.
Description
•