1034474 - B2G NFC: onpeerlost is never called

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

onpeerlost is never called, 
The line 
        gMessageManager.notifyPeerEvent(this.currentPeerAppId, NFC.NFC_PEER_EVENT_LOST);
seems to have some problem, as the this.currentPeerAppId is null.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

add reset onpeerlost callback.

Comment 3

[Dimi Lee [:dimi]]

Comment on attachment 8451429 [details] [diff] [review]
Patch.

Review of attachment 8451429 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thank you

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://hg.mozilla.org/integration/b2g-inbound/rev/daf571adfe69

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/daf571adfe69

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8451429 [details] [diff] [review]
Patch.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug 933136 

User impact if declined: 
onpeerlost cannnot be notified.

Testing completed: 
Testing on manually on device.

Risk to taking this patch (and alternatives if risky): 
Gaia app cannot receive onpeerlost callback.

String or UUID changes made by this patch:
No

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Yoshi Huang[:allstars.chh] from comment #6)
> Risk to taking this patch (and alternatives if risky): 
> Gaia app cannot receive onpeerlost callback.
> 
Sorry, the risk should be none.

Comment 8

[bhavana bajaj [:bajaj]]

(In reply to Yoshi Huang[:allstars.chh] from comment #6)
> Comment on attachment 8451429 [details] [diff] [review]
> Patch.
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #):
> Bug 933136 
> 
> User impact if declined: 
> onpeerlost cannnot be notified.
> 
> Testing completed: 
> Testing on manually on device.
> 
> Risk to taking this patch (and alternatives if risky): 
> Gaia app cannot receive onpeerlost callback.
> 
> String or UUID changes made by this patch:
> No

Hi Yoshi,

This seems to be an old regression, so not sure why we would take this on 2.0. We are taking very limited fixes on 2.0 at this point and this does not look like a blocker (2.0+), so  hoping its fine to resolve this in 2.1. 

Also can you help on understanding what the end user scenario/experience is in this case "Gaia app cannot receive onpeerlost callback." ?

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to bhavana bajaj [:bajaj] from comment #8)
> Hi Yoshi,
> 
> This seems to be an old regression, so not sure why we would take this on
> 2.0. We are taking very limited fixes on 2.0 at this point and this does not
> look like a blocker (2.0+), so  hoping its fine to resolve this in 2.1. 
>
It's already fixed in 2.1

> Also can you help on understanding what the end user scenario/experience is
> in this case "Gaia app cannot receive onpeerlost callback." ?
Actually the Gaia apps don't listen to this onpeerlost callback right now. (Therefore the problem isn't discovered in the first place). So there won't be any UX change even without this patch checkin.

But from engineering side I'd suggest we should checkin this patch.
In the case that if some certified app will need this callback to know the other device is already out of range.
Also we provide a onpeerlost callback in our Web NFC API, however it would be a problem if the implementation doesn't support this.

Comment 10

[bhavana bajaj [:bajaj]]

(In reply to Yoshi Huang[:allstars.chh] from comment #9)
> (In reply to bhavana bajaj [:bajaj] from comment #8)
> > Hi Yoshi,
> > 
> > This seems to be an old regression, so not sure why we would take this on
> > 2.0. We are taking very limited fixes on 2.0 at this point and this does not
> > look like a blocker (2.0+), so  hoping its fine to resolve this in 2.1. 
> >
> It's already fixed in 2.1
> 
> > Also can you help on understanding what the end user scenario/experience is
> > in this case "Gaia app cannot receive onpeerlost callback." ?
> Actually the Gaia apps don't listen to this onpeerlost callback right now.
> (Therefore the problem isn't discovered in the first place). So there won't
> be any UX change even without this patch checkin.
> 
> But from engineering side I'd suggest we should checkin this patch.
> In the case that if some certified app will need this callback to know the
> other device is already out of range.
> Also we provide a onpeerlost callback in our Web NFC API, however it would
> be a problem if the implementation doesn't support this.

I don't think this is blocking a critical end-user case. Might be good to fix it up from engg side, but the user impact has to be really high and basically something we would stop ship the release for, to land anything on 2.0 at this point.

This is fixed in 2.1 so it will get out next release.