1195604 - [cleanup] Integrate similar BluetoothResultHandlers into basic one

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

Integrate the following result handlers into |BasicResultHandler| since they behave the same:
- CancelDiscoveryResultHandler
- CreateBondResultHandler
- GetRemoteServicesResultHandler
- RemoveBondResultHandler
- SetAdapterPropertyResultHandler
- StartDiscoveryResultHandler

Comment 1

[Ben Tian (inactive)]

Attachment: Patch 1 (v1): Integrate similar BluetoothResultHandlers into basic one

Comment 2

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

Comment on attachment 8649068 [details] [diff] [review]
Patch 1 (v1): Integrate similar BluetoothResultHandlers into basic one

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

Hi! There's a bug in the patch and the current code. So r- for now.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +705,5 @@
>  
>    return NS_OK;
>  }
>  
> +class BluetoothServiceBluedroid::BasicResultHandler final

Please call this |DispatchReplyErrorResultHandler| to indicate what it actually does.

@@ +714,3 @@
>      nsTArray<nsRefPtr<BluetoothReplyRunnable>>& aRunnableArray,
>      BluetoothReplyRunnable* aRunnable)
>    : mRunnableArray(aRunnableArray)

Field initializers should have an indention of 4 spaces here.

@@ +715,5 @@
>      BluetoothReplyRunnable* aRunnable)
>    : mRunnableArray(aRunnableArray)
>    , mRunnable(aRunnable)
> +  {
> +    MOZ_ASSERT(mRunnable);

That's not correct AFAICT. Users push a runnable pointer into an array, even if the pointer is |nullptr|. On errors, we have to remove the nullptr pointer as well.

@@ +727,5 @@
>      DispatchReplyError(mRunnable, aStatus);
>    }
>  
>  private:
>    nsTArray<nsRefPtr<BluetoothReplyRunnable>> mRunnableArray;

This line looks like a bug, doesn't it?

|OnError| is supposed to remove the runnable from the |m*Array| in |BluetoothServiceBluedroid|, but this code doesn't do that. This line should be a reference to the array: 'nsTArray<nsRefPtr<BluetoothReplyRunnable>>& mRunnableArray'.

Comment 3

[Ben Tian (inactive)]

Attachment: [final] Patch 1: Integrate similar BluetoothResultHandlers into basic one, r=tzimmermann

Changes:
- rename |BasicResultHandler| to |DispatchReplyErrorResultHandler|
- fix indention of field initializers
- handle case that runnable pointer is |nullptr|
- keep reference to the runnable and GetDeviceRequest array in result handler

Comment 4

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

Comment on attachment 8649148 [details] [diff] [review]
[final] Patch 1: Integrate similar BluetoothResultHandlers into basic one, r=tzimmermann

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

Thanks for fixing all indention. r+ with the one final bug fixes.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +724,2 @@
>      mRunnableArray.RemoveElement(mRunnable);
> +    if (mRunnable) {

The previous version was correct here. You might have pushed nullptr into the array. In case of an error, you also have to remove it.

Comment 5

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

Sorry, I mis-read the patch. The current version is correct. Please land it! :)

Comment 6

[Ben Tian (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=30f74c54da17

Comment 7

[Ben Tian (inactive)]

Set dependency to avoid code landing conflict.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/83d57a912d96

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/83d57a912d96