The user can initiate BT pairing via NFC. The most common use case is with BT enabled speakers or headsets. 1) The user can tap an NFC enabled BT device (e.g. BT speakers, BT headsets etc) to allow easy pairing with that BT device. No typical BT interactions (choosing a device to pair, entering pin codes) are necessary. a) The above can happen at any point/screen on the mobile (as long as NFC is turned on). b) If BT is turned off, the user will be prompted to turn on BT. c) If music is already playing on the mobile, tapping on a NFC enabled BT speaker/headset to initiate a BT connection will lead to pairing, connecting the BT device and then transferring the music playback to the speaker/headset.

Hi Gabriele, are you interested in handling this bug?

Happy to do this as per bug 903259 comment 1.

Alive, this bug will also be fixed in NFC manager app.

Hi, do we have a clear view on the target milestone on this bug now? Thanks.

It seems better to finish this bug before sprint5. If you think it isn't reasonable, please update target milestone.

Garner, I wonder if you would fix this bug in NFC manager.

I believe Arno just finished the static pairing, so I'll assign to him.

This is the first patch for handling a static handover via NFC. This patch basically contains a whole bunch of NDEF parsing functions to pull out the Bluetooth MAC address of the device to be paired with. Note that NFC Manager requires some changes to call functions in this patch. Garner is currently giving the NFC manager a serious facelift and I will add the patches for NFC manager once he is done. This code was successfully used to pair a Motorola Boom Bluetooth headset with a FFOS device.

Comment on attachment 831799 [details] [diff] [review] 0001-Bug-903305-Utility-functions-for-handover-handling.patch I think Gabriele is very busy in handling koi+ bugs...:(. Transfer this bug to Evelyn.

(In reply to Ken Chang from comment #10) > Comment on attachment 831799 [details] [diff] [review] > 0001-Bug-903305-Utility-functions-for-handover-handling.patch > > I think Gabriele is very busy in handling koi+ bugs...:(. Transfer this bug > to Evelyn. Evelyn who?

(In reply to arno from comment #11) Evelyn Hung, she will help review you patch.

(In reply to Ken Chang from comment #12) > (In reply to arno from comment #11) > Evelyn Hung, she will help review you patch. sorry, I hadn't noticed that you already reassigned the bug. That's why I was asking for her name.

Comment on attachment 831799 [details] [diff] [review] 0001-Bug-903305-Utility-functions-for-handover-handling.patch Review of attachment 831799 [details] [diff] [review]: ----------------------------------------------------------------- The flow isn't clear to me. So here is my understanding: Someone(NFC manager?) will call HandoverManager.pair() and pass a ndef message. The message will be parsed by NdefHandoverCodec and other utility functions, then finally get a bluetooth mac address for pairing. If BT has already on, do pair immediately, otherwise, turn on BT and add a callback. If I'm correct, then I think your BT part isn't doing the thing as I described. You shouldn't call |InitHandoverManager| while the script is loaded, because there is nothing we can do until getting a BT address. Regarding your utility object and functions , like NdefUtils, NdefConsts, Buffer, NdefCodec, and NdefHandoverCodec, please wrap into one or two object(s) with some private objects and functions. Don't expose too much to global namespace. ::: apps/system/js/nfc_handover.js @@ +4,5 @@ > +/** > + * This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. > + */ Gaia is Apache License, and we normally don't add licensing blurbs on top of the files. Please remove these three lines. @@ +14,5 @@ > +/******************************************************************************* > + * NdefUtils: Some common utilities functions. > + */ > +var NdefUtils = { > + DEBUG: true, DEBUG flag should be turned off by default. @@ +48,5 @@ > + /** > + * Debug method > + */ > + debug: function debug(msg, optObject) { > + if (NdefUtils.DEBUG) { nit: use `this.DEBUG` @@ +464,5 @@ > + bluetooth: null, > + defaultAdapter: null, > + bluetoothMAC: null, > + > + bluetoothEnabled: function bluetoothEnabled() { These function names are confused. |bluetoothEnabled| does pair stuff, and |pair| does enabling bluetooth. @@ +465,5 @@ > + defaultAdapter: null, > + bluetoothMAC: null, > + > + bluetoothEnabled: function bluetoothEnabled() { > + if (HandoverManager.defaultAdapter == null) { use 'this' instead of 'HandoverManager'. @@ +485,5 @@ > + * message. If it contains a Bluetooth Simple Pairing Record, Bluetooth will > + * be enabled (if not already) and the pairing with the remote device will > + * be performed. > + */ > + pair: function pair(ndef) { Who will call this pair function? How can you make sure this function is called and assign a value to bluetoothMAC before calling bluetoothEnabled? @@ +518,5 @@ > + } > + HandoverManager.settings = window.navigator.mozSettings; > + HandoverManager.bluetooth = window.navigator.mozBluetooth; > + > + HandoverManager.bluetooth.onadapteradded = function bt_adapterAdded() { please use addEventListener to bind an event callback because there is somebody else listen to the same event. @@ +522,5 @@ > + HandoverManager.bluetooth.onadapteradded = function bt_adapterAdded() { > + HandoverManager.bluetoothEnabled(); > + }; > + > + var req = HandoverManager.bluetooth.getDefaultAdapter(); You should call getDefaultAdapter() in the callback of onaddapteradded event, which means the defaultadapter is ready.

(In reply to arno from comment #8) > Created attachment 831799 [details] [diff] [review] > 0001-Bug-903305-Utility-functions-for-handover-handling.patch > If you can send a pull request on the Github instead of a patch file, I will appreciate. A pull request will go through our test cases, and it's easier to merge. You can refer this wiki page to know more: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Platform/Gaia/Hacking#Submitting_a_patch

Addressed all of Evelyn's comments. This pull request also contains the changes to NFC Manager that show how the handover API is used.

Comment on attachment 833364 [details] Fix for reviewers comments it seems that you didn't git rebase your branch, there are 250+ commits in this pull request! I can't find your commit within them, sorry.

(In reply to Evelyn Hung [:evelyn] from comment #17) > Comment on attachment 833364 [details] > Fix for reviewers comments > > it seems that you didn't git rebase your branch, there are 250+ commits in > this pull request! I can't find your commit within them, sorry. If you have troubles on git or github, a patch file is still good to me. I appreciate your effort.

My apologies, Evelyn. Our master was not current which is why there were so many strange commits. My bad. This pull request should be fine now. Please note that this pull request contains two commits: I squashed our entire NFC code base in one commit (parts of which are currently under review) and one commit that only contains the handover code I would like you to review. This should make it easier for you to identify my changes.

(In reply to arno from comment #19) > Created attachment 8334904 [details] > Fix for reviewers comments > > My apologies, Evelyn. Our master was not current which is why there were so > many strange commits. My bad. This pull request should be fine now. Please > note that this pull request contains two commits: I squashed our entire NFC > code base in one commit (parts of which are currently under review) and one > commit that only contains the handover code I would like you to review. This > should make it easier for you to identify my changes. Thanks. I'm reviewing, but I may need one more day to finish my review.

Comment on attachment 8334904 [details] Fix for reviewers comments Despite my comments on Github, I have some overall comments and questions: 1. mixing patterns: I see at least 3 ways to define classes and 2 ways to protect a function to be private. I think you might have some reasons so I'd like hear them. In Gaia, we usually choose either module pattern(for protect method as private) or object with prototype functions. The biggest concern in your code is that memory consumption because every instance will keep a copy of functions which are the same. Why not use prototype? On the other hand, your private functions usually access HandoverManager's properties via `self.xxx`, so why not define these properties private (e.g., var DEBUG), too? 2. Some functions are actually do the same or similar things. Especially those in xxxUtils objects. Bad smell here. 3. Why we don't parse ndef message in Gecko? Please also checkout my rest comments on Github.

Is the target milestone still valid?

Thanks for your feedback. This version tries to address your concerns and it also adds support for a sendFile() API (untested since required BT API is not yet available). The lazy loading of the .js file is not done and will be fixed together with the loading of nfc_manager.js. For other remarks I will respond inline in your comment.

(In reply to Evelyn Hung [:evelyn] from comment #21) > Comment on attachment 8334904 [details] > 1. mixing patterns: I see at least 3 ways to define classes and 2 ways to > protect a function to be private. I think you might have some reasons so I'd > like hear them. In Gaia, we usually choose either module pattern(for protect > method as private) or object with prototype functions. The biggest concern > in your code is that memory consumption because every instance will keep a > copy of functions which are the same. Why not use prototype? > On the other hand, your private functions usually access HandoverManager's > properties via `self.xxx`, so why not define these properties private (e.g., > var DEBUG), too? I removed classes where possible and used prototypes as you suggested. The Buffer class is still necessary since it might be instantiated more than once (to unmarshall nested data structures). > 2. Some functions are actually do the same or similar things. Especially > those in xxxUtils objects. Bad smell here. I suppose you mean NdefUtil. There are three functions in that module: toUTF8(), fromUTF8() and equalArrays(). All doing different things as their names imply. Can you please elaborate what you mean with "bad smell"? > 3. Why we don't parse ndef message in Gecko? Gecko knows about NDEF messages but does not need to know the content of those NDEF messages. That is handled in Gaia. Note that if you want to push down Handover Manager one layer, you also have to do so for the NFC Manager. The reviewer for the latter is fine with this architecture. Please consult him. > Please also checkout my rest comments on Github. I addressed all github comments except the lazy loading. We'll do this together with the lazy loading of the NFC Manager. The hardcoded MAC address is because of a bug in the BT stack that prevents me from reading my own address. Please note that Sid (CCed on this bug) will work on your next round of feedback as I won't be available.

(In reply to arno from comment #24) > (In reply to Evelyn Hung [:evelyn] from comment #21) > > Comment on attachment 8334904 [details] > > 1. mixing patterns: I see at least 3 ways to define classes and 2 ways to > > protect a function to be private. I think you might have some reasons so I'd > > like hear them. In Gaia, we usually choose either module pattern(for protect > > method as private) or object with prototype functions. The biggest concern > > in your code is that memory consumption because every instance will keep a > > copy of functions which are the same. Why not use prototype? > > On the other hand, your private functions usually access HandoverManager's > > properties via `self.xxx`, so why not define these properties private (e.g., > > var DEBUG), too? > > I removed classes where possible and used prototypes as you suggested. The > Buffer class is still necessary since it might be instantiated more than > once (to unmarshall nested data structures). > Nice. > > 2. Some functions are actually do the same or similar things. Especially > > those in xxxUtils objects. Bad smell here. > > I suppose you mean NdefUtil. There are three functions in that module: > toUTF8(), fromUTF8() and equalArrays(). All doing different things as their > names imply. Can you please elaborate what you mean with "bad smell"? > I was saying NdefUtil and NfcUtil. Both of them have equalArrays, fromUTF8, and toUTF8. > > 3. Why we don't parse ndef message in Gecko? > > Gecko knows about NDEF messages but does not need to know the content of > those NDEF messages. That is handled in Gaia. Note that if you want to push > down Handover Manager one layer, you also have to do so for the NFC Manager. > The reviewer for the latter is fine with this architecture. Please consult > him. > > > Please also checkout my rest comments on Github. > > I addressed all github comments except the lazy loading. We'll do this > together with the lazy loading of the NFC Manager. The hardcoded MAC address > is because of a bug in the BT stack that prevents me from reading my own > address. > Okay.. then please add the bug number in comment, like: '//XXX: gecko bug xxxxxx' > Please note that Sid (CCed on this bug) will work on your next round of > feedback as I won't be available. I'm reviewing the patch, more feedback are coming. I also want to test this patch if I can get one device with NFC.

(In reply to arno from comment #23) > Created attachment 8336479 [details] > Restructuring and fixes > > Thanks for your feedback. This version tries to address your concerns and it > also adds support for a sendFile() API (untested since required BT API is > not yet available). Please don't add other feature into this patch, we don't want to merge code that it not working and untested. You should file another bug and flag feedback? there.

(In reply to Evelyn Hung [:evelyn] from comment #25) > I'm reviewing the patch, more feedback are coming. I also want to test this > patch if I can get one device with NFC. Hi Evelyn, If you would like have a trial, you can ask Dimi to provide the device and image for you.

(In reply to Evelyn Hung [:evelyn] from comment #25) > > > 2. Some functions are actually do the same or similar things. Especially > > > those in xxxUtils objects. Bad smell here. > > > > I suppose you mean NdefUtil. There are three functions in that module: > > toUTF8(), fromUTF8() and equalArrays(). All doing different things as their > > names imply. Can you please elaborate what you mean with "bad smell"? > > > > I was saying NdefUtil and NfcUtil. Both of them have equalArrays, fromUTF8, > and toUTF8. OK, you are right that NFC Manager has similar utility functions. The cleanest way would be to add a file nfc_util.js and put all common code there. The NFC Manager is under review by Alive and since you guys don't like patches changing apart from your change requests, I opted for this solution. Can you and Alive please discuss and decide how you would like to handle this? > Okay.. then please add the bug number in comment, like: '//XXX: gecko bug > xxxxxx' BT is also a fast moving train. That particular issue has been fixed and the hard coded MAC address will be removed from the next patch. > I'm reviewing the patch, more feedback are coming. I also want to test this > patch if I can get one device with NFC. Please note that the only thing working right now is BT pairing. You need to hold a BT device (such as NFC-enabled headset) to the device and it will be paired. None of the file sharing functionality can be tested because of 915533 but Eric said they are working hard on landing this.

(In reply to Evelyn Hung [:evelyn] from comment #26) > (In reply to arno from comment #23) > > Created attachment 8336479 [details] > > Restructuring and fixes > > > > Thanks for your feedback. This version tries to address your concerns and it > > also adds support for a sendFile() API (untested since required BT API is > > not yet available). > Please don't add other feature into this patch, we don't want to merge code > that it not working and untested. You should file another bug and flag > feedback? there. Sid will wire up the sendFile() with the DOM API. As I mentioned, file transfer can currently not be tested because of bug 915533 (I did add a dependency to this bug). I guess you want the whole file sharing code be moved to a new bug?

> > Sid will wire up the sendFile() with the DOM API. As I mentioned, file > transfer can currently not be tested because of bug 915533 (I did add a > dependency to this bug). I guess you want the whole file sharing code be > moved to a new bug? yes, to make this patch clear and in a reasonable scope.

Comment on attachment 8336479 [details] Restructuring and fixes feedback+ I think there is no big problem in this patch, only a nits and clean up need to be addressed: 1. please move HandoverManager.prototype.handleHandover* prototype declaration out from HandoverManager declaration. Defining prototype inside HandoverManager will also create a function copy and overwrite prototype object in every instance, is it what you want? 2. we can exclude "NFC code base" commit now, I'm totally understanding the whole flow. Thanks for making this for my reference. 3. as I suggest in my comment 30, move the whole file sharing code to another new bug. Let this patch only do things matched bug title - BT pairing.

(In reply to arno from comment #28) > (In reply to Evelyn Hung [:evelyn] from comment #25) > > > > 2. Some functions are actually do the same or similar things. Especially > > > > those in xxxUtils objects. Bad smell here. > > > > > > I suppose you mean NdefUtil. There are three functions in that module: > > > toUTF8(), fromUTF8() and equalArrays(). All doing different things as their > > > names imply. Can you please elaborate what you mean with "bad smell"? > > > > > > > I was saying NdefUtil and NfcUtil. Both of them have equalArrays, fromUTF8, > > and toUTF8. > > OK, you are right that NFC Manager has similar utility functions. The > cleanest way would be to add a file nfc_util.js and put all common code > there. The NFC Manager is under review by Alive and since you guys don't > like patches changing apart from your change requests, I opted for this > solution. Can you and Alive please discuss and decide how you would like to > handle this? > Well, crossing reference patches in two bugs is good to us, better than having duplicate code in each patch. Alive has r+ bug 860910, and I feel like it's not a big problem that I want to argue here. A follow-up bug to move these common utilities to nfc.util is good to me.

(In reply to Evelyn Hung [:evelyn] from comment #31) > Comment on attachment 8336479 [details] > Restructuring and fixes > > feedback+ I think there is no big problem in this patch, only a nits and > clean up need to be addressed: > 1. please move HandoverManager.prototype.handleHandover* prototype > declaration out from HandoverManager declaration. Defining prototype inside > HandoverManager will also create a function copy and overwrite prototype > object in every instance, is it what you want? So, when I move the prototype declaration outside of HandoverManager, I no longer have access to private members/functions of HandoverManager due to JavaScript scoping rules (e.g., call debug(), doAction() etc from within handleHandoverSelect()). Note that HandoverManager will only ever be instantiated once (at the end of the file where the singleton is stored in 'handoverManager'), so technically the prototype would not even be required here. If you still want this to be a prototype, I would have to expose more private API. Please let me know your preference. > 2. we can exclude "NFC code base" commit now, I'm totally understanding the > whole flow. Thanks for making this for my reference. OK. > 3. as I suggest in my comment 30, move the whole file sharing code to > another new bug. Let this patch only do things matched bug title - BT > pairing. Will do.

Sorry, forgot to set the need info flag. Please reply to comment #33 when you get the chance.

(In reply to arno from comment #33) > (In reply to Evelyn Hung [:evelyn] from comment #31) > > Comment on attachment 8336479 [details] > > Restructuring and fixes > > > > feedback+ I think there is no big problem in this patch, only a nits and > > clean up need to be addressed: > > 1. please move HandoverManager.prototype.handleHandover* prototype > > declaration out from HandoverManager declaration. Defining prototype inside > > HandoverManager will also create a function copy and overwrite prototype > > object in every instance, is it what you want? > > So, when I move the prototype declaration outside of HandoverManager, I no > longer have access to private members/functions of HandoverManager due to > JavaScript scoping rules (e.g., call debug(), doAction() etc from within > handleHandoverSelect()). Note that HandoverManager will only ever be > instantiated once (at the end of the file where the singleton is stored in > 'handoverManager'), so technically the prototype would not even be required > here. If you still want this to be a prototype, I would have to expose more > private API. Please let me know your preference. > yeah, you are right, so don't attached on prototype object in this case. Thanks for clarifying.

Changes: - Remove file handover implementation (this is handled in bug 933093) - Remove prototype for public API in HandoverManager since it is used as a singleton - Squash into one commit

Comment on attachment 8339217 [details] Implementation of BT Pairing via NFC Handover I was scared by 40 file changes in your PR. I thought I was reviewing BT pairing part only. Many of these files belongs to nfc-demo, they are fine, but please move the whole nfc-demo to test_apps/, we don't want this app to be built-in when shipping. Files under Browser, Dialer, Email apps, I can't see why you touch them, can you explain? BTW, you also need to rebase your patch because nfc_manager has been merged.

(In reply to Evelyn Hung [:evelyn] from comment #37) > Comment on attachment 8339217 [details] > Implementation of BT Pairing via NFC Handover > > I was scared by 40 file changes in your PR. I thought I was reviewing BT > pairing part only. Many of these files belongs to nfc-demo, they are fine, > but please move the whole nfc-demo to test_apps/, we don't want this app to > be built-in when shipping. Files under Browser, Dialer, Email apps, I can't > see why you touch them, can you explain? > BTW, you also need to rebase your patch because nfc_manager has been merged. Doing this from a hotel room while on vacation might not have been the best idea. I will clean this up ASAP.

Rebased against b2g/master.

Comment on attachment 8340584 [details] Implementation of BT Pairing via NFC Handover r+, but travis test failed, not sure if it's because of your patch. https://travis-ci.org/mozilla-b2g/gaia/builds/14736065 I will also test it on a device (on Monday, I don't have one in hand now).

NFC is not a committed feature for 1.3, so this should not block the release.

> I will also test it on a device (on Monday, I don't have one in hand now). FYI, if not already done, you'd need a gecko with the Uint8Array fix ot ObjectWrapper (https://github.com/svic/mozilla-central/) to test it.

(In reply to Garner Lee from comment #42) > > I will also test it on a device (on Monday, I don't have one in hand now). > > FYI, if not already done, you'd need a gecko with the Uint8Array fix ot > ObjectWrapper (https://github.com/svic/mozilla-central/) to test it. Thanks, I'm trying.

I don't have a headset that support NFC on hand, so I use a Nexus4 and do some hacks to make pair process moving on. (my hacks: fake ndefMsg with handover_select type, and hard-code BT mac to be paired) So the patch works as expected except a pairing confirmation pops-up. Per spec, I guess we will do silent pairing here, but since gecko BT stack has no idea the defaultAdapter.pair() is from NFC, so the pair flow goes as usual: send "bluetooth-pairing-request" to Settings app to display a confirmation screen. It looks like we need some tricks to bypass this UI. Please file a follow-up bug. The patch is still failed on travis, I've re-run it many times. running now: https://travis-ci.org/mozilla-b2g/gaia/builds/14736065

(In reply to Evelyn Hung [:evelyn] from comment #44) > > The patch is still failed on travis, I've re-run it many times. > running now: https://travis-ci.org/mozilla-b2g/gaia/builds/14736065 Still can't figure out why this patch keep failing on Travis... I tend to merge it if I can identify it's not your fault, but I can't do it now, Gaia tree closed.

(In reply to Evelyn Hung [:evelyn] from comment #44) > I don't have a headset that support NFC on hand, so I use a Nexus4 and do > some hacks to make pair process moving on. > (my hacks: fake ndefMsg with handover_select type, and hard-code BT mac to > be paired) nice. > So the patch works as expected except a pairing confirmation pops-up. Per > spec, I guess we will do silent pairing here, but since gecko BT stack has > no idea the defaultAdapter.pair() is from NFC, so the pair flow goes as > usual: send "bluetooth-pairing-request" to Settings app to display a > confirmation screen. It looks like we need some tricks to bypass this UI. > Please file a follow-up bug. We might also discuss that this should be the wanted behavior. Android does prompt the user to confirm the pairing. Perhaps you guys can decide internally. > The patch is still failed on travis, I've re-run it many times. > running now: https://travis-ci.org/mozilla-b2g/gaia/builds/14736065 Yeah. I've seen that too but I don't see anything related to NFC in the failed travis run.

> > So the patch works as expected except a pairing confirmation pops-up. Per > > spec, I guess we will do silent pairing here, but since gecko BT stack has > > no idea the defaultAdapter.pair() is from NFC, so the pair flow goes as > > usual: send "bluetooth-pairing-request" to Settings app to display a > > confirmation screen. It looks like we need some tricks to bypass this UI. > > Please file a follow-up bug. > > We might also discuss that this should be the wanted behavior. Android does > prompt the user to confirm the pairing. Perhaps you guys can decide > internally. > Okay, then I will ask UX's opinion. > > The patch is still failed on travis, I've re-run it many times. > > running now: https://travis-ci.org/mozilla-b2g/gaia/builds/14736065 > > Yeah. I've seen that too but I don't see anything related to NFC in the > failed travis run. Finally green!

Merged into gaia master: https://github.com/mozilla-b2g/gaia/commit/88c692478b24704970e41008bed54e90d372bbc3

NFC isn't a committed feature, so removing blocking flag.