Closed Bug 1031253 Opened 6 years ago Closed 6 years ago

Reject promise if StartStopGonkBluetooth failed

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: yrliou, Assigned: yrliou)

References

Details

(Whiteboard: webbt-api)

Attachments

(1 file, 2 obsolete files)

Should dispatch a bluetoothreply to reject promise for BluetoothAdapter::Enable() and BluetoothAdapter::Disable if StartStopGonkBluetooth failed.
* Fix error handling bug for bt on/off.
Attachment #8447836 - Flags: review?(btian)
Comment on attachment 8447836 [details] [diff] [review]
Bug 1031253 - Patch(v1): Reject promise if StartStopGonkBluetooth failed.

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

::: dom/bluetooth2/BluetoothService.cpp
@@ +380,5 @@
>     * even if Bluetooth is already enabled.
>     *
>     * Please see bug 892392 for more information.
>     */
>    if (aIsStartup || !sBluetoothService->IsEnabled()) {

Remove redundant |sBluetoothService->|.

@@ +382,5 @@
>     * Please see bug 892392 for more information.
>     */
>    if (aIsStartup || !sBluetoothService->IsEnabled()) {
>      // Switch Bluetooth on
>      if (NS_FAILED(sBluetoothService->StartInternal(aRunnable))) {

Ditto.

@@ +384,5 @@
>    if (aIsStartup || !sBluetoothService->IsEnabled()) {
>      // Switch Bluetooth on
>      if (NS_FAILED(sBluetoothService->StartInternal(aRunnable))) {
>        BT_WARNING("Bluetooth service failed to start!");
> +      return NS_ERROR_FAILURE;

We should return the return value of |StartInternal(aRunnable))| instead of NS_ERROR_FAILURE.

@@ +441,5 @@
>     * even if Bluetooth is disabled.
>     *
>     * Please see bug 892392 for more information.
>     */
>    if (aIsStartup || sBluetoothService->IsEnabled()) {

Ditto.

@@ +443,5 @@
>     * Please see bug 892392 for more information.
>     */
>    if (aIsStartup || sBluetoothService->IsEnabled()) {
>      // Switch Bluetooth off
>      if (NS_FAILED(sBluetoothService->StopInternal(aRunnable))) {

Ditto.

@@ +445,5 @@
>    if (aIsStartup || sBluetoothService->IsEnabled()) {
>      // Switch Bluetooth off
>      if (NS_FAILED(sBluetoothService->StopInternal(aRunnable))) {
>        BT_WARNING("Bluetooth service failed to stop!");
> +      return NS_ERROR_FAILURE;

Ditto.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +933,5 @@
>      }
> +
> +    // Reject Promise
> +    if(aRunnable) {
> +      DispatchBluetoothReply(aRunnable, BluetoothValue(),

Error strings suffice for BluetoothReplyRunnable w/ DOMRequest but w/ promise we need to pass corresponding nsresult error. Please file a followup bug to make BluetoothReplyRunnable reply with nsresult errors.

@@ +935,5 @@
> +    // Reject Promise
> +    if(aRunnable) {
> +      DispatchBluetoothReply(aRunnable, BluetoothValue(),
> +                             NS_LITERAL_STRING(ERR_START_BLUETOOTH));
> +      sChangeAdapterStateRunnableArray.RemoveElementAt(0);

sChangeAdapterStateRunnableArray.RemoveElement(aRunnable);

@@ +967,5 @@
> +    // Reject Promise
> +    if(aRunnable) {
> +      DispatchBluetoothReply(aRunnable, BluetoothValue(),
> +                             NS_LITERAL_STRING(ERR_STOP_BLUETOOTH));
> +      sChangeAdapterStateRunnableArray.RemoveElementAt(0);

Ditto.
Blocks: 1032755
* Address review comments
* Follow up bug of nsresult error is Bug 1032755
Attachment #8447836 - Attachment is obsolete: true
Attachment #8447836 - Flags: review?(btian)
Attachment #8448630 - Flags: review?(btian)
Comment on attachment 8448630 [details] [diff] [review]
Bug 1031253 - Patch(v2): Reject promise if StartStopGonkBluetooth failed.

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

r=me. Thanks!
Attachment #8448630 - Flags: review?(btian) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5f64dfc096cd
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.