901587 - Remove mouse_event_shim.js from fm radio app

Status: RESOLVED FIXED

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

Description

[Kevin Grandon :kgrandon]

Refer to bug 861735 ,

We'd better remove dependency of this mouse_event_shim.js for below reasons.
1. prevent hidden bug in the future.
2. improve load time and responsiveness
3. gecko already dispatch events by itself.

Comment 1

[Kevin Grandon :kgrandon]

Attachment: Github pull request pointer

Djf - maybe you could review this one? It appears we already had touch event support here, just renaming the events.

Comment 2

[David Flanagan [:djf]]

Kevin,

I'm not at all familiar with this app, so the review is going to take some more time. I've been at a team workweek, and have vacation early next week, so it will be at least Wednesday. Sorry.

I took a quick look and see that this app uses the typical mouse event pattern of registering the mousemove and mouseup handlers in mousedown and unregistering them later.  The only reason to do that is that mouse drags can leave the element they started in and we need to capture all mousemove events.

But touch events are guaranteed to always be delivered to the element that the touchstart went to, so there's an opportunity here to simplify the code and just register touchstart/touchmove/touchend statically without the dynamic adding and removing of listeners on each touch.

Since this isn't a blocker that has to land right away, I'd prefer to see a larger patch that includes this simplification.  

So basically, I guess I'm saying that I'm inclined to give an r- here, but haven't looked carefully enough to be sure. 

setting needinfo just so you see this.

Comment 3

[Kevin Grandon :kgrandon]

Comment on attachment 785827 [details]
Github pull request pointer

Hi Dave,

Thanks for the info - Sorry, I must've mis-read the owners, but I basically agree with you. I'll see if it makes sense to simplify this at all, and if we have time to do so. Clearing review flag for now.

Thanks!

Comment 4

[Kevin Grandon :kgrandon]

Comment on attachment 785827 [details]
Github pull request pointer

Hi Tim - 

Module page has you listed as an owner of this app, so adding you as a reviewer for this.

Dave had raised some points about cleaning up the logic in the app now that we don't need this shim. I took a look, and it doesn't seem like there's a *lot* that we can clean up, but we could possibly rework the event listeners a bit.

Anyway, I'd like to land as-is for the slight performance gain and perform a follow-up if you think necessary.

Thanks!

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 785827 [details]
Github pull request pointer

It would stop the app from working in B2G/Desktop or Simulator right? Not sure if that is acceptable or not...

I also agree w/ djf that the app itself need a follow-up rework.

Comment 6

[Kevin Grandon :kgrandon]

Everything should still be fine on desktop as we should now be able to support touch events. Thanks tim!

Landed in master: https://github.com/mozilla-b2g/gaia/commit/56e58399087828c7da5e1a408cc7c17efbcfc888