1005880 - [Homescreen] Velocity calculation yields NaN when called twice for the same timestamp.

Status: RESOLVED FIXED

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

Description

[Jan Keromnes [:janx]]

In some conditions, velocity calculations are requested so often that two of them can happen at the same timestamp, causing a division by 0.

The visual effect of this bug is that when panning slowly, the homescreen occasionally jumps back to original position while the user is still panning.

Comment 1

[Jan Keromnes [:janx]]

Attachment: pull request

Comment 2

[Jan Keromnes [:janx]]

Comment on attachment 8417364 [details] [review]
pull request

Hi David, since you originally wrote this code I'm sending the review to you.

Context:
- I'm working on a tool to show touch interaction with a device's screen in bug 965293
- We are conditionally disabling the touchmove fast pass in bug 985886, so that touchmove events on the homescreen can be tracked and shown on the screen
- Together with the minimal delays my tool introduces in the handling of touch events, this causes some velocity calculations to happen on the same timestamps, provoking divide-by-zero exceptions and erratic homescreen behavior (snapping back to original position while the user is still panning).

My bailout fixes that.

Comment 3

[David Flanagan [:djf]]

Comment on attachment 8417364 [details] [review]
pull request

Returning from calculateVelocity early seems fine when dt is 0.

But removing the call to calcuateVelocity in the !enabled case is not okay and will cause problems for code that calls getVelocity(), so please put that line back in before landing.

Also, I'm setting feedback? for Vivien so that he is aware of this change. This is my code, and I'm comfortable reviewing it, but I haven't touched the homescreen app for over a year, and shouldn't be the only one who knows about this change.

Comment 4

[David Flanagan [:djf]]

Also, the travis build is failing. I tried to restart it but it failed again.  I don't think it is because of your patch. Maybe the gaia-try build is okay, but I don't know how to find that for this PR.  In any case, just remember to ensure that the tests pass before landing.

Comment 5

[Jan Keromnes [:janx]]

Thanks for the review David!

I added back the call to `calcuateVelocity(evt)`, and will see that Travis is happy about the change before landing.

Also, I briefly talked to Vivien about this change already, but thank you for flagging him to make sure.

Comment 6

[Jan Keromnes [:janx]]

Travis was happy: https://travis-ci.org/mozilla-b2g/gaia/builds/25193266

Merged: https://github.com/mozilla-b2g/gaia/commit/44e903034

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8417364 [details] [review]
pull request

(In reply to David Flanagan [:djf] from comment #3)
> Comment on attachment 8417364 [details] [review]
> pull request
> 
> Returning from calculateVelocity early seems fine when dt is 0.
> 
> But removing the call to calcuateVelocity in the !enabled case is not okay
> and will cause problems for code that calls getVelocity(), so please put
> that line back in before landing.
> 
> Also, I'm setting feedback? for Vivien so that he is aware of this change.
> This is my code, and I'm comfortable reviewing it, but I haven't touched the
> homescreen app for over a year, and shouldn't be the only one who knows
> about this change.

Hey David. FWIW I sit very often next to Jan and so I have seen this code change already ;)