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

RESOLVED FIXED in 2.2 S1 (5dec)

Status

Firefox OS
NFC
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: allstars, Assigned: allstars)

Tracking

unspecified
2.2 S1 (5dec)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [p=2])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 years ago
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)

Updated

3 years ago
Assignee: nobody → allstars.chh
(Assignee)

Comment 1

3 years ago
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
(Assignee)

Updated

3 years ago
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
(Assignee)

Comment 2

3 years ago
Created attachment 8522872 [details] [diff] [review]
Patch.
(Assignee)

Updated

3 years ago
Attachment #8522872 - Flags: review?(dlee)
(Assignee)

Comment 3

3 years ago
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)
(Assignee)

Comment 5

3 years ago
(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+
(Assignee)

Comment 7

3 years ago
Created attachment 8524287 [details] [diff] [review]
Patch. v2.
Attachment #8522872 - Attachment is obsolete: true
Attachment #8524287 - Flags: review+
(Assignee)

Comment 8

3 years ago
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)

Comment 9

3 years ago
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-
(Assignee)

Comment 11

3 years ago
Created attachment 8525113 [details] [diff] [review]
Patch. v3

By smaug's suggestion I use a different approach, r? to Dimi again.
Attachment #8524287 - Attachment is obsolete: true
Attachment #8525113 - Flags: review?(dlee)

Updated

3 years ago
Attachment #8525113 - Flags: review?(dlee) → review+
(Assignee)

Comment 12

3 years ago
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)

Updated

3 years ago
Attachment #8525113 - Flags: review?(bugs) → review+
(Assignee)

Comment 13

3 years ago
Created attachment 8526577 [details] [diff] [review]
Patch. v3.

Updated patch subject.
Attachment #8525113 - Attachment is obsolete: true
Attachment #8526577 - Flags: review+
(Assignee)

Comment 14

3 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/d913bffae661
(Assignee)

Updated

3 years ago
Whiteboard: [p=2]
Target Milestone: --- → 2.2 S1 (5dec)
https://hg.mozilla.org/mozilla-central/rev/d913bffae661
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.