1181479 - Implement GATT Server service management

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Bruce Sun [:brsun]]

GATT Server service management:
 - Add services
 - Remove services

Comment 1

[Bruce Sun [:brsun]]

Attachment: bug1181479_0001_refine_stringtouuid.patch

WIP patch

Comment 2

[Bruce Sun [:brsun]]

Attachment: bug1181479_0002_bluetoothgattserver_service_management_webidl.patch

WIP patch

Comment 3

[Bruce Sun [:brsun]]

Attachment: bug1181479_0003_bluetoothgattserver_service_management_ipc.patch

WIP patch

Comment 4

[Bruce Sun [:brsun]]

Attachment: bug1181479_0001_refine_stringtouuid.v1.patch

Comment 5

[Bruce Sun [:brsun]]

Attachment: bug1181479_0002_bluetoothgattserver_service_management_webidl.v1.patch

Comment 6

[Bruce Sun [:brsun]]

Attachment: bug1181479_0003_bluetoothgattserver_service_management_ipc.v1.patch

Comment 7

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

Comment on attachment 8649238 [details] [diff] [review]
bug1181479_0001_refine_stringtouuid.v1.patch

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

LGTM.

Comment 8

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

Comment on attachment 8649239 [details] [diff] [review]
bug1181479_0002_bluetoothgattserver_service_management_webidl.v1.patch

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

Hi Bruce,

Please see my questions below.

I would suggest to combine p2 and p3 since they're really connected, it's hard to just review one of them at a time.
And it's a bit of confusing that some parts of p2 would be modified in p3.
You may still pull out some parts into an independent patch as long as the code logic is clear in each patch.

Thanks,
Jocelyn

::: dom/bluetooth/bluetooth2/BluetoothGattCharacteristic.cpp
@@ +66,5 @@
>    , mProperties(aChar.mProperties)
>    , mWriteType(aChar.mWriteType)
> +  , mAttRole(ATT_CLIENT_ROLE)
> +  , mActive(true)
> +  , mCharacteristicHandle(0x0000)

Would mActive and mCharacteristic handle value ever changed/used for ATT_CLIENT_ROLE?
Could you put some comments in the header file to describe what's the difference between these member values and their usage between ATT_SERVER_ROLE and ATT_CLIENT_ROLE?

