Closed Bug 963556 Opened 6 years ago Closed 6 years ago

[B2G][NFC] Create shared library for NFC

Categories

(Firefox OS Graveyard :: NFC, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dgarnerlee, Assigned: dgarnerlee)

References

Details

(Whiteboard: [FT:RIL])

Attachments

(2 files, 3 obsolete files)

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.
Blocks: b2g-nfc
Duplicate of this bug: 943691
Thanks, Garner. I think Michal need this lib for developing contacts sharing. 
So when do you plan to finish this?
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.
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.
Flags: needinfo?(arno)
Update. Made NDEF const, and updated Ndef --> NDEF function names.
Attachment #8366911 - Attachment is obsolete: true
Assignee: nobody → dgarnerlee
Blocks: 964186
No longer blocks: 964186
Depends on: 964186
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.
Flags: needinfo?(arno)
(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.
Updated with Arno's comments. Fixed SMARTPOSTER_ACTION capitalization. Arno, can you post the NDEFRecord encode function somewhere?
Attachment #8366977 - Attachment is obsolete: true
Attachment #8371151 - Flags: review?(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.
Attachment #8371151 - Flags: review?(alive)
Blocks: 969217
Flags: needinfo?(arno)
Depends on: 969136
Garner: just emailed you my NDEF.encode() implementation.
Flags: needinfo?(arno)
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.
Attachment #8370940 - Attachment is obsolete: true
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.
Attachment #8371151 - Flags: review?(alive)
Flags: needinfo?(arno)
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.
Attachment #8371151 - Flags: review?(alive) → review+
(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).
Blocks: 973132
Blocks: 973133
(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"
Flags: needinfo?(arno)
Whiteboard: [FT:RIL]
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
Flags: needinfo?(alive)
Duplicate of this bug: 977025
Please request review again if you think it's necessary. It's unclear what's going to be checked to me.
Flags: needinfo?(alive)
Alive: aside from the new unit tests, no major changes (just renames).

Hi Ken,
We can still land to master, or should we wait?
Flags: needinfo?(kchang)
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?
Flags: needinfo?(kchang)
Flags: needinfo?(alive)
Blocks: 975268
Garner, please create another pull request to bubble-tea and ask for my review. Thanks!
Flags: needinfo?(alive)
Attachment #8384796 - Flags: review?(alive)
Comment on attachment 8384796 [details] [review]
bubble-tea branch pull req (duplicate).

r+ with nits
Attachment #8384796 - Flags: review?(alive) → review+
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?)
Flags: needinfo?(alive)
test_context_menu_activity_picker failing will be fixed by bug 981965.
Flags: needinfo?(alive)
All green: https://travis-ci.org/mozilla-b2g/gaia/builds/20709966

Bubble-tea merge needed.
Flags: needinfo?(alive)
bubble-tea is freezed right now, we will do merge next week after v1.4 is branched.
Flags: needinfo?(alive)
Merger(s): When 1.4 is branched, Check-in needed.
Keywords: checkin-needed
bubble-tea: cd6f2ee78b577f769f09dc13f80591b93e134711

Leaving this open because I assume we aren't resolving bugs like these until they hit master.
Keywords: checkin-needed
Ken: when is bubble-tea merging into master (a few dependencies are waiting for it).
Flags: needinfo?(ryanvm)
Flags: needinfo?(kchang)
Flags: needinfo?(ryanvm)
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....
Flags: needinfo?(kchang) → needinfo?(alive)
travis-ci is passed, merged.

https://github.com/mozilla-b2g/gaia/commit/3ee5d595f8c347e6479856b3efa72fa079faacf6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Blocks: 974725
Duplicate of this bug: 973132
You need to log in before you can comment on or make changes to this bug.