Closed Bug 972742 Opened 10 years ago Closed 10 years ago

[Music] Handle nfc api correctly for not breaking the player

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: dkuo, Assigned: dkuo)

References

Details

(Keywords: regression, smoketest, Whiteboard: [CR 618879])

Attachments

(1 file)

The logic of checking the nfc api is not correctly handled, we need to fix it or it has broken the player. The root cause is found and I am working on it.
Jim/Punam,

The nfc patch in bug 948363 broke the music player if any gecko branch does not have mozNfc api(like firefox nightly), because navigator.mozNfc is not always existed. Actually the patch is pretty trivial, and to be more understandable/maintainable I also did a little code rearrangement, can one of you review this and probably land it to quickly fix the broken player on master? thanks a lot!
Attachment #8376339 - Flags: review?(squibblyflabbetydoo)
Attachment #8376339 - Flags: review?(pdahiya)
Blocks: 972789
Comment on attachment 8376339 [details] [review]
Handle mozNfc api correctly

Thanks Dominic for the fix. I have tested the patch on master and it fixes the error JavaScript Error:
>> "TypeError: navigator.mozNfc is undefined" {file:
>> "app://music.gaiamobile.org/js/Player.js" line: 876}]

in logs and the play and share button are functional with this fix. Its r+ from me. I will run it by Jim also as he is more familiar with music app and than land fix on master.
Attachment #8376339 - Flags: review?(pdahiya) → review+
Dominque, if this patch fixes Bug 972789, please land get this landed asap onto trunk.  it is causing a smoketest blocker to fail.   

Thanks.
Flags: needinfo?(dkuo)
Comment on attachment 8376339 [details] [review]
Handle mozNfc api correctly

Stealing this review from Jim since Punam asked me to look and since Tony is very eager to get this landed.

I'm giving r+ because we need to fix the smoketests.

But I'd give r- otherwise because it seems really inefficient to set onpeerready on every single event that occurs on the player.  Also, it is not entirely clear to me that onpeerready ever actually gets cleared. It would depend on whether any events are delivered to Player.handleEvents() after it is no longer the current mode.

Let's file a followup bug and move the onpeerready setting and clearing code to ModeManager.start()
Attachment #8376339 - Flags: review?(squibblyflabbetydoo) → review+
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
No longer blocks: 972789
Filed bug 973086 as a followup.
(In reply to Tony Chung [:tchung] from comment #3)
> Dominque, if this patch fixes Bug 972789, please land get this landed asap
> onto trunk.  it is causing a smoketest blocker to fail.   
> 
> Thanks.

I think it did fixed bug 972789 and it was landed by Punam, sorry about breaking the smoketest.
Flags: needinfo?(dkuo)
Whiteboard: [CR 618879]
Verified fixed.

Bug 972789 no longer reproduces, able to play a song, pause, skip, return to previous...

BuildID: 20140219040204
Gaia: ac06cfbd2baf6494ffbb668cc599e3892cd5e17b
Gecko: bf0e76f2a7d4
Version: 30.0a1
v1.2-devices.cfg
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: