Closed Bug 1181479 Opened 5 years ago Closed 5 years ago

Implement GATT Server service management

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox44 fixed)

RESOLVED FIXED
FxOS-S8 (02Oct)
Tracking Status
firefox44 --- fixed

People

(Reporter: brsun, Assigned: brsun)

References

Details

Attachments

(4 files, 21 obsolete files)

5.07 KB, patch
brsun
: review+
Details | Diff | Splinter Review
5.34 KB, patch
brsun
: review+
Details | Diff | Splinter Review
201.13 KB, patch
brsun
: review+
Details | Diff | Splinter Review
10.02 KB, patch
brsun
: review+
Details | Diff | Splinter Review
GATT Server service management:
 - Add services
 - Remove services
Blocks: 1181482
Depends on: 1181480
WIP patch
Assignee: nobody → brsun
Attachment #8647486 - Attachment is obsolete: true
Attachment #8649238 - Flags: review?(joliu)
Attachment #8647487 - Attachment is obsolete: true
Attachment #8649239 - Flags: review?(joliu)
Attachment #8647488 - Attachment is obsolete: true
Attachment #8649240 - Flags: review?(joliu)
Comment on attachment 8649238 [details] [diff] [review]
bug1181479_0001_refine_stringtouuid.v1.patch

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

LGTM.
Attachment #8649238 - Flags: review?(joliu) → review+
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.
Attachment #8649239 - Flags: review?(joliu)
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.
Attachment #8649239 - Attachment is obsolete: true
Attachment #8649240 - Attachment is obsolete: true
Attachment #8649240 - Flags: review?(joliu)
Attachment #8651730 - Flags: review?(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.
Attachment #8651730 - Flags: review?(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.
Attachment #8649238 - Attachment is obsolete: true
Attachment #8654807 - Flags: review+
Use |nsresult| as the return type of |GenerateUuid()|.
Attachment #8654809 - Flags: review?(joliu)
Address comment 10 and comment 11.
Attachment #8651730 - Attachment is obsolete: true
Attachment #8654810 - Flags: review?(joliu)
Address comment 15.
Attachment #8654809 - Attachment is obsolete: true
Attachment #8655359 - Flags: review?(joliu)
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|.
Attachment #8654810 - Attachment is obsolete: true
Attachment #8654810 - Flags: review?(joliu)
Attachment #8655361 - Flags: review?(joliu)
Attachment #8655359 - Flags: review?(joliu) → review+
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.
Attachment #8655361 - Flags: review?(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 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
Address comment 18, comment 19, comment 20.
Attachment #8655361 - Attachment is obsolete: true
Attachment #8656576 - Flags: review?(joliu)
Sync the coding style.
Attachment #8656580 - Flags: review?(joliu)
Attachment #8656580 - Flags: review?(joliu) → review+
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 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?
Attachment #8656576 - Flags: review?(joliu)
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?
Address comment 24.
Attachment #8656576 - Attachment is obsolete: true
Attachment #8657992 - Flags: review?(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.
> > > @@ +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
(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 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.
Attachment #8657992 - Flags: review?(joliu)
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?
Attachment #8657992 - Attachment is obsolete: true
Attachment #8660545 - Flags: review?(joliu)
Remove more redundant |BluetoothService| checking while handling comment 30.
Attachment #8656580 - Attachment is obsolete: true
Attachment #8660547 - Flags: review?(joliu)
Comment on attachment 8660547 [details] [diff] [review]
bug1181479_0004_sync_coding_style.v2.patch

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

LGTM.
Attachment #8660547 - Flags: review?(joliu) → review+
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...
Attachment #8660545 - Flags: review?(joliu) → review+
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.
Attachment #8660546 - Flags: feedback?(joliu)
Attachment #8660548 - Flags: feedback?(joliu)
> @@ +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.
Address comment 36.
Attachment #8660545 - Attachment is obsolete: true
Attachment #8660546 - Attachment is obsolete: true
Attachment #8660548 - Attachment is obsolete: true
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
Attachment #8661609 - Flags: review?(mrbkap)
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
Attachment #8661609 - Attachment is obsolete: true
Attachment #8661609 - Flags: review?(mrbkap)
Attachment #8661687 - Flags: review?(mrbkap)
Attachment #8661687 - Flags: review?(mrbkap) → review+
Attachment #8654807 - Attachment is obsolete: true
Attachment #8664066 - Flags: review+
Attachment #8655359 - Attachment is obsolete: true
Attachment #8664067 - Flags: review+
Attachment #8660547 - Attachment is obsolete: true
Attachment #8664069 - Flags: review+
b2g-inbound should be the right integration repository.

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=e02b9668db81
You need to log in before you can comment on or make changes to this bug.