Closed Bug 929891 Opened 6 years ago Closed 6 years ago

[Bluetooth][bluedroid] Enable/Disable Bluetooth

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

References

Details

Attachments

(1 file, 5 obsolete files)

Enable / Disable Bluetooth.
Gaia can get events (AdapterAdded / Disabled)
Attachment #820885 - Attachment is obsolete: true
Attachment #820885 - Flags: review?(echou)
This patch only enable/disable bluetooth, gaia can get AdapterAdded/Disabled event
Attachment #820898 - Flags: review?(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())
Attachment #820898 - Attachment is obsolete: true
Attachment #820898 - Flags: review?(echou)
Attachment #822114 - Attachment is obsolete: true
Attachment #822114 - Flags: review?(echou)
> @@ +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.
Attachment #822131 - Attachment is obsolete: true
Attachment #822131 - Flags: review?(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.
Attachment #822134 - Attachment is obsolete: true
Attachment #822134 - Flags: review?(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.
Attachment #822236 - Flags: review?(echou) → review+
I also built JB 4.3 + Bluedroid combination.
https://hg.mozilla.org/mozilla-central/rev/80734c181020
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.