973086 - [music] refactor nfc event registration code

Status: RESOLVED FIXED

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

Description

[David Flanagan [:djf]]

This is a followup to bug 948363.

That bug enabled NFC in the music app, but sets and resets the onpeerready event handler every time an event occurs in the Player module.  It is not clear to me that this code is guaranteed to clear the event handler when we leave player mode.

I think the code would be much simpler if we just modified ModeManager.start() to set the event handler when we enter player mode and clear it when we leave.

Comment 1

[David Flanagan [:djf]]

Dominic, can you take this, please?

Comment 2

[Dominic Kuo [:dkuo]]

(In reply to David Flanagan [:djf] from comment #0)
> This is a followup to bug 948363.
> 
> That bug enabled NFC in the music app, but sets and resets the onpeerready
> event handler every time an event occurs in the Player module.  It is not
> clear to me that this code is guaranteed to clear the event handler when we
> leave player mode.
> 
> I think the code would be much simpler if we just modified
> ModeManager.start() to set the event handler when we enter player mode and
> clear it when we leave.

Thanks David, I think so, too. At first I misunderstood the usage of the nfc api so I didn't aware of it might break the player while I was reviewing, and I also agree we should put the nfc logic into ModeManager, since the ModeManager knows it's in the player mode or not. Taking this.

Comment 3

[Dominic Kuo [:dkuo]]

Attachment: Put nfc event registration code in ModeManager

Jim,

This is the patch that refactor nfc event registration code. I was confused by the first patch so didn't realize the nfc event registration code was in the wrong place, would you please review it? thanks.

David,

I think the patch is simple so probably just need you feedback instead of a detail review from you, please comment if you still think there is any issues, thanks.

Comment 4

[Jim Porter (:squib)]

This looks good for the most part; I want to run the code tomorrow to be sure it doesn't break things, but I expect I'll r+ it. I have a couple of minor comments on Github, though.

Comment 5

[David Flanagan [:djf]]

Comment on attachment 8376977 [details] [review]
Put nfc event registration code in ModeManager

Looks good to me. Thanks, Dominic.

Comment 6

[Dominic Kuo [:dkuo]]

Thanks for the review and feedback from Jim and David.

Jim, I have addressed the issues that David and you commented on github, and please check it again and let me know if there is anything I need to change, thanks.

Comment 7

[Jim Porter (:squib)]

Comment on attachment 8376977 [details] [review]
Put nfc event registration code in ModeManager

Looks good!

Comment 8

[David Flanagan [:djf]]

Dominic,

Is this ready to land?

Comment 9

[Hema Koka [:hema]]

updating sprint milestone and not blocking

Comment 10

[Dominic Kuo [:dkuo]]

(In reply to David Flanagan [:djf] from comment #8)
> Dominic,
> 
> Is this ready to land?

I have addressed the issues on github and it's ready to land, though I don't have a chance to check the travis got green or not, but we should land this after 1.4 branch out, right? it's a refactor work and not a blocker.

Comment 11

[Dominic Kuo [:dkuo]]

Landed on master: 2bc2e08939d007978d7dd96c48392fec47db15fb