963556 - [B2G][NFC] Create shared library for NFC

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Garner Lee]

NFC enabled apps are likely to need a basic, but largely similar needs to manipulate NDEF Records in tag and P2P read and write scenarios. Create a shared javascript library file for apps to use.

Comment 2

[Ken Chang[:kenkai]]

Thanks, Garner. I think Michal need this lib for developing contacts sharing. 
So when do you plan to finish this?

Comment 3

[Garner Lee]

A library would need to be defined for a basic subset useful for all apps, but I can get a first pass out for comments by the certified app module owners.

Comment 4

[Garner Lee]

Attachment: 0001-WIP-Update-system-and-nfc-demo-app-to-use-new-shared.patch

WIP refactoring patch. nfc_util.js is now shared for all apps to use, and nfc_manager_util.js has additional manager and handover specific functions. 

Device handover unit testing needed. I have no NFC handover type devices currently.

Comment 5

[Garner Lee]

Attachment: (v2) 0001-WIP-Update-system-and-nfc-demo-app-to-use-new-shared.patch

Update. Made NDEF const, and updated Ndef --> NDEF function names.

Comment 6

[arno]

Good work on some much needed cleanup. Some quick comments/suggestions:

- Remove NfcManagerUtil.parseNDEF() and use NfcUtil.parseNDEF() instead.
- The comment explaining the usage of NfcUtil.parseNDEF() is not accurate anymore.
- The comment in NfcUtil should also mention the other exported functions.
- In NfcUtil.doParseNDEF() and other functions you reference the NDEF constants via "this.NDEF.*". I think you need to remove "this." since you moved 'NDEF' to the global namespace.
- In an earlier version of the HandoverManager implementation I also wrote an encoding of NDEF messages. Evelyn asked me to remove it since it wasn't used in the HandoverManager. If we create a general purpose library, perhaps we can add this to NfcUtil? If you agree, I will email you my implementation.

Comment 7

[Garner Lee]

(In reply to arno from comment #6)
> - In an earlier version of the HandoverManager implementation I also wrote
> an encoding of NDEF messages. Evelyn asked me to remove it since it wasn't
> used in the HandoverManager. If we create a general purpose library, perhaps
> we can add this to NfcUtil? If you agree, I will email you my implementation.

Please attach/email the encoding impl.

Comment 8

[Garner Lee]

Attachment: (v3) 0001-WIP-Update-system-and-nfc-demo-app-to-use-new-shared.patch

Updated with Arno's comments. Fixed SMARTPOSTER_ACTION capitalization. Arno, can you post the NDEFRecord encode function somewhere?

Comment 9

[Garner Lee]

Attachment: (v1) Create shared library for NFC

Comment 10

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8371151 [details] [review]
(v1) Create shared library for NFC

I recall that I said move the class declaration out of object method and it seems not occur. Please do.

And strongly acquire unit test for such big change.

Comment 11

[arno]

Garner: just emailed you my NDEF.encode() implementation.

Comment 12

[Garner Lee]

Comment on attachment 8370940 [details] [diff] [review]
(v3) 0001-WIP-Update-system-and-nfc-demo-app-to-use-new-shared.patch

Marking old patch obsolete. the github pull request is newer, and debugged for NFC handover.

Comment 13

[Garner Lee]

Comment on attachment 8371151 [details] [review]
(v1) Create shared library for NFC

Alive:
Rebased.

Buffer moved out of NfcUtil into it's own global scope object. NFC Handover is now pairing and working in the latest set against an NFC enabled portable speaker (manual connect after pairing is needed, but of of scope of this bug).

All previous functionality was unit tested as working.

Arno: I added your encodeNDEF function. Please check the util function comments for accuracy.

Comment 14

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8371151 [details] [review]
(v1) Create shared library for NFC

When I say unit test I don't mean manually testing.
We NEED nfc_manager_test under gaia/apps/system/test/unit.

Next time please come up with that because I have seen many typo-like error and the test coverage of nfc_manager is exactly, 0%.

If you have trouble writing unit test please tell me.

Comment 15

[Garner Lee]

(In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #14)
> Comment on attachment 8371151 [details] [review]
> (v1) Create shared library for NFC

> We NEED nfc_manager_test under gaia/apps/system/test/unit.

I hear you, although nfc_manager realistically also invloves the lower code stack too due to session management. The new shared libray code can use automated existance and unit tests however (open a new bug?).

We should make that a topic for the next weekly meeting (I mentioned the need for automated testing of reading/writing/NFC on/NFC off last week as well).

Comment 16

[arno]

(In reply to Garner Lee from comment #13)
> Comment on attachment 8371151 [details] [review]
> Arno: I added your encodeNDEF function. Please check the util function
> comments for accuracy.

I would propose the following comment:

"encodeNDEF: takes an array of NDEFRecords and returns an Array of octets representing the binary encoding of the NDEF message according to the NFC Forum"

Comment 17

[Garner Lee]

I've updated the names of the shared NfcUtil to NfcUtils to be more in line with other utility files in the existing gaia code. This may need re-review. This of breaks the other gaia app code that has included the "NfcUtil" class, but those should be easy to fix at the same time switching to use the shared version.

Unit test will now also pass on Travis CI (with MockMozNDEFRecord) that doesn't have NFC compiled in. Followup Bug 973133 for navigator DOM level tests.

https://travis-ci.org/mozilla-b2g/gaia/builds/19696896

Comment 20

[(inactive after 6/18) Alive Kuo [:alive]]

Please request review again if you think it's necessary. It's unclear what's going to be checked to me.

Comment 21

[Garner Lee]

Alive: aside from the new unit tests, no major changes (just renames).

Hi Ken,
We can still land to master, or should we wait?

Comment 22

[Ken Chang[:kenkai]]

It is for gaia part and there is a bubble-tea branch for Gaia, I think Alive may be able to provide more clear answer.
Alive, what do you think? Can we land this patch in Gaia?

Comment 23

[(inactive after 6/18) Alive Kuo [:alive]]

Garner, please create another pull request to bubble-tea and ask for my review. Thanks!

Comment 24

[Garner Lee]

Attachment: bubble-tea branch pull req (duplicate).

Comment 25

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8384796 [details] [review]
bubble-tea branch pull req (duplicate).

r+ with nits

Comment 26

[Garner Lee]

Updated. test_context_menu_activity_picker seems broken on bubble-tea currently on the lastest head (probably unrelated).

What is step 5 here: https://wiki.mozilla.org/Gaia/Team/Taipei/BubbleTea
5. Merge your patch. (<-- Who should do that merge?)

Comment 27

[(inactive after 6/18) Alive Kuo [:alive]]

test_context_menu_activity_picker failing will be fixed by bug 981965.

Comment 28

[Garner Lee]

All green: https://travis-ci.org/mozilla-b2g/gaia/builds/20709966

Bubble-tea merge needed.

Comment 29

[(inactive after 6/18) Alive Kuo [:alive]]

bubble-tea is freezed right now, we will do merge next week after v1.4 is branched.

Comment 30

[Garner Lee]

Merger(s): When 1.4 is branched, Check-in needed.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

bubble-tea: cd6f2ee78b577f769f09dc13f80591b93e134711

Leaving this open because I assume we aren't resolving bugs like these until they hit master.

Comment 32

[Garner Lee]

Ken: when is bubble-tea merging into master (a few dependencies are waiting for it).

Comment 33

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Alive, which patches in bubble-tea this patch is depend on? Can we have a list so we could land this group of patches together? I believe Yuren and Fred is working on this too....

Comment 34

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

pull request to master:
https://github.com/mozilla-b2g/gaia/pull/17369

travis-ci:
https://travis-ci.org/mozilla-b2g/gaia/builds/21163018

try server:
https://tbpl.mozilla.org/?tree=Try&rev=f86b9af3bd7b

Comment 35

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

travis-ci is passed, merged.

https://github.com/mozilla-b2g/gaia/commit/3ee5d595f8c347e6479856b3efa72fa079faacf6