Closed Bug 791147 Opened 7 years ago Closed 7 years ago

[b2g-bluetooth] Implement pairing from remote side

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: cjones, Assigned: qdot)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
Assignee: nobody → kyle
Depends on: 791182
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. :)
blocking-basecamp: --- → ?
triage team, please + this for blocking-basecamp.   We can't pair a phone to a computer successfully.
blocking-basecamp: ? → +
Attachment #662040 - Attachment is obsolete: true
Attachment #662045 - Flags: review?(bent.mozilla)
Attachment #662041 - Attachment is obsolete: true
Attachment #662046 - Flags: review?(bent.mozilla)
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 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?
Attachment #662045 - Flags: review?(bent.mozilla) → review+
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.
Attachment #662046 - Flags: review?(bent.mozilla) → review+
(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.
Target Milestone: --- → mozilla18
Blocks: 795458
You need to log in before you can comment on or make changes to this bug.