1006308 - (webbt-api-onoff) Implement BT on/off

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

* BT on/off: https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothAdapter
Properties
  readonly attribute BluetoothAdapterState state;
  readonly attribute DOMString address;
  readonly attribute DOMString name;
  readonly attribute boolean discoverable;
  readonly attribute boolean discovering;
Event handler
  attribute EventHandler onattributechanged;
Methods
  Promise<void> enable();
  Promise<void> disable();
BluetoothAdapterState
BluetoothAdapterAttribute

Comment 1

[Jocelyn Liu [:jocelyn] [:joliu]]

Note: BluetoothAttributeEvent is needed while firing onattributechanged

Comment 2

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Patch1: Implement adapter enable/disable functions

* Add Enable and Disable functions in BT adapter and services.
* Change StartStopBluetooth/StartBluetooth/StopBluetooth function signature to include BluetoothReplyRunnable since we need to wrap a promise.

Comment 3

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Patch2: Add StartInternal/StopInternal ipc requests

* Add StartInternal and StopInternal ipc request

Comment 4

[Jocelyn Liu [:jocelyn] [:joliu]]

Note: Event firing and Promise::Resolve() is still under revised.

Comment 5

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Patch1(v2): Implement adapter enable/disable functions

* Add Enable and Disable functions in BT adapter and services.
* Change StartStopBluetooth/StartBluetooth/StopBluetooth function signature to include BluetoothReplyRunnable since we need to wrap a promise.

Comment 6

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Patch2(v2): Add StartInternal/StopInternal ipc requests

* Add StartInternal and StopInternal ipc request

Comment 7

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Patch3: Add SetJsVal() util function to get JS::Value from BluetoothValue

*Add an util function to convert BluetoothValue to JS::Value

Comment 8

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Patch4: Fire onattributechanged event while turn on/off BT

* Revise the behavior when adapter received PropertyChanged to fire onattributechanged
* In ToggleBtAck::Run(), fire PropertyChanged to adapter when BT state changes to disabled/enabled
* Resolve promise for BluetoothAdapter.Enable()/Disable()

Comment 9

[Ben Tian (inactive)]

Comment on attachment 8430659 [details] [diff] [review]
Patch1(v2): Implement adapter enable/disable functions

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

Please see my comments below.

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +167,5 @@
> +  bool
> +  ParseSuccessfulReply(JS::MutableHandle<JS::Value> aValue)
> +  {
> +    mPromise->MaybeResolve(
> +        NS_LITERAL_STRING("Enable/Disable task resolved"));

Why this string for Promise<void>?

@@ +177,5 @@
> +  ReleaseMembers()
> +  {
> +    BluetoothReplyRunnable::ReleaseMembers();
> +    mPromise = nullptr;
> +  }

nit: newline.

@@ +195,5 @@
>    , mDiscovering(false)
>    , mPairable(false)
>    , mPowered(false)
>    , mIsRooted(false)
> +  , mState(BluetoothAdapterState::Enabled) //TODO: Change to Disabled

Detail the comment about when to change the init state to Disabled.

nit: move this line to right before |mDiscoverable(false)| to clean warning during compilation.

@@ +700,5 @@
> +already_AddRefed<Promise>
> +BluetoothAdapter::EnableDisable(bool aEnable)
> +{
> +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(GetOwner());
> +  if (!global) {

Use macro |NS_ENSURE_TRUE| instead.

@@ +706,5 @@
> +    return nullptr;
> +  }
> +  nsRefPtr<Promise> promise = new Promise(global);
> +
> +  if ((aEnable && (mState != BluetoothAdapterState::Disabled)) ||

nit: removing the parenthesis of != condition is clearer.
if ((aEnable && mState != BluetoothAdapterState::Disabled) ||
    (!aEnable && mState != BluetoothAdapterState::Enabled)) {

@@ +710,5 @@
> +  if ((aEnable && (mState != BluetoothAdapterState::Disabled)) ||
> +      (!aEnable && (mState != BluetoothAdapterState::Enabled))) {
> +    promise->MaybeReject(
> +        NS_LITERAL_STRING(
> +          "Invalid status to enable/disable BT"));

Set error string "InvalidAdapterStateError" as [1] for applications to handle errors. Moreover, you can define some common errors as [2].
[1] http://dxr.mozilla.org/mozilla-central/source/dom/telephony/Telephony.cpp#264
[2] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/BluetoothProfileManagerBase.h#15

nit: wrap into one line.

@@ +718,5 @@
> +  BluetoothService* bs = BluetoothService::Get();
> +  if (!bs) {
> +    promise->MaybeReject(
> +        NS_LITERAL_STRING(
> +          "Enable/Disable BT failed due to get bt service failed"));

Ditto.

@@ +722,5 @@
> +          "Enable/Disable BT failed due to get bt service failed"));
> +    return promise.forget();
> +  }
> +
> +  mState = aEnable ?

nit: it's clearer to rewrite as
mState = aEnable ? BluetoothAdapterState::Enabling
                 : BluetoothAdapterState::Disabling;

@@ +730,5 @@
> +    new EnableDisableAdapterTask(promise);
> +
> +  if(NS_FAILED(bs->EnableDisable(aEnable, result))) {
> +    promise->MaybeReject(
> +        NS_LITERAL_STRING("Failed to enable/disable the adapter"));

Ditto.

@@ +740,5 @@
>  already_AddRefed<Promise>
>  BluetoothAdapter::Enable()
>  {
> +  nsRefPtr<Promise> promise =
> +    EnableDisable(true);

nit: wrap into one line.

@@ +748,5 @@
>  already_AddRefed<Promise>
>  BluetoothAdapter::Disable()
>  {
> +  nsRefPtr<Promise> promise =
> +    EnableDisable(false);

Ditto.

::: dom/bluetooth2/BluetoothAdapter.h
@@ +122,5 @@
>      SetAuthorization(const nsAString& aDeviceAddress, bool aAllow,
>                       ErrorResult& aRv);
>  
>    already_AddRefed<Promise>
> +    EnableDisable(bool aEnable);

nit: wrap into one line as it doesn't exceed 80 characters.

@@ +128,1 @@
>      Enable();

Ditto.

@@ +128,3 @@
>      Enable();
>    already_AddRefed<Promise>
>      Disable();

Ditto.

::: dom/bluetooth2/BluetoothService.cpp
@@ +623,5 @@
>      }
>  
>      SWITCH_BT_DEBUG(value.toBoolean());
>  
>      return NS_OK;

Remove this return since this func returns immediately.

@@ +820,5 @@
>                                      JS::UndefinedHandleValue);
>  }
> +
> +/**
> + * Enable/Disable the local adapter.

nit: newline.

@@ +823,5 @@
> +/**
> + * Enable/Disable the local adapter.
> + * There is only one adapter on the mobile in current use cases.
> + * In addition, bluedroid couldn't enable/disable a single adapter.
> + * So currently we will turn on/off BT to enable/disable the adapter.

nit: newline.

@@ +833,5 @@
> +                                BluetoothReplyRunnable* aRunnable)
> +{
> +  sToggleInProgress = true;
> +  return StartStopBluetooth(aEnable, false, aRunnable);
> +}

nit: move this function before |Notify| to group with |AdapterAddedReceived| and |TryFiringAdapterAdded|.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ -1,2 @@
> -/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */
> -/* vim: set ts=2 et sw=2 tw=80: */

Why remove these two lines?

@@ +52,5 @@
>  static nsTArray<nsRefPtr<BluetoothReplyRunnable> > sChangeDiscoveryRunnableArray;
>  static nsTArray<nsRefPtr<BluetoothReplyRunnable> > sGetDeviceRunnableArray;
>  static nsTArray<nsRefPtr<BluetoothReplyRunnable> > sSetPropertyRunnableArray;
>  static nsTArray<nsRefPtr<BluetoothReplyRunnable> > sUnbondingRunnableArray;
> +static nsTArray<nsRefPtr<BluetoothReplyRunnable> > sEnableDisableAdapterRunnableArray;

nit: alphabetical order.

How about rename to sChangeAdapterStateRunnableArray to conform with sChangeDiscoveryRunnableArray?

@@ +782,4 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  if(aRunnable) {

Add comment to explain why aRunnable may be nullptr.

@@ +804,4 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  if(aRunnable) {

Ditto.

::: dom/bluetooth2/bluez/BluetoothDBusService.cpp
@@ +2101,2 @@
>  {
> +  MOZ_ASSERT(!aRunnable);

nit: newline.

@@ +2230,2 @@
>  {
> +  MOZ_ASSERT(!aRunnable);

Ditto.

::: dom/bluetooth2/ipc/BluetoothServiceChildProcess.h
@@ +202,5 @@
> +  StartInternal(BluetoothReplyRunnable* aRunnable) MOZ_OVERRIDE;
> +
> +  virtual nsresult
> +  StopInternal(BluetoothReplyRunnable* aRunnable) MOZ_OVERRIDE;
> +

Why are these functions protected instead of public?

Comment 10

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Patch1(v3): Implement adapter enable/disable functions

* Address review comments listed in Comment9
* Fold original patch 2(ipc) into patch1

Comment 11

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Patch1(v4): Implement adapter enable/disable functions

* Notify adapter for the state property change part is extracted from patch4 and added into this patch

Comment 12

[Ben Tian (inactive)]

Comment on attachment 8433273 [details] [diff] [review]
Patch1(v4): Implement adapter enable/disable functions

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

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +167,5 @@
> +
> +  bool
> +  ParseSuccessfulReply(JS::MutableHandle<JS::Value> aValue)
> +  {
> +    mPromise->MaybeResolve(true);

Add comment to note we return boolean for Promise<void>.

@@ +708,5 @@
> +  NS_ENSURE_TRUE(global, nullptr);
> +
> +  nsRefPtr<Promise> promise = new Promise(global);
> +
> +  if ((aEnable && mState != BluetoothAdapterState::Disabled) ||

I suggest to rearrange code as following to make enable and disable logic clearer, and call BluetoothService function right after getting it.

  if (aEnable) {
    // Turn on bluetooth
    if (mState != BluetoothAdapterState::Disabled) {
      promise->MaybeReject(ERR_INVALID_ADAPTER_STATE);
      return promise.forget();
    }
    mState = BluetoothAdapterState::Enabling;
  } else {
    // Turn off bluetooth
    if (mState != BluetoothAdapterState::Enabled) {
      promise->MaybeReject(ERR_INVALID_ADAPTER_STATE);
      return promise.forget();
    }
    mState = BluetoothAdapterState::Disabling;
  }

  // TODO: Fire attribute changed event for this state change
  nsRefPtr<BluetoothReplyRunnable> result = new EnableDisableAdapterTask(promise);

  BluetoothService* bs = BluetoothService::Get();
  if (!bs) {
    promise->MaybeReject(ERR_CHANGE_ADAPTER_STATE);
    return promise.forget();
  }

::: dom/bluetooth2/BluetoothAdapter.h
@@ +25,5 @@
>  
>  BEGIN_BLUETOOTH_NAMESPACE
>  
> +#define ERR_INVALID_ADAPTER_STATE "InvalidAdapterStateError"
> +#define ERR_CHANGE_ADAPTER_STATE "ChangeAdapterStateError"

Move error definition to BluetoothAdapter.cpp, the file where they are used.

::: dom/bluetooth2/BluetoothService.cpp
@@ +752,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  InfallibleTArray<BluetoothNamedValue> props;
> +  uint32_t state = aEnable ?

I suggest to leave the state logic in BluetoothAdapter to keep BluetoothService clean from BluetoothAdapterState. Fire the event with only a boolean of enabled or not.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +147,5 @@
>    {
>      MOZ_ASSERT(NS_IsMainThread());
>  
> +    /*
> +     * Cleanup static adapter properties and notify adapter to clean them

nit: a newline to make TODO item obvious.

@@ +150,5 @@
> +    /*
> +     * Cleanup static adapter properties and notify adapter to clean them
> +     * TODO: clean up and notify Discovering also
> +     */
> +    sAdapterBdAddress = EmptyString();

Use sAdapterBdAddress.Truncate() and sAdapterBdName.Truncate() to avoid creating a new EmptyString().

@@ +806,4 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  // aRunnable will be null during startup and shutdown

1. "nullptr"
2. "only during startup" since |StartInternal| is not called during shutdown.

@@ +829,4 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  // aRunnable will be null during startup and shutdown

"nullptr"

::: dom/bluetooth2/ipc/BluetoothServiceChildProcess.cpp
@@ +377,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
> +BluetoothServiceChildProcess::StartInternal(BluetoothReplyRunnable* aRunnable)

Move implementation of |StartInternal| and |StopInternal| after |GetAdaptersInternal| as their order in header file.

Comment 13

[Ben Tian (inactive)]

Comment on attachment 8433273 [details] [diff] [review]
Patch1(v4): Implement adapter enable/disable functions

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

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +736,5 @@
>  already_AddRefed<Promise>
>  BluetoothAdapter::Enable()
>  {
> +  nsRefPtr<Promise> promise = EnableDisable(true);
> +  return promise.forget();

One more thing: you could return |EnableDisable(true)| directly.

@@ +743,5 @@
>  already_AddRefed<Promise>
>  BluetoothAdapter::Disable()
>  {
> +  nsRefPtr<Promise> promise = EnableDisable(false);
> +  return promise.forget();

Ditto.

Comment 14

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Patch1(v5): Implement adapter enable/disable functions

* Addressed review comments in Comment12 and Comment13.

Comment 15

[Ben Tian (inactive)]

Comment on attachment 8433867 [details] [diff] [review]
Patch1(v5): Implement adapter enable/disable functions

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

r=me with nit addressed. Thanks for the effort!

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +24,5 @@
>  #include "BluetoothService.h"
>  #include "BluetoothUtils.h"
>  
> +#define ERR_INVALID_ADAPTER_STATE "InvalidAdapterStateError"
> +#define ERR_CHANGE_ADAPTER_STATE "ChangeAdapterStateError"

nit: add extra space to align error strings.

#define ERR_INVALID_ADAPTER_STATE "InvalidAdapterStateError"
#define ERR_CHANGE_ADAPTER_STATE  "ChangeAdapterStateError"

Comment 16

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Patch: Implement adapter enable/disable functions, r=btian

Thanks for reviewing, Ben!

Comment 17

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Dev] Gaia patch for testing BT on/off

Comment 18

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Patch: Implement adapter enable/disable functions, r=btian

attached the wrong file previously..

Comment 19

[Ben Tian (inactive)]

Remove 'checkin-needed' first. Will check in this fix after bug 1019372 patches.

Comment 20

[Ben Tian (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/2e1364395956

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2e1364395956