802590 - [b2g-bluetooth] Add function "ConfirmReceivingFile()" to DOM API

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Eric Chou [:ericchou] [:echou]]

When Gaia gets a system message about "remote device is requesting to send a file to us", a dialog will pop up for user being able to decide to accept it or not. So we need a new method for this, and it should be added to nsIDOMBluetoothAdapter as well as other Object Push related functions, such as sendFile() and stopSendingFile().

Comment 1

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 1: v1: DOM API change

* Added function  "confirmReceivingFile()" to nsIDOMBluetoothAdapter
* Implementation of confirmReceivingFile() in BluetoothAdapter

Comment 2

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 2: v1: IPC Implementation of the newly added function

* Implemented the call flow from child(BluetoothAdapter) to parent(BluetoothOppManager)

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 672300 [details] [diff] [review]
patch 2: v1: IPC Implementation of the newly added function

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

::: dom/bluetooth/ipc/BluetoothServiceChildProcess.cpp
@@ +352,5 @@
> +  BluetoothReplyRunnable* aRunnable)
> +{
> +  if(aConfirm) {
> +    SendRequest(aRunnable,
> +                ConfirmReceivingFileRequest(nsString(aDeviceAddress)));

Nit: Just return early, no else.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +2558,5 @@
> +{
> +  // Currently we only support one device sending one file at a time,
> +  // so we don't need aDeviceAddress here because the target device
> +  // has been determined when calling 'Connect()'. Nevertheless, keep
> +  // it for future use.

Nit: Check for main thread

Comment 4

[Eric Chou [:ericchou] [:echou]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #3)
> Comment on attachment 672300 [details] [diff] [review]
> patch 2: v1: IPC Implementation of the newly added function
> 
> Review of attachment 672300 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/ipc/BluetoothServiceChildProcess.cpp
> @@ +352,5 @@
> > +  BluetoothReplyRunnable* aRunnable)
> > +{
> > +  if(aConfirm) {
> > +    SendRequest(aRunnable,
> > +                ConfirmReceivingFileRequest(nsString(aDeviceAddress)));
> 
> Nit: Just return early, no else.
> 

Sorry but I don't quite understand this. Here I use aConfirm to decide which request (ConfirmReceivingFileRequest or DenyReceivingFileRequest) should be sent, just like how we did for those pairing related functions. 

Could you please give more information to me? Thanks.

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ah, my comment should've gone on the next line. The way you're doing this is right, I just had a stylistic super-nit that it should be if (aConfirm) { ...; return; } with no else block after it (and what's currently in the else block can just follow since it'll excute when the if branch doesn't). Returning early when the if branch triggers.

Comment 6

[Eric Chou [:ericchou] [:echou]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #5)
> Ah, my comment should've gone on the next line. The way you're doing this is
> right, I just had a stylistic super-nit that it should be if (aConfirm) {
> ...; return; } with no else block after it (and what's currently in the else
> block can just follow since it'll excute when the if branch doesn't).
> Returning early when the if branch triggers.

Got it! Thanks for the explanation. :)

Comment 7

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 3: v1: Implemented the logic after user responded

We'll send a "Continue/Success" response to remote if we got confirmation from the user. On the other hand, we'll send an "Unauthorized/Unauthorized(with Final bit set)" response if the user does not want to receive the file.

Comment 8

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 2: final: IPC Implementation of the newly added function, r=qdot

Nits picked.

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 672762 [details] [diff] [review]
patch 3: v1: Implemented the logic after user responded

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

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +220,5 @@
>  BluetoothOppManager::ConfirmReceivingFile(bool aConfirm,
>                                            BluetoothReplyRunnable* aRunnable)
>  {
> +  if (!mWaitingForConfirmationFlag) {
> +    NS_WARNING("We are not waiting for a confirmation now.");

Nit: Probably don't need to land this message?

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 672762 [details] [diff] [review]
patch 3: v1: Implemented the logic after user responded

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

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +220,5 @@
>  BluetoothOppManager::ConfirmReceivingFile(bool aConfirm,
>                                            BluetoothReplyRunnable* aRunnable)
>  {
> +  if (!mWaitingForConfirmationFlag) {
> +    NS_WARNING("We are not waiting for a confirmation now.");

Ok, ignore my nit. I misread the context for this.

Comment 11

[Eric Chou [:ericchou] [:echou]]

try:
https://tbpl.mozilla.org/?tree=Try&rev=cd7a9ab58780
https://tbpl.mozilla.org/?tree=Try&rev=c366c260775a

Comment 12

[Eric Chou [:ericchou] [:echou]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfca7b8e5c71
https://hg.mozilla.org/integration/mozilla-inbound/rev/93f9402ae7bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/96184c897bfc

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/dfca7b8e5c71
https://hg.mozilla.org/mozilla-central/rev/93f9402ae7bc
https://hg.mozilla.org/mozilla-central/rev/96184c897bfc

Comment 14

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 672294 [details] [diff] [review]
patch 1: v1: DOM API change

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 792683 (Bluetooth ObjectPush)
User impact if declined: They won't be able to choose to accept or reject the file transfer request.
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): Should be fairly low risk, only added a new function.
String or UUID changes made by this patch: dom/bluetooth/nsIDOMBluetoothAdapter UUID change

It's a newly added function in nsIDOMBluetoothAdapter, and this function will only be used by Bluetooth file-sharing app(which has not been done yet) for v1.

Comment 15

[Alex Keybl [:akeybl]]

Comment on attachment 672294 [details] [diff] [review]
patch 1: v1: DOM API change

blocking-basecamp+ and very little chance of causing any regressions in our other shipping products. Approved.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6553d2484e2c
https://hg.mozilla.org/releases/mozilla-aurora/rev/b2ed60d05f2f
https://hg.mozilla.org/releases/mozilla-aurora/rev/bee23c0aba17