Closed Bug 1097527 Opened 10 years ago Closed 9 years ago

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

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S1 (5dec)

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

Details

(Whiteboard: [p=2])

Attachments

(1 file, 3 obsolete files)

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.
Assignee: nobody → allstars.chh
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.
Summary: ontag/peerlost shouldn't be fired if App doesn't listen ontagfound/peerfound → ontag/peerlost shouldn't be fired if App doesn't receive ontagfound/peerfound
Summary: ontag/peerlost shouldn't be fired if App doesn't receive ontagfound/peerfound → ontag/peerlost shouldn't be fired if App doesn't listen or receive ontagfound/peerfound
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 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.
Attachment #8522872 - Flags: review?(dlee)
(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 on attachment 8522872 [details] [diff] [review]
Patch.

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

r+ because of question answered
Attachment #8522872 - Flags: review+
Attached patch Patch. v2. (obsolete) — Splinter Review
Attachment #8522872 - Attachment is obsolete: true
Attachment #8524287 - Flags: review+
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
Attachment #8524287 - Flags: review?(bugs)
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 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)
Attachment #8524287 - Flags: review?(bugs) → review-
Attached patch Patch. v3 (obsolete) — Splinter Review
By smaug's suggestion I use a different approach, r? to Dimi again.
Attachment #8524287 - Attachment is obsolete: true
Attachment #8525113 - Flags: review?(dlee)
Attachment #8525113 - Flags: review?(dlee) → review+
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).
Attachment #8525113 - Flags: review?(bugs)
Attachment #8525113 - Flags: review?(bugs) → review+
Attached patch Patch. v3.Splinter Review
Updated patch subject.
Attachment #8525113 - Attachment is obsolete: true
Attachment #8526577 - Flags: review+
Whiteboard: [p=2]
Target Milestone: --- → 2.2 S1 (5dec)
https://hg.mozilla.org/mozilla-central/rev/d913bffae661
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.