Closed Bug 1054217 Opened 8 years ago Closed 8 years ago

B2G NFC: Remove targetsBySessionTokens

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S3 (29aug)

People

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

References

Details

(Whiteboard: [p=1])

Attachments

(4 files, 3 obsolete files)

When WebNFC (Bug 674741) landed it uses some pattern from RIL like _registerMessage, _unregisterTarget and _sendTargetMessage.
And the reason why RIL uses this pattern multiple-apps should be notified when some unsolicitied response happens.

However this is not the case for NFC, for example the onpeerready is notified to the app which System app tells Gecko.

So those unnecessary things should be removed.
Comment on attachment 8473621 [details] [diff] [review]
Part 1: Add onPeerLost in gMessageManager

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

::: dom/nfc/gonk/Nfc.js
@@ +299,5 @@
> +      // For peerlost, the message is delievered to the target which
> +      // registered onpeerready and onpeerready has been called before.
> +      if (this.isPeerReadyTarget(appId) && this.isPeerReadyCalled(appId)) {
> +        this.notifyPeerEvent(appId, NFC.NFC_PEER_EVENT_LOST, sessionToken);
> +        this.currentPeerAppId = null;

one question, should we clear |currentPeerAppId| whenever onPeerLost is called or only above if case is true ?
(In reply to Dimi Lee[:dimi][:dlee] from comment #5)
> ::: dom/nfc/gonk/Nfc.js
> @@ +299,5 @@
> > +      // For peerlost, the message is delievered to the target which
> > +      // registered onpeerready and onpeerready has been called before.
> > +      if (this.isPeerReadyTarget(appId) && this.isPeerReadyCalled(appId)) {
> > +        this.notifyPeerEvent(appId, NFC.NFC_PEER_EVENT_LOST, sessionToken);
> > +        this.currentPeerAppId = null;
> 
> one question, should we clear |currentPeerAppId| whenever onPeerLost is
> called or only above if case is true ?

Currently we only know a session is lost, we don't know if the session is for P2P or for a tag. And this.currentPeerAppId should be reset to null only for a P2P session.

It's in my TODO list to keep the session information, which is a MUST otherwise we don't know a taglost or a peerlost should be fired.
Attachment #8473621 - Flags: review?(dlee) → review+
Comment on attachment 8473622 [details] [diff] [review]
Part 2: remove targetsBySessionTokens

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

Looks good to me, thanks!

::: dom/nfc/gonk/Nfc.js
@@ +269,4 @@
>        }
>  
>        switch (msg.name) {
>          case "NFC:SetSessionToken":

Just confused about the naming of "SetSessionToken", now it seems we only check session.
We could rename 'Set' to 'Check' if you think it is more appropriate

@@ +273,5 @@
>            if (msg.json.sessionToken !== this.nfc.sessionTokenMap[this.nfc._currentSessionId]) {
>              debug("Received invalid Session Token: " + msg.json.sessionToken + " - Do not register this target");
>              return NFC.NFC_ERROR_BAD_SESSION_ID;
>            }
> +          debug("Registering target for this SessionToken : " + msg.json.sessionToken);

We don't do register target now
Attachment #8473622 - Flags: review?(dlee) → review+
Comment on attachment 8473623 [details] [diff] [review]
Part 3: refine Nfc.receiveMessage

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

::: dom/nfc/gonk/Nfc.js
@@ +508,5 @@
>        this.sendNfcErrorResponse(message, NFC.NFC_ERROR_BAD_SESSION_ID);
>        return null;
>      }
>  
> +    this.targetsByRequestId[message.json.requestId] = message.target;

delete this.targetsByRequestId[message.json.requestId]; for default case or
move this after switch.
Attachment #8473623 - Flags: review?(dlee) → review+
Because renaming involves changing UUID, so I created another part for this change.
Attachment #8474463 - Flags: review?(dlee)
move to after switch.
Attachment #8473623 - Attachment is obsolete: true
Attachment #8474464 - Flags: review+
Attachment #8474463 - Flags: review?(dlee) → review+
typo of the bug id in the subject.
Attachment #8474463 - Attachment is obsolete: true
Attachment #8474495 - Flags: review+
You need to log in before you can comment on or make changes to this bug.