1223806 - Move Bluetooth Core interface into separate class

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

The core Bluetooth backend interfaces are currently provided by |BluetoothInterface| and implemented by |BluetoothDaemonInterface|.

There shoudl be a separate interface |BluetoothCoreInterface| with implementation. |BluetoothInterface| will only be responsible for the connection to the Bluetooth daemon.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1223806: Add Bluetooth Core interface, notification and result handler

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1223806: Convert Bluetooth to |BluetoothCoreResultHandler|

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1223806: Convert Bluetooth to |BluetoothCoreNotificationHandler|

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 1223806: Add |BluetoothDaemonCoreInterface|

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [05] Bug 1223806: Convert Bluetooth to |BluetoothCoreInterface|

Comment 6

[Bruce Sun [:brsun]]

Hi Thomas,

Sorry for the late reply.

Basically these patches look good to me. But since Ben has some concrete ideas on refining interfaces (see bug 1221477), I would suggest to let him discuss more on these changes with you. I am more than happy to input my comments if debate occurs.

Comment 7

[Ben Tian (inactive)]

Comment on attachment 8686069 [details] [diff] [review]
[04] Bug 1223806: Add |BluetoothDaemonCoreInterface|

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

LGTM

Comment 8

[Ben Tian (inactive)]

Comment on attachment 8686066 [details] [diff] [review]
[01] Bug 1223806: Add Bluetooth Core interface, notification and result handler

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

LGTM

Comment 9

[Ben Tian (inactive)]

Comment on attachment 8686067 [details] [diff] [review]
[02] Bug 1223806: Convert Bluetooth to |BluetoothCoreResultHandler|

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

LGTM

Comment 10

[Ben Tian (inactive)]

Comment on attachment 8686068 [details] [diff] [review]
[03] Bug 1223806: Convert Bluetooth to |BluetoothCoreNotificationHandler|

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

LGTM

Comment 11

[Ben Tian (inactive)]

Comment on attachment 8686070 [details] [diff] [review]
[05] Bug 1223806: Convert Bluetooth to |BluetoothCoreInterface|

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

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +144,4 @@
>      BluetoothService* bs = BluetoothService::Get();
>      NS_ENSURE_TRUE_VOID(bs);
>  
> +    auto coreInterface = sBtInterface->GetBluetoothCoreInterface();

How about remember |sBtCoreInterface| to save these additional checks?

@@ +1152,5 @@
>  
> +  auto coreInterface = sBtInterface->GetBluetoothCoreInterface();
> +  if (!coreInterface) {
> +    DispatchReplyError(aRunnable, STATUS_FAIL);
> +    return NS_ERROR_FAILURE;

Why do some methods return NS_OK but some returns NS_ERROR_FAILURE?

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [05] Bug 1223806: Convert Bluetooth to |BluetoothCoreInterface| (v2)

Changes since v1:

  - store core-interface pointer for service manager

This change should also resolve the comment about the return values.

Comment 13

[Ben Tian (inactive)]

Comment on attachment 8687972 [details] [diff] [review]
[05] Bug 1223806: Convert Bluetooth to |BluetoothCoreInterface| (v2)

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

LGTM. Note macros in [1] should be revised as well. I'll open a followup bug for it.

Comment 14

[Ben Tian (inactive)]

Comment on attachment 8687972 [details] [diff] [review]
[05] Bug 1223806: Convert Bluetooth to |BluetoothCoreInterface| (v2)

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

One more place to revise.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +2246,3 @@
>      // Bluetooth scan mode is SCAN_MODE_CONNECTABLE by default, i.e., it should
>      // be connectable and non-discoverable.
>      NS_ENSURE_TRUE_VOID(sBtInterface);

Replace with |sBtCoreInterface|.

Comment 15

[Ben Tian (inactive)]

Thomas,

Two more questions after patch 5 is landed:
1) Does non-null |sBtCoreInterface| implies non-null |sBtInterface|?
2) Does "BluetoothService::IsEnabled() == true" implies non-null |sBtCoreInterface|?

(In reply to Ben Tian [:btian] from comment #14)
> Comment on attachment 8687972 [details] [diff] [review]
> [05] Bug 1223806: Convert Bluetooth to |BluetoothCoreInterface| (v2)
> 
> Review of attachment 8687972 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One more place to revise.
> 
> ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
> @@ +2246,3 @@
> >      // Bluetooth scan mode is SCAN_MODE_CONNECTABLE by default, i.e., it should
> >      // be connectable and non-discoverable.
> >      NS_ENSURE_TRUE_VOID(sBtInterface);
> 
> Replace with |sBtCoreInterface|.

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi

Yes, both should be true.

(In reply to Ben Tian [:btian] from comment #15)
> Thomas,
> 
> Two more questions after patch 5 is landed:
> 1) Does non-null |sBtCoreInterface| implies non-null |sBtInterface|?

I don't think the current code is very strict about setting |sBtInterface| to nullptr, but the intend is that a valid address in |sBtCoreInterface| always implies a valid address in |sBtInterface|. The start and stop machinery in |BluetoothServiceBluedroid| should take care of this.

> 2) Does "BluetoothService::IsEnabled() == true" implies non-null
> |sBtCoreInterface|?

Yes, |sBtCoreInterface::Enable| is required to enable Bluetooth. There's no way to enable Bluetooth without calling it first.

Comment 17

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1223806: Convert Bluetooth to |BluetoothCoreResultHandler| (v2)

Changes since v1:

  - rebased onto latest m-c

Comment 18

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [05] Bug 1223806: Convert Bluetooth to |BluetoothCoreInterface| (v3)

Changes since v2:

  - rebased onto latest m-c
  - fixed test for |sBtCoreInterface|

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/958acda55a11
https://hg.mozilla.org/integration/b2g-inbound/rev/e01293abbc3d
https://hg.mozilla.org/integration/b2g-inbound/rev/9bbf22cb4f63
https://hg.mozilla.org/integration/b2g-inbound/rev/489a24c0094b
https://hg.mozilla.org/integration/b2g-inbound/rev/64cd192b8d12

Comment 20

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks for the review!

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=64cd192b8d12

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/958acda55a11
https://hg.mozilla.org/mozilla-central/rev/e01293abbc3d
https://hg.mozilla.org/mozilla-central/rev/9bbf22cb4f63
https://hg.mozilla.org/mozilla-central/rev/489a24c0094b
https://hg.mozilla.org/mozilla-central/rev/64cd192b8d12