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

RESOLVED FIXED in Firefox 18

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: ericchou, Assigned: ericchou)

Tracking

unspecified
mozilla19
ARM
Gonk (Firefox OS)
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

(Whiteboard: [LOE:S])

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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().
(Assignee)

Updated

6 years ago
Assignee: nobody → echou
Blocks: 792683
blocking-basecamp: --- → ?
Whiteboard: [LOE:S]
(Assignee)

Comment 1

6 years ago
Created attachment 672294 [details] [diff] [review]
patch 1: v1: DOM API change

* Added function  "confirmReceivingFile()" to nsIDOMBluetoothAdapter
* Implementation of confirmReceivingFile() in BluetoothAdapter
Attachment #672294 - Flags: superreview?(mrbkap)
Attachment #672294 - Flags: review?(kyle)
(Assignee)

Comment 2

6 years ago
Created attachment 672300 [details] [diff] [review]
patch 2: v1: IPC Implementation of the newly added function

* Implemented the call flow from child(BluetoothAdapter) to parent(BluetoothOppManager)
Attachment #672300 - Flags: review?(kyle)
blocking-basecamp: ? → +
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
Attachment #672300 - Flags: review?(kyle) → review+
(Assignee)

Comment 4

6 years ago
(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.
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.
(Assignee)

Comment 6

6 years ago
(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. :)
(Assignee)

Comment 7

6 years ago
Created attachment 672762 [details] [diff] [review]
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.
Attachment #672762 - Flags: review?(kyle)
(Assignee)

Comment 8

6 years ago
Created attachment 672764 [details] [diff] [review]
patch 2: final: IPC Implementation of the newly added function, r=qdot

Nits picked.
Attachment #672300 - Attachment is obsolete: true
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?
Attachment #672762 - Flags: review?(kyle) → review+
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.

Updated

6 years ago
Attachment #672294 - Flags: superreview?(mrbkap) → superreview+
https://hg.mozilla.org/mozilla-central/rev/dfca7b8e5c71
https://hg.mozilla.org/mozilla-central/rev/93f9402ae7bc
https://hg.mozilla.org/mozilla-central/rev/96184c897bfc
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(Assignee)

Comment 14

6 years ago
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.
Attachment #672294 - Flags: approval-mozilla-aurora?
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.
Attachment #672294 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.