929891 - [Bluetooth][bluedroid] Enable/Disable Bluetooth

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Enable / Disable Bluetooth.
Gaia can get events (AdapterAdded / Disabled)

Comment 1

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 929891 - [Bluetooth][bluedroid] Enable/Disable Bluetooth

Comment 2

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 929891 - [Bluetooth][bluedroid] Enable/Disable Bluetooth

This patch only enable/disable bluetooth, gaia can get AdapterAdded/Disabled event

Comment 3

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 820898 [details] [diff] [review]
Bug 929891 - [Bluetooth][bluedroid] Enable/Disable Bluetooth

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

Hi Shawn, please see my comments below. Most are nits. Happy to review again once new patch is ready.

::: dom/bluetooth/BluetoothServiceBluedroid.cpp
@@ +67,5 @@
> +/**
> + *  Static callback functions
> + */
> +static void
> +AdapterStateChangeCallback(bt_state_t status)

nit: s/status/aStatus

@@ +82,5 @@
> +    signalName = NS_LITERAL_STRING("Disabled");
> +  }
> +
> +  BluetoothValue aValue = true;
> +  BluetoothSignal signal(signalName, NS_LITERAL_STRING(KEY_MANAGER), aValue);

Since this is the only place aValue is used, how about just

  BluetoothSignal signal(signalName, NS_LITERAL_STRING(KEY_MANAGER), BluetoothValue(true));

@@ +111,5 @@
> +  }
> +  module->methods->open(module, BT_HARDWARE_MODULE_ID, &device);
> +  sBtDevice = (bluetooth_device_t *)device;
> +  sBtInterface = sBtDevice->get_bluetooth_interface();
> +  BT_LOGD("Bluetooth HAL loaded");

super-nit: please add an extra line here. Then it would be more consistent with what you have done in EnsureBluetoothHalUnload().

@@ +120,5 @@
> +EnsureBluetoothHalUnload()
> +{
> +  int err = 0;
> +  sBtInterface = nullptr;
> +  BT_LOGD("Bluetooth HAL unloaded (%s)", strerror(err));

Doesn't seem to be needed since err would be always 0 in this case. Please refine this function.

@@ +127,5 @@
> +}
> +
> +static nsresult
> +StartStopGonkBluetooth(bool aShouldEnable)
> +{

nit: MOZ_ASSERT(!NS_IsMainThread())

Comment 4

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 929891 - [Bluetooth][bluedroid] Enable/Disable Bluetooth

Comment 5

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 929891 - [Bluetooth][bluedroid] Enable/Disable Bluetooth

Comment 6

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

> @@ +120,5 @@
> > +EnsureBluetoothHalUnload()
> > +{
> > +  int err = 0;
> > +  sBtInterface = nullptr;
> > +  BT_LOGD("Bluetooth HAL unloaded (%s)", strerror(err));
> 
> Doesn't seem to be needed since err would be always 0 in this case. Please
> refine this function.
I think this function shall be removed since it's unused function, and only assign null.

Comment 7

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 929891 - [Bluetooth][bluedroid] Enable/Disable Bluetooth

Comment 8

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 822134 [details] [diff] [review]
Bug 929891 - [Bluetooth][bluedroid] Enable/Disable Bluetooth

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

::: dom/bluetooth/BluetoothServiceBluedroid.cpp
@@ +125,5 @@
> +
> +  if (!EnsureBluetoothHalLoad()) {
> +    BT_LOGR("Failed to load bluedroid library.\n");
> +    return NS_ERROR_FAILURE;
> +  }

I just found that there is a slight difference between this function and the same name function in BluetoothGonkService.cpp. The other function would return NS_OK if "Gaia requests to enable(disable) BT when Bluetooth is enabled(disabled)", but NS_ERROR_FAILURE would be returned here under the same circumstances.

Please add the condition check to ensure the logic in both functions are the same.

Comment 9

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 929891 - [Bluetooth][bluedroid] Enable/Disable Bluetooth

Comment 10

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 822236 [details] [diff] [review]
Bug 929891 - [Bluetooth][bluedroid] Enable/Disable Bluetooth

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

Looks good. Thanks.

Comment 11

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

https://tbpl.mozilla.org/?tree=Try&rev=27c41c082427

Comment 12

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I also built JB 4.3 + Bluedroid combination.

Comment 13

[Eric Chou [:ericchou] [:echou]]

https://hg.mozilla.org/integration/b2g-inbound/rev/80734c181020

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80734c181020