791147 - [b2g-bluetooth] Implement pairing from remote side

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Bug 755943 just now landed ipc support for bluetooth.  Let's hope that pairing is easy!

Kyle, Ben and I suggest that we use this work as a hand-off bug.  Let's walk through what's required to implement this and then have the "bluetooth team" implement.  We'll all hold hands and skip through the happy fields all the way.

Comment 1

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

Just an update, we are now officially blocked on this. We can get to a pairing code dialog in gaia, but we end up hitting "implement me" messages. So I guess I'll work on this now. :)

Comment 2

[Tony Chung [:tchung]]

triage team, please + this for blocking-basecamp.   We can't pair a phone to a computer successfully.

Comment 3

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

Attachment: Patch 1 (v1, WIP) - Make Bluetooth Pairing Replies use DOMRequests

Comment 4

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

Attachment: Patch 2 (v1, WIP) - Hook up Bluetooth Pairing Replies to OOP

Comment 5

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

Attachment: Patch 1 (v2) - Make Bluetooth Pairing Replies use DOMRequests

Comment 6

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

Attachment: Patch 2 (v2) - Hook up Bluetooth Pairing Replies to OOP

Comment 7

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

There's some pretty hardcore copypasta happening here that I'm happy to clean up in a refactoring followup (honestly we could use a big refactoring followup for B2G in general). Just trying to get the pairing flow done ASAP at the moment.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 662045 [details] [diff] [review]
Patch 1 (v2) - Make Bluetooth Pairing Replies use DOMRequests

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

r=me with the first thing fixed and the second thing explained

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +608,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsRefPtr<BluetoothVoidReplyRunnable> results = new BluetoothVoidReplyRunnable(req);
> +  req.forget(aRequest);

Here and below, don't set aRequest unless you return NS_OK.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +581,5 @@
>  {
> +#ifdef MOZ_WIDGET_GONK
> +  // Due to the fact that we're running two dbus loops on desktop implicitly by
> +  // being gtk based, sometimes we'll get signals/reply coming in on the main
> +  // thread. There's not a lot we can do about that for the time being and it

This seems crazy to me... Surely DBus won't call this particular callback function on random threads?

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 662046 [details] [diff] [review]
Patch 2 (v2) - Hook up Bluetooth Pairing Replies to OOP

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

r=me with this fixed.

::: dom/bluetooth/ipc/BluetoothParent.h
@@ +148,5 @@
>    bool
>    DoRequest(const DevicePropertiesRequest& aRequest);
> +
> +	bool
> +	DoRequest(const SetPinCodeRequest& aRequest);

Nit: These are all using tabs.

::: dom/bluetooth/ipc/BluetoothServiceChildProcess.h
@@ +106,5 @@
>  
>    virtual bool
>    SetPairingConfirmationInternal(const nsAString& aDeviceAddress,
> +                                 bool aConfirm,
> +                                 BluetoothReplyRunnable* aRunnable) MOZ_OVERRIDE;

Nit: These go past 80 chars.

::: dom/bluetooth/ipc/PBluetooth.ipdl
@@ +74,5 @@
> +
> +struct SetPairingConfirmationRequest
> +{
> +  nsString path;
> +  bool confirm;

Here it would make more sense to have two request types rather than one with a bool. E.g.:

  struct ConfirmPairingRequest
  {
    nsString path;
  };

  struct DenyPairingRequest
  {
    nsString path;
  };

Same for SetAuthorizationRequest.

Comment 10

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

(In reply to ben turner [:bent] from comment #8)
> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +581,5 @@
> >  {
> > +#ifdef MOZ_WIDGET_GONK
> > +  // Due to the fact that we're running two dbus loops on desktop implicitly by
> > +  // being gtk based, sometimes we'll get signals/reply coming in on the main
> > +  // thread. There's not a lot we can do about that for the time being and it
> 
> This seems crazy to me... Surely DBus won't call this particular callback
> function on random threads?


This has to do with how we deal with dbus. On desktop linux, there's a dbus signal handler implicit in our gtk pump on the main thread, as well as our own dbus pump in ipc/dbus/DBusThread. Since the callback is called in the context of whichever thread picks it up, that's where the issue happens. This is definitely something we should file a followup to handle, but for now it's only an issue in desktop testing. This won't happen on the phone since we completely control dbus there.

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f09f3b91889
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d5013260334

Comment 12

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

Attachment: Patch 1 (Final) - Make Bluetooth Pairing Replies use DOMRequests

Comment 13

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

Attachment: Patch 2 (Final) - Hook up Bluetooth Pairing Replies to OOP

Comment 14

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

Relevent try info at https://tbpl.mozilla.org/?tree=Try&rev=438bead09ab5

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/3f09f3b91889
https://hg.mozilla.org/mozilla-central/rev/1d5013260334