1058490 - B2G NFC: Add a sessionHelper in Chrome Process

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

We could have a SessionHelper that could help us know whether this session is for a Tag or for a Device.

Comment 1

[Krzysztof Mioduszewski[:tauzen]]

I assume this should be a stateless helper. It might have one method which would return session type tag/device/unknown or two methods isTag/isDevice returning true/false. Both solutions would take message from NfcService as the argument. 

Yoshi is this proposition ok with you?

Comment 2

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

Attachment: Patch

ah, sorry, I already have some WIP patch and is about to be finished.
So taking this bug back

Comment 3

[Dimi Lee [:dimi]]

Comment on attachment 8487065 [details] [diff] [review]
Patch

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

Just one question for this patch, others looks good to me

::: dom/nfc/gonk/Nfc.js
@@ +333,5 @@
> +  },
> +
> +  unregisterSession: function unregisterSession(sessionId) {
> +    if (sessionId != this.id) {
> +      dump("Unpaired sessionId: " + sessionId + "current Id:" + this.id);

should we return here or it is your intend to just print debug message ?

Comment 4

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

Attachment: Patch v2.

As discussed with Dimi,
SessionHelper should be able to handle multiple current sessions, updated patch to meet this requirement

Comment 5

[Dimi Lee [:dimi]]

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

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

Hi Yoshi,
I would like to discuss something about getCurrentToken. For multiple session case, 
we will only compare one of the token in tokenMap for like // Sanity check on sessionToken & NFC:CheckSessionToken

Is that the behavior we want ?

::: dom/nfc/gonk/Nfc.js
@@ +343,5 @@
> +    for (let id in this.tokenMap) {
> +      if (this.tokenMap[id]) {
> +        return this.tokenMap[id].token;
> +      }
> +    }

could be simplified using |for each|
for each (let item in this.tokenMap) {
  if (item) {   <-- unless it is possible to have null value in tokenMap
    return item.token;
  }
}

same as below

@@ +358,5 @@
> +    return 0;
> +  },
> +
> +  isP2PSession: function isP2PSession(id) {
> +    return (this.tokenMap[id] != null) && this.tokenMap[id].isP2P;

suggest use |this.tokenMap[id] && this.tokenMap[id].isP2P| because of it will be a little bit more intuitive

Comment 6

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

(In reply to Dimi Lee[:dimi][:dlee] from comment #5)
> Comment on attachment 8493647 [details] [diff] [review]
> Patch v2.
> 
> Review of attachment 8493647 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Yoshi,
> I would like to discuss something about getCurrentToken. For multiple
> session case, 
> we will only compare one of the token in tokenMap for like // Sanity check
> on sessionToken & NFC:CheckSessionToken
> 
> Is that the behavior we want ?
> 
I think I'll change getCurrentToken to
- IsSessionValid(sessionToken) to check the sessionToken is still valid.
- HasOngoingSession to check currently there's ongoing session right now.


> ::: dom/nfc/gonk/Nfc.js
> @@ +343,5 @@
> > +    for (let id in this.tokenMap) {
> > +      if (this.tokenMap[id]) {
> > +        return this.tokenMap[id].token;
> > +      }
> > +    }
> 
> could be simplified using |for each|
> for each (let item in this.tokenMap) {
>   if (item) {   <-- unless it is possible to have null value in tokenMap
>     return item.token;
>   }
> }
> 
For each is drecated now.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for_each...in


> same as below
> 
> @@ +358,5 @@
> > +    return 0;
> > +  },
> > +
> > +  isP2PSession: function isP2PSession(id) {
> > +    return (this.tokenMap[id] != null) && this.tokenMap[id].isP2P;
> 
> suggest use |this.tokenMap[id] && this.tokenMap[id].isP2P| because of it
> will be a little bit more intuitive

if this.tokenMap[id] is undefined then this function will return 'undefined', not 'false'.
So I added "!= null" here

Comment 7

[Dimi Lee [:dimi]]

(In reply to Yoshi Huang[:allstars.chh] from comment #6)
> (In reply to Dimi Lee[:dimi][:dlee] from comment #5)
> > Comment on attachment 8493647 [details] [diff] [review]
> > Patch v2.
> > 
> > Review of attachment 8493647 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Hi Yoshi,
> > I would like to discuss something about getCurrentToken. For multiple
> > session case, 
> > we will only compare one of the token in tokenMap for like // Sanity check
> > on sessionToken & NFC:CheckSessionToken
> > 
> > Is that the behavior we want ?
> > 
> I think I'll change getCurrentToken to
> - IsSessionValid(sessionToken) to check the sessionToken is still valid.
> - HasOngoingSession to check currently there's ongoing session right now.
> 
> 
> > ::: dom/nfc/gonk/Nfc.js
> > @@ +343,5 @@
> > > +    for (let id in this.tokenMap) {
> > > +      if (this.tokenMap[id]) {
> > > +        return this.tokenMap[id].token;
> > > +      }
> > > +    }
> > 
> > could be simplified using |for each|
> > for each (let item in this.tokenMap) {
> >   if (item) {   <-- unless it is possible to have null value in tokenMap
> >     return item.token;
> >   }
> > }
> > 
> For each is drecated now.
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/
> for_each...in
Thanks for the info

Comment 8

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

Attachment: patch v3.

Comment 9

[Dimi Lee [:dimi]]

Comment on attachment 8494333 [details] [diff] [review]
patch v3.

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

Looks good, thanks

Comment 10

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

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

Try:
https://tbpl.mozilla.org/?tree=Try&rev=3da036abe433

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3a637a2dc1b0