1054217 - B2G NFC: Remove targetsBySessionTokens

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

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 2

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

Attachment: Part 1: Add onPeerLost in gMessageManager

Comment 3

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

Attachment: Part 2: remove targetsBySessionTokens

Comment 4

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

Attachment: Part 3: refine Nfc.receiveMessage

Comment 5

[Dimi Lee [:dimi]]

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 ?

Comment 6

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

(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.

Comment 7

[Dimi Lee [:dimi]]

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

Comment 8

[Dimi Lee [:dimi]]

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.

Comment 9

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

Attachment: Part 3: rename setSessionToken to checkSessionToken.

Because renaming involves changing UUID, so I created another part for this change.

Comment 10

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

Attachment: Part 4: refine Nfc.receiveMessage v2. (former Part 3)

move to after switch.

Comment 11

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

Attachment: Part 2: remove targetsBySessionTokens v2.

rebase

Comment 12

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

Attachment: Part 3: rename setSessionToken to checkSessionToken.

typo of the bug id in the subject.

Comment 13

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

https://hg.mozilla.org/integration/b2g-inbound/rev/fc5e2e27ebd5
https://hg.mozilla.org/integration/b2g-inbound/rev/772e7313b9c0
https://hg.mozilla.org/integration/b2g-inbound/rev/8b12a86faf74
https://hg.mozilla.org/integration/b2g-inbound/rev/b6b405a253d9

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fc5e2e27ebd5
https://hg.mozilla.org/mozilla-central/rev/772e7313b9c0
https://hg.mozilla.org/mozilla-central/rev/8b12a86faf74
https://hg.mozilla.org/mozilla-central/rev/b6b405a253d9