1097527 - ontag/peerlost shouldn't be fired if App doesn't listen or receive ontagfound/peerfound

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

If the app doesn't listen to ontagfound/peerfound event, when the tag/device is removed, we shouldn't fire ontag/peerlost to the app.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Even the app has listened to onpeer/tagfound, but it is still possible that app doesn't receive this because it's in background, (Bug 1082440), in this case ontag/peerlost shouldn't be fired neither.

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Patch.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

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

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

::: dom/nfc/nsNfc.js
@@ +335,5 @@
>      }
>  
> +    // If this callback is used to notify onpeerready, but content doesn't register
> +    // onpeerready, bail out. And same for onpeerfound.
> +    if ((isPeerReady && (this.events.indexOf("peerready") == -1)) ||

This line is not neccesary, since notifyPeerFound with isPeerReady is true will only be called when Content has registered onpeerready, I'll remove this check.

Comment 4

[Dimi Lee [:dimi]]

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

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

::: dom/nfc/nsNfc.js
@@ +5,5 @@
>  /* Copyright © 2013, Deutsche Telekom, Inc. */
>  
>  "use strict";
>  
> +const DEBUG = true;

Is this intentional ?

@@ +237,5 @@
>      });
>    },
>  
> +  eventListenerWasAdded: function eventListenerWasAdded(eventType) {
> +    if (eventType == "tagfound" || eventType == "peerfound" || eventType == "peerready") {

Could consider
if (["tagfound", "peerfound", "peerready"].indexOf(eventType) != -1) {

@@ +250,5 @@
>      this._nfcContentHelper.registerTargetForPeerReady(appId);
>    },
>  
> +  eventListenerWasRemoved: function eventListenerWasRemoved(eventType) {
> +    if (eventType == "tagfound" || eventType == "peerfound" || evevtType == "peerready") {

I think this 'if' check could be removed

@@ +321,4 @@
>      }
>  
> +    this.nfcTag.isLost = true;
> +    this.nfcTag = null;

We don't need to check sessionToken now ? same question for peer lost.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

(In reply to Dimi Lee[:dimi][:dlee] from comment #4)
> > +    this.nfcTag.isLost = true;
> > +    this.nfcTag = null;
> We don't need to check sessionToken now ? same question for peer lost.

Right now this API only supports connecting one tag at the same time, if this API could support multiple tags at the same time, ontaglost should have a tag parameter to indicate which tag is lost. However We didn't support this yet so I removed those sessionToken check.

Comment 6

[Dimi Lee [:dimi]]

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

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

r+ because of question answered

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Patch. v2.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Comment on attachment 8524287 [details] [diff] [review]
Patch. v2.

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

Add r? to smaug for the eventListenerWasAdded

Hi, Smaug
In this patch I just check the event listener in eventListenerWasAdded, however I am not sure if checking 
setting the event handler in [1] is also neccesary.

Will it be possible that the setter of event handler is called but not for eventListenerWasAdded?

Thanks

[1] http://lxr.mozilla.org/mozilla-central/source/dom/nfc/nsNfc.js#246

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Well, setting an event handler which was set earlier to some valid value to null will remove the handler/listener and EventListenerWasRemoved will be called.

So eventListenerWasAdded/eventListenerWasRemoved catch all the cases when a listener/handler is added or removed, and event handler stuff is only for setting/getting onfoo property.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8524287 [details] [diff] [review]
Patch. v2.

You shouldn't need this.events array.
You could just do
Cc["@mozilla.org/eventlistenerservice;1"].getService(Ci.nsIEventListenerService)
                                         .hasListenerFor(this.__DOM_IMPL__, eventname);


(We could perhaps add hasEventListenerFor as a [ChromeOnly] method to EventTarget if that ends up being useful in many places)

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Patch. v3

By smaug's suggestion I use a different approach, r? to Dimi again.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Comment on attachment 8525113 [details] [diff] [review]
Patch. v3

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

Hi, smaug
I've used eventListenerService by your suggestion.
Could you help to review this again ?

Thanks

(I noticed that subject of patch is scamble, I'll update this again).

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Patch. v3.

Updated patch subject.

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

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

Comment 15

[Carsten Book [:Tomcat]]

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