Closed Bug 802590 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: echou, Assigned: echou)

References

Details

(Whiteboard: [LOE:S])

Attachments

(3 files, 1 obsolete file)

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: nobody → echou
Blocks: 792683
blocking-basecamp: --- → ?
Whiteboard: [LOE:S]
* Added function  "confirmReceivingFile()" to nsIDOMBluetoothAdapter
* Implementation of confirmReceivingFile() in BluetoothAdapter
Attachment #672294 - Flags: superreview?(mrbkap)
Attachment #672294 - Flags: review?(kyle)
* Implemented the call flow from child(BluetoothAdapter) to parent(BluetoothOppManager)
Attachment #672300 - Flags: review?(kyle)
blocking-basecamp: ? → +
Attachment #672294 - Flags: review?(kyle) → review+
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+
(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.
(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. :)
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)
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.
Attachment #672294 - Flags: superreview?(mrbkap) → superreview+
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.