1233715 - tag_visibility_monitor_shared.js doesn't wait long enough for scroll events

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Shared, defect)

Description

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

The test in tag_visibility_monitor_shared.js has a TimeForScrollAndVisibilityMonitorEvents delay which it uses to wait for scroll events to be dispatched. However, in bug 1209970 we are changing slightly how the scroll event is dispatched, so that it happens once per refresh driver tick. This is going to help avoid unnecessary relayouts and should generally make things smoother. The test in question does a bunch of scroll changes, and expects the scroll event to be dispatched immediately after each operation. With the platform changes though, we'll batch up the operations and fire the scroll event less frequently. This results in a test failure.

To fix the test, we need to either:
(a) Increase TimeForScrollAndVisibilityMonitorEvents to something > 16ms
(b) Listen for scroll events instead of having a fixed timeout
(c) Maybe do something like a rAF instead

I've tested (a) and (b) and they both work, I haven't tested (c) but if that's the preferred option I can do that instead.

Comment 1

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

Kevin, any thoughts on which of the above approaches is preferred? Feel free to redirect the needinfo - picking you since you were the last one to touch the file, even though it was just moving stuff around.

Comment 2

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

Actually the rAF approach takes too long, it causes the test time to exceed 10s. I'll make a pull request for (b) which seems the cleanest.

Comment 3

[GitHub Autolander Bot]

Attachment: [gaia] staktrace:scrolltick > mozilla-b2g:master

Comment 4

[Kevin Grandon :kgrandon]

Comment on attachment 8700022 [details] [review]
[gaia] staktrace:scrolltick > mozilla-b2g:master

This is probably something that David should probably review, but since this is only a test change, I'm fine leaving a R+ here to get you unblocked. I do think it would be good to have David audit this though, either before or after landing. Thanks!

Comment 5

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

Thanks!

Comment 6

[GitHub Autolander Bot]

https://github.com/mozilla-b2g/gaia/pull/33621

The pull request could not be applied to the integration branch. Please try again after current integration is complete. You may need to rebase your branch against the target branch.

Comment 7

[[:fabrice] Fabrice Desré]

https://github.com/mozilla-b2g/gaia/commit/ba6fd79d4b3142dae8c83db82d2f458dcb99cd77

Comment 8

[David Flanagan [:djf]]

Comment on attachment 8700022 [details] [review]
[gaia] staktrace:scrolltick > mozilla-b2g:master

Thanks for bringing this to my attention, Kevin. Looks okay to me.

I wrote the original visibility_monitor.js code, but it is no longer in use the current tag_visiblity_monitor.js was written by an intern who is long gone, and it is only used in the contacts app. (And I'd guess that it will eventually be replaced by gaia-fast-list or something similar). Anyway, my point is that I don't know much about this.

Comment 9

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/ba6fd79d4b3142dae8c83db82d2f458dcb99cd77

Comment 10

[yifan [:yifan][:yliao]]

[Blocking Requested - why for this release]:

v2.5 will also need this patch to fix unit test failure.

Comment 11

[Fischer [:Fischer]]

@ Kartikaya,
Could this affect Gaia v2.5 ?
If yes, it may need to be uplifted v2.5.
Could you please take a look ?
Thank you

Comment 12

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

The pre-existing problem with the test could affect 2.5, sure, but it should be pretty rare I think. Do you have data on how frequently this test is failing?

Comment 13

[Carsten Book [:Tomcat]]

mahe, josh: seems this need approval for 2.5

Comment 14

[Josh Cheng [:josh]]

Hi YiFan,
Could you help to answer comment 12 and assess whether we need this for 2.5 TV?

Comment 15

[yifan [:yifan][:yliao]]

From the test links in the merged pull requests for v2.5 ( https://github.com/mozilla-b2g/gaia/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Amerged+base%3Av2.5 ), it seems to fail in every GU19 test.

Comment 16

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

Hm, ok. I'm not convinced this patch will fix that problem but it shouldn't hurt to try. You can uplift it to 2.5 with a=test-only.

Comment 17

[Mahendra Potharaju [:mahe]]

Cleanring NI