B2G NFC: Add a sessionHelper in Chrome Process

RESOLVED FIXED in 2.1 S5 (26sep)

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: allstars.chh, Unassigned)

Tracking

unspecified
2.1 S5 (26sep)
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [p=2])

Attachments

(1 attachment, 2 obsolete attachments)

We could have a SessionHelper that could help us know whether this session is for a Tag or for a Device.
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?
Assignee: nobody → kmioduszewski
Blocks: 1003268
Flags: needinfo?(allstars.chh)
Created attachment 8487065 [details] [diff] [review]
Patch

ah, sorry, I already have some WIP patch and is about to be finished.
So taking this bug back
Assignee: kmioduszewski → allstars.chh
Flags: needinfo?(allstars.chh)
(Assignee)

Updated

4 years ago
Attachment #8487065 - Attachment description: WIP Patch → Patch
Attachment #8487065 - Flags: review?(dlee)
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 ?
Attachment #8487065 - Flags: review?(dlee) → review+
(Assignee)

Updated

4 years ago
Blocks: 1069177
Created attachment 8493647 [details] [diff] [review]
Patch v2.

As discussed with Dimi,
SessionHelper should be able to handle multiple current sessions, updated patch to meet this requirement
Attachment #8487065 - Attachment is obsolete: true
Attachment #8493647 - Flags: review?(dlee)
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
Attachment #8493647 - Flags: review?(dlee)
(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
(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
Created attachment 8494333 [details] [diff] [review]
patch v3.
Attachment #8493647 - Attachment is obsolete: true
Attachment #8494333 - Flags: review?(dlee)
Comment on attachment 8494333 [details] [diff] [review]
patch v3.

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

Looks good, thanks
Attachment #8494333 - Flags: review?(dlee) → review+
(Assignee)

Updated

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