1164498 - Replace |DispatchBluetoothReply| by v2 interfaces

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

We can reduce the differences between Bluetooth v1 and v2 by replacing |DispatchBluetoothReply| with either |DispatchReplySuccess| or |DispatchReplyError|.

Comment 1

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

Attachment: [01] Bug 1164498: Cleanup Bluetooth utility functions for dispatching replies

Comment 2

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

Attachment: [02] Bug 1164498: Remove |DispatchBluetoothReply| from |BluetoothServiceBluedroid|

Comment 3

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

Attachment: [03] Bug 1164498: Remove |DispatchBluetoothReply| from |BluetoothProfileController|

Comment 4

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

Attachment: [04] Bug 1164498: Remove |DispatchBluetoothReply| from |BluetoothHfpManger|

Comment 5

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

Attachment: [05] Bug 1164498: Remove |DispatchBluetoothReply|

Comment 6

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=20af10cb1ca8

Comment 7

[Ben Tian (inactive)]

Comment on attachment 8605435 [details] [diff] [review]
[01] Bug 1164498: Cleanup Bluetooth utility functions for dispatching replies

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

::: dom/bluetooth/BluetoothUtils.cpp
@@ +282,5 @@
>  {
>    MOZ_ASSERT(aRunnable);
>    MOZ_ASSERT(aValue.type() != BluetoothValue::T__None);
>  
> +#if MOZ_B2G_BT_API_V2

Remove the whole #if since the same assertion already exists above.

::: dom/bluetooth/BluetoothUtils.h
@@ +166,5 @@
>   * Dispatch failed bluetooth reply with error status.
>   *
> + * TODO: Remove this function. |BluetoothStatus| is an internal
> + *       error code and not supposed to be exported. Its value
> + *       should be converted to a string before dispatching it.

We should keep it since Promises rejection doesn't accept error strings (bug 1016560). As a result, bug 1032755 converts |BluetoothStatus| to nsresult for promise rejection. That's why we have this function.

Comment 8

[Ben Tian (inactive)]

Comment on attachment 8605438 [details] [diff] [review]
[02] Bug 1164498: Remove |DispatchBluetoothReply| from |BluetoothServiceBluedroid|

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

r=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1178,5 @@
>  #else
>    int requestedDeviceCount = aDeviceAddress.Length();
>    if (requestedDeviceCount == 0) {
>      InfallibleTArray<BluetoothNamedValue> emptyArr;
> +    DispatchReplySuccess(aRunnable, BluetoothValue(emptyArr));

We can remove |BluetoothValue| since |BluetoothNamedValue| is a BluetoothValue [1].

  DispatchReplySuccess(aRunnable, emptyArr);

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluetooth1/ipc/BluetoothTypes.ipdlh#24

@@ +1214,5 @@
>  #else
>    void StartDiscovery() override
>    {
>      MOZ_ASSERT(NS_IsMainThread());
> +    DispatchReplySuccess(mRunnable, true);

Simplify to following since [2].
  
  DispatchReplySuccess(mRunnable);

[2] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/BluetoothUtils.cpp#277

@@ +1263,5 @@
>  #else
>    void CancelDiscovery() override
>    {
>      MOZ_ASSERT(NS_IsMainThread());
> +    DispatchReplySuccess(mRunnable, true);

Ditto.

@@ +1533,5 @@
>    { }
>  
>    void PinReply() override
>    {
> +    DispatchReplySuccess(mRunnable, BluetoothValue(true));

Ditto.

@@ +1633,5 @@
>    { }
>  
>    void SspReply() override
>    {
> +    DispatchReplySuccess(mRunnable, BluetoothValue(true));

Ditto.

@@ +2367,5 @@
>  
>    // Send reply for SetProperty
>  
>    if (!sSetPropertyRunnableArray.IsEmpty()) {
> +    DispatchReplySuccess(sSetPropertyRunnableArray[0], BluetoothValue(true));

Ditto.

@@ +2936,5 @@
>                                         NS_LITERAL_STRING(KEY_ADAPTER),
>                                         BluetoothValue(propertiesChangeArray)));
>  
>        if (bonded && !sBondingRunnableArray.IsEmpty()) {
> +        DispatchReplySuccess(sBondingRunnableArray[0], BluetoothValue(true));

Ditto.

@@ +2941,4 @@
>  
>          sBondingRunnableArray.RemoveElementAt(0);
>        } else if (!bonded && !sUnbondingRunnableArray.IsEmpty()) {
> +        DispatchReplySuccess(sUnbondingRunnableArray[0], BluetoothValue(true));

Ditto.

Comment 9

[Ben Tian (inactive)]

Comment on attachment 8605439 [details] [diff] [review]
[03] Bug 1164498: Remove |DispatchBluetoothReply| from |BluetoothProfileController|

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

r=me with comment addressed.

::: dom/bluetooth/bluetooth1/BluetoothProfileController.cpp
@@ +264,5 @@
>  
>    // The action has completed, so the DOM request should be replied then invoke
>    // the callback.
>    if (mSuccess) {
> +    DispatchReplySuccess(mRunnable, BluetoothValue(true));

Simplify to
  DispatchReplySuccess(mRunnable);

Comment 10

[Ben Tian (inactive)]

Comment on attachment 8605441 [details] [diff] [review]
[04] Bug 1164498: Remove |DispatchBluetoothReply| from |BluetoothHfpManger|

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

LGTM.

Comment 11

[Ben Tian (inactive)]

Comment on attachment 8605442 [details] [diff] [review]
[05] Bug 1164498: Remove |DispatchBluetoothReply|

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

LGTM.

Comment 12

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

Attachment: [01] Bug 1164498: Cleanup Bluetooth utility functions for dispatching replies (v2)

Changes since v1:

  - further cleanup according to review

Thanks for the rational on the use of BluetoothStatus. I was afraid that BluetoothStatus would some day become (unintentionally) part of the WebAPI. Looks like that's been solved already. :)

Comment 13

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

Attachment: [02] Bug 1164498: Remove |DispatchBluetoothReply| from |BluetoothServiceBluedroid| (v2)

Changes since v1:

  - simplified calls to |DispatchReplySuccess| according to review

Comment 14

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

Attachment: [03] Bug 1164425: Cleanup interfaces of |ListenSocket| (v2)

Changes since v1:

  - simplified calls to |DispatchReplySuccess| according to review

Comment 15

[Ben Tian (inactive)]

Comment on attachment 8606972 [details] [diff] [review]
[01] Bug 1164498: Cleanup Bluetooth utility functions for dispatching replies (v2)

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

LGTM.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/7f2717436cfa
https://hg.mozilla.org/integration/b2g-inbound/rev/ff414c3f20f0
https://hg.mozilla.org/integration/b2g-inbound/rev/4e8ad5389059
https://hg.mozilla.org/integration/b2g-inbound/rev/baa0862beb07
https://hg.mozilla.org/integration/b2g-inbound/rev/77f2b3ba0280

Comment 17

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

Thanks Ben!

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

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/7f2717436cfa
https://hg.mozilla.org/mozilla-central/rev/ff414c3f20f0
https://hg.mozilla.org/mozilla-central/rev/4e8ad5389059
https://hg.mozilla.org/mozilla-central/rev/baa0862beb07
https://hg.mozilla.org/mozilla-central/rev/77f2b3ba0280