@@ +99,5 @@
> +  , mActive(false)
> +  , mCharacteristicHandle(0x0000)
> +{
> +  MOZ_ASSERT(aOwner);
> +  MOZ_ASSERT(aCharacteristic);

Should be aService?

@@ +183,5 @@
>    NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
>  
> +  BT_ENSURE_TRUE_REJECT(mAttRole == ATT_CLIENT_ROLE,
> +                        promise,
> +                        NS_ERROR_NOT_AVAILABLE);

On MDN[1], this is described as:
 An operation could not be completed because some other necessary component or resource was not available.

I think it's not our case here, please find another one to fit our case here.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Errors

::: dom/bluetooth/bluetooth2/BluetoothGattCharacteristic.h
@@ +183,5 @@
>     */
>    nsTArray<uint8_t> mValue;
>  
>    /**
> +   * Permissions of this GATT descriptor.

characteristic?

::: dom/bluetooth/bluetooth2/BluetoothGattDescriptor.h
@@ +145,5 @@
> +   */
> +  BluetoothGattAttrPerm mPermissions;
> +
> +  /**
> +   * ATT role of this GATT service.

s/service/descriptor

::: dom/bluetooth/bluetooth2/BluetoothGattServer.cpp
@@ +53,5 @@
> +              const BluetoothGattService* aServicePtr) const
> +  {
> +    return aService.get() == aServicePtr;
> +  }
> +};

nit: how about revising it as similar as https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluetooth2/BluetoothDevice.h?offset=500#206-228 and put it in the header file?

@@ +122,5 @@
>  BluetoothGattServer::Invalidate()
>  {
>    mValid = false;
>  
> +  mServices.Clear();

nit: remove the newline before this line.

::: dom/bluetooth/bluetooth2/BluetoothGattService.cpp
@@ +53,5 @@
> +  const BluetoothGattServiceInit& aInit)
> +  : mOwner(aOwner)
> +  , mAttRole(ATT_SERVER_ROLE)
> +  , mActive(false)
> +  , mServiceHandle(0x0000)

Add mUuidStr(aInit.mUuid) or assign it below.

@@ +57,5 @@
> +  , mServiceHandle(0x0000)
> +{
> +  memset(&mServiceId, 0, sizeof(mServiceId));
> +  StringToUuid(aInit.mUuid, mServiceId.mId.mUuid);
> +  mServiceId.mIsPrimary = aInit.mIsPrimary;

Do we need to have instance id assigned also?

::: dom/webidl/BluetoothGattCharacteristic.webidl
@@ +54,5 @@
>    Promise<void> startNotifications();
>    [NewObject]
>    Promise<void> stopNotifications();
> +  [NewObject]
> +  Promise<BluetoothGattDescriptor> addDescriptor(DOMString uuid,

DITTO.
And please help to add comments for start/stopNotifications to indicate they are only for client role.

::: dom/webidl/BluetoothGattServer.webidl
@@ +23,5 @@
>    [NewObject]
>    Promise<void> disconnect(DOMString address);
> +
> +  /**
> +   * Add/Remove to the local BLE device with the service.

Suggest to revise as "Add/Remove BLE services to the local GATT server."

::: dom/webidl/BluetoothGattService.webidl
@@ +15,5 @@
>    readonly attribute boolean                                isPrimary;
>    readonly attribute DOMString                              uuid;
>    readonly attribute unsigned short                         instanceId;
> +
> +  [NewObject]

Please indicate that these functions will be rejected for applications in client role since they are for server role only.

Comment 9

[Bruce Sun [:brsun]]

Attachment: bug1181479_0002_bluetoothgattserver_service_management.v2.patch

Address most of comment 8.

Hi Jocelyn,
Please kindly refer to the comments below.

> ::: dom/bluetooth/bluetooth2/BluetoothGattServer.cpp
> @@ +53,5 @@
> > +              const BluetoothGattService* aServicePtr) const
> > +  {
> > +    return aService.get() == aServicePtr;
> > +  }
> > +};
> 
> nit: how about revising it as similar as
> https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluetooth2/
> BluetoothDevice.h?offset=500#206-228 and put it in the header file?
> 

Since |ServicePtrComparator| is designed to compare the address only, not to compare the content itself, this comparator might not as general as the referred one.

> @@ +57,5 @@
> > +  , mServiceHandle(0x0000)
> > +{
> > +  memset(&mServiceId, 0, sizeof(mServiceId));
> > +  StringToUuid(aInit.mUuid, mServiceId.mId.mUuid);
> > +  mServiceId.mIsPrimary = aInit.mIsPrimary;
> 
> Do we need to have instance id assigned also?
> 

The instance id is only meaningful in the client mode, and the value would be set to 0 after |memset()| has executed in the server mode.

Comment 10

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

Comment on attachment 8651730 [details] [diff] [review]
bug1181479_0002_bluetoothgattserver_service_management.v2.patch

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

Hi Bruce,

Please see my comments and questions below.
For GattManager.cpp and ReplyRunnable.h/cpp, I just glanced it, will take a closer look on these three files in the next round.

If possible, please also provide an interdiff between this version and the next one.

Thanks,
Jocelyn

::: dom/bluetooth/bluetooth2/BluetoothGattCharacteristic.cpp
@@ +500,5 @@
> +  /* The characteristic should not be actively acting with the Bluetooth
> +   * backend. Otherwise, descriptors cannot be added into the characteristic. */
> +  BT_ENSURE_TRUE_REJECT(!mActive,
> +                        promise,
> +                        NS_ERROR_ALREADY_INITIALIZED);

FAILURE or UNEXPECTED might be better.

If we set a limitation that we can not add descriptors when the characteristic is active, I would suggest that we also forbid addChar/addIncludedSrvc when mService.mActive is true.
My reason is that it would be weird that we can add a characteristic, but we can't add descriptors on it.
addIncludedSrvc is also forbid to be consistent.

::: dom/bluetooth/bluetooth2/BluetoothGattServer.cpp
@@ +62,5 @@
>    : mOwner(aOwner)
>    , mServerIf(0)
>    , mValid(true)
> +{
> +  GenerateUuid(mAppUuid);

It's possible that mAppUuid is still an empty string because the generation is failed, right?
How we're going to handle this case?

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

@@ +87,5 @@
> +    arr[0].value().get_BluetoothGattServiceId();
> +  BluetoothAttributeHandle serviceHandle =
> +    arr[1].value().get_BluetoothAttributeHandle();
> +
> +  for (size_t i = 0; i < mPendingServices.Length(); ++i) {

I think we could use indexOf directly.

@@ +115,5 @@
> +    arr[1].value().get_BluetoothAttributeHandle();
> +  BluetoothAttributeHandle characteristicHandle =
> +    arr[2].value().get_BluetoothAttributeHandle();
> +
> +  for (size_t i = 0; i < mPendingServices.Length(); ++i) {

Suggest to add a nsDefaultComparator <nsRefPtr<BluetoothGattService>,BluetoothAttributeHandle> in GattService.h and use indexOf here.

@@ +147,5 @@
> +    arr[2].value().get_BluetoothAttributeHandle();
> +  BluetoothAttributeHandle descriptorHandle =
> +    arr[3].value().get_BluetoothAttributeHandle();
> +
> +  for (size_t i = 0; i < mPendingServices.Length(); ++i) {

Suggest to add a nsDefaultComparator <nsRefPtr<BluetoothGattService>,BluetoothAttributeHandle> in GattService.h and use indexOf here.

@@ +217,5 @@
>  void
>  BluetoothGattServer::Invalidate()
>  {
>    mValid = false;
> +  mServices.Clear();

is it possible that mPendingServices is not empty here?

@@ +481,5 @@
> +                              descriptors[j]);
> +
> +      AppendTask(descriptorTask.forget());
> +    }
> +  }

Should we add StartServiceTask into the queue?

@@ +560,5 @@
> +
> +void
> +BluetoothGattServer::AddServiceTask::OnErrorFired()
> +{
> +  mServer->mServices.RemoveElement(mService, ServicePtrComparator());

Why we remove it from mServices instead of mPendingServices?

@@ +569,5 @@
> +                       STATUS_FAIL);
> +    return;
> +  }
> +
> +  bs->GattServerRemoveServiceInternal(

Why is this needed?

@@ +598,5 @@
> +  BT_ENSURE_TRUE_REJECT(bs, promise, NS_ERROR_NOT_AVAILABLE);
> +
> +  mPendingServices.AppendElement(&aService);
> +
> +  bs->GattServerAddServiceInternal(mAppUuid,

If we are already in the progress of adding characteristics, descriptors of the other service, this add service request will be issued to the backend, right?
Will this new service affect the operations of adding char,descs for the previous service?

::: dom/bluetooth/bluetooth2/BluetoothGattServer.h
@@ +6,5 @@
>  
>  #ifndef mozilla_dom_bluetooth_BluetoothGattServer_h
>  #define mozilla_dom_bluetooth_BluetoothGattServer_h
>  
> +#include "BluetoothGattService.h"

#include "mozilla/dom/BluetoothGattService.h" and move after BluetoothCommon.h

@@ +147,5 @@
> +    nsRefPtr<BluetoothGattService> mService;
> +  };
> +  friend class StartServiceTask;
> +
> +  class ReverseAddServiceTask final : public BluetoothReplyRunnable

nit: Suggest to use CancelAddServiceTask

::: dom/bluetooth/bluetooth2/BluetoothGattService.cpp
@@ +135,5 @@
> +  MOZ_ASSERT(BluetoothAttRole == ATT_SERVER_ROLE);
> +  MOZ_ASSERT(!mActive);
> +  MOZ_ASSERT(!aCharacteristicHandle);
> +
> +  for (size_t i = 0; i < mCharacteristics.Length(); ++i) {

Could we use mCharacteristics.IndexOf(aCharacteristicId) directly here?

@@ +156,5 @@
> +  MOZ_ASSERT(!mActive);
> +  MOZ_ASSERT(!aCharacteristicHandle);
> +  MOZ_ASSERT(!aDescriptorHandle);
> +
> +  for (size_t i = 0; i < mCharacteristics.Length(); ++i) {

How about we add a explicit specialization (nsDefaultComparator<nsRefPtr<BluetoothGattCharacteristic>, BluetoothAttributeHandle>)in BluetoothGattCharacteristic.h?
Then we could use indexOf directly.

::: dom/bluetooth/bluetooth2/BluetoothGattService.h
@@ +120,5 @@
> +  {
> +  public:
> +    AddCharacteristicTask(BluetoothGattService* aService,
> +                          BluetoothGattCharacteristic* aCharacteristic,
> +                           Promise* aPromise);

nit: alignment

@@ +185,5 @@
> +   * Assign AppUuid of this GATT service.
> +   *
> +   * @param aAppUuid The value of AppUuid.
> +   */
> +  void AssignAppUuid(const nsAString& aAppUuid);

nit: Why not use Set* here and other similar functions?

::: dom/webidl/BluetoothGattCharacteristic.webidl
@@ +29,4 @@
>  };
>  
>  [CheckAnyPermissions="bluetooth"]
>  interface BluetoothGattCharacteristic

Before |BluetoothGattCharacteristic| interface definition, could you add some comments to describe that this might be in server role (hosted in local gatt server) or client role (hosted in remote gatt server)?

Then it would be easier for other people to understand what's server/client role and why some APIs would be rejected directly with incorrect role.

@@ +54,5 @@
>    [NewObject]
>    Promise<void> startNotifications();
>    [NewObject]
>    Promise<void> stopNotifications();
> +  [NewObject]

put the extended attribute after the comment below.

@@ +57,5 @@
>    Promise<void> stopNotifications();
> +  [NewObject]
> +
> +  /**
> +   * Add BLE descriptors to the local GATT characteristic. This API will be

nit: s/This API/The promise

::: dom/webidl/BluetoothGattService.webidl
@@ +3,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +[CheckAnyPermissions="bluetooth", Constructor(BluetoothGattServiceInit init)]

Please also add comments before this interface as I mentioned in GattChar.webidl

@@ +25,5 @@
> +    DOMString uuid,
> +    GattPermissions permissions,
> +    GattCharacteristicProperties properties,
> +    ArrayBuffer value);
> +    

nit: remote extra spaces.

Comment 11

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

Comment on attachment 8651730 [details] [diff] [review]
bug1181479_0002_bluetoothgattserver_service_management.v2.patch

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

::: dom/bluetooth/bluetooth2/BluetoothReplyRunnable.h
@@ +119,5 @@
> +    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(SubTask)
> +  public:
> +    SubTask(BluetoothReplyTaskQueue* aRootQueue,
> +            SubReplyRunnable* aReply);
> +    virtual ~SubTask();

../../../../gecko-dev/dom/bluetooth/bluetooth2/BluetoothReplyRunnable.h: In member function 'MozExternalRefCountType mozilla::dom::bluetooth::BluetoothReplyTaskQueue::SubTask::AddRef()':
../../dist/include/nsISupportsImpl.h:82:3: error: static assertion failed: Reference-counted class SubTask should not have a public destructor. Make this class's destructor non-public

please use 'protected' for destructor.

Comment 12

[Bruce Sun [:brsun]]

Attachment: bug1181479_0001_refine_stringtouuid.rebase.patch

Comment 13

[Bruce Sun [:brsun]]

Attachment: bug1181479_0002_refine_generateuuid.v1.patch

Use |nsresult| as the return type of |GenerateUuid()|.

Comment 14

[Bruce Sun [:brsun]]

Attachment: bug1181479_0003_bluetoothgattserver_service_management.v3.patch

Address comment 10 and comment 11.

Comment 15

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

Comment on attachment 8654809 [details] [diff] [review]
bug1181479_0002_refine_generateuuid.v1.patch

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

Please also revise callers of this function in this patch to handle error results from the return value.

https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothGattManager.cpp#620
https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/webapi/BluetoothGatt.cpp#116
https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/webapi/BluetoothGattServer.cpp#140

Comment 16

[Bruce Sun [:brsun]]

Attachment: bug1181479_0002_refine_generateuuid.v2.patch

Address comment 15.

Comment 17

[Bruce Sun [:brsun]]

Attachment: bug1181479_0003_bluetoothgattserver_service_management.v4.patch

Replace |BluetoothGattServer::mPendingServices| array with one |BluetoothGattServer::mPendingService| reference pointer.

Add |BluetoothGattCharacteristic.permissions| attribute.

Add |BluetoothGattDescriptor.permissions| attribute.

Declare |BluetoothAttributeHandle| as one structure instead of one typedef of |uint16_t|.

Comment 18

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

Comment on attachment 8655361 [details] [diff] [review]
bug1181479_0003_bluetoothgattserver_service_management.v4.patch

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

Hi Bruce,

Please see my comments below.

Thanks,
Jocelyn

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp
@@ +1384,5 @@
>  
>  nsresult
> +UnpackPDU(DaemonSocketPDU& aPDU, BluetoothAttributeHandle& aOut)
> +{
> +  return UnpackPDU(aPDU, UnpackConversion<int32_t, uint16_t>(aOut.mHandle));

Do we have a check somewhere to ensure that the input value can fit into uint16_t?

::: dom/bluetooth/common/webapi/BluetoothGattCharacteristic.cpp
@@ +104,5 @@
> +  memset(&mCharId, 0, sizeof(mCharId));
> +  StringToUuid(aCharacteristicUuid, mCharId.mUuid);
> +
> +  // permissions
> +  if (aPermissions.mRead) {

How about we have a utility function in BluetoothUtils.h to get BluetoothGattAttrPerm from the dictionary value?

@@ +130,5 @@
> +    mPermissions |= GATT_ATTR_PERM_BIT_WRITE_SIGNED_MITM;
> +  }
> +
> +  // properties
> +  if (aProperties.mBroadcast) {

DITTO.

@@ +271,5 @@
> +    if (descriptorUuid == aDescriptorUuid) {
> +      mDescriptors[i]->AssignDescriptorHandle(aDescriptorHandle);
> +      break;
> +    }
> +  }

Suggest to add an explicit specialization of function templates in Descriptor.h.
So we could write similar as

size_t index = mDescriptors.IndexOf(aDescriptorUuid);
mDescriptors[index]->AssignDescriptorHandle(aDescriptorHandle);

@@ +524,5 @@
> +                                aDescriptorUuid,
> +                                aPermissions,
> +                                aValue);
> +
> +  DispatchReplySuccess(new AddDescriptorTask(this, descriptor, promise));

As we agreed in https://bugzilla.mozilla.org/show_bug.cgi?id=1181482#c8, please resolve directly here.

::: dom/bluetooth/common/webapi/BluetoothGattCharacteristic.h
@@ +65,5 @@
> +
> +  BluetoothGattCharProp GetProperties() const
> +  {
> +    return mProperties;
> +  }

Move these two functions to others since they're not the binding functions for getting webidl attributes

@@ +169,5 @@
> +   * Assign the handle value for one of the descriptor within this GATT
> +   * characteristic. This function would be called only after a valid handle
> +   * value is retrieved from the Bluetooth backend.
> +   *
> +   * @param aDescriptorUuid [in] BluetoothUuid of this GATT descriptor.

nit: s/this GATT descriptor/the target GATT descriptor

@@ +170,5 @@
> +   * characteristic. This function would be called only after a valid handle
> +   * value is retrieved from the Bluetooth backend.
> +   *
> +   * @param aDescriptorUuid [in] BluetoothUuid of this GATT descriptor.
> +   * @param aDescriptorHandle [in] The handle value of this GATT descriptor.

DITTO.

@@ +257,5 @@
> +   */
> +  const BluetoothAttRole mAttRole;
> +
> +  /**
> +   * Activeness of this GATT characteristic. True means this service does react

insert an empty line before True.

::: dom/bluetooth/common/webapi/BluetoothGattDescriptor.cpp
@@ +97,5 @@
> +  memset(&mDescriptorId, 0, sizeof(mDescriptorId));
> +  StringToUuid(aDescriptorUuid, mDescriptorId.mUuid);
> +
> +  // permissions
> +  if (aPermissions.mRead) {

Use an utility function as I mentioned in Characteristic.cpp.

::: dom/bluetooth/common/webapi/BluetoothGattDescriptor.h
@@ +181,5 @@
> +   */
> +  const BluetoothAttRole mAttRole;
> +
> +  /**
> +   * Activeness of this GATT descriptor. True means this service does react

nit: empty line before True.

@@ +193,5 @@
> +   */
> +  bool mActive;
> +
> +  /**
> +   * Handle for this GATT descriptor. The value is only valid if |mAttRole|

nit: empty line before The.

::: dom/bluetooth/common/webapi/BluetoothGattServer.cpp
@@ +212,5 @@
>    if (mServerIf > 0) {
>      bs->UnregisterGattServerInternal(mServerIf, nullptr);
>    }
>  
> +  if (!mAppUuid.IsEmpty()) {

check if signal registered?

@@ +493,5 @@
> +{
> +  BluetoothService* bs = BluetoothService::Get();
> +  if (NS_WARN_IF(!bs)) {
> +    DispatchReplyError(new BluetoothVoidReplyRunnable(nullptr, mPromise),
> +                       STATUS_FAIL);

Could we also use MaybeReject directly here and in other tasks below?

::: dom/bluetooth/common/webapi/BluetoothGattService.cpp
@@ +134,5 @@
> +  MOZ_ASSERT(!mActive);
> +  MOZ_ASSERT(!aCharacteristicHandle);
> +
> +  size_t index = mCharacteristics.IndexOf(aCharacteristicUuid);
> +  NS_ENSURE_TRUE_VOID(index != mCharacteristics.NoIndex);

mCharacteristics[index]->AssignCharacteristicHandle(aCharactersitcHandle);

@@ +137,5 @@
> +  size_t index = mCharacteristics.IndexOf(aCharacteristicUuid);
> +  NS_ENSURE_TRUE_VOID(index != mCharacteristics.NoIndex);
> +  nsRefPtr<BluetoothGattCharacteristic> characteristic =
> +    mCharacteristics.ElementAt(index);
> +  NS_ENSURE_TRUE_VOID(characteristic);

is this possible?

@@ +153,5 @@
> +  MOZ_ASSERT(!aCharacteristicHandle);
> +  MOZ_ASSERT(!aDescriptorHandle);
> +
> +  size_t index = mCharacteristics.IndexOf(aCharacteristicHandle);
> +  NS_ENSURE_TRUE_VOID(index != mCharacteristics.NoIndex);

DITTO.

@@ +267,5 @@
> +                                    aPermissions,
> +                                    aProperties,
> +                                    aValue);
> +
> +  DispatchReplySuccess(new AddCharacteristicTask(this,

Resolve directly.

@@ +332,5 @@
> +  BT_ENSURE_TRUE_REJECT(aIncludedService.mActive,
> +                        promise,
> +                        NS_ERROR_UNEXPECTED);
> +
> +  DispatchReplySuccess(new AddIncludedServiceTask(this,

Resolve directly.

::: dom/bluetooth/common/webapi/BluetoothGattService.h
@@ +143,5 @@
> +  private:
> +    nsRefPtr<BluetoothGattService> mService;
> +    nsRefPtr<BluetoothGattService> mIncludedService;
> +  };
> +  friend class AddIncludedServiceTask;

These tasks are now unnecessary if we resolve directly, right?

::: dom/bluetooth/ipc/BluetoothServiceChildProcess.h
@@ +318,5 @@
> +  GattServerAddServiceInternal(
> +    const nsAString& aAppUuid,
> +    const BluetoothGattServiceId& aServiceId,
> +    uint16_t aHandleCount,
> +    BluetoothReplyRunnable* aRunnable);

Add override for these functions.

::: dom/webidl/BluetoothGattCharacteristic.webidl
@@ +30,5 @@
> +
> +/**
> + * BluetoothGattCharacteristic could be in the server role as some part of a
> + * local GATT server, or in the client role as some part of a remote GATT
> + * client. 

DITTO in GattService.webidl

@@ +55,5 @@
>  
>    /**
>     * Start or stop subscribing notifications of this characteristic from the
> +   * remote GATT server. This API will be rejected if this characteristic is in
> +   * the server role.

A new line before "This API..."

@@ +63,5 @@
>    [NewObject]
>    Promise<void> stopNotifications();
> +
> +  /**
> +   * Add BLE descriptors to the local GATT characteristic. The promise will be

a BLE descriptor

@@ +64,5 @@
>    Promise<void> stopNotifications();
> +
> +  /**
> +   * Add BLE descriptors to the local GATT characteristic. The promise will be
> +   * rejected if this characteristic is in the client role.

DITTO in GattService.webidl

in the client role since ...

::: dom/webidl/BluetoothGattDescriptor.webidl
@@ +5,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/**
> + * BluetoothGattDescriptor could be in the server role as some part of a local
> + * GATT server, or in the client role as some part of a remote GATT client. 

Please see my comments in the BluetoothGattService.webidl

::: dom/webidl/BluetoothGattService.webidl
@@ +4,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/**
> + * BluetoothGattService could be in the server role as some part of a local

nit: suggest to revise as "as a service provided by a local...

@@ +5,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/**
> + * BluetoothGattService could be in the server role as some part of a local
> + * GATT server, or in the client role as some part of a remote GATT client. 

suggest to revise as "as a service provided by a remote gatt server"

remove the trailing space

@@ +20,5 @@
>    readonly attribute DOMString                              uuid;
>    readonly attribute unsigned short                         instanceId;
> +
> +  /**
> +   * Add BLE characteristics to the local GATT service. This API will be

s/Add BLE characteristics/Add a BLE characteristic

add a newline before "This API..."

@@ +21,5 @@
>    readonly attribute unsigned short                         instanceId;
> +
> +  /**
> +   * Add BLE characteristics to the local GATT service. This API will be
> +   * rejected if this service is in the client role.

Suggest to revise as "This API will be rejected if ... client role since a GATT client is not allowed to manipulate the characteristic list in a remote GATT server."

an other option here,

@@ +32,5 @@
> +    ArrayBuffer value);
> +
> +  /**
> +   * Add BLE included services to the local GATT service. This API will be
> +   * rejected if this service is in the client role.

DITTO.

Comment 19

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

Comment on attachment 8655361 [details] [diff] [review]
bug1181479_0003_bluetoothgattserver_service_management.v4.patch

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

::: dom/bluetooth/common/webapi/BluetoothGattService.cpp
@@ +107,5 @@
>  
> +void
> +BluetoothGattService::AssignAppUuid(const nsAString& aAppUuid)
> +{
> +  MOZ_ASSERT(BluetoothAttRole == ATT_SERVER_ROLE);

Should be |mAttRole == ATT_SERVER_ROLE|, please fix it in other places also.

Comment 20

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

Comment on attachment 8655361 [details] [diff] [review]
bug1181479_0003_bluetoothgattserver_service_management.v4.patch

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

Sorry for not finding out these small pieces at the first place.
You could see compile errors when building debug build.

::: dom/bluetooth/common/webapi/BluetoothGattCharacteristic.cpp
@@ +262,5 @@
> +  const BluetoothAttributeHandle& aDescriptorHandle)
> +{
> +  MOZ_ASSERT(BluetoothAttRole == ATT_SERVER_ROLE);
> +  MOZ_ASSERT(!mActive);
> +  MOZ_ASSERT(!aDescriptorHandle);

Here and other similar places should be !aDescriptorHandle.mHandle

::: dom/bluetooth/common/webapi/BluetoothGattServer.cpp
@@ +68,5 @@
>  
>  void
> +BluetoothGattServer::HandleServiceHandleUpdated(const BluetoothValue& aValue)
> +{
> +  MOZ_ASSERT(v.type() == BluetoothValue::TArrayOfBluetoothNamedValue);

aValue

::: dom/bluetooth/common/webapi/BluetoothGattService.cpp
@@ +202,5 @@
> +  : BluetoothReplyRunnable(nullptr, aPromise)
> +  , mService(aService)
> +  , mCharacteristic(aCharacteristic)
> +{
> +  MOZ_ASSERT(mServer);

mService

@@ +282,5 @@
> +  : BluetoothReplyRunnable(nullptr, aPromise)
> +  , mService(aService)
> +  , mIncludedService(aIncludedService)
> +{
> +  MOZ_ASSERT(mServer);

mService

Comment 21

[Bruce Sun [:brsun]]

Attachment: bug1181479_0003_bluetoothgattserver_service_management.v5.patch

Address comment 18, comment 19, comment 20.

Comment 22

[Bruce Sun [:brsun]]

Attachment: bug1181479_0004_sync_coding_style.v1.patch

Sync the coding style.

Comment 23

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

Comment on attachment 8656576 [details] [diff] [review]
bug1181479_0003_bluetoothgattserver_service_management.v5.patch

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

My review is still in progress, post these comments first since I can't transfer my draft to other devices.

::: dom/bluetooth/common/webapi/BluetoothGattService.h
@@ +244,5 @@
> +   */
> +  const BluetoothAttRole mAttRole;
> +
> +  /**
> +   * Activeness of this GATT service. True means this service does react with

an empty line before True.

::: dom/webidl/BluetoothGattCharacteristic.webidl
@@ +30,5 @@
> +
> +/**
> + * BluetoothGattCharacteristic could be in the server role as a characteristic
> + * provided by a local GATT server, or in the client role as a characteristic
> + * provided by a remote GATT server. 

trailing space.

@@ +64,5 @@
>    [NewObject]
>    Promise<void> stopNotifications();
> +
> +  /**
> +   * Add BLE a descriptor to the local GATT characteristic.

a BLE

::: dom/webidl/BluetoothGattDescriptor.webidl
@@ +5,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/**
> + * BluetoothGattDescriptor could be in the server role as some part of a local
> + * GATT server, or in the client role as some part of a remote GATT client. 

trailing space

Please also revise as what I mentioned in my previous comment.


::: dom/webidl/BluetoothGattService.webidl
@@ +4,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/**
> + * BluetoothGattService could be in the server role as some part of a local

nit: suggest to revise as "as a service provided by a local...

@@ +10,5 @@
> + */
> +/**
> + * BluetoothGattDescriptor could be in the server role as a descriptor provided
> + * by a local GATT server, or in the client role as a descriptor provided by a
> + * remote GATT server. 

trailing space

::: dom/webidl/BluetoothGattService.webidl
@@ +6,5 @@
>  
> +/**
> + * BluetoothGattService could be in the server role as a service provided by a
> + * local GATT server, or in the client role as a service provided by a remote
> + * GATT server. 

trailing space

@@ +21,5 @@
>    readonly attribute DOMString                              uuid;
>    readonly attribute unsigned short                         instanceId;
> +
> +  /**
> +   * Add BLE a characteristic to the local GATT service.

a BLE

@@ +35,5 @@
> +    GattCharacteristicProperties properties,
> +    ArrayBuffer value);
> +
> +  /**
> +   * Add BLE included services to the local GATT service.

a BLE included service

@@ +38,5 @@
> +  /**
> +   * Add BLE included services to the local GATT service.
> +   *
> +   * This API will be rejected if this service is in the client role since a
> +   * GATT client is not allowed to manipulate the characteristic list in a

included service

Comment 24

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

Comment on attachment 8656576 [details] [diff] [review]
bug1181479_0003_bluetoothgattserver_service_management.v5.patch

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

Please see my questions and comments below, thanks.

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +1865,5 @@
> +
> +  ENSURE_GATT_INTF_IS_READY_VOID(aRunnable);
> +
> +  size_t index = sServers->IndexOf(aAppUuid, 0 /* Start */, UuidComparator());
> +  if (index == sServers->NoIndex) {

Shouldn't we reject in this case?
So as below.

::: dom/bluetooth/common/BluetoothReplyRunnable.cpp
@@ +59,5 @@
>      mPromise->MaybeResolve(aVal);
>    }
>  
> +  OnSuccessFired();
> +

remove this new line.

@@ +83,5 @@
>      mPromise->MaybeReject(rv);
>    }
>  
> +  OnErrorFired();
> +

DITTO.

@@ +229,5 @@
> +
> +  nsRefPtr<SubTask> task(aTask);
> +
> +  if (task) {
> +    mTasks.AppendElement(task.forget());

Why we need to do forget here?

@@ +258,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aSubReply);
> +
> +  nsRefPtr<SubReplyRunnable> reply = aSubReply;

What's this for?

@@ +275,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aSubReply);
> +
> +  nsRefPtr<SubReplyRunnable> reply = aSubReply;

DITTO.

@@ +308,5 @@
> +
> +void
> +BluetoothReplyTaskQueue::OnError()
> +{
> +}

How about putting OnSuccess and onError in header file only?

::: dom/bluetooth/common/BluetoothUtils.h
@@ +53,5 @@
>  nsresult
>  GenerateUuid(nsAString &aUuidString);
>  
> +void
> +GattPermissionsToDictionary(BluetoothGattAttrPerm aBits,

Could you also write some comments for these utility functions?

::: dom/bluetooth/common/webapi/BluetoothGattCharacteristic.cpp
@@ +53,5 @@
>    NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
>    NS_INTERFACE_MAP_ENTRY(nsISupports)
>  NS_INTERFACE_MAP_END
>  
> +const uint16_t BluetoothGattCharacteristic::sHandleCount = 2;

Write a comment to explain why the handleCount = 2 here.

::: dom/bluetooth/common/webapi/BluetoothGattCharacteristic.h
@@ +23,5 @@
>  }
>  
>  BEGIN_BLUETOOTH_NAMESPACE
>  
> +class BluetoothGattDescriptor;

is this needed?

@@ +253,5 @@
> +   */
> +  bool mActive;
> +
> +  /**
> +   * Handle for this GATT characteristic.

of

::: dom/bluetooth/common/webapi/BluetoothGattDescriptor.h
@@ +190,5 @@
> +   */
> +  bool mActive;
> +
> +  /**
> +   * Handle for this GATT descriptor.

of

::: dom/bluetooth/common/webapi/BluetoothGattServer.cpp
@@ +283,5 @@
> +      return false;
> +    }
> +
> +    bs->GattServerAddIncludedServiceInternal(
> +      mServer->mAppUuid,

Looks like we define it as a friend class for getting the value from mAppUuid only?
How about we add a member function in GattServer for this?

Same thing applies to other Add* tasks.

@@ +321,5 @@
> +
> +    BluetoothUuid uuid;
> +    mCharacteristic->GetUuid(uuid);
> +    bs->GattServerAddCharacteristicInternal(
> +      mServer->mAppUuid,

DITTO.

@@ +363,5 @@
> +
> +    BluetoothUuid uuid;
> +    mDescriptor->GetUuid(uuid);
> +    bs->GattServerAddDescriptorInternal(
> +      mServer->mAppUuid,

DITTO.

@@ +400,5 @@
> +      return false;
> +    }
> +
> +    bs->GattServerStartServiceInternal(
> +      mServer->mAppUuid,

DITTO.

@@ +430,5 @@
> +
> +  void ReleaseMembers() override
> +  {
> +    BluetoothReplyRunnable::ReleaseMembers();
> +    mService = nullptr;

mServer = nullptr also?

@@ +445,5 @@
> +private:
> +  void OnSuccessFired() override
> +  {
> +    mServer->mPendingService = nullptr;
> +

remove the new line.

@@ +446,5 @@
> +  void OnSuccessFired() override
> +  {
> +    mServer->mPendingService = nullptr;
> +
> +    mPromise->MaybeReject(NS_ERROR_NOT_AVAILABLE);

add a comment to describe that why we reject here.

@@ +452,5 @@
> +
> +  void OnErrorFired() override
> +  {
> +    mServer->mPendingService = nullptr;
> +

DITTO.

@@ +479,5 @@
> +    mService->GetIncludedServices(includedServices);
> +    for (size_t i = 0; i < includedServices.Length(); ++i) {
> +      nsRefPtr<SubTask> includedServiceTask =
> +        new AddIncludedServiceTask(this, mServer, mService, includedServices[i]);
> +      AppendTask(includedServiceTask.forget());

May I ask why the parameter type of AppendTask need to be Already_AddRefed?
Couldn't we use raw pointer and do |AppendTask(new xxxTask(..))| here and below?

@@ +542,5 @@
> +  : public BluetoothReplyRunnable
> +{
> +public:
> +  AddServiceTask(BluetoothGattServer* aServer,
> +                     BluetoothGattService* aService,

alignment

@@ +544,5 @@
> +public:
> +  AddServiceTask(BluetoothGattServer* aServer,
> +                     BluetoothGattService* aService,
> +                     Promise* aPromise)
> +    : BluetoothReplyRunnable(nullptr, nullptr)

add a comment after : BluetoothReplyRunnable(nullptr, nullptr)

@@ +579,5 @@
> +                                                            mService,
> +                                                            mPromise);
> +    nsresult rv = NS_DispatchToMainThread(runnable.forget());
> +
> +    if (NS_WARN_IF(NS_FAILED(rv))) { /* something wrong!!! */

remove the comment?

@@ +581,5 @@
> +    nsresult rv = NS_DispatchToMainThread(runnable.forget());
> +
> +    if (NS_WARN_IF(NS_FAILED(rv))) { /* something wrong!!! */
> +      mServer->mPendingService = nullptr;
> +

remove this new line.

@@ +589,5 @@
> +
> +  virtual void OnErrorFired() override
> +  {
> +    mServer->mPendingService = nullptr;
> +

DITTO.

@@ +690,5 @@
> +  BluetoothService* bs = BluetoothService::Get();
> +  BT_ENSURE_TRUE_REJECT(bs, promise, NS_ERROR_NOT_AVAILABLE);
> +
> +  bs->GattServerRemoveServiceInternal(
> +    mAppUuid, aService.GetServiceHandle(), new RemoveServiceTask(this,

How about using VoidReplyRunnable directly and notify GattServer a service has been removed after?

::: dom/bluetooth/common/webapi/BluetoothGattService.cpp
@@ +3,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "BluetoothReplyRunnable.h"

is this needed?

Comment 25

[Bruce Sun [:brsun]]

Hi Jocelyn,

Thank you soooooo much for reviewing my poor patches. I'll address your comments in the next patch. Please kindly also refer to answers inline below.

> @@ +229,5 @@
> > +
> > +  nsRefPtr<SubTask> task(aTask);
> > +
> > +  if (task) {
> > +    mTasks.AppendElement(task.forget());
> 
> Why we need to do forget here?
> 

Because using |forget()| can reduce the overhead of redundant reference counting +1/-1.

> @@ +258,5 @@
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  MOZ_ASSERT(aSubReply);
> > +
> > +  nsRefPtr<SubReplyRunnable> reply = aSubReply;
> 
> What's this for?
> 

I'll remove it.

> @@ +308,5 @@
> > +
> > +void
> > +BluetoothReplyTaskQueue::OnError()
> > +{
> > +}
> 
> How about putting OnSuccess and onError in header file only?
> 

The implementations are put in the source file by convention, and I don't have strong opinions not to put them in the header file. But not sure what benefit would be to put them in the header file. Would you mind sharing more on this?

> ::: dom/bluetooth/common/webapi/BluetoothGattServer.cpp
> @@ +283,5 @@
> > +      return false;
> > +    }
> > +
> > +    bs->GattServerAddIncludedServiceInternal(
> > +      mServer->mAppUuid,
> 
> Looks like we define it as a friend class for getting the value from
> mAppUuid only?
> How about we add a member function in GattServer for this?
> 
> Same thing applies to other Add* tasks.
> 

We could have one public function to get |mAppUuid|, but it might be only used in these inner tasks. Some other tasks would access other member variables (ex. |mServices|, |mPendingService|) of |BluetoothGattServer|. How about simply let inner tasks be friends of |BluetoothGattServer| and let them directly access whatever they want?

> @@ +479,5 @@
> > +    mService->GetIncludedServices(includedServices);
> > +    for (size_t i = 0; i < includedServices.Length(); ++i) {
> > +      nsRefPtr<SubTask> includedServiceTask =
> > +        new AddIncludedServiceTask(this, mServer, mService, includedServices[i]);
> > +      AppendTask(includedServiceTask.forget());
> 
> May I ask why the parameter type of AppendTask need to be Already_AddRefed?
> Couldn't we use raw pointer and do |AppendTask(new xxxTask(..))| here and
> below?
> 

Because I would like to let |AppenTask()| take the ownership of the address and make the caller not able to refer to the address anymore.

Basically if we want to handle the ownership of one pointer across functions precisely, probably we need to consider how the caller/callee behaves with it.

From the caller's perspective, when the caller calls |SomeFunction(SomeType*)|, the caller probably cannot sure whether this function will management the ownership properly. So the only thing the caller can do for this probably is always using |nsRefPtr| or another smart pointer to manage the address at the caller side.

From the callee's perspective, when the callee receives on pointer from |SomeFunction(SomeType*)|, the callee definitive cannot sure whether the caller will manage the life-cycle of the pointer for it. So the only thing the callee can do is always using |nsRefPtr| or another smart pointer to manage the address at the callee side.

Regarding to the type of the parameter, if we decided to transfer the ownership through one function call, it would be better to explicitly declare the type as |Already_AddRefed<T>| to force the use case. So that we can guarantee the ownership can be transferred properly as expected, and the caller can safely discard the pointer without any concern.

> @@ +690,5 @@
> > +  BluetoothService* bs = BluetoothService::Get();
> > +  BT_ENSURE_TRUE_REJECT(bs, promise, NS_ERROR_NOT_AVAILABLE);
> > +
> > +  bs->GattServerRemoveServiceInternal(
> > +    mAppUuid, aService.GetServiceHandle(), new RemoveServiceTask(this,
> 
> How about using VoidReplyRunnable directly and notify GattServer a service
> has been removed after?
> 

The target service will be removed from |BluetoothGattServer::mServices| only if |RemoveServiceTask| executes successfully. If we use |VoidReplyRunnable| and encounters a error when removing the target service from the server, what would be the proper behavior we could do?

Comment 26

[Bruce Sun [:brsun]]

Attachment: bug1181479_0003_bluetoothgattserver_service_management.v6.patch

Address comment 24.

Comment 27

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

(In reply to Bruce Sun [:brsun] from comment #25)
> Hi Jocelyn,
> 
> Thank you soooooo much for reviewing my poor patches. I'll address your
> comments in the next patch. Please kindly also refer to answers inline below.
> 
> > @@ +229,5 @@
> > > +
> > > +  nsRefPtr<SubTask> task(aTask);
> > > +
> > > +  if (task) {
> > > +    mTasks.AppendElement(task.forget());
> > 
> > Why we need to do forget here?
> > 
> 
> Because using |forget()| can reduce the overhead of redundant reference
> counting +1/-1.
> 
> > @@ +308,5 @@
> > > +
> > > +void
> > > +BluetoothReplyTaskQueue::OnError()
> > > +{
> > > +}
> > 
> > How about putting OnSuccess and onError in header file only?
> > 
> 
> The implementations are put in the source file by convention, and I don't
> have strong opinions not to put them in the header file. But not sure what
> benefit would be to put them in the header file. Would you mind sharing more
> on this?
> 

I think it's easier for developers to get that there's a default version which does nothing in the base class when they looking at the ReplyRunnable.h while they want to inherit this class.

> > ::: dom/bluetooth/common/webapi/BluetoothGattServer.cpp
> > @@ +283,5 @@
> > > +      return false;
> > > +    }
> > > +
> > > +    bs->GattServerAddIncludedServiceInternal(
> > > +      mServer->mAppUuid,
> > 
> > Looks like we define it as a friend class for getting the value from
> > mAppUuid only?
> > How about we add a member function in GattServer for this?
> > 
> > Same thing applies to other Add* tasks.
> > 
> 
> We could have one public function to get |mAppUuid|, but it might be only
> used in these inner tasks. Some other tasks would access other member
> variables (ex. |mServices|, |mPendingService|) of |BluetoothGattServer|. How
> about simply let inner tasks be friends of |BluetoothGattServer| and let
> them directly access whatever they want?
> 

I think public functions for getting member values is reasonable, and we have GetUuid in some classes already.
Also, we don't need to declare many classes in BluetoothServer.h.
These are my main reasons for proposing that approach.

But I do agree it's a bit more consistent to making them all friend classes, you may keep this approach if that makes more sense to you, thanks.

> > @@ +690,5 @@
> > > +  BluetoothService* bs = BluetoothService::Get();
> > > +  BT_ENSURE_TRUE_REJECT(bs, promise, NS_ERROR_NOT_AVAILABLE);
> > > +
> > > +  bs->GattServerRemoveServiceInternal(
> > > +    mAppUuid, aService.GetServiceHandle(), new RemoveServiceTask(this,
> > 
> > How about using VoidReplyRunnable directly and notify GattServer a service
> > has been removed after?
> > 
> 
> The target service will be removed from |BluetoothGattServer::mServices|
> only if |RemoveServiceTask| executes successfully. If we use
> |VoidReplyRunnable| and encounters a error when removing the target service
> from the server, what would be the proper behavior we could do?

DispatchReplyError should be called for both approaches.
I meant to notify GattServer when the operation succeeded.

Comment 28

[Bruce Sun [:brsun]]

> > > @@ +308,5 @@
> > > > +
> > > > +void
> > > > +BluetoothReplyTaskQueue::OnError()
> > > > +{
> > > > +}
> > > 
> > > How about putting OnSuccess and onError in header file only?
> > > 
> > 
> > The implementations are put in the source file by convention, and I don't
> > have strong opinions not to put them in the header file. But not sure what
> > benefit would be to put them in the header file. Would you mind sharing more
> > on this?
> > 
> 
> I think it's easier for developers to get that there's a default version
> which does nothing in the base class when they looking at the
> ReplyRunnable.h while they want to inherit this class.
> 

It does help developers to get the idea that the base class does nothing on some functions by simply glancing on the header file. But in general cases, if developers need to implement a derived class, they would jump to the default implementation for reference. If we gather all the implementation of one class in the source file, they can switch one fewer file while looking up the base implementation.

BTW, there are some other functions which also have empty implementation in the file |BluetoothReplyRunnable.cpp|. Not sure if it is good to move all of them into the header file. Should we sync the coding style and gather all those empty functions as well?

> > > @@ +690,5 @@
> > > > +  BluetoothService* bs = BluetoothService::Get();
> > > > +  BT_ENSURE_TRUE_REJECT(bs, promise, NS_ERROR_NOT_AVAILABLE);
> > > > +
> > > > +  bs->GattServerRemoveServiceInternal(
> > > > +    mAppUuid, aService.GetServiceHandle(), new RemoveServiceTask(this,
> > > 
> > > How about using VoidReplyRunnable directly and notify GattServer a service
> > > has been removed after?
> > > 
> > 
> > The target service will be removed from |BluetoothGattServer::mServices|
> > only if |RemoveServiceTask| executes successfully. If we use
> > |VoidReplyRunnable| and encounters a error when removing the target service
> > from the server, what would be the proper behavior we could do?
> 
> DispatchReplyError should be called for both approaches.
> I meant to notify GattServer when the operation succeeded.

It seems bluedroid only calls |btgatt_server_callbacks_t.service_deleted_cb| only after |btgatt_server_interface_t.delete_service| has been called[1]. Probably we don't have use cases that the Bluetooth backend triggers this callback function actively without a previous |delete_service| function call.

[1] http://androidxref.com/5.1.1_r6/xref/external/bluetooth/bluedroid/bta/gatt/bta_gatts_act.c#544

Comment 29

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

(In reply to Bruce Sun [:brsun] from comment #28)
> > > > @@ +308,5 @@
> > > > > +
> > > > > +void
> > > > > +BluetoothReplyTaskQueue::OnError()
> > > > > +{
> > > > > +}
> > > > 
> > > > How about putting OnSuccess and onError in header file only?
> > > > 
> > > 
> > > The implementations are put in the source file by convention, and I don't
> > > have strong opinions not to put them in the header file. But not sure what
> > > benefit would be to put them in the header file. Would you mind sharing more
> > > on this?
> > > 
> > 
> > I think it's easier for developers to get that there's a default version
> > which does nothing in the base class when they looking at the
> > ReplyRunnable.h while they want to inherit this class.
> > 
> 
> It does help developers to get the idea that the base class does nothing on
> some functions by simply glancing on the header file. But in general cases,
> if developers need to implement a derived class, they would jump to the
> default implementation for reference. If we gather all the implementation of
> one class in the source file, they can switch one fewer file while looking
> up the base implementation.
> 

I think it's also helpful when we need to trace this specific function along with the overview of the class in the future.
Personally I would prefer put them in the header file, not a strong opinion though.

> BTW, there are some other functions which also have empty implementation in
> the file |BluetoothReplyRunnable.cpp|. Not sure if it is good to move all of
> them into the header file. Should we sync the coding style and gather all
> those empty functions as well?
> 

Maybe for virtual functions that does nothing only at the this moment?

> > > > @@ +690,5 @@
> > > > > +  BluetoothService* bs = BluetoothService::Get();
> > > > > +  BT_ENSURE_TRUE_REJECT(bs, promise, NS_ERROR_NOT_AVAILABLE);
> > > > > +
> > > > > +  bs->GattServerRemoveServiceInternal(
> > > > > +    mAppUuid, aService.GetServiceHandle(), new RemoveServiceTask(this,
> > > > 
> > > > How about using VoidReplyRunnable directly and notify GattServer a service
> > > > has been removed after?
> > > > 
> > > 
> > > The target service will be removed from |BluetoothGattServer::mServices|
> > > only if |RemoveServiceTask| executes successfully. If we use
> > > |VoidReplyRunnable| and encounters a error when removing the target service
> > > from the server, what would be the proper behavior we could do?
> > 
> > DispatchReplyError should be called for both approaches.
> > I meant to notify GattServer when the operation succeeded.
> 
> It seems bluedroid only calls |btgatt_server_callbacks_t.service_deleted_cb|
> only after |btgatt_server_interface_t.delete_service| has been called[1].
> Probably we don't have use cases that the Bluetooth backend triggers this
> callback function actively without a previous |delete_service| function call.
> 
> [1]
> http://androidxref.com/5.1.1_r6/xref/external/bluetooth/bluedroid/bta/gatt/
> bta_gatts_act.c#544

OK, then I'm also fine with using a task.

Comment 30

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

Comment on attachment 8657992 [details] [diff] [review]
bug1181479_0003_bluetoothgattserver_service_management.v6.patch

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

Hi Bruce,

Almost there, thanks a lot for the effort!
Please see my comments below.

If possible, please provide an interdiff this time or make sure the difftool on bugzilla can work (maybe it will work without rebase).

Thanks,
Jocelyn

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +1737,5 @@
> +               (int)aStatus);
> +    MOZ_ASSERT(mServer->mAddServiceState.mRunnable);
> +
> +    BluetoothService* bs = BluetoothService::Get();
> +    NS_ENSURE_TRUE_VOID(bs);

I think we can remove these two lines.

@@ +1764,5 @@
> +    BT_WARNING("BluetoothGattServerInterface::RegisterServer failed: %d",
> +               (int)aStatus);
> +
> +    BluetoothService* bs = BluetoothService::Get();
> +    NS_ENSURE_TRUE_VOID(bs);

DITTO. So as in other |OnError| below.

@@ +3015,5 @@
>            "ConnectPeripheral failed due to registration failed"));
>        server->mConnectPeripheralRunnable = nullptr;
>      }
>  
> +    if (server->mAddServiceState.mRunnable) {

else if?

@@ +3016,5 @@
>        server->mConnectPeripheralRunnable = nullptr;
>      }
>  
> +    if (server->mAddServiceState.mRunnable) {
> +      // Reject the connect peripheral request

add service

@@ +3043,5 @@
>        aServerIf, deviceAddr, true /* direct connect */, TRANSPORT_AUTO,
>        new ConnectPeripheralResultHandler(server, deviceAddr));
>    }
> +
> +  if (server->mAddServiceState.mRunnable) {

else if?

::: dom/bluetooth/common/BluetoothReplyRunnable.cpp
@@ +159,5 @@
> +{
> +  mRootQueue->OnErrorFired(this);
> +}
> +
> +class VoidSubReplyRunnable final

Suggest to put it in the header file to be consistent with BluetoothReplyRunnable&BluetoothVoidReplyRunnable.

@@ +251,5 @@
> +  return NS_OK;
> +}
> +
> +void
> +BluetoothReplyTaskQueue::OnSuccessFired(SubReplyRunnable* aSubReply)

Can we rename to something like OnSubtaskSuccessFired?
The function name is a bit of confusing here since we haven't fire a success reply for the task queue itself.

@@ +266,5 @@
> +  }
> +}
> +
> +void
> +BluetoothReplyTaskQueue::OnErrorFired(SubReplyRunnable* aSubReply)

DITTO.

@@ +279,5 @@
> +BluetoothReplyTaskQueue::FireSuccessReply()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  OnSuccess();

We can rename to OnSuccessFired to be consistent with BluetoothReplyRunnable.

@@ +289,5 @@
> +BluetoothReplyTaskQueue::FireErrorReply()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  OnError();

DITTO.

::: dom/bluetooth/common/webapi/BluetoothGattCharacteristic.cpp
@@ +73,5 @@
>    const BluetoothGattCharAttribute& aChar)
>    : mOwner(aOwner)
>    , mService(aService)
>    , mCharId(aChar.mId)
>    , mProperties(aChar.mProperties)

initialize mPermissions also?

@@ +251,5 @@
>  
> +void
> +BluetoothGattCharacteristic::GetUuid(BluetoothUuid& aUuid) const
> +{
> +  StringToUuid(mUuidStr, aUuid);

use mCharId.mUuid directly?

::: dom/bluetooth/common/webapi/BluetoothGattDescriptor.cpp
@@ +62,5 @@
>    const BluetoothGattId& aDescriptorId)
>    : mOwner(aOwner)
>    , mCharacteristic(aCharacteristic)
>    , mDescriptorId(aDescriptorId)
> +  , mAttRole(ATT_CLIENT_ROLE)

do we need to initialize mPermissions also?

@@ +151,5 @@
>  
> +void
> +BluetoothGattDescriptor::GetUuid(BluetoothUuid& aUuid) const
> +{
> +  StringToUuid(mUuidStr, aUuid);

isn't there already a mDescriptorId.mUuid?

::: dom/bluetooth/common/webapi/BluetoothGattServer.cpp
@@ +52,5 @@
>    : mOwner(aOwner)
>    , mServerIf(0)
>    , mValid(true)
> +{
> +  if (NS_SUCCEEDED(GenerateUuid(mAppUuid)) && !mAppUuid.IsEmpty()) {

How about we check if the generated uuid is empty and return fail in patch2?
Then we could just check the return status of |GenerateUuid|.

@@ +211,5 @@
>  
>    if (mServerIf > 0) {
> +    bs->UnregisterGattServerInternal(mServerIf,
> +                                     new BluetoothVoidReplyRunnable(nullptr,
> +                                                                    nullptr));

new BluetoothVoidReplyRunnable(nullptr) should be enough.

@@ +447,5 @@
> +    mPromise = nullptr;
> +  }
> +
> +protected:
> +  bool ParseSuccessfulReply(JS::MutableHandle<JS::Value> aValue) override

How about we derived from BluetoothVoidReplyRunnable to save this?

@@ +577,5 @@
> +    mPromise = nullptr;
> +  }
> +
> +protected:
> +  bool ParseSuccessfulReply(JS::MutableHandle<JS::Value> aValue) override

How about we derived from BluetoothVoidReplyRunnable?

::: dom/bluetooth/common/webapi/BluetoothGattService.cpp
@@ +243,5 @@
> +                        NS_ERROR_UNEXPECTED);
> +
> +  /* The service should not be actively acting with the Bluetooth backend.
> +   * Otherwise, included services cannot be added into the service. */
> +  BT_ENSURE_TRUE_REJECT(!mActive, promise, NS_ERROR_UNEXPECTED);

I think we should document this limitation in webidl and wiki. (for all add* functions)

::: dom/webidl/BluetoothGattServer.webidl
@@ +9,5 @@
>  {
>    // Fired when a remote device has been connected/disconnected
>    attribute EventHandler  onconnectionstatechanged;
>  
> +  [Cached, Pure] readonly attribute sequence<BluetoothGattService> services;

nit: Put attributes in the first section of the interface.

Comment 31

[Bruce Sun [:brsun]]

Attachment: bug1181479_0003_bluetoothgattserver_service_management.v7.patch

Address comment 30.

> with the overview of the class in the future.
> Personally I would prefer put them in the header file, not a strong opinion
> though.
> Maybe for virtual functions that does nothing only at the this moment?

The current coding style of |BluetoothReplyRunnable| is a little confusing to me. Most definitions are placed in the source file, except for |BluetoothReplyRunnable::SetError()| and |BluetoothVoidReplyRunnable::ParseSuccessfulReply|. One of the exception is a non-virtual function, the other one is a virtual one. None of them are empty functions. In order not to make the coding style of |BluetoothReplyRunnable| more and more complicate in such small files (there are only around 100 lines in the header file and the source files), I would place all the definitions in the source file if your don't mind.

> @@ +3015,5 @@
> >            "ConnectPeripheral failed due to registration failed"));
> >        server->mConnectPeripheralRunnable = nullptr;
> >      }
> >  
> > +    if (server->mAddServiceState.mRunnable) {
> 
> else if?
> 

Since the registration would be triggered by connecting and adding services, we need to continue each of them after the registration has been successfully finished. But it seems these two co-existing operations are not handled well in my previous patch, I merged some logic into one handler this time. Thanks for point this out.

> @@ +251,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +void
> > +BluetoothReplyTaskQueue::OnSuccessFired(SubReplyRunnable* aSubReply)
> 
> Can we rename to something like OnSubtaskSuccessFired?
> The function name is a bit of confusing here since we haven't fire a success
> reply for the task queue itself.
> 

How about |OnSubReplySuccessFired()|? The timing to call this function is changed a little bit later as well to sync with the function name.

I also found another issue in my original patch. If |AddServiceTaskQueue| fails, the promise would be rejected twice in |CancelAddServiceTask| and |BluetoothReplyTaskQueue::FireErrorReply()|. The fix is contained in this patch. Thanks for pointing this out again.

> ::: dom/bluetooth/common/webapi/BluetoothGattServer.cpp
> @@ +52,5 @@
> >    : mOwner(aOwner)
> >    , mServerIf(0)
> >    , mValid(true)
> > +{
> > +  if (NS_SUCCEEDED(GenerateUuid(mAppUuid)) && !mAppUuid.IsEmpty()) {
> 
> How about we check if the generated uuid is empty and return fail in patch2?
> Then we could just check the return status of |GenerateUuid|.

To exam whether |mAppUuid| is empty or not probably is more related to the caller logic. By digging into the implementation of |GenerateUuid(nsAString &aUuidString)|, it seems |aUuidString| won't be empty if it executes successfully. Would you suggest the caller not to test the emptiness of UUID string by always trusting the return value of the generating function?

Comment 32

[Bruce Sun [:brsun]]

Attachment: bug1181479_0003_bluetoothgattserver_service_management.v6-v7.diff

Interdiff between attachment 8657992 [details] [diff] [review] and attachment 8660545 [details] [diff] [review].

Comment 33

[Bruce Sun [:brsun]]

Attachment: bug1181479_0004_sync_coding_style.v2.patch

Remove more redundant |BluetoothService| checking while handling comment 30.

Comment 34

[Bruce Sun [:brsun]]

Attachment: bug1181479_0004_sync_coding_style.v1-v2.diff

Interdiff between attachment 8656580 [details] [diff] [review] and attachment 8660547 [details] [diff] [review].

Comment 35

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

Comment on attachment 8660547 [details] [diff] [review]
bug1181479_0004_sync_coding_style.v2.patch

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

LGTM.

Comment 36

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

Comment on attachment 8660545 [details] [diff] [review]
bug1181479_0003_bluetoothgattserver_service_management.v7.patch

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

r=me, thanks a lot for your time and effort.

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +1487,5 @@
> +      mServer->mAddServiceState.Reset();
> +    }
> +
> +    mServer->mIsRegistering = false;
> +

remove the empty line.

@@ +1588,5 @@
>  
>      // connect will be proceeded after server registered
>      sBluetoothGattInterface->RegisterServer(
>        uuid, new RegisterServerResultHandler(server));
>    }

The logic is a bit of unclear by looking to this function only.
I would suggest to add a simple comment here to describe that this connect request will be processed after the ongoing server registration is done.
Or add a comment before this if section to describe all the cases here and the corresponding actions.

@@ +1799,5 @@
> +
> +    // add service will be proceeded after server registered
> +    sBluetoothGattInterface->RegisterServer(
> +      uuid, new RegisterServerResultHandler(server));
> +  }

DITTO.

@@ +2967,2 @@
>    BluetoothService* bs = BluetoothService::Get();
> +

remove this empty line, and so as below.

@@ +2967,3 @@
>    BluetoothService* bs = BluetoothService::Get();
> +
> +  if (!bs || aStatus != GATT_STATUS_SUCCESS) {

Please open a follow up bug to reject requests in other places if bs is null.

::: dom/bluetooth/common/webapi/BluetoothGattServer.cpp
@@ +448,5 @@
> +private:
> +  void OnSuccessFired() override
> +  {
> +    mServer->mPendingService = nullptr;
> +    mPromise->MaybeReject(NS_ERROR_NOT_AVAILABLE);

I think FAIL would be better here.

@@ +454,5 @@
> +
> +  void OnErrorFired() override
> +  {
> +    mServer->mPendingService = nullptr;
> +    mPromise->MaybeReject(NS_ERROR_NOT_AVAILABLE);

DITTO.

::: dom/webidl/BluetoothGattService.webidl
@@ +39,5 @@
> +   * Add a BLE included service to the local GATT service.
> +   *
> +   * This API will be rejected if this service is in the client role since a
> +   * GATT client is not allowed to manipulate the included service list in a
> +   * remote GATT server. The being added included service should be an existing

The included service to be added should be an...

Comment 37

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

Comment on attachment 8660546 [details] [diff] [review]
bug1181479_0003_bluetoothgattserver_service_management.v6-v7.diff

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

Thanks for the interdiff.

Comment 38

[Bruce Sun [:brsun]]

> @@ +2967,3 @@
> >    BluetoothService* bs = BluetoothService::Get();
> > +
> > +  if (!bs || aStatus != GATT_STATUS_SUCCESS) {
> 
> Please open a follow up bug to reject requests in other places if bs is null.
> 

The follow-up bug is created as bug 1205162.

Comment 39

[Bruce Sun [:brsun]]

Attachment: bug1181479_0003_bluetoothgattserver_service_management.v8.patch

Address comment 36.

Comment 40

[Bruce Sun [:brsun]]

Comment on attachment 8661609 [details] [diff] [review]
bug1181479_0003_bluetoothgattserver_service_management.v8.patch

Hi Blake,

In order to provide the facilities to management the Bluetooth GATT service, some of Bluetooth related WebAPIs would be added by this bug:
 - BluetoothGattServer.addService()[1]
 - BluetoothGattServer.removeService()[2]
 - BluetoothGattService.addCharacteristic()[3]
 - BluetoothGattService.addIncludedService()[4]
 - BluetoothGattCharacteristic.addDescriptor()[5]

Would you please kindly help review this patch regarding to the WebAPI parts?

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattServer#addService.28BluetoothGattService_service.29
[2] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattServer#removeService.28BluetoothGattService_service.29
[3] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattService#addCharacteristic.28DOMString_uuid.2C_GattPermissions_permissions.2C_GattCharacteristicProperties_properties.2C_ArrayBuffer_value.29
[4] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattService#addIncludedService.28BluetoothGattService_service.29
[5] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattCharacteristic#addDescriptor.28DOMString_uuid.2C_GattPermissions_permissions.2C_ArrayBuffer_value.29

Comment 41

[Bruce Sun [:brsun]]

Attachment: bug1181479_0003_bluetoothgattserver_service_management.v9.patch

Thanks for the reminder from Jocelyn on bug 118148[1], some functions need to return earlier after the reply has been dispatched.

Hi Blake,
Kindly ignore the previous patch and use this one instead. Sorry for the inconvenience.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1181483#c5

Comment 42

[Bruce Sun [:brsun]]

Attachment: bug1181479_0001_refine_stringtouuid.commit.patch

Comment 43

[Bruce Sun [:brsun]]

Attachment: bug1181479_0002_refine_generateuuid.commit.patch

Comment 44

[Bruce Sun [:brsun]]

Attachment: bug1181479_0003_bluetoothgattserver_service_management.commit.patch

Comment 45

[Bruce Sun [:brsun]]

Attachment: bug1181479_0004_sync_coding_style.commit.patch

Comment 46

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/784aae3f40bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c07214ac152
https://hg.mozilla.org/integration/mozilla-inbound/rev/89c64eb24508
https://hg.mozilla.org/integration/mozilla-inbound/rev/778062b31fd9

Comment 47

[Bruce Sun [:brsun]]

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

Comment 48

[Bruce Sun [:brsun]]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=778062b31fd9

Comment 49

[Bruce Sun [:brsun]]

Backout all the commits in comment 46 due to wrong integration repository.

remote: View your changes here:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/302e3afc26b5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e67c69fc15a1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/72b0e3dd91ee
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/59f692be81f8
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=59f692be81f8

Comment 50

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/a072161e2fe3
https://hg.mozilla.org/integration/b2g-inbound/rev/8ea6cf517844
https://hg.mozilla.org/integration/b2g-inbound/rev/e9ad05a19d60
https://hg.mozilla.org/integration/b2g-inbound/rev/e02b9668db81

Comment 51

[Bruce Sun [:brsun]]

b2g-inbound should be the right integration repository.

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

Comment 52

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

https://hg.mozilla.org/mozilla-central/rev/a072161e2fe3
https://hg.mozilla.org/mozilla-central/rev/8ea6cf517844
https://hg.mozilla.org/mozilla-central/rev/e9ad05a19d60
https://hg.mozilla.org/mozilla-central/rev/e02b9668db81