Related to Bug 903259 (Gaia): NFCPeer needs a new API to handle handover requests. NFC is used for setting up the connection, but does not do the actual transfers. The Pairing is done by tapping devices together and exchanging relevant bits of connection data (hardware address), and the specific case of file transfer is done by transferring a data blob from one device to another over that static or negotiated connection. From a discussion: > 1. BT in gecko needs a new pairing API which argument is the > address(currently it's the device). > 2. BT app in gaia needs some change to activity message handler to deal with > background launched case. > 3. ActivityWindowFactory/ActivityWindow needs to manipulate the new > disposition and launches the activity in background. > 4. WebNFC needs some new API: sendFile, handover?. > 5. NFCManager in gaia::system needs some change. > Proposed usage: window.navigator.mozNfc.onpeerfound = function(nfcPeer) { // Negotiates WiFi or BT connection, presenting UI DOMRequest r = nfcPeer.sendFile(blob); r.onsuccess = function(e) { // Transfer successfully completed, update UI }; r.onerror = function(e) { // Transfer did not succeed, update UI }; };

Proposal for file transfer under webNFC: https://docs.google.com/document/d/1X00d3aB5ZltWfaqbFEWtB_h_fFpHAQUXVgiyh9yxeMg/edit?usp=sharing

Just filed a bug for point 1. (bug 933113)

About the sendFile API, refer this http://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/BluetoothAdapter.h#128 http://mxr.mozilla.org/mozilla-central/source/dom/webidl/BluetoothAdapter.webidl#118

I am sure it's 1.3+. The target milestone should be 11/8. If you don't agree this date, please provide your suggestion.

Garner, when can you provide a patch for this bug?

^--Sid?

Actually, Arno is working on the DOM part. Static handover (bluetooth headset) may be done this week. We'd need some BT APIs to actually negotiate the handover transfer connection (Eric will provide API?). A negotiated handover (NDEF messages between devices to determine a common transfer connection) may take longer. The MozNFCPeer.sendNDEF(ndefrecs) API has fewer dependencies, and should start to be integrated into the apps once the DOM lands in Bug 674741 (which blocks Bug 933136).

(In reply to Garner Lee from comment #7) > Actually, Arno is working on the DOM part. Static handover (bluetooth > headset) may be done this week. Should we assign this bug to Arno...:)

(In reply to Ken Chang from comment #8) > Should we assign this bug to Arno...:) Assigning to Arno :)

I've just added a dependency to Bug 903305. The static handover is so closely related to the negotiated handover (in terms of PDU exchange) that 903305 needs to land first. Once that happened, implementing the DOM API should be easy.

Eric: what API do I need to use for incoming file transfers assuming I have the BT MAC address from the sending side and the sending side initiated the handover?

(In reply to arno from comment #11) > Eric: what API do I need to use for incoming file transfers assuming I have > the BT MAC address from the sending side and the sending side initiated the > handover? First, please listen to the following system messages: 'bluetooth-opp-receiving-file-confirmation', 'bluetooth-opp-transfer-start', 'bluetooth-opp-update-progress', 'bluetooth-opp-transfer-complete' Gaia Reference: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/bluetooth_transfer.js **************************** bluetooth-opp-transfer-start = Description = Indicates that sending / receiving process has started = Parameter = address : [string] Bluetooth remote device address received : [bool] true - receiving, false - sending fileName : [string] file name fileLength : [uint32_t] file length (if the length is 0 it means unknown length) contentType : [string] File MIME Type (if it's empty it means the MIME type is not available) **************************** bluetooth-opp-transfer-complete = Description = Indicates that sending / receiving process has ended = Parameter = address : [string] Bluetooth remote device address success : [bool] true - success, false - fail (for some reason) received : [bool] true - receiving, false - sending fileName : [string] file name fileLength : [uint32_t] file length (if the length is 0 it means unknown length) contentType : [string] File MIME Type (if it's empty it means the MIME type is not available) **************************** bluetooth-opp-update-progress = Description = Would be sent when sending / receiving every 50 kb = Parameter = address : [string] Bluetooth remote device address received : [bool] true - receiving, false - sending processedLength : [uint32_t] the length of file content which has been sent. fileLength : [uint32_t] file length (if the length is 0 it means unknown length) **************************** bluetooth-opp-receiving-file-confirmation = Description = Indicates that we have received a "file pushing" request from a remote Bluetooth device. After this system message was sent, Gecko would be blocked until Gaia calls defaultAdapter.confirmReceivingFile([remote address], [accept?]); = Parameter = address : [string] Bluetooth remote device address fileName : [string] file name fileLength : [uint32_t] file length (if the length is 0 it means unknown length) contentType : [string] File MIME Type (if it's empty it means the MIME type is not available) ***************************** Please note that Bluetooth file transfer can't work at all on Nexus 4 for now (bug 915533). Users are not only unable to share files but also unable to receive files.

This patch extends the BT Pairing implementation of HandoverManager by adding an implementation for file transfer. This patch only adds to nfc_manager.js and nfc_handover.js. A separate patch for wiring this up with the DOM API will be submitted.

Comment on attachment 8339220 [details] Implementation of file transfer via NFC Handover r- 1. Please rebase your PR first, leave changes handling file transfer and remove irrelevant changes since they are belongs to bug 903305. 2. Why don't you utilize bluetooth file transfer app?

(In reply to Evelyn Hung [:evelyn] from comment #14) > Comment on attachment 8339220 [details] > Implementation of file transfer via NFC Handover > > r- > 1. Please rebase your PR first, leave changes handling file transfer and > remove irrelevant changes since they are belongs to bug 903305. will do. > 2. Why don't you utilize bluetooth file transfer app? I'm using the API that Eric communicated to me. Note that a file transfer via a NFC handover will require modifications to apps that can send files (video, images, etc). The apps basically register for onpeerready() and then do a sendFile() in the callback in response to the P2P "shrinking UI". This might be the reason Eric hasn't mentioned the bluetooth file transfer app.

Eric: can you please respond to Evelyn's comment #14 and my answer in #15?

This pull request contains the changes to HandoverManager to handle a negotiated handover via NFC. Since it is based on the initial version of HandoverManager that is still under review (bug 903305) there are two commits in this pull request.

NFC isn't a committed feature for 1.3, so this should not be blocking the release. It's only targeted.

Hi Evenly, Thanks for your suggestion. I wonder if it is possible to use the mechanism of what Arno provide for BT file transfer currently. And we can use the 933116 as a followup bug to improve this design later. What do you think?

Hi Arno, > > 2. Why don't you utilize bluetooth file transfer app? > > I'm using the API that Eric communicated to me. Note that a file transfer > via a NFC handover will require modifications to apps that can send files > (video, images, etc). The apps basically register for onpeerready() and then > do a sendFile() in the callback in response to the P2P "shrinking UI". This > might be the reason Eric hasn't mentioned the bluetooth file transfer app. I did mention Bluetooth file transfer app, which is a centralized app used for Bluetooth file transfer in FxOS system. There are two ways to do Bluetooth file transfer. The first one is handling all events and call BT functions in NFCManager, and the other is NFC telling BT app which device should the file be sent to so that BT app can deal with all BT stuff for you. The main problem of the latter function would be the interface of NFC Manager and BT app has to be designed well, so that BT app can know 'which device should I send to (also remember not showing UI for user to choose a device)', 'which file is going to be sent', etc. I wouldn't say if the first way is better or not since I'm not a Gaia professional, however both should be able to work.

(In reply to Ken Chang from comment #19) > Hi Evenly, Thanks for your suggestion. I wonder if it is possible to use the > mechanism of what Arno provide for BT file transfer currently. And we can > use the 933116 as a followup bug to improve this design later. What do you > think? I'd like to have a discussion with Alive and Ian (BT app owner). I don't think redo every BT file transfer stuff here will be easier than reuse BT app. BTW, so is this bug for gecko implementation or Gaia? I'm confused by bug summary. :-(

Comment on attachment 8340740 [details] Implementation of HandoverManager for file transfer redirect to Ian who knows more BT file transfer things than me. Ian will review your patch per our discussion on "reusing BT app or not".

Comment on attachment 8340740 [details] Implementation of HandoverManager for file transfer Hi Arno, As previous discussion with Evelyn, Alive, and Ken, we agree and suggest to do sending file via BT in system app. And we will help to guide the implementation in your patch. We won't pass the sending file request to Bluetooth App via web activity.(bug 933116) I have a quickly review for send/receive file via bluetooth section. I think the architecture should be as following. * Send file - You could provide a send file API in system/js/bluetooth_transfer.js(https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/bluetooth_transfer.js). - And it will need to do the same procedure for fire a fake event like "iac-bluetoothTransfercomms". In fact, we could call onFileSending() directly while NFC module create the sending file request. Then, the function onFilesSending() will do follow up sending file flow. Otherwise, you will break the sending file queue. And the notification message won't be pop out in this case. * Receive file - You have to identify the incoming file which is come from NFC or not. If yes, we should not pop out a confirmation dialog. Because we have no way to identify this case via bluetooth system message "bluetooth-opp-receiving-file-confirmation". I think you have to maintain a flag or mode to check the situation. And confirm the incoming file request directly in function onReceivingFileConfirmation().(https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/bluetooth_transfer.js#L149) Once you do confirmReceivingFile()(https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/bluetooth_transfer.js#L205), the UI flow will sync with original BT file transfer. Please revise the patch to sync the architecture. And set the flag to me again when your patch is ready. Thank you so much.

(In reply to Ian Liu [:ianliu] from comment #23) > * Send file > - You could provide a send file API in > system/js/bluetooth_transfer.js(https://github.com/mozilla-b2g/gaia/blob/ > master/apps/system/js/bluetooth_transfer.js). > - And it will need to do the same procedure for fire a fake event like > "iac-bluetoothTransfercomms". In fact, we could call onFileSending() > directly while NFC module create the sending file request. Then, the > function onFilesSending() will do follow up sending file flow. Otherwise, > you will break the sending file queue. And the notification message won't be > pop out in this case. Two quick questions on the sending file side: (1) for onFilesSending(evt), can you please tell me the properties I need to set in 'evt' to do the fake event? (2) do I still need to call DefaultAdapter.sendFile() or is this done implicitly somewhere else? > * Receive file > [...] I got this to work by making the modifications you suggested. :)

(In reply to arno from comment #24) > (In reply to Ian Liu [:ianliu] from comment #23) > > * Send file > > - You could provide a send file API in > > system/js/bluetooth_transfer.js(https://github.com/mozilla-b2g/gaia/blob/ > > master/apps/system/js/bluetooth_transfer.js). > > - And it will need to do the same procedure for fire a fake event like > > "iac-bluetoothTransfercomms". In fact, we could call onFileSending() > > directly while NFC module create the sending file request. Then, the > > function onFilesSending() will do follow up sending file flow. Otherwise, > > you will break the sending file queue. And the notification message won't be > > pop out in this case. > > Two quick questions on the sending file side: > (1) for onFilesSending(evt), can you please tell me the properties I need to > set in 'evt' to do the fake event? You could follow up "sendingFilesSchedule" object(https://github.com/mozilla-b2g/gaia/blob/master/apps/bluetooth/js/transfer.js#L145) for the evt parameter. > (2) do I still need to call DefaultAdapter.sendFile() or is this done > implicitly somewhere else? You have to expose(implement) the "DefaultAdapter.sendFile()" API in the object(https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/bluetooth_transfer.js#L8). And reuse Bluetooth.getAdapter() again. Then, I suppose the wrapper is ready, you will call BluetoothTransfer.sendFile() and give the "sendingFilesSchedule" object in the same time. That would be more maintainable in the future. > > > * Receive file > > [...] > > I got this to work by making the modifications you suggested. :) Nice catch:)

(In reply to Ian Liu [:ianliu] from comment #25) > > Two quick questions on the sending file side: > > (1) for onFilesSending(evt), can you please tell me the properties I need to > > set in 'evt' to do the fake event? > You could follow up "sendingFilesSchedule" > object(https://github.com/mozilla-b2g/gaia/blob/master/apps/bluetooth/js/ > transfer.js#L145) for the evt parameter. Thanks. But how do I get the filename? Blob does not seem to have a property for that and in transfer.js there seem to be two different sources for the filename and the blob: activity.source.data.filenames (line 146) and activity.source.data.blob (line 153). Note that in the NFC API there is only a sendFile(blob) (with no explicit filename parameter). > > (2) do I still need to call DefaultAdapter.sendFile() or is this done > > implicitly somewhere else? > You have to expose(implement) the "DefaultAdapter.sendFile()" API in the > object(https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/ > bluetooth_transfer.js#L8). And reuse Bluetooth.getAdapter() again. Then, I > suppose the wrapper is ready, you will call BluetoothTransfer.sendFile() and > give the "sendingFilesSchedule" object in the same time. That would be more > maintainable in the future. For the BT pairing implementation (bug 903305) I listen to 'adapteradded' events to retrieve the DefaultAdapter. This patch has been r+'ed and serves as the foundation for this bug. I hope you will accept that solution for now.

(In reply to arno from comment #26) > (In reply to Ian Liu [:ianliu] from comment #25) > > > Two quick questions on the sending file side: > > > (1) for onFilesSending(evt), can you please tell me the properties I need to > > > set in 'evt' to do the fake event? > > You could follow up "sendingFilesSchedule" > Thanks. But how do I get the filename? Blob does not seem to have a property > for that and in transfer.js there seem to be two different sources for the > filename and the blob: activity.source.data.filenames (line 146) and > activity.source.data.blob (line 153). Note that in the NFC API there is only > a sendFile(blob) (with no explicit filename parameter). > Looks like you're not able to get the filename. The the filenames are useless in transfer.js. I create a bug 946134 for removing the property. And will pass the number of files(numberOfFiles) for reference count. The patch is in reviewing process. You could suppose to base on the patch and pass the number of files for it.(https://bugzilla.mozilla.org/show_bug.cgi?id=946134#c1) > > > (2) do I still need to call DefaultAdapter.sendFile() or is this done > > > implicitly somewhere else? > > You have to expose(implement) the "DefaultAdapter.sendFile()" API in the > > object(https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/ > > bluetooth_transfer.js#L8). And reuse Bluetooth.getAdapter() again. Then, I > > suppose the wrapper is ready, you will call BluetoothTransfer.sendFile() and > > give the "sendingFilesSchedule" object in the same time. That would be more > > maintainable in the future. > > For the BT pairing implementation (bug 903305) I listen to 'adapteradded' > events to retrieve the DefaultAdapter. This patch has been r+'ed and serves > as the foundation for this bug. I hope you will accept that solution for now. For reusing adapter, if Evelyn has reviewed it in part of pairing device, that would be okay for me. :) Thanks.

Clear the ni flag via comment 27.

Ben: I'm having some issues with automatic pairing of two devices during a file transfer via a NFC handover. The NFC handover exchanges the MAC addresses between the two devices. On FFOS, I call adapter.pair(remote_mac), but I still get the pairing dialog on both FFOS and the Android device (that is trying to send a file). Note that when you do the same between two Android devices, the pairing happens implicitly without a pairing dialog popping up. Here is how you can reproduce it: - Gaia: https://github.com/svic/gaia/tree/handover-devel - Gecko: https://github.com/svic/mozilla-central/tree/master - After flashing the device, go to Settings and enable NFC - On the FFOS, unlock screen and stay on homescreen - On an Android device, go to Gallery and open any picture. - Approach the two devices back-to-back. Android will do the "shrinking UI" - On the Android device, tap on the "shrinking UI". This will trigger the file transfer When you do this, you will note that both FFOS and Android show the pairing dialog (this shouldn't happen). Here is the location where the HandoverManager triggers the pairing after the MAC addresses have been exchanged between the two devices (via appropriate NFC NDEF messages): https://github.com/svic/gaia/blob/handover-devel/apps/system/js/nfc_handover.js#L597

Hi Arno, Please apply my patch to your codebase. Pairing UI shouldn't appear afterwards. Please let me know if it works. Thank you.

(In reply to Eric Chou [:ericchou] [:echou] from comment #30) > Created attachment 8342998 [details] [diff] [review] > Insecure-Socket-for-NFC-pairing-temp-linkkey.patch > > Hi Arno, > > Please apply my patch to your codebase. Pairing UI shouldn't appear > afterwards. > > Please let me know if it works. Thank you. Eric: yes, that works. For incoming file transfers I no longer get the pairing dialog. However, is it possible that a similar patch is needed for outgoing file transfers? When FFOS is the initiator, I once again get the pairing request.

This patch adds support for file transfer to HandoverManager. It uses the BT file transfer app to handle the request. You can test incoming file transfers by using an Android device and beaming an image to the FFOS device. The sending side requires the DOM API to be wired up with HandoverManager.handleFileTransfer(). That patch will be provided separately.

(In reply to arno from comment #31) > (In reply to Eric Chou [:ericchou] [:echou] from comment #30) > > Created attachment 8342998 [details] [diff] [review] > > Insecure-Socket-for-NFC-pairing-temp-linkkey.patch > > > > Hi Arno, > > > > Please apply my patch to your codebase. Pairing UI shouldn't appear > > afterwards. > > > > Please let me know if it works. Thank you. > > Eric: yes, that works. For incoming file transfers I no longer get the > pairing dialog. However, is it possible that a similar patch is needed for > outgoing file transfers? When FFOS is the initiator, I once again get the > pairing request. Sure, I'll make both outgoing and incoming work. Since BT file transfer app on Android uses temp link key, we have to do the same thing to avoid prompting pairing dialog, which is sad. I'll update bug number here once the it's filed.

(In reply to Eric Chou [:ericchou] [:echou] from comment #33) > (In reply to arno from comment #31) > > (In reply to Eric Chou [:ericchou] [:echou] from comment #30) > > > Created attachment 8342998 [details] [diff] [review] > > > Insecure-Socket-for-NFC-pairing-temp-linkkey.patch > > > > > > Hi Arno, > > > > > > Please apply my patch to your codebase. Pairing UI shouldn't appear > > > afterwards. > > > > > > Please let me know if it works. Thank you. > > > > Eric: yes, that works. For incoming file transfers I no longer get the > > pairing dialog. However, is it possible that a similar patch is needed for > > outgoing file transfers? When FFOS is the initiator, I once again get the > > pairing request. > > Sure, I'll make both outgoing and incoming work. Since BT file transfer app > on Android uses temp link key, we have to do the same thing to avoid > prompting pairing dialog, which is sad. > > I'll update bug number here once the it's filed. I'll resolve bug 947060 as soon as possible.

Comment on attachment 8343207 [details] Support for file transfer in HandoverManager I have reviewed send file section via bluetooth. And leave some comment on Github. For receiving file section, it's working fine. For sending file, please check them again. It's better to have follow up patch for organisation these modules. Once you have revised it, please set review again for me. Thanks.

Fixed all of Ian's Github comments.

Question: What's the performance like for sendFile? When passing blob, is that raw data IPC copied between processes/apps/chrome?

(In reply to Garner Lee from comment #37) > Question: What's the performance like for sendFile? When passing blob, is > that raw data IPC copied between processes/apps/chrome? Passing blob does not pass the whole file content between different process. Only necessary information would be passed, otherwise it may be a super heavy operation. We don't have performance analysis about sending/receiving files via Bluetooth.

(In reply to Eric Chou [:ericchou] [:echou] from comment #38) > (In reply to Garner Lee from comment #37) > > Question: What's the performance like for sendFile? When passing blob, is > > that raw data IPC copied between processes/apps/chrome? > > Passing blob does not pass the whole file content between different process. > Only necessary information would be passed, otherwise it may be a super > heavy operation. > > We don't have performance analysis about sending/receiving files via > Bluetooth. To elaborate slightly, it depends on where the data for the blob lives. If it lives in a file on disk we just duplicate the file descriptor and hand that over with some meta data. If the file lives in memory we do copy the data and send that over the wire.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #39) > (In reply to Eric Chou [:ericchou] [:echou] from comment #38) > > (In reply to Garner Lee from comment #37) > > > Question: What's the performance like for sendFile? When passing blob, is > > > that raw data IPC copied between processes/apps/chrome? > > > > Passing blob does not pass the whole file content between different process. > > Only necessary information would be passed, otherwise it may be a super > > heavy operation. > > > > We don't have performance analysis about sending/receiving files via > > Bluetooth. > > To elaborate slightly, it depends on where the data for the blob lives. If > it lives in a file on disk we just duplicate the file descriptor and hand > that over with some meta data. If the file lives in memory we do copy the > data and send that over the wire. Clearer. Thanks, Kyle.

Thanks Eric, Kyle. Makes sense.

Comment on attachment 8344231 [details] Support for file transfer in HandoverManager Looks like you have migrated the Bluetooth sendFile API to bluetooth_transfer.js. That would be maintainable in the future. Good work:) For the 'this' code pattern, I think you have revised. We have too many times to access 'self' instead of 'this'. That will make the function scoop un-maintainable. Also leave some suggestion on Github. Thanks.

Defer to 1.4+ per triage meeting.

Hi Arno, According to bug 948874(https://bugzilla.mozilla.org/show_bug.cgi?id=948874#c0), we did not receive any event in nfc_handover.js. with debug=true. Is there any other blocking in NFC manager or Gecko error? Thanks.

Fixes for all of Ian's comments. Amongst others, I've completely removed 'self'. I personally find the code less readable now, but no more 'self'. :)

(In reply to arno from comment #45) > Fixes for all of Ian's comments. Amongst others, I've completely removed > 'self'. I personally find the code less readable now, but no more 'self'. :) Thanks for your revising effort. But you use constructor to define many methods/functions. It will make the performance lower. I leave some comment on Github.

Comment on attachment 8346371 [details] Support for file transfer in HandoverManager For Bluetooth file transfer section, that is okay for me. But the whole "HandoverManager" module, I recommend Alive to be the reviewer. He is the system app peer. Pass the decision to him.

Comment on attachment 8346371 [details] Support for file transfer in HandoverManager Canceling, please set review to evelyn since she reviewed this. My suggestions: 1. Try not to have a big constructor function. Use prototype. 2. Try not to put different module in different file, e.g. Buffer constructor. 3. Remove redundant codes. (parse). It would ease your life when you want to move them to gecko. 4. Unit test is highly recommended.

Evelyn: it seems the hot potato has come back to you! In this pull request I reverted some changes where I misunderstood Ian. I addressed all of his comments based on my interpretation of his feedback. Lets take this from the top and discuss the changes you would like to have done. One note on an earlier question/comment from you: this bug is about adding new DOM API for sending a blob. This also requires changes to the Gaia system app where the handover is handled. Sid will very soon upload the matching Gecko patches. If you prefer, I will create a new bug just for the Gaia changes with the next iteration of your feedback.

It is not 1.3 now.

Add support for SendFile Nfc DOM API

Eric: we have two issues with the BT handover: (1) the patch for bypassing the pairing dialog does not seem to work. Even with the patch applied, we get the pairing dialog for outgoing file transfers. (2) we manage to crash the device when trying defaultAdapter.sendFile(mac, new Blob([], {type: 'image/png'}));

Hi Arno, (In reply to arno from comment #54) > Eric: we have two issues with the BT handover: > (1) the patch for bypassing the pairing dialog does not seem to work. Even > with the patch applied, we get the pairing dialog for outgoing file > transfers. Please rebase your branch onto the latest m-c, or you can cherry-pick the patch of bug 947060 manually. (https://hg.mozilla.org/mozilla-central/rev/c1a13692027d) > (2) we manage to crash the device when trying defaultAdapter.sendFile(mac, > new Blob([], {type: 'image/png'})); It's because Gecko Bluetooth only accepts nsIDOMFile object (http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIDOMFile.idl#30). So please pick an existing file from storage and give it another try. It should just work.

(In reply to arno from comment #49) > Created attachment 8346667 [details] > Support for file transfer in HandoverManager > > Evelyn: it seems the hot potato has come back to you! In this pull request I > reverted some changes where I misunderstood Ian. I addressed all of his > comments based on my interpretation of his feedback. Lets take this from the > top and discuss the changes you would like to have done. One note on an > earlier question/comment from you: this bug is about adding new DOM API for > sending a blob. This also requires changes to the Gaia system app where the > handover is handled. Sid will very soon upload the matching Gecko patches. > If you prefer, I will create a new bug just for the Gaia changes with the > next iteration of your feedback. I'm reviewing.. and yes, please file a bug for Gaia changes. Thanks.

Quick feedback: I don't want to be picky but Alive is not happy to see there is a function with 700-line code. I'm so sorry I didn't point this out when I was reviewing bug 903305, because I thought we were in a tight schedule. Considering unit test, it's better to use object pattern and move NdefHandoverCodec and rest parsing things out of HandoverManager. We should also move NdefUtils out and even remove redundant code in NFCManager. To be concrete: 1. move NdefHandoverCodec and rest parsing things out 2. move NdefUtils out, and file a follow-up bug for remove redundant code in NFCManager. (or do it in this patch, I'm fine) 3. use object pattern for HandoverManager. It's better for unit test, and also align with NFCManager. 4. file bugs for adding unit test. Please also checkout comment 48, I'm echoing Alive's opinion.

Comment on attachment 8346667 [details] Support for file transfer in HandoverManager I think it's good at BT file transfer part. We are looking for a code structure re-org to make the flow more clear and testable.

(In reply to Evelyn Hung [:evelyn] from comment #56) > (In reply to arno from comment #49) > > Created attachment 8346667 [details] > > Support for file transfer in HandoverManager > > > > Evelyn: it seems the hot potato has come back to you! In this pull request I > > reverted some changes where I misunderstood Ian. I addressed all of his > > comments based on my interpretation of his feedback. Lets take this from the > > top and discuss the changes you would like to have done. One note on an > > earlier question/comment from you: this bug is about adding new DOM API for > > sending a blob. This also requires changes to the Gaia system app where the > > handover is handled. Sid will very soon upload the matching Gecko patches. > > If you prefer, I will create a new bug just for the Gaia changes with the > > next iteration of your feedback. > > I'm reviewing.. and yes, please file a bug for Gaia changes. Thanks. Since the whole conversation has happened here and we are almost done, I'm fine *without* a separate bug for gaia. Please ignore my comment 56.

I've done a major refactoring of the code, basically changing its structure back to when you told me not to clutter the global namespace. I've created one global object NfcUtil that now has all NDEF/Handover codecs. Please take a look at NfcUtil.createBuffer(). I didn't know a better way to do without adding more to the global namespace. I've not changed NFC Manager beyond what I had to do for this bug. We already filed bug 943691 a while ago. I will clean up NFC Manager once this bug lands.

Hi Eric, To follow up on Arno's question, we still have same two issues. Issue#1: We are still not able to create a proper File blob. B2G still crashes! (Perhaps, you may want to file a security bug to track this). Here is how we pick a file blob for testing purposes. Can you tell us anything is wrong with this? var pick = new MozActivity({ name: "pick", data: { type: ["image/png", "image/jpg", "image/jpeg"]
 } }); pick.onsuccess = function () {
 var fakeBlob = this.result.blob; var req = nfcPeer.sendFile(fakeBlob); } Issue#2 : We still get the 'pairing dialog' for outgoing pairing transfer. Please note that we do have the following patch in our code base: https://hg.mozilla.org/mozilla-central/rev/c1a13692027d

Hi Sid, (In reply to Siddartha P from comment #61) > Hi Eric, > To follow up on Arno's question, we still have same two issues. > > Issue#1: We are still not able to create a proper File blob. B2G still > crashes! (Perhaps, you may want to file a security bug to track this). Here > is how we pick a file blob for testing purposes. > Can you tell us anything is wrong with this? > > var pick = new MozActivity({ > name: "pick", > data: { > type: ["image/png", "image/jpg", "image/jpeg"]
 } > }); > pick.onsuccess = function () {
 > var fakeBlob = this.result.blob; > var req = nfcPeer.sendFile(fakeBlob); > } > ok, I'll take a look today. > Issue#2 : We still get the 'pairing dialog' for outgoing pairing transfer. > Please note that we do have the following patch in our code base: > https://hg.mozilla.org/mozilla-central/rev/c1a13692027d Hm, then could you please try to apply the following patch? diff --git a/dom/bluetooth/bluedroid/BluetoothOppManager.cpp b/dom/bluetooth/bluedroid/BluetoothOppManager.cpp --- a/dom/bluetooth/bluedroid/BluetoothOppManager.cpp +++ b/dom/bluetooth/bluedroid/BluetoothOppManager.cpp @@ -263,17 +263,17 @@ BluetoothOppManager::ConnectInternal(con BluetoothService* bs = BluetoothService::Get(); if (!bs || sInShutdown || mSocket) { OnSocketConnectError(mSocket); return; } mSocket = - new BluetoothSocket(this, BluetoothSocketType::RFCOMM, false, true); + new BluetoothSocket(this, BluetoothSocketType::RFCOMM, false, false); mSocket->Connect(aDeviceAddress, -1); } void BluetoothOppManager::HandleShutdown() { MOZ_ASSERT(NS_IsMainThread()); sInShutdown = true;

Comment on attachment 8348443 [details] Support for file transfer in HandoverManager The code is clear to me now, thanks! r+ with the file name nit addressed. Do we have unit-test bugs?

Comment on attachment 8347823 [details] [diff] [review] (v1) Part 2: SendFile support in chrome process Review of attachment 8347823 [details] [diff] [review]: ----------------------------------------------------------------- See https://bugzilla.mozilla.org/show_bug.cgi?id=948850#c1, please update the documentation first. I already got some Gaia developers asking me about your API. Also try to add some diagrams here for better understanding, which app will call sendFile, then which app will receive the system message. Clear r? for the questions for blob and requestId. ::: dom/system/gonk/Nfc.js @@ +295,5 @@ > return null; > } > > // Add extra permission check for two IPC Peer events: > // 'NFC:CheckP2PRegistration' , 'NFC:NotifyUserAcceptedP2P' update comment here. @@ +581,5 @@ > this.sendToWorker("close", message.json); > break; > + case "NFC:SendFile": > + // Chrome process is the arbitrator / mediator between Content process > + // that issued nfc 'sendFile' operation and system-process (content) I guess you want to say "system app (content process)"? ::: dom/system/gonk/NfcContentHelper.js @@ +216,5 @@ > }); > return request; > }, > > + sendFile: function sendFile(window, data, sessionToken) { data? I thought it was 'blob'. @@ +228,5 @@ > + > + cpmm.sendAsyncMessage("NFC:SendFile", { > + requestId: requestId, > + sessionToken: sessionToken, > + blob: data.blob data.blob? Can you elaborate the dict for the blob in the IDL? ::: dom/system/gonk/nsINfcContentHelper.idl @@ +55,5 @@ > + * Returns DOMRequest, if initiation of send file operation is successful > + * then 'onsuccess' is called else 'onerror' > + */ > + nsIDOMDOMRequest sendFile(in nsIDOMWindow window, > + in jsval aBlob, blob, unless you want to rename all other arguments to start with 'a'. @@ +123,5 @@ > + * @param window > + * Current window > + * > + * @param status > + * Status of sendFile operation (SUCCESS : 0 / FAILURE: 1) const these two intergers, as you did for NFC_EVENT_PEER_READY/LOST @@ +130,5 @@ > + * Request ID of SendFile DOM Request > + */ > + void notifySendFileStatus(in nsIDOMWindow window, > + in octet status, > + in DOMString requestId); What's the requestId here? Where does this come from?

Hi Arno, > (2) we manage to crash the device when trying defaultAdapter.sendFile(mac, > new Blob([], {type: 'image/png'})); I just tried this command and no crash occurred on my device. In addition, I'd like to correct what I said in comment 55, Gecko Bluetooth is capable of dealing with both Blob and File. During my test, I did find a problem which is that the OBEX session won't get released if you send a blob with length 0. I'll take time fixing this but I don't think this will block NFC implementation. All you have to do is create a blob with content. For instance, you could start from sending a txt file which contains string 'hello world': + var blob = new Blob(['hello', ' world'], {type: 'text/plain'}); + defaultAdapter.sendFile([mac], blob); It works on my device. Of course you could also change the type to 'image/png', create a valid .png file and send. Would you mind trying again?

So why do we add nfc-send-file-status event? Couldn't we expose just some API to the system app to send file? (Actually, the same question for nfc-p2p-user-accept)

Comment on attachment 8347821 [details] [diff] [review] (v1) Part 1: SendFile DOM API ...that would make using the API simpler.

(In reply to Eric Chou [:ericchou] [:echou] from comment #55) > Hi Arno, > > (In reply to arno from comment #54) > > Eric: we have two issues with the BT handover: > > (1) the patch for bypassing the pairing dialog does not seem to work. Even > > with the patch applied, we get the pairing dialog for outgoing file > > transfers. > > Please rebase your branch onto the latest m-c, or you can cherry-pick the > patch of bug 947060 manually. > (https://hg.mozilla.org/mozilla-central/rev/c1a13692027d) > > > (2) we manage to crash the device when trying defaultAdapter.sendFile(mac, > > new Blob([], {type: 'image/png'})); > Update: bug 915602 seems like this. We'll start to investigate why it would crash. To avoid this, you could try 1) make devices paired with each other before transferring files or 2) set a 3 sec timeout before calling sendFile(), just like what bluetooth app does now (https://github.com/mozilla-b2g/gaia/blob/master/apps/bluetooth/js/deviceList.js#L266)

> 2) set a 3 sec timeout before calling sendFile(), just like what bluetooth > app does now > > (https://github.com/mozilla-b2g/gaia/blob/master/apps/bluetooth/js/ > deviceList.js#L266) Evenly, as talked with Eric, he is looking into this bug. But in order not to block the development process of NFC. I would like to suggest Arno using the current method used in deviceList.js to avoid this problem. Is it Okay for you?

(In reply to Ken Chang[:ken] from comment #69) > > 2) set a 3 sec timeout before calling sendFile(), just like what bluetooth > > app does now > > > > (https://github.com/mozilla-b2g/gaia/blob/master/apps/bluetooth/js/ > > deviceList.js#L266) > > Evenly, as talked with Eric, he is looking into this bug. But in order not > to block the development process of NFC. I would like to suggest Arno using > the current method used in deviceList.js to avoid this problem. Is it Okay > for you? yes, please do. Thanks.

Thanks Yoshi, Olli for your review comments. I will address them. Yoshi, I have attached the pdf that briefly states the 'sendFile' flow between various processes. Please give your feedback.

Olli, >> So why do we add nfc-send-file-status event? >> Couldn't we expose just some API to the system app to send file? >> (Actually, the same question for nfc-p2p-user-accept) Do you think it is OK to add two new nfc DOM APIs that are visible to all nfc gaia applications. Though these nfc applications may not be able to use these apis (as the APIs will be protected by 'nfc-manager' perms). What do you suggest ? Thanks,

Hi Kyle, I have created a brief PDF file: SendFile.pdf flow describing its flow. Could you please suggest us the way to send / relay a blob from Content process to Chrome process. In the pdf, it is at STEP#4. Upon further testing on my side, it looks like the blob that I tried to send using cpmm (from NfcContentHelper.js) is not received correctly on the other side by Chrome (Nfc.js). Any suggestions? Note that this blob would be of type 'nsIDOMFile'.

(In reply to Siddartha P from comment #73) > Do you think it is OK to add two new nfc DOM APIs that are visible to all > nfc gaia applications. Though these nfc applications may not be able to use > these apis (as the APIs will be protected by 'nfc-manager' perms). What do > you suggest ? We need a small separate API which is visible only when nfc-manager perm is set.

I think you can even just use the NFC API but have it implement [NoInterfaceObject, Func="Navigator::HasNfcManagerSupport"] interface NFCManager {...} Then MozNfc implements NFCManager; Or if that doesn't work, add the methods to MozNFC but enable them with Func="Navigator::HasNfcManagerSupport" Navigator::HasNfcManagerSupport would do the permission check.

(In reply to Olli Pettay [:smaug] from comment #76) > I think you can even just use the NFC API but have it implement > [NoInterfaceObject, Func="Navigator::HasNfcManagerSupport"] > interface NFCManager {...} > > > Then > MozNfc implements NFCManager; > WebIDL Implementation question: Is it NoInterfaceObject hides that NFC Manager stuff, or the HasNfcManagerSupport somehow hide the base?

NoInterfaceObject hides the interface from the global scope. So window.YourNoInterfaceObject would be undefined

Sorry for taking a bit longer. Fixed all nits. I've added the delay in BluetoothTransfer since that is where sendFile() is called. I've added the identical comment as in deviceList.js.

Comment on attachment 8349887 [details] Support for file transfer in NfcHandoverManager r=ehung looks good to me. I don't have a device to test if it works, but the code logic is okay.

Hi Yoshi, >> please update the documentation first. >> I already got some Gaia developers asking me about your API. Done updated the wiki, with sendFile sniplet >> Also try to add some diagrams here for better understanding, >> which app will call sendFile, then which app will receive the system message. Done . Attached the 'SendFile flow' attachment for your review @@ +295,5 @@ > return null; > } > > // Add extra permission check for two IPC Peer events: > // 'NFC:CheckP2PRegistration' , 'NFC:NotifyUserAcceptedP2P' >> update comment here. Done >> I guess you want to say >> "system app (content process)"? done > + sendFile: function sendFile(window, data, sessionToken) { >> data? I thought it was 'blob'. Actually the blob that is being passed from nsNfc.js needs to 'object wrapped'. Hence had to use jsval. If the blob is not object wrapped, I see the following exceptions in the logs at the time of calling NfcContentHelper's sendFile interface. E/GeckoConsole( 1031): [JavaScript Error: "Permission denied to access property 'toString'"] E/GeckoConsole( 1031): [JavaScript Error: "Permission denied to access property 'message'"] E/GeckoConsole( 1031): [JavaScript Error: "uncaught exception: unknown (can't convert to string)"] > + > + cpmm.sendAsyncMessage("NFC:SendFile", { > + requestId: requestId, > + sessionToken: sessionToken, > + blob: data.blob >> data.blob? Same explanation as above point. Any suggestions please ? >> Can you elaborate the dict for the blob in the IDL? done. >> blob, unless you want to rename all other arguments to start with 'a'. done. >> const these two intergers, as you did for NFC_EVENT_PEER_READY/LOST ('status' value is sent by System App). The value of 'status' is not checked / processed in Nfc.js. Eventually Nfc.js notifies the right content process (nfc gaia application) and simply relays the 'status' value. In NfcContentHelper.js, this value is checked against NFC.GECKO_NFC_ERROR_SUCCESS Updated the idl with the same vals. > + void notifySendFileStatus(in nsIDOMWindow window, > + in octet status, > + in DOMString requestId); >> What's the requestId here? >> Where does this come from? 'requestId' is relayed by system app to Nfc.js which in turn relays this back to right / appropriate content process. NfcContentHelper uses this to handle the response and fires either success / failure based on the 'status'. In the attached ' sendFile flow PDF' this requestID is created at STEP#4.

Hi Olli, >> I think you can even just use the NFC API but have it implement >> [NoInterfaceObject, Func="Navigator::HasNfcManagerSupport"] >> interface NFCManager {...} >> Then MozNfc implements NFCManager; This comment will be addressed in a separate bug https://bugzilla.mozilla.org/show_bug.cgi?id=952217 The dom patch now contains only 'sendFile' changes. Also note that I had to perform a 'slice()' operation on the 'blob' data. Otherwise, when NfcContentHelper.js relays this data to Nfc.js (Chrome process), the blob data seems to be getting deleted (I guess) immediately. So the blob the Nfc.js sends to system application is corrupted / freed. What do you think ?

Comment on attachment 8350791 [details] [diff] [review] (v2) Part 2: SendFile support in chrome process r=allstars.chh Review of attachment 8350791 [details] [diff] [review]: ----------------------------------------------------------------- I think some part of this patch should also be moved to Bug 952217.

Hi Yoshi, As suggested, I have removed 'NotifySendFileStatus' part from earlier patch.

Comment on attachment 8356227 [details] [diff] [review] (v2b) Part 2: SendFile support in chrome process r=allstars.chh Review of attachment 8356227 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/NfcContentHelper.js @@ +49,5 @@ > "NFC:ConnectResponse", > "NFC:CloseResponse", > "NFC:CheckP2PRegistrationResponse", > + "NFC:PeerEvent", > + "NFC:SendFileResponse" This message will be received until Bug 952217 is landed, either: - Remove this message. And do it in Bug 952217. or - Add a TODO here. ::: dom/system/gonk/nsINfcContentHelper.idl @@ +40,5 @@ > nsIDOMDOMRequest connect(in nsIDOMWindow window, in unsigned long techType, in DOMString sessionToken); > nsIDOMDOMRequest close(in nsIDOMWindow window, in DOMString sessionToken); > > /** > + * Initiate Send file operation You didn't update UUID.

Fixed both the comments

tryResults of the patches : https://tbpl.mozilla.org/?tree=Try&rev=f9e10d23b9ae

Check-in needed for Gecko. Waiting on Travis results for Gaia. Arno?

https://hg.mozilla.org/integration/b2g-inbound/rev/b211721ef063 https://hg.mozilla.org/integration/b2g-inbound/rev/18836f818241 https://hg.mozilla.org/integration/b2g-inbound/rev/0b211385b164 I will merge the PR when the b-i push is green. Should this have tests?

(In reply to Garner Lee from comment #91) > Waiting on Travis results for Gaia. Arno? Travis has repeatable failures. Presumably because Gaia needs the underlying Gecko changes first? Let's re-run Travis tomorrow after this merges to m-c.

https://hg.mozilla.org/mozilla-central/rev/b211721ef063 https://hg.mozilla.org/mozilla-central/rev/18836f818241 https://hg.mozilla.org/mozilla-central/rev/0b211385b164

Master: https://github.com/mozilla-b2g/gaia/commit/9397654096519ce88b2b624278098cb1601724e4