1063449 - Implement GATT connection related attributes, methods, and event handler

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

* BluetoothDevice: https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothDevice#connectGatt.28boolean_aAutoConnect.29
Property
  readonly attribute BluetoothGatt? gatt;
Method
  [NewObject, Throws] Promise<BluetoothGatt> connectGatt(boolean aAutoConnect);

* BluetoothGatt: https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGatt
Property
  readonly attribute BluetoothConnectionState connectionState;
Event handler
           attribute EventHandler onconnectionstatechanged;
Methods
  [NewObject, Throws] Promise<void> connect();
  [NewObject, Throws] Promise<void> disconnect();
  [NewObject, Throws] Promise<short> readRemoteRssi();

Comment 1

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

Attachment: [WIP] Bug 1063449: Implement GATT client connection related functionalities.

Comment 2

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

Attachment: Bug 1063449 - Patch1: Add and implement GATT client connection related bluetooth Web APIs.

This patch mainly covers:
  1) device.connectGatt() to start and connect as a GATT client to remote device.
  2) Implement register/unregister client, connect/disconnect methods in BluetoothGatt object.
  3) Add/Modify related webidls for 1) and 2).

Comment 3

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

Attachment: Bug 1063449 - Patch2: Implement gatt client connection related functions, result handlers, notification handlers in BluetoothGattManger.

Comment 4

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

Attachment: [Local] Bug 1063449 - Patch3: Marionette tests for gatt client connection.

Comment 5

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

Comment on attachment 8540016 [details] [diff] [review]
Bug 1063449 - Patch1: Add and implement GATT client connection related bluetooth Web APIs.

Hi Ben,

Please help to review my patches for gatt client connection APIs.
Patch summary:
Patch1:
  1) add connectGatt in BluetoothDevice2.webidl and implement this API.
  2) add gatt client connection related APIs in BluetoothGatt.webidl and implement it.

Patch2:
  Implement connection related functions, result handlers, notification handlers in BluetoothGattManager to interact with BT stack and send necessary signals to BluetoothGatt and BluetoothDevice.

Patch3:
  marionette test for testing connection related APIs.

Note that, readRemoteRssi() is not added yet, will implement this function in another bug.
Besides, gatt.close() is added to let applications explicitly close unused gatt clients.

I will also request a DOM peer for webidl review afterwards.

Thanks,
Jocelyn

Comment 6

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

Forgot to mention one thing.
For connectGatt,
"discovers services offered by a remote LE device as well as their characteristics and descriptors" described in the document[1] is not implemented yet since we haven't implement BluetoothGattService, BluetoothGattCharacteristic, and BluetoothGattDescriptor.

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothDevice#connectGatt.28boolean_aAutoConnect.29

Comment 7

[Ben Tian (inactive)]

Comment on attachment 8540016 [details] [diff] [review]
Bug 1063449 - Patch1: Add and implement GATT client connection related bluetooth Web APIs.

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

Please see my comment below. In sum,
 1) BluetoothGatt object should be created after ConnectGatt succeeds,
 2) I wonder if we can register client and connect at once in parent process, and
 3) be consistent on either 'gatt' or 'GATT.'

::: dom/bluetooth2/BluetoothCommon.h
@@ +186,5 @@
> +/**
> + * When a remote BLE device gets connected / disconnected, we'll dispatch an
> + * event
> + */
> +#define GATT_CONNECTION_STATUS_CHANGED_ID    "connectionstatechanged"

Replace STATUS with STATE to be consistent.

::: dom/bluetooth2/BluetoothDevice.cpp
@@ +235,5 @@
> +
> +  nsRefPtr<Promise> promise = Promise::Create(global, aRv);
> +  NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
> +
> +  BT_ENSURE_TRUE_REJECT(mType == BluetoothDeviceType::Le ||

Also ensure mGatt is nullptr.

@@ +244,5 @@
> +    new ConnectGattTask(promise,
> +                        NS_LITERAL_STRING("ConnectGatt"),
> +                        this);
> +
> +  mGatt = BluetoothGatt::Create(GetOwner(), aAutoConnect, mAddress, result);

Based on [1], |mGatt| should be created when |ConnectGattTask| gets successful reply. Otherwise |mGatt| remains even if |ConnectGattTask| fails; also the reply runnable argument |result| becomes redundant accordingly. 

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothDevice#connectGatt.28boolean_aAutoConnect.29

::: dom/bluetooth2/BluetoothDevice.h
@@ +193,5 @@
> +
> +  /**
> +   * Gatt client object to interact with the remote device.
> +   */
> +  nsRefPtr<BluetoothGatt> mGatt;

Set mGatt=nullptr in BluetoothDevice constructor.

::: dom/bluetooth2/BluetoothGatt.cpp
@@ +105,5 @@
> +  MOZ_ASSERT(aWindow);
> +
> +  NS_ENSURE_TRUE(aRunnable, nullptr);
> +
> +  // Generate a random UUID for this gatt client

Wrap UUID generation into a function.

@@ +219,5 @@
> +{
> +  BT_API2_LOGR("Gatt connection state changes to: %d", int(aState));
> +  mConnectionState = aState;
> +
> +  // Dispatch connectionstatechanged event to application

Wrap empty event dispatching into a function, so that |onservicechanged| [2] can reuse it.

[2] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGatt#onservicechanged

@@ +235,5 @@
> +
> +void
> +BluetoothGatt::HandleClientRegistered(const BluetoothValue& aValue)
> +{
> +  MOZ_ASSERT(aValue.type() == BluetoothValue::Tuint32_t);

nit: newline after this line.

@@ +245,5 @@
> +
> +  // Connect to the remote device
> +  bs->ConnectGattClientInternal(mClientIf,
> +                                mDeviceAddr,
> +                                !mAutoConnect,

Leave comment to explain why |!mAutoConnect| here.

@@ +256,5 @@
> +  BT_LOGD("[D] %s", NS_ConvertUTF16toUTF8(aData.name()).get());
> +
> +  BluetoothValue v = aData.value();
> +  if (aData.name().EqualsLiteral("ClientRegistered")) {
> +    HandleClientRegistered(v);

Can we register client & connect at once in parent process rather than calling 'connect' from child process?

@@ +268,5 @@
> +      v.get_bool() ? BluetoothConnectionState::Connected
> +                   : BluetoothConnectionState::Disconnected;
> +    UpdateConnectionState(state);
> +  } else {
> +    BT_WARNING("Not handling device signal: %s",

Should be 'gatt' signal.

::: dom/bluetooth2/BluetoothGatt.h
@@ +9,5 @@
> +
> +#include "mozilla/Attributes.h"
> +#include "mozilla/DOMEventTargetHelper.h"
> +#include "mozilla/dom/BluetoothGattBinding.h"
> +#include "BluetoothCommon.h"

nit: alphabetical order.

@@ +51,5 @@
> +   * Methods (Web API Implementation)
> +   ***************************************************************************/
> +  already_AddRefed<Promise> Connect(ErrorResult& aRv);
> +  already_AddRefed<Promise> Disconnect(ErrorResult& aRv);
> +  already_AddRefed<Promise> Close(ErrorResult& aRv);

We don't have |Close| function in [3]. Can you explain its usage difference from |Disconnect|? 

[3] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGatt#BluetoothGatt

@@ +95,5 @@
> +  void UnregisterClient();
> +
> +  /**
> +   * Trigger connect operation after receiving "ClientRegistered" signal.
> +   * This signal will be received when application sucessfully registerd itself

typo: su'c'cessfully register'e'd

@@ +98,5 @@
> +   * Trigger connect operation after receiving "ClientRegistered" signal.
> +   * This signal will be received when application sucessfully registerd itself
> +   * as a gatt client from bluetooth stack.
> +   *
> +   * @param aValue [in] BluetoothValue

Specify |aValue|'s meaning, not only 'BluetoothValue'.

::: dom/bluetooth2/BluetoothService.h
@@ +306,5 @@
>    SendInputMessage(const nsAString& aDeviceAddresses,
>                     const nsAString& aMessage) = 0;
>  
> +  /**
> +   * Register a gatt client. (platform specific implementation)

nit: be consistent on 'gatt' or 'GATT'

@@ +314,5 @@
> +                             const nsAString& aDeviceAddress,
> +                             BluetoothReplyRunnable* aRunnable) = 0;
> +
> +  /**
> +   * Unregister a gatt client. (platform specific implementation)

Ditto.

@@ +321,5 @@
> +  UnregisterGattClientInternal(int aClientIf,
> +                               BluetoothReplyRunnable* aRunnable) = 0;
> +
> +  /**
> +   * Connect to a remote GATT server. (platform specific implementation)

'Connect GATT client to a remote server' is clearer.

@@ +330,5 @@
> +                            bool aIsDirect,
> +                            BluetoothReplyRunnable* aRunnable) = 0;
> +
> +  /**
> +   * Disconnect from a remote GATT server. (platform specific implementation)

Ditto. 'Disconnect GATT client from a remote GATT server.'

::: dom/webidl/BluetoothGatt.webidl
@@ +8,5 @@
> +interface BluetoothGatt : EventTarget
> +{
> +  readonly attribute BluetoothConnectionState       connectionState;
> +
> +  // Fired when the connection state attribute changed

Use |connectionState| directly for consistency.

  // Fired when attribute connectionState changed

Comment 8

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

Comment on attachment 8540017 [details] [diff] [review]
Bug 1063449 - Patch2: Implement gatt client connection related functions, result handlers, notification handlers in BluetoothGattManger.

Cancel review request for patch2 since it will be revised when addressing above review comments.

Comment 9

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

Attachment: Bug 1063449 - Patch1(v2): Add and implement GATT client connection related bluetooth Web APIs.

* API change:
  use gatt.connect() directly instead of device.connectGatt()
* auto connect is not covered yet since the limitation of bluedroid stack, will open a follow up bug to trace it

Comment 10

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

Attachment: [Local patch, do not commit] Bug 1063449 - Patch2(v2): Marionette tests for gatt client connection.

Update test.

Comment 11

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

Comment on attachment 8554491 [details] [diff] [review]
Bug 1063449 - Patch1(v2): Add and implement GATT client connection related bluetooth Web APIs.

Hi Ben,

I apologize that this patch is quite different from v1 since we change our gatt connection API after internal discussion[1].
Please review my patch again.

This patch mainly covers:
1) construct/deconstruct of gatt object
2) register/unregister clientIf from bluedroid stack
3) implement gatt connect/disconnect API
4) address previous review comments

A follow-up bug has been created to track auto connect functionality.

Thanks,
Jocelyn

[1] https://docs.google.com/a/mozilla.com/document/d/1tAuJMUuiBInAGEcQXRpjY14Z7PnCuZw0LIZ6ocROew4/edit#heading=h.9xswxcz6rzqn

Comment 12

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

Attachment: Bug 1063449 - Patch1(v2): Add and implement GATT client connection related bluetooth Web APIs.

Remove Throws for connect/disconnect methods in BluetoothGatt.webidl base on
bug 1113827.

Comment 13

[Ben Tian (inactive)]

Comment on attachment 8555033 [details] [diff] [review]
Bug 1063449 - Patch1(v2): Add and implement GATT client connection related bluetooth Web APIs.

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

Please see my comment below.

::: dom/bluetooth2/BluetoothDevice.cpp
@@ +98,5 @@
>  }
>  
>  BluetoothDevice::~BluetoothDevice()
>  {
> +  mGatt = nullptr;

nit: add newline here.

::: dom/bluetooth2/BluetoothGatt.cpp
@@ +7,5 @@
> +#include "BluetoothReplyRunnable.h"
> +#include "BluetoothService.h"
> +#include "BluetoothUtils.h"
> +#include "nsIUUIDGenerator.h"
> +#include "nsServiceManagerUtils.h"

nit: alphabetical order

@@ +29,5 @@
> +BluetoothGatt::BluetoothGatt(nsPIDOMWindow* aWindow,
> +                             const nsAString& aDeviceAddr)
> +  : DOMEventTargetHelper(aWindow)
> +  , mConnectionState(BluetoothConnectionState::Disconnected)
> +  , mDeviceAddr(aDeviceAddr)

Init |mAppUuid| and |mClientIf| in constructor.

@@ +62,5 @@
> +  nsID uuid;
> +  rv = uuidGenerator->GenerateUUIDInPlace(&uuid);
> +  NS_ENSURE_SUCCESS_VOID(rv);
> +
> +  char uuidBuffer[NSID_LENGTH];

Leave comment to explain operation here (ToProvidedString/Substring).

::: dom/bluetooth2/BluetoothInterface.h
@@ +635,5 @@
>    virtual void ReadRemoteRssi() { }
>    virtual void GetDeviceType() { }
>    virtual void SetAdvData() { }
> +
> +  virtual void UnregisterClient() { }

Leave comment to explain why you move the function to here.

::: dom/bluetooth2/bluedroid/BluetoothGattManager.cpp
@@ +7,3 @@
>  #include "BluetoothGattManager.h"
>  #include "BluetoothCommon.h"
>  #include "BluetoothUtils.h"

nit: alphabetical order

@@ +16,5 @@
>  #include "MainThreadUtils.h"
>  #include "nsIObserverService.h"
>  #include "nsThreadUtils.h"
>  
> +#define ENSURE_GATT_CLIENT_IF_IS_READY_VOID(runnable)                         \

Replace CLIENT_IF with CLIENT_INTF since _IF is misunderstood easily as 'if' instead of 'interface'.

@@ +58,5 @@
> +  nsRefPtr<BluetoothReplyRunnable> mConnectRunnable;
> +  nsRefPtr<BluetoothReplyRunnable> mDisconnectRunnable;
> +  nsRefPtr<BluetoothReplyRunnable> mUnregisterClientRunnable;
> +
> +  ~BluetoothGattClient() {

nit: move { to the next line, and move destructor right after constructor.

@@ +69,5 @@
>  /*
>   * Static functions
>   */
> +static int
> +GetClientIndex(const nsAString& aAppUuid)

Suggest to replace these two functions with comparators for IndexOf/RemoveElement functions. Please refer to [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothDevice.h#213

@@ +254,5 @@
> +  void OnError(BluetoothStatus aStatus) MOZ_OVERRIDE
> +  {
> +    BT_WARNING(
> +      "BluetoothGattClientInterface::RegisterClient failed: %d, uuid = %s",
> +      (int)aStatus, NS_ConvertUTF16toUTF8(mAppUuid).get());

nit: newline here.

@@ +261,5 @@
> +
> +    int clientIndex = GetClientIndex(mAppUuid);
> +    NS_ENSURE_FALSE_VOID(clientIndex < 0);
> +
> +    BluetoothSignal signal(

Wrap this section into a function to reuse.

@@ +267,5 @@
> +      mAppUuid,
> +      BluetoothValue(false)); // Disconnected
> +    bs->DistributeSignal(signal);
> +
> +    if (sClients[clientIndex].mConnectRunnable) {

Wrap this section into a member function of |BluetoothGattClient|.

@@ +271,5 @@
> +    if (sClients[clientIndex].mConnectRunnable) {
> +      NS_NAMED_LITERAL_STRING(errorStr, "Register GATT client failed");
> +      DispatchBluetoothReply(sClients[clientIndex].mConnectRunnable,
> +                             BluetoothValue(),
> +                             errorStr);

Should |mConnectRunnable| be cleaned here?

@@ +278,5 @@
> +    sClients.RemoveElementAt(clientIndex);
> +  }
> +
> +private:
> +  nsString mAppUuid;

Can we remember |BluetoothGattClient| directly in these result handlers, instead of AppUuid or ClientIf that requires additional mapping? Also we can append to client array AFTER registration succeeds.

@@ +364,5 @@
> +
> +  void OnError(BluetoothStatus aStatus) MOZ_OVERRIDE
> +  {
> +    BT_WARNING("BluetoothGattClientInterface::Connect failed: %d, uuid = %s",
> +               (int)aStatus, NS_ConvertUTF16toUTF8(mAppUuid).get());

nit: newline here.

@@ +417,5 @@
> +      true, // direct connect
> +      new ConnectResultHandler(aAppUuid));
> +  } else {
> +    BluetoothUuid uuid;
> +    StringToUuid(NS_ConvertUTF16toUTF8(aAppUuid).get(), uuid);

Can we store BluetoothUuid in BluetoothGatt directly to save this conversion?

@@ +509,5 @@
> +
> +  BluetoothService* bs = BluetoothService::Get();
> +  NS_ENSURE_TRUE_VOID(bs);
> +
> +  if (aStatus) {

|aStatus != STATUS_SUCESS| is clearer.

@@ +538,5 @@
> +    NS_LITERAL_STRING("ClientRegistered"),
> +    uuid, BluetoothValue(uint32_t(aClientIf)));
> +  bs->DistributeSignal(signal);
> +
> +  // Client just registered, proceed existing connect request.

Replace existing w/ remaining to be clearer.

::: dom/bluetooth2/bluedroid/BluetoothUtils.cpp
@@ +47,5 @@
>    aString.AssignLiteral(uuidStr);
>  }
>  
> +void
> +StringToUuid(const char* aString, BluetoothUuid& aUuid) {

nit: move { to the next line.

@@ +68,5 @@
> +  memcpy(&aUuid.mUuid[8], &uuid3, 2);
> +  memcpy(&aUuid.mUuid[10], &uuid4, 4);
> +  memcpy(&aUuid.mUuid[14], &uuid5, 2);
> +
> +  return;

Remove this redundant return.

Comment 14

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

Hi Ben,

Please kindly see my questions below.

(In reply to Ben Tian [:btian] from comment #13)
> Comment on attachment 8555033 [details] [diff] [review]
> Bug 1063449 - Patch1(v2): Add and implement GATT client connection related
> bluetooth Web APIs.
> 
> Review of attachment 8555033 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +261,5 @@
> > +
> > +    int clientIndex = GetClientIndex(mAppUuid);
> > +    NS_ENSURE_FALSE_VOID(clientIndex < 0);
> > +
> > +    BluetoothSignal signal(
> 
> Wrap this section into a function to reuse.
> 
Could we use comments here since it's just two lines of codes?

> @@ +267,5 @@
> > +      mAppUuid,
> > +      BluetoothValue(false)); // Disconnected
> > +    bs->DistributeSignal(signal);
> > +
> > +    if (sClients[clientIndex].mConnectRunnable) {
> 
> Wrap this section into a member function of |BluetoothGattClient|.
> 
It might not be a good way since we need to add many functions depends on the number of type of runnables.
I might use comment first to try to make the semantic clearer. 

> @@ +278,5 @@
> > +    sClients.RemoveElementAt(clientIndex);
> > +  }
> > +
> > +private:
> > +  nsString mAppUuid;
> 
> Can we remember |BluetoothGattClient| directly in these result handlers,
> instead of AppUuid or ClientIf that requires additional mapping? Also we can
> append to client array AFTER registration succeeds.
> 

Thanks for the suggestion for using BluetoothGattClient in result handlers directly.
Agree that we should remember GattClient directly in these handlers.
Though I think we cannot append to client array after the registration succeed since we cannot access the client from |RegisterClientNotification| when registration succeeds.

> @@ +417,5 @@
> > +      true, // direct connect
> > +      new ConnectResultHandler(aAppUuid));
> > +  } else {
> > +    BluetoothUuid uuid;
> > +    StringToUuid(NS_ConvertUTF16toUTF8(aAppUuid).get(), uuid);
> 
> Can we store BluetoothUuid in BluetoothGatt directly to save this conversion?
> 
I store it as nsString type since I think it's more useful most of the time.
For instance, register/unregister the signal observer in BluetoothGatt by using this string.
IMHO, store as BluetoothUuid will also introduce some conversions at some point and not that useful from the content process.
What do you think?

> @@ +509,5 @@
> > +
> > +  BluetoothService* bs = BluetoothService::Get();
> > +  NS_ENSURE_TRUE_VOID(bs);
> > +
> > +  if (aStatus) {
> 
> |aStatus != STATUS_SUCESS| is clearer.

I might use comment at this moment since it's actually GATT status code.
But it seems unnecessary to add a enum for that since we don't use status other than GATT_SUCCESS at this moment.

Comment 15

[Ben Tian (inactive)]

(In reply to jocelyn [:jocelyn] from comment #14)
> > Wrap this section into a function to reuse.
> > 
> Could we use comments here since it's just two lines of codes?

Sure. I'm fine as long as it helps to understand.

> > Wrap this section into a member function of |BluetoothGattClient|.
> > 
> It might not be a good way since we need to add many functions depends on
> the number of type of runnables.
> I might use comment first to try to make the semantic clearer. 

No problem.

> > Can we remember |BluetoothGattClient| directly in these result handlers,
> > instead of AppUuid or ClientIf that requires additional mapping? Also we can
> > append to client array AFTER registration succeeds.
> > 
> 
> Thanks for the suggestion for using BluetoothGattClient in result handlers
> directly.
> Agree that we should remember GattClient directly in these handlers.
> Though I think we cannot append to client array after the registration
> succeed since we cannot access the client from |RegisterClientNotification|
> when registration succeeds.

OK. Then please keep the original flow.

> > Can we store BluetoothUuid in BluetoothGatt directly to save this conversion?
> > 
> I store it as nsString type since I think it's more useful most of the time.
> For instance, register/unregister the signal observer in BluetoothGatt by
> using this string.
> IMHO, store as BluetoothUuid will also introduce some conversions at some
> point and not that useful from the content process.
> What do you think?

If so I prefer to remove type |BluetoothUuid| to save redundant conversion, since only bt_uuid_t (in bluedroid) and nsString (in gecko) are really required for uuid representation. Please open another bug to remove |BluetoothUuid| and seek Thomas's opinion.

> I might use comment at this moment since it's actually GATT status code.
> But it seems unnecessary to add a enum for that since we don't use status
> other than GATT_SUCCESS at this moment.

OK. Comment helps as well.

Comment 16

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

Attachment: Bug 1063449 - Patch1(v3): Add and implement GATT client connection related bluetooth Web APIs.

* Address review comments.

TODO: open the follow up bug to replace BluetoothUuid with nsString.

Comment 17

[Ben Tian (inactive)]

Comment on attachment 8560309 [details] [diff] [review]
Bug 1063449 - Patch1(v3): Add and implement GATT client connection related bluetooth Web APIs.

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

Please see my comment below and check when to resolve |mUnregisterRunnable|.

::: dom/bluetooth2/bluedroid/BluetoothGattManager.cpp
@@ +65,5 @@
> +  int mClientIf;
> +  int mConnId;
> +  nsRefPtr<BluetoothReplyRunnable> mConnectRunnable;
> +  nsRefPtr<BluetoothReplyRunnable> mDisconnectRunnable;
> +  nsRefPtr<BluetoothReplyRunnable> mUnregisterClientRunnable;

When will |mUnregisterClientRunnable| be resolved?

@@ +241,5 @@
> +{
> +public:
> +  RegisterClientResultHandler(BluetoothGattClient* aClient)
> +  : mClient(aClient)
> +  { }

MOZ_ASSERT(mClient)

@@ +280,5 @@
> +{
> +public:
> +  UnregisterClientResultHandler(BluetoothGattClient* aClient)
> +  : mClient(aClient)
> +  { }

MOZ_ASSERT(mClient)

@@ +303,5 @@
> +    BT_WARNING("BluetoothGattClientInterface::UnregisterClient failed: %d",
> +               (int)aStatus);
> +
> +    // Reject the unregister request
> +    if (mClient->mUnregisterClientRunnable) {

Replace with MOZ_ASSERT since |mUnregisterRunnable| should not be null.

@@ +368,5 @@
> +      BluetoothValue(false)); // Disconnected
> +    bs->DistributeSignal(signal);
> +
> +    // Reject the connect request
> +    if (mClient->mConnectRunnable) {

Replace with MOZ_ASSERT since |mConnectRunnable| should not be null.

@@ +393,5 @@
> +  ENSURE_GATT_CLIENT_INTF_IS_READY_VOID(aRunnable);
> +
> +  size_t index = sClients.IndexOf(aAppUuid, 0 /* Start */, UuidComparator());
> +  if (index == sClients.NoIndex) {
> +    // BluetoothGattClient client(aAppUuid, aDeviceAddr);

Remove this extra comment.

@@ +397,5 @@
> +    // BluetoothGattClient client(aAppUuid, aDeviceAddr);
> +    nsRefPtr<BluetoothGattClient> client =
> +      new BluetoothGattClient(aAppUuid, aDeviceAddr);
> +    sClients.AppendElement(client);
> +    index = sClients.Length() - 1;

Set |index| first so you don't have to minus 1.

@@ +446,5 @@
> +      mAppUuid,
> +      BluetoothValue(true)); // Connected
> +    bs->DistributeSignal(signal);
> +
> +    // Reject the disconnect request

Add MOZ_ASSERT(mDisconnectRunnable)

@@ +520,5 @@
> +
> +    // Reject the connect request
> +    if (sClients[index]->mConnectRunnable) {
> +      NS_NAMED_LITERAL_STRING(errorStr,
> +                              "Connect failed due to registeration failed");

typo: registration

@@ +523,5 @@
> +      NS_NAMED_LITERAL_STRING(errorStr,
> +                              "Connect failed due to registeration failed");
> +      DispatchBluetoothReply(sClients[index]->mConnectRunnable,
> +                             BluetoothValue(),
> +                             errorStr);

Set |mConnectRunnable| as nullptr here.

Comment 18

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

Attachment: Bug 1063449 - Patch1(v4): Add and implement GATT client connection related bluetooth Web APIs.

Hi Ben,

Please review my updated patch, thanks.

Update summary:
* Resolve unregister request in |UnregisterClientResultHandler::UnregisterClient()|
* Address review comments

Comment 19

[Ben Tian (inactive)]

Comment on attachment 8561180 [details] [diff] [review]
Bug 1063449 - Patch1(v4): Add and implement GATT client connection related bluetooth Web APIs.

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

r=me with comment addressed. Please also request DOM peer review for webidl related change. Thanks for the effort!

::: dom/bluetooth2/bluedroid/BluetoothGattManager.cpp
@@ +299,5 @@
> +      BluetoothValue(true));
> +    bs->DistributeSignal(signal);
> +
> +    // Resolve the unregister request
> +    MOZ_ASSERT(mClient->mUnregisterClientRunnable);

Move the assertion to beginning of this function since this function should not be run at all if the assertion fails.

@@ +314,5 @@
> +    BT_WARNING("BluetoothGattClientInterface::UnregisterClient failed: %d",
> +               (int)aStatus);
> +
> +    // Reject the unregister request
> +    MOZ_ASSERT(mClient->mUnregisterClientRunnable);

Ditto.

@@ +380,5 @@
> +      BluetoothValue(false)); // Disconnected
> +    bs->DistributeSignal(signal);
> +
> +    // Reject the connect request
> +    MOZ_ASSERT(mClient->mConnectRunnable);

Ditto. Place the assertion after warning.

@@ +454,5 @@
> +      BluetoothValue(true)); // Connected
> +    bs->DistributeSignal(signal);
> +
> +    // Reject the disconnect request
> +    MOZ_ASSERT(mClient->mDisconnectRunnable);

Ditto.

Comment 20

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

Attachment: Bug 1063449 - Patch1(v5): Add and implement GATT client connection related bluetooth Web APIs. r=btian

Thanks for the reviewing, Ben!

Comment 21

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

Comment on attachment 8562540 [details] [diff] [review]
Bug 1063449 - Patch1(v5): Add and implement GATT client connection related bluetooth Web APIs. r=btian

Hi Boris,

This patch is the first patch of our new GATT API[1] for enabling Bluetooth Low Energy on Firefox OS, and has been reviewed by Ben, one of our bluetooth peers.
Is it OK to ask your help for reviewing the DOM facing part?

The patch mainly covers
1) BluetoothDevice.webidl/cpp/h:
  Add a gatt property in BluetoothDevice for interacting with remote BLE devices[1]
2) BluetoothGatt.webidl/cpp/h:
  Connection related APIs in BluetoothGatt interface [2]
3) BluetoothGattManager.cpp/h:
  Interact with Bluetooth stack and distribute signal to BluetoothGatt object for connection status change.
4) Other files like moz.build or for ipc requests

Could you kindly help to review part 1) and 2)?

Thanks,
Jocelyn

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothDevice#gatt
[2] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGatt

Comment 22

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

Attachment: Bug 1063449 - Patch1(v5): Add and implement GATT client connection related bluetooth Web APIs. r=btian

Sorry, I accidentally attached v4 instead of v5 last time.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8562558 [details] [diff] [review]
Bug 1063449 - Patch1(v5): Add and implement GATT client connection related bluetooth Web APIs. r=btian

>+++ b/dom/bluetooth2/BluetoothDevice.cpp
>+#include "BluetoothGatt.h"

The other .cpp file includes this as "mozilla/dom/bluetooth/BluetoothGatt.h".  Please be consistent about how you include it.

>+  , mGatt(nullptr)

Why?  The nsRefPtr ctor already initializes to null.

> BluetoothDevice::~BluetoothDevice()
>+  mGatt = nullptr;

Also not needed.

>+BluetoothDevice::Gatt()

So the IDL says this getter never returns null, but...

>+  NS_ENSURE_TRUE(mType == BluetoothDeviceType::Le ||
>+                 mType == BluetoothDeviceType::Dual,
>+                 nullptr);

...it clearly can return null, and then we will crash.  Please either change this to never return null, change it to throw if mType is wrong, or change the IDL to indicate that this getter can return null.

>+++ b/dom/bluetooth2/BluetoothGatt.cpp
>+BluetoothGatt::GenerateUuid(nsAString &aUuidString)
>+  aUuidString.Assign(Substring(uuidString, 1, NSID_LENGTH - 3));

Please document that the -3 is because NSID_LENGTH includes not only the {} but also the null terminator.

Also, please file a followup bug to add a method on nsIUUIDGenerator that returns the string you're trying to produce here.  There are multiple places in the tree that want that sort of thing....

>+BluetoothGatt::DisconnectFromOwner()

Why do you not need to UnregisterGattClientInternal here?

>+BluetoothGatt::Connect(ErrorResult& aRv)

This needs more documentation in the IDL.  For example, this will reject if already connected or connecting (which sorta makes sense) but will also reject if disconnecting.  Why is it not OK to connect() while in the "disconnecting" state?

>+BluetoothGatt::Disconnect(ErrorResult& aRv)

Similar here: document.  And why not OK to disconnect() while in the "connecting" state?

>+BluetoothGatt::UpdateConnectionState(BluetoothConnectionState aState)

Need to document that connect/disconnect fire the event sync before returning, right?

>+++ b/dom/bluetooth2/BluetoothGatt.h
>+#include "BluetoothCommon.h"

Shouldn't this be included from mozilla/dom/bluetooth?

>+  class Promise;

No indent here.

>+class BluetoothGatt MOZ_FINAL : public DOMEventTargetHelper

Doesn't this need to decl cycle collection (inherited)?

>+  int mClientIf;

Worth documenting the difference between this being 0 and nonzero.

>+++ b/dom/bluetooth2/BluetoothInterface.h
>+   * This is a workaround here

That's quite odd.  Why is the crash happening?  This part I'm not super-happy about.

>+++ b/dom/bluetooth2/BluetoothUtils.cpp
>+StringToUuid(const char* aString, BluetoothUuid& aUuid)

Are you guaranteed that aString is something we created?  Because if not, this function seems to have a serious lack of error checking.

If you _are_ guaranteed this, document why.

>+++ b/dom/bluetooth2/bluedroid/BluetoothGattManager.cpp
>+static nsTArray<nsRefPtr<BluetoothGattClient> > sClients;

This will show up as a memory leak, no?  Also, a static constructor.  Don't do this, please.  You probably want something using StaticAutoPtr here.

I didn't read most of the BluetoothGattManager code very carefully.

r=me with the above issues addressed

Comment 24

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

Hi Boris,

Thanks for the review.
Please see my comments below.
(In reply to Boris Zbarsky [:bz] from comment #23)
> Comment on attachment 8562558 [details] [diff] [review]
> Bug 1063449 - Patch1(v5): Add and implement GATT client connection related
> bluetooth Web APIs. r=btian
> 
> >+BluetoothGatt::Connect(ErrorResult& aRv)
> 
> This needs more documentation in the IDL.  For example, this will reject if
> already connected or connecting (which sorta makes sense) but will also
> reject if disconnecting.  Why is it not OK to connect() while in the
> "disconnecting" state?
> 
Disconnect operation can be completed in a moment, we think there's no use cases that
need to call connect during this short period.
Hence, we only handle this operation when we're in disconnected status.

> >+BluetoothGatt::Disconnect(ErrorResult& aRv)
> 
> Similar here: document.  And why not OK to disconnect() while in the
> "connecting" state?
> 

The reason behind this is, for a established connection, our BT stack(bluedroid)
will call a callback(then links to DisconnectNotification) to notify us if the
disconnect operation succeeded or failed, thus we can properly handle the result and resolve/reject promise.

However, if the connection is not established yet, currently there's no callback
will be called by bluedroid (it's due to bluedroid implementation) to indicate the
operation result.

Thus, we didn't allow to disconnect during connecting.
Besides, there's a timeout from stack that won't cause connect operation to be forever.

> >+++ b/dom/bluetooth2/BluetoothInterface.h
> >+   * This is a workaround here
> 
> That's quite odd.  Why is the crash happening?  This part I'm not
> super-happy about.
> 

Thanks to Thomas Zimmermann, this is fixed by Bug 1131653, I have removed this change.

Comment 25

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

Attachment: Bug 1063449 - Patch1(v6): Add and implement GATT client connection related bluetooth Web APIs. r=btian, r=bz

Address bz's review comments.

1) Fix header inclusion
2) Make gatt a nullable property in BluetoothDevice.webidl
3) Add CYCLE_COLLECTION macros for BluetoothGatt
4) Use StaticAutoPtr for sClients
5) Add documentation comments

TODO: open a follow-up bug for nsIUUIDGenerator.

Hi Boris,

I have revised my patch based on your comments.
But didn't revise the connect/disconnect behavior, please see comment 24.
I will revise again if you disagree with the current behavior.

Thanks,
Jocelyn

Comment 26

[Boris Zbarsky [:bzbarsky]]

> Disconnect operation can be completed in a moment, we think there's no use cases that
> need to call connect during this short period.

OK.  I'm not saying that's not the case.  I'm saying this needs to be documented in the IDL.  Specifically, you need to document what connect() and disconnect() will do in various connectionState states.

Comment 27

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

(In reply to Boris Zbarsky [:bz] from comment #26)
> > Disconnect operation can be completed in a moment, we think there's no use cases that
> > need to call connect during this short period.
> 
> OK.  I'm not saying that's not the case.  I'm saying this needs to be
> documented in the IDL.  Specifically, you need to document what connect()
> and disconnect() will do in various connectionState states.

Ah, OK. Sorry for the misunderstanding. :)
I will document them in the IDL.
Thanks again for reviewing my patch.

Comment 28

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

Attachment: [Final] Bug 1063449 - Add and implement GATT client connection related bluetooth Web APIs. r=btian, r=bz

* Document more details for connect/disconnect methods in BluetoothGatt.webidl.

Comment 29

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

No try server result since bluetooth2 won't be built.

Comment 30

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

follow-up bug for the uuid generation part.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/5fa3cd10706a

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5fa3cd10706a