712037 - [viewport] Insufficient logging to diagnose viewport bugs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P4)

Description

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

Many viewport-related bugs are being reported, but many of these issues are hard to reproduce, so we need more logging in the published builds to capture necessary information. This logging can be removed if/when all the bugs are fixed.

Comment 1

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

Attachment: Logging!

Comment 2

[Chris Lord [:cwiiis]]

Comment on attachment 582873 [details] [diff] [review]
Logging!

Review of attachment 582873 [details] [diff] [review]:
-----------------------------------------------------------------

While we definitely want more logging for error cases, this would make us far too verbose. It'd be impossible to debug anything else without filtering out these statements first (and I wonder if logging so frequently may have a noticeable effect on performance). Would it be possible to depend on a pref or environment variable? If not, is this really a good idea? I'm open to being persuaded.

Comment 3

[Patrick Walton (:pcwalton)]

I'm ok with this amount of logging, but could we have a more succinct description of the viewport metrics? That would cut down the chatter a bit.

Comment 4

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

Attachment: (1/2) Add a toString() on the Viewport for succinct stringification

Comment 5

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

Attachment: (2/2) Add logging for viewport and panzoom changes

As per the discussion during the gfx vidyo meeting, we really need this in to be able to diagnose these paper cut issues. I've compacted down the message so it's less intrusive and has a smaller performance impact. And we can take this out once we have the bugs figured out.

Comment 6

[Chris Lord [:cwiiis]]

Comment on attachment 582965 [details] [diff] [review]
(1/2) Add a toString() on the Viewport for succinct stringification

Review of attachment 582965 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me.

Comment 7

[Chris Lord [:cwiiis]]

Comment on attachment 582966 [details] [diff] [review]
(2/2) Add logging for viewport and panzoom changes

Review of attachment 582966 [details] [diff] [review]:
-----------------------------------------------------------------

Also looks fine. My only comment is that perhaps the logging in LayerController should also log the given parameters where appropriate (so all functions except setViewportMetrics).

Comment 8

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

Thanks for the reviews!

> Also looks fine. My only comment is that perhaps the logging in
> LayerController should also log the given parameters where appropriate (so
> all functions except setViewportMetrics).

For most of those functions the parameters just get set straight into the viewport, so the viewport dump includes the the parameters indirectly. I added the zoom factor to the scale functions though, since that might be useful.

Comment 9

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

Landed on m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/93c52bc7b68c
https://hg.mozilla.org/integration/mozilla-inbound/rev/3186b335b70b

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/93c52bc7b68c
https://hg.mozilla.org/mozilla-central/rev/3186b335b70b