Closed Bug 1063444 Opened 5 years ago Closed 5 years ago

Implement LE scan related methods and event handler

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox40 fixed)

RESOLVED FIXED
2.2 S12 (15may)
Tracking Status
firefox40 --- fixed

People

(Reporter: ben.tian, Assigned: jaliu)

References

Details

(Whiteboard: [webbt-api])

Attachments

(1 file, 18 obsolete files)

92.34 KB, patch
Details | Diff | Splinter Review
* start/stopLeScan: https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothAdapter#startLeScan.28sequence.3CDOMString.3E_aServiceUuids.29
  [NewObject, Throws] Promise<BluetoothDiscoveryHandle> startLeScan(sequence<DOMString> aServiceUuids);
  [NewObject, Throws] Promise<void> stopLeScan(BluetoothDiscoveryHandle aDiscoveryHandle);

* BluetoothLeDeviceEvent: https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothLeDeviceEvent
Assignee: nobody → btian
Blocks: 933357
Depends on: 1054830
Whiteboard: [webbt-api]
Assignee: btian → jaliu
Attachment #8546322 - Attachment is obsolete: true
See Also: → 1146792
- Rebase
Attachment #8546323 - Attachment is obsolete: true
Attachment #8573764 - Attachment is obsolete: true
Attachment #8582233 - Flags: review?(shuang)
Attachment #8582233 - Flags: feedback?(joliu)
Comment on attachment 8582233 [details] [diff] [review]
Implement LE scan related methods and LE device event (v3)

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

Hi Jamin,

Please see my comments below.
I haven't take a clear look into BluetoothAdapter.cpp/h, will do it in the next round after these comments are addressed.

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +1094,5 @@
>  
>    // Remove the device with the same address
>    nsString deviceAddress = arr[0].value().get_nsString();
> +
> +  // Remove the device with the same address

redundent.

::: dom/bluetooth2/BluetoothCommon.h
@@ +286,5 @@
>  
>  struct BluetoothUuid {
>    uint8_t mUuid[16];
> +
> +  inline bool isEqualTo(const BluetoothUuid aUuid) const

no necessary if you use BluetoothGattClient.

::: dom/bluetooth2/BluetoothDevice.cpp
@@ +324,5 @@
>    return mGatt;
>  }
>  
> +void
> +BluetoothDevice::ParsePropertiesFromAdvData(const nsTArray<uint8_t>& aAdvData)

Please leave comments to explain this function.

::: dom/bluetooth2/BluetoothDevice.h
@@ +144,5 @@
>    bool IsDeviceAttributeChanged(BluetoothDeviceAttribute aType,
>                                  const BluetoothValue& aValue);
>  
> +  /**
> +   * Parse advertising data to get and update device properties.

Just state |to update| is clear enough.

Besides, I think the most important thing of this function is updating device properties?
How about something like |UpdatePropertiesFromAdvData|?
I don't have a strong opinion on this though.

::: dom/bluetooth2/BluetoothDiscoveryHandle.cpp
@@ +87,5 @@
> +  MOZ_ASSERT(aLeDevice);
> +
> +  nsTArray<nsString> remoteUuids;
> +  aLeDevice->GetUuids(remoteUuids);
> +

Leave a comment before the below section to state why we need to filter the result.

::: dom/bluetooth2/BluetoothDiscoveryHandle.h
@@ +56,5 @@
> +
> +  virtual ~BluetoothDiscoveryHandle();
> +
> +  /**
> +   * Client interface ID of GATT HALL

nit:
GATT client interface ID given by bluetooth stack.

@@ +58,5 @@
> +
> +  /**
> +   * Client interface ID of GATT HALL
> +   */
> +  int mClientIf;

Why is mClientIf needed for DiscoveryHandle?

::: dom/bluetooth2/BluetoothInterface.h
@@ +646,5 @@
> +  const BluetoothUuid GetAppUuid() { return mAppUuid; }
> +
> +  private:
> +    int mClientIf;
> +    BluetoothUuid mAppUuid;

Please use |BluetoothGattClient| for them.
https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/bluedroid/BluetoothGattManager.cpp#42

::: dom/bluetooth2/BluetoothLeDeviceEvent.cpp
@@ +173,5 @@
> +  JS::ExposeObjectToActiveJS(mScanRecord);
> +  aScanRecord.set(mScanRecord);
> +
> +  return;
> +}

Before looking into this event implementation, please make sure event codegen couldn't support our case here.
Use event code gen whenever possible, if it's not working, please open a bug for this.

::: dom/bluetooth2/BluetoothLeDeviceEvent.h
@@ +63,5 @@
> +};
> +
> +END_BLUETOOTH_NAMESPACE
> +
> +#endif // mozilla_dom_bluetooth_BluetoothLeDeviceEvent_h

DITTO.

::: dom/bluetooth2/BluetoothService.h
@@ +197,5 @@
> +   * Stops an ongoing Bluetooth LE device scan.
> +   *
> +   * @return NS_OK if LeScan stopped correctly, false otherwise
> +   */
> +  virtual nsresult

nsresult looks unnecessary to me since we will always return NS_OK?
If you worried about consistency, you may also revise the function signature of |Start/StopDiscoveryInternal|.

::: dom/bluetooth2/bluedroid/BluetoothDaemonHelpers.cpp
@@ +873,5 @@
>      CONVERT(PROPERTY_ADAPTER_DISCOVERY_TIMEOUT, 0x09),
>      CONVERT(PROPERTY_REMOTE_FRIENDLY_NAME, 0x0a),
>      CONVERT(PROPERTY_REMOTE_RSSI, 0x0b),
>      CONVERT(PROPERTY_REMOTE_VERSION_INFO, 0x0c),
> +    CONVERT(PROPERTY_REMOTE_ADV_DATA, 0x0d),

why we need to revise DaemonHelpers now?

::: dom/bluetooth2/bluedroid/BluetoothGattHALInterface.cpp
@@ +520,5 @@
>    int status = BT_STATUS_UNSUPPORTED;
>  #endif
>  
>    if (aRes) {
> +    if (aStart) {

Why need this if-else for doing the same thing?

@@ +912,5 @@
>    status = BT_STATUS_UNSUPPORTED;
>  #endif
>  
>    if (aRes) {
> +    aRes->SetClientIf(aClientIf);

remove this if you use BluetoothGattClient.

::: dom/bluetooth2/bluedroid/BluetoothGattManager.cpp
@@ +115,5 @@
>  
> +void
> +BluetoothGattManager::StartLeScan(const nsTArray<nsString>& aServiceUuids,
> +                                  BluetoothGattClientResultHandler* aRes)
> +{

You probably need to add these lines.
https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/bluedroid/BluetoothGattManager.cpp#441-444

@@ +131,5 @@
> +      uuid1.m3[4], uuid1.m3[5], uuid1.m3[6], uuid1.m3[7],
> +      uuid2.m3[0], uuid2.m3[1], uuid2.m3[2], uuid2.m3[3],
> +      uuid2.m3[4], uuid2.m3[5], uuid2.m3[6], uuid2.m3[7]
> +    }};
> +    aRes->SetAppUuid(appUuid);

You may extract |BluetoothGatt:GenerateUuid| into a utility function in |BluetoothUtils|.

@@ +139,5 @@
> +    sBluetoothGattClientInterface->RegisterClient(appUuid, nullptr);
> +
> +    // Put the result hander of LeScan into a queue, it will be used when gatt
> +    // client registered.
> +    mLeScanResultHandlers.AppendElement(aRes);

Could we use BluetoothGattClient for this?

@@ +143,5 @@
> +    mLeScanResultHandlers.AppendElement(aRes);
> +}
> +
> +void
> +BluetoothGattManager::StopLeScan(int aClientIf,

similar to StartLeScan.

@@ +607,5 @@
> +      sBluetoothGattClientInterface->Scan(aClientIf, true, mLeScanResultHandlers[i]);
> +      break;
> +    }
> +  }
> +

Please use BluetoothGattClient and revise the logic below for handling LeScan case.

@@ +677,5 @@
> +  BluetoothService* bs = BluetoothService::Get();
> +  if (bs) {
> +    bs->DistributeSignal(BluetoothSignal(NS_LITERAL_STRING("LeDeviceFound"),
> +                                     NS_LITERAL_STRING(KEY_ADAPTER),
> +                                     BluetoothValue(properties)));

bs->DistributeSignal(NS_LITERAL_STRING("LeDeviceFound"),
                     NS_LITERAL_STRING(KEY_ADAPTER),
                     BluetoothValue(properties)));

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +563,5 @@
>  private:
>    BluetoothReplyRunnable* mRunnable;
>  };
>  
> +class StartLeScanResultHandler MOZ_FINAL

Replace with final according to Bug 1145631.

@@ +583,5 @@
> +    BT_WARNING("Received error code %d", (int)aStatus);
> +    DispatchReplyError(mRunnable, BluetoothValue((int)GetClientIf()));
> +  }
> +
> +  private:

no indent here.

@@ +587,5 @@
> +  private:
> +    nsRefPtr<BluetoothReplyRunnable> mRunnable;
> +};
> +
> +class StopLeScanResultHandler MOZ_FINAL

DITTO.

@@ +607,5 @@
> +    BT_WARNING("Received error code %d", (int)aStatus);
> +    DispatchReplyError(mRunnable, aStatus);
> +  }
> +
> +  private:

DITTO.

@@ +634,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +  ENSURE_BLUETOOTH_IS_READY(aRunnable, NS_OK);
> +
> +  BluetoothGattManager* gatt = BluetoothGattManager::Get();
> +  if (!gatt) {

Use ENSURE_GATT_MGR_IS_READY_VOID(gatt, aRunnable) here.

@@ +639,5 @@
> +    nsAutoString errorStr;
> +    errorStr.AssignLiteral("Calling StartLeScan() failed");
> +    DispatchReplyError(aRunnable, errorStr);
> +  } else {
> +    gatt->StartLeScan(aServiceUuids, new StartLeScanResultHandler(aRunnable));

I would prefer we put GATT implementation details in GattMgr instead of ServiceBluedroid.

Please just pass the runnable into GattMgr and move those ResultHandlers implementation into GattMgr and use |BluetoothGattClient| in those handlers if needed.

@@ +646,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +BluetoothServiceBluedroid::StopLeScanInternal(

DITTO.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.h
@@ +59,5 @@
>  
> +  virtual nsresult StartLeScanInternal(const nsTArray<nsString>& aServiceUuids,
> +                                       BluetoothReplyRunnable* aRunnable);
> +  virtual nsresult StopLeScanInternal(uint16_t aClientIf,
> +                                      BluetoothReplyRunnable* aRunnable);

Why we don't need to add these into DBusService.h/cpp?

::: dom/bluetooth2/ipc/BluetoothServiceChildProcess.cpp
@@ +168,5 @@
> +                                              uint16_t aClientIf,
> +                                              BluetoothReplyRunnable* aRunnable)
> +{
> +  SendRequest(aRunnable, StopLeScanRequest(aClientIf));
> +  return NS_OK;

As I mentioned, I think we can just use void.

::: dom/bluetooth2/ipc/BluetoothServiceChildProcess.h
@@ +72,5 @@
>    StartDiscoveryInternal(BluetoothReplyRunnable* aRunnable) MOZ_OVERRIDE;
>  
>    virtual nsresult
> +  StopLeScanInternal(uint16_t aClientIf,
> +                     BluetoothReplyRunnable* aRunnable) MOZ_OVERRIDE;

replace MOZ_OVERRIDE by override according to Bug 1145631.

::: dom/webidl/BluetoothAdapter2.webidl
@@ +96,5 @@
>    Promise<void> unpair(DOMString deviceAddress);
>  
>    sequence<BluetoothDevice> getPairedDevices();
>  
> +  [NewObject, Throws]

Throws is not needed anymore.

@@ +99,5 @@
>  
> +  [NewObject, Throws]
> +  Promise<BluetoothDiscoveryHandle> startLeScan(sequence<DOMString> aServiceUuids);
> +
> +  [NewObject, Throws]

DITTO.

::: dom/webidl/BluetoothLeDeviceEvent.webidl
@@ +11,5 @@
> +  readonly attribute BluetoothDevice? device;
> +  readonly attribute short rssi;
> +  [Throws]
> +  readonly attribute ArrayBuffer scanRecord;
> +};

You need to add this new event into test.
For example, https://dxr.mozilla.org/mozilla-central/source/dom/events/test/test_all_synthetic_events.html#59
Attachment #8582233 - Flags: feedback?(joliu)
> ::: dom/bluetooth2/bluedroid/BluetoothDaemonHelpers.cpp
> @@ +873,5 @@
> >      CONVERT(PROPERTY_ADAPTER_DISCOVERY_TIMEOUT, 0x09),
> >      CONVERT(PROPERTY_REMOTE_FRIENDLY_NAME, 0x0a),
> >      CONVERT(PROPERTY_REMOTE_RSSI, 0x0b),
> >      CONVERT(PROPERTY_REMOTE_VERSION_INFO, 0x0c),
> > +    CONVERT(PROPERTY_REMOTE_ADV_DATA, 0x0d),
> 
> why we need to revise DaemonHelpers now?
> 

Please ignore my previous question here.
Do we also need to add in HALHelpers too?
- Revise previous patch based on #comment 5
Attachment #8582233 - Attachment is obsolete: true
(In reply to jocelyn [:jocelyn] from comment #5)
> Comment on attachment 8582233 [details] [diff] [review]
> ::: dom/bluetooth2/BluetoothDiscoveryHandle.h
> @@ +58,5 @@
> > +
> > +  /**
> > +   * Client interface ID of GATT HALL
> > +   */
> > +  int mClientIf;
> 
> Why is mClientIf needed for DiscoveryHandle?

Since the API 'stopLeScan()' is used to stop the specified onoging scan which would notify the given BluetoothDiscoveryHandle. I think we need to keep the clientIf of LE scan and use it to find the ongoing scan when we want to stop it. 

> ::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.h
> @@ +59,5 @@
> >  
> > +  virtual nsresult StartLeScanInternal(const nsTArray<nsString>& aServiceUuids,
> > +                                       BluetoothReplyRunnable* aRunnable);
> > +  virtual nsresult StopLeScanInternal(uint16_t aClientIf,
> > +                                      BluetoothReplyRunnable* aRunnable);
> 
> Why we don't need to add these into DBusService.h/cpp?

I think we should.
Thank you for pointing it out.
(In reply to jocelyn [:jocelyn] from comment #6)
> > ::: dom/bluetooth2/bluedroid/BluetoothDaemonHelpers.cpp
> > @@ +873,5 @@
> > >      CONVERT(PROPERTY_ADAPTER_DISCOVERY_TIMEOUT, 0x09),
> > >      CONVERT(PROPERTY_REMOTE_FRIENDLY_NAME, 0x0a),
> > >      CONVERT(PROPERTY_REMOTE_RSSI, 0x0b),
> > >      CONVERT(PROPERTY_REMOTE_VERSION_INFO, 0x0c),
> > > +    CONVERT(PROPERTY_REMOTE_ADV_DATA, 0x0d),
> > 
> > why we need to revise DaemonHelpers now?
> > 
> 
> Please ignore my previous question here.
> Do we also need to add in HALHelpers too?

I removed this line from DaemonHelpers (and HALHelpers) on #attachment 8585330 [details] [diff] [review] since BlueDroid don't treat 'ADV_DATA' as a property of adapter.
Attachment #8585330 - Flags: feedback?(joliu)
- Add error handling for RegisterClientNotification
Attachment #8585330 - Attachment is obsolete: true
Attachment #8585330 - Flags: feedback?(joliu)
Attachment #8585878 - Flags: feedback?(joliu)
Comment on attachment 8585878 [details] [diff] [review]
Implement LE scan related methods and LE device event (v5)

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

Hi Jamin,

Please see my comments below.
I haven't take a closer look at UpdatePropertiesFromAdvData() function, will do it in the next round.

Thanks,
Jocelyn

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +120,5 @@
> +                                       mServiceUuids);
> +
> +    const BluetoothValue& v = mReply->get_BluetoothReplySuccess().value();
> +
> +    if (v.type() == BluetoothValue::Tint32_t) {

Let's use uint32_t for this directly since it will not < 0 based on bluedroid implementation.

@@ +121,5 @@
> +
> +    const BluetoothValue& v = mReply->get_BluetoothReplySuccess().value();
> +
> +    if (v.type() == BluetoothValue::Tint32_t) {
> +      discoveryHandle->setClientIf(v.get_int32_t());

SetClientIf?

@@ +427,5 @@
>      init.mStatus = status;
>      nsRefPtr<BluetoothStatusChangedEvent> event =
>        BluetoothStatusChangedEvent::Constructor(this, aData.name(), init);
>      DispatchTrustedEvent(event);
> +  } else if (aData.name().EqualsLiteral(REQUEST_MEDIA_PLAYSTATUS_ID)) {

Why is this section added?

@@ +501,5 @@
>  
>    // Return BluetoothDiscoveryHandle in StartDiscoveryTask
>    nsRefPtr<BluetoothReplyRunnable> result =
>      new StartDiscoveryTask(this, promise);
> +

nit: remove this empty line

@@ +537,5 @@
>    nsRefPtr<BluetoothReplyRunnable> result =
>      new BluetoothVoidReplyRunnable(nullptr /* DOMRequest */,
>                                     promise,
>                                     NS_LITERAL_STRING("StopDiscovery"));
> +

nit: remove this empty line.

@@ +562,5 @@
> +                        NS_ERROR_DOM_INVALID_STATE_ERR);
> +
> +  BluetoothService* bs = BluetoothService::Get();
> +  BT_ENSURE_TRUE_REJECT(bs, NS_ERROR_NOT_AVAILABLE);
> +

nit: Remove this extra empty line.

@@ +567,5 @@
> +
> +  nsRefPtr<BluetoothReplyRunnable> result =
> +    new StartLeScanTask(this, promise, aServiceUuids);
> +
> +

nit: remote these two empty lines.

@@ +592,5 @@
> +
> +  BluetoothService* bs = BluetoothService::Get();
> +  BT_ENSURE_TRUE_REJECT(bs, NS_ERROR_NOT_AVAILABLE);
> +
> +  int clientIf = aDiscoveryHandle.getClientIf();

GetClientIf?

@@ +606,5 @@
> +    if (mLeScanHandleArray[i]->getClientIf() == clientIf) {
> +      mLeScanHandleArray.RemoveElementAt(i);
> +      break;
> +    }
> +  }

Is StopLeScan always succeeded?
If not, we will remove the handle even if it failed.

@@ +1010,5 @@
> +  nsTArray<uint8_t> advData;
> +  for (uint32_t i = 0; i < values.Length(); ++i) {
> +    nsString name = values[i].name();
> +    BluetoothValue value = values[i].value();
> +    if (name.EqualsLiteral("Rssi")) {

a MOZ_ASSERT(value.type() ==...) here?

@@ +1011,5 @@
> +  for (uint32_t i = 0; i < values.Length(); ++i) {
> +    nsString name = values[i].name();
> +    BluetoothValue value = values[i].value();
> +    if (name.EqualsLiteral("Rssi")) {
> +      rssi = value.get_int32_t();

ReadRemoteRssi task uses uint32_t for this value, could we also use uint32_t here?

@@ +1015,5 @@
> +      rssi = value.get_int32_t();
> +    } else if (name.EqualsLiteral("GattAdv")) {
> +      MOZ_ASSERT(value.type() == BluetoothValue::TArrayOfuint8_t);
> +      advData = value.get_ArrayOfuint8_t();
> +    }

print a warning in else that we received an unexpected/unhandled value.

@@ +1019,5 @@
> +    }
> +  }
> +
> +  // Create a temporary scanned BluetoothDevice to check existence
> +  nsRefPtr<BluetoothDevice> scanedDevice =

scannedDevice

@@ +1031,5 @@
> +    // Existing device, discard temporary discovered device
> +    scanedDevice = mDevices[index];
> +  }
> +
> +  // Notify application of scanned device via discovery handle

devices

::: dom/bluetooth2/BluetoothAdapter.h
@@ +187,5 @@
> +   *
> +   * Append a BluetoothDiscoveryHandle for LeScan to notify each handle when
> +   * Bluetooth adapter receives 'LeDeviceFound' Bluetooth signal.
> +   *
> +   * @param aDiscoveryHandle [in] Discovery handle to set.

append.

@@ +358,5 @@
>     */
>    nsRefPtr<BluetoothDiscoveryHandle> mDiscoveryHandleInUse;
>  
>    /**
> +   * Handle to fire 'ondevicefound' event handler for scanned device

Handles.

@@ +363,5 @@
> +   *
> +   * Each non-stopped LeScan process has a LeScan handle which is
> +   * responsible to dispatch LeDeviceEvent.
> +   */
> +  nsTArray<nsRefPtr<BluetoothDiscoveryHandle> > mLeScanHandleArray;

Doesn't it need to be added into cycle collection in BluetoothAdapter.cpp?

::: dom/bluetooth2/BluetoothDevice.cpp
@@ +327,5 @@
> +void
> +BluetoothDevice::UpdatePropertiesFromAdvData(const nsTArray<uint8_t>& aAdvData)
> +{
> +  unsigned int offset = 0;
> +  // An AD type data field have at least two bypes. One for the data type, one

typo, bytes.

::: dom/bluetooth2/BluetoothDiscoveryHandle.cpp
@@ +9,5 @@
>  
>  #include "mozilla/dom/bluetooth/BluetoothTypes.h"
>  #include "mozilla/dom/BluetoothDeviceEvent.h"
>  #include "mozilla/dom/BluetoothDiscoveryHandleBinding.h"
> +#include "mozilla/dom/bluetooth/BluetoothLeDeviceEvent.h"

nit: alphabet order.

@@ +103,5 @@
> +    }
> +    if (matches) {
> +      break;
> +    }
> +  }

Suggest to revise similar as

bool matched = false;
for (size_t index = 0; index < remoteUuids.Length(); index++) {
  if (mServiceUuids.Contains(remoteUuids[index])) {
    matched = true;
    break;
  }
}

@@ +105,5 @@
> +      break;
> +    }
> +  }
> +
> +  // Dispach dispatch 'devicefound 'event only if

Remove redundant dispatch.

@@ +108,5 @@
> +
> +  // Dispach dispatch 'devicefound 'event only if
> +  //  - the UUIDs of device matchs the given UUIDs.
> +  //  - the given UUIDs is empty.
> +  if (matches || !mServiceUuids.Length() || !remoteUuids.Length()) {

Suggest to use *.IsEmpty() here.

::: dom/bluetooth2/BluetoothDiscoveryHandle.h
@@ +37,3 @@
>    IMPL_EVENT_HANDLER(devicefound);
>  
> +  inline int getClientIf()

GetClientIf()?

@@ +40,5 @@
> +  {
> +    return mClientIf;
> +  };
> +
> +  inline void setClientIf(int aClientIf)

DITTO.

@@ +61,5 @@
> +   */
> +  int mClientIf;
> +
> +  /**
> +   * A DOMString array of service UUIDs to discovery / scan for.

Is this array only used in LeScan case?
If so, specify it's Le only and will be empty for classic discovery here.

::: dom/bluetooth2/BluetoothInterface.h
@@ +599,5 @@
>  {
>  public:
>    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(BluetoothGattClientResultHandler)
>  
> +  BluetoothGattClientResultHandler() { }

Please remove this constructor.

::: dom/bluetooth2/BluetoothLeDeviceEvent.cpp
@@ +11,5 @@
> +#include "mozilla/HoldDropJSObjects.h"
> +#include "mozilla/dom/Nullable.h"
> +#include "mozilla/dom/PrimitiveConversions.h"
> +#include "mozilla/dom/TypedArray.h"
> +#include "BluetoothDevice.h"

Use alphabet order for headers.

::: dom/bluetooth2/BluetoothLeDeviceEvent.h
@@ +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/. */
> +
> +#ifndef mozilla_dom_bluetooth_BluetoothLeDeviceEvent_h

Use lowercase to be consistent with our current code base.

@@ +7,5 @@
> +#ifndef mozilla_dom_bluetooth_BluetoothLeDeviceEvent_h
> +#define mozilla_dom_bluetooth_BluetoothLeDeviceEvent_h
> +
> +#include "BluetoothCommon.h"
> +#include "mozilla/dom/BluetoothLeDeviceEventBinding.h"

alphabet order.

@@ +28,5 @@
> +  int16_t mRssi;
> +  JS::Heap<JSObject*> mScanRecord;
> +
> +public:
> +  virtual BluetoothLeDeviceEvent* AsBluetoothLeDeviceEvent();

I saw there's an override keyword at end of this line in BluetoothPairingEvent.h generated by event codegen, could you check if that is needed here?

@@ +30,5 @@
> +
> +public:
> +  virtual BluetoothLeDeviceEvent* AsBluetoothLeDeviceEvent();
> +
> +  virtual JSObject* WrapObjectInternal(JSContext* aCx) override;

virtual JSObject* WrapObjectInternal(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;

@@ +34,5 @@
> +  virtual JSObject* WrapObjectInternal(JSContext* aCx) override;
> +
> +  static already_AddRefed<BluetoothLeDeviceEvent>
> +    Constructor(EventTarget* aOwner,
> +                const nsAString& aURL,

Should be aType?

@@ +36,5 @@
> +  static already_AddRefed<BluetoothLeDeviceEvent>
> +    Constructor(EventTarget* aOwner,
> +                const nsAString& aURL,
> +                const BluetoothLeDeviceEventInit& aEventInitDict,
> +                const nsTArray<uint8_t>& aScanRecord);

How about we pass device and rssi directly without wrap them into EventInitDict?
It's a bit odd to me that we only pull out the scanRecord.

@@ +54,5 @@
> +
> +  int16_t Rssi() const;
> +
> +  void GetScanRecord(JSContext* cx,
> +                    JS::MutableHandle<JSObject*> aScanRecord,

nit: fix intent here.

@@ +63,5 @@
> +};
> +
> +END_BLUETOOTH_NAMESPACE
> +
> +#endif // mozilla_dom_bluetooth_BluetoothLeDeviceEvent_h

DITTO.

::: dom/bluetooth2/bluedroid/BluetoothGattManager.cpp
@@ +364,5 @@
> +  StartLeScanResultHandler(BluetoothGattClient* aClient)
> +    : mClient(aClient)
> +  { }
> +
> +  void Scan()

override

@@ +367,5 @@
> +
> +  void Scan()
> +  {
> +    DispatchReplySuccess(mClient->mStartLeScanRunnable,
> +                         BluetoothValue((int)mClient->mClientIf));

Suggest to use uint32_t instead.

Don't we need to null out LeScanRunnable here?

@@ +373,5 @@
> +
> +  void OnError(BluetoothStatus aStatus) override
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +    BT_WARNING("Received error code %d", (int)aStatus);

Suggest to make it consistent with other result handlers.

@@ +375,5 @@
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +    BT_WARNING("Received error code %d", (int)aStatus);
> +    DispatchReplyError(mClient->mStartLeScanRunnable,
> +                       BluetoothValue((int)mClient->mClientIf));

DITTO. (uint32_t and null out runnable)

Besides, should we unreigster this client if startLeScan failed?

@@ +390,5 @@
> +   StopLeScanResultHandler(BluetoothReplyRunnable* aRunnable, int aClientIf)
> +     : mRunnable(aRunnable), mClientIf(aClientIf)
> +  { }
> +
> +  void Scan()

override

@@ +400,5 @@
> +    if (index != sClients->NoIndex) {
> +      sBluetoothGattClientInterface->UnregisterClient(mClientIf, nullptr);
> +
> +      nsRefPtr<BluetoothGattClient> client = sClients->ElementAt(index);
> +      sClients->RemoveElement(client);

Combine these two lines into sClients->RemoveElementAt(index);

@@ +407,5 @@
> +
> +  void OnError(BluetoothStatus aStatus) override
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +    BT_WARNING("Received error code %d", (int)aStatus);

Suggest to make it consistent with other result handlers.

@@ +432,5 @@
> +  if (index == sClients->NoIndex) {
> +    index = sClients->Length();
> +
> +    sClients->AppendElement(new BluetoothGattClient(appUuidString, EmptyString()));
> +  }

Should we continue to use existed client if index != NoIndex?
Since we GenerateUuid() every time in this function, why would the client with the same Uuid already existed?
It looks like an unexpected error that should be handled to me, what do you think?

@@ +691,5 @@
> +                         NS_LITERAL_STRING(
> +                           "StartLeScan failed due to registration failed"));
> +      client->mStartLeScanRunnable = nullptr;
> +    }
> +

Suggest to use if else-if here since these two cases are exclusive.

@@ +717,5 @@
> +    sBluetoothGattClientInterface->Scan(
> +      aClientIf, true /* start */,
> +      new StartLeScanResultHandler(client));
> +  }
> +

DITTO.

@@ +736,5 @@
> +
> +  BluetoothValue properties = InfallibleTArray<BluetoothNamedValue>();
> +
> +  nsTArray<uint8_t> advData;
> +  for (unsigned int i = 0; i < sizeof(aAdvData.mAdvData); ++i) {

Suggest to use uint32_t.
You may also pass BluetoothGattAdvData structure directly to content process to avoid this section, what do you think?

@@ +741,5 @@
> +    advData.AppendElement(aAdvData.mAdvData[i]);
> +  }
> +
> +  BT_APPEND_NAMED_VALUE(properties.get_ArrayOfBluetoothNamedValue(), "Address", nsString(aBdAddr));
> +  BT_APPEND_NAMED_VALUE(properties.get_ArrayOfBluetoothNamedValue(), "Rssi", (int32_t) aRssi);

Suggest to use uint32_t.

@@ +745,5 @@
> +  BT_APPEND_NAMED_VALUE(properties.get_ArrayOfBluetoothNamedValue(), "Rssi", (int32_t) aRssi);
> +  BT_APPEND_NAMED_VALUE(properties.get_ArrayOfBluetoothNamedValue(), "GattAdv", advData);
> +
> +  BluetoothService* bs = BluetoothService::Get();
> +  if (bs) {

I prefer to be consistent with other parts first.
We can open another bug for replacing all NS_ENSURE_*.
What do you think?

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +535,5 @@
>  BluetoothServiceBluedroid::StartDiscoveryInternal(
>    BluetoothReplyRunnable* aRunnable)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  

Add ENSURE_BLUETOOTH_IS_READY_VOID(aRunnable) for start/stopDiscovery and start/stopLeScan.

::: dom/bluetooth2/bluez/BluetoothDBusService.h
@@ +214,5 @@
>                                    const char* aInterface,
>                                    void (*aCB)(DBusMessage *, void *),
>                                    BluetoothReplyRunnable* aRunnable);
>  
> +  void SendDiscoveryMessage(const char* aMessageName,

Why is there no corresponding change in DBusService.cpp?
By the way, I think we don't need to refactor this one right now since it's bluez only.
Though I'm totally OK if you want to revise this.

::: dom/bluetooth2/ipc/BluetoothServiceChildProcess.cpp
@@ +151,2 @@
>  BluetoothServiceChildProcess::StopDiscoveryInternal(
>                                                BluetoothReplyRunnable* aRunnable)

Can you also fix the intent for here and below?

::: dom/bluetooth2/ipc/BluetoothTypes.ipdlh
@@ +16,5 @@
>   * UTF16 strings. Can also hold key-value pairs for dictionary-ish access.
>   */
>  union BluetoothValue
>  {
> +  int32_t;

Is this for clientIf and rssi?
If so, I think we can use uint32_t directly.

::: dom/webidl/BluetoothAdapter2.webidl
@@ +97,5 @@
>  
>    sequence<BluetoothDevice> getPairedDevices();
>  
> +  [NewObject]
> +  Promise<BluetoothDiscoveryHandle> startLeScan(sequence<DOMString> aServiceUuids);

We don't need the 'a' prefix for arguments of webidl method.

@@ +100,5 @@
> +  [NewObject]
> +  Promise<BluetoothDiscoveryHandle> startLeScan(sequence<DOMString> aServiceUuids);
> +
> +  [NewObject]
> +  Promise<void> stopLeScan(BluetoothDiscoveryHandle aDiscoveryHandle);

DITTO.
Attachment #8585878 - Flags: feedback?(joliu)
- revise previous patch based on #comment 11
Attachment #8585878 - Attachment is obsolete: true
- add a comment
Attachment #8590584 - Attachment is obsolete: true
Attachment #8590590 - Flags: feedback?(joliu)
(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #11)
> Comment on attachment 8585878 [details] [diff] [review]
> Implement LE scan related methods and LE device event (v5)

Hi Jocelyn,
Thank you for reviewing the patch.
Most of your comments are addressed in #Attachment 8590590 [details] [diff].

> @@ +1011,5 @@
> > +  for (uint32_t i = 0; i < values.Length(); ++i) {
> > +    nsString name = values[i].name();
> > +    BluetoothValue value = values[i].value();
> > +    if (name.EqualsLiteral("Rssi")) {
> > +      rssi = value.get_int32_t();
> 
> ReadRemoteRssi task uses uint32_t for this value, could we also use uint32_t
> here?
Since RSSI is a signed integer and could be negative, I prefer to use int32_t.

To be consistent with 'ReadRemoteRssi' task, I change the data type in 'ReadRemoteRssi' in #Attachment 8590590 [details] [diff].

> @@ +28,5 @@ > +  int16_t mRssi;
> > +  JS::Heap<JSObject*> mScanRecord;
> > +
> > +public:
> > +  virtual BluetoothLeDeviceEvent* AsBluetoothLeDeviceEvent();
> I saw there's an override keyword at end of this line in BluetoothPairingEvent.h generated by event
> codegen, could you check if that is needed here?

Thank you for pointing it out.
I revise BluetoothLeDeviceEvent.cpp/.h in #Attachment 8590590 [details] [diff].
This function has been removed.

> @@ +36,5 @@
> > +  static already_AddRefed<BluetoothLeDeviceEvent>
> > +    Constructor(EventTarget* aOwner,
> > +                const nsAString& aURL,
> > +                const BluetoothLeDeviceEventInit& aEventInitDict,
> > +                const nsTArray<uint8_t>& aScanRecord);
> 
> How about we pass device and rssi directly without wrap them into
> EventInitDict?
> It's a bit odd to me that we only pull out the scanRecord.
I removed this function and put these attributes into 'BluetoothLeDeviceEventInit', including aScanRecord.

> @@ +736,5 @@
> > +
> > +  BluetoothValue properties = InfallibleTArray<BluetoothNamedValue>();
> > +
> > +  nsTArray<uint8_t> advData;
> > +  for (unsigned int i = 0; i < sizeof(aAdvData.mAdvData); ++i) {
> > Suggest to use uint32_t. You may also pass BluetoothGattAdvData structure directly to content
> > process to avoid this section, what do you think?
The for-loop is truly annoying, I replace it by appendElements().

I prefer to use uint8_t rather than uint32_t since the size of advData is not multiples of 32

I'm not sure if it's appropriate to convert BluetoothGattAdvData into BluetoothValue. It think nsTArray<uint8_t> might be easier to extract out from properties array.


@@ +741,5 @@ > +    advData.AppendElement(aAdvData.mAdvData[i]);
> +  }
> +
> +  BT_APPEND_NAMED_VALUE(properties.get_ArrayOfBluetoothNamedValue(), "Address", nsString(aBdAddr));
> +  BT_APPEND_NAMED_VALUE(properties.get_ArrayOfBluetoothNamedValue(), "Rssi", (int32_t) aRssi);
> Suggest to use uint32_t.

I prefer to use signed int32_t since RSSI might be negative.
- Rebase (the codebase includes patches in Bug 1146355)
- Modify BluetoothLeDevice.webidl to make BluetoothDevice non-nullable.
Attachment #8590590 - Attachment is obsolete: true
Attachment #8590590 - Flags: feedback?(joliu)
Attachment #8591495 - Flags: review?(joliu)
Comment on attachment 8591495 [details] [diff] [review]
Implement LE scan related methods and LE device event (v7)

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

Hi Jamin,

My main concerns here are 
1) We're using mDevices directly though details remain unclear to me in this patch.
For example, what would happen if we call both startDiscovery and startLeScan?
I'm not sure it's a good idea to use the same array for LeScan.
We might need to consider more details to avoid break classic APIs.
2) 
Another thing is it doesn't seems that we could support multiple LeScan correctly in this patch, we are trying to do this in this patch, right?

If you want to, we could also narrow down the scope here and pull out some details to some follow-up bugs.

A friendly reminder, you might need to make sure it can be built on both v1 and v2 since we have a shared backend now.

Please see my comments below.

Thanks,
Jocelyn

::: dom/bindings/Bindings.conf
@@ +154,5 @@
>  'BluetoothDiscoveryHandle': {
>      'nativeType': 'mozilla::dom::bluetooth::BluetoothDiscoveryHandle',
>  },
>  
> +'BluetoothLeDeviceEvent': {

alphabet order on the interface name.

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +96,5 @@
>    nsString mAppUuid;
>    nsString mDeviceAddr;
>    int mClientIf;
>    int mConnId;
> +  nsRefPtr<BluetoothReplyRunnable> mStartLeScanRunnable;

What would happen if app call StartLeScan multiple times, will the current one be replaced?

@@ +412,5 @@
> +    : mClient(aClient)
> +  { }
> +
> +  void Scan() override
> +  {

MOZ_ASSERT(mClient > 0);

@@ +428,5 @@
> +    // Unreigster client if startLeScan failed
> +    size_t index = sClients->IndexOf(mClient->mClientIf, 0 /* Start */,
> +                                     ClientIfComparator());
> +    if (index != sClients->NoIndex) {
> +      sBluetoothGattClientInterface->UnregisterClient(mClient->mClientIf, nullptr);

Is it possible that this calling this API returns error? (BT_STATUS!=SUCCESS)

@@ +475,5 @@
> +  int mClientIf;
> +};
> +
> +void
> +BluetoothGattManager::StartLeScan(const nsTArray<nsString>& aServiceUuids,

This function doesn't seems to be able to handle multiple le scan requests from an adapter?
For example, if we started a lescan request, then we start another one, it will be using another clientIf.
Then StopLeScan won't stop all lescans for an application but just stop BluetoothAdapter::mDiscoveryHandle[0].

@@ +479,5 @@
> +BluetoothGattManager::StartLeScan(const nsTArray<nsString>& aServiceUuids,
> +                                  BluetoothReplyRunnable* aRunnable)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aRunnable);

nit: insert a newline after this.

::: dom/bluetooth/bluedroid/BluetoothGattManager.h
@@ +30,5 @@
>  
> +  void StartLeScan(const nsTArray<nsString>& aServiceUuids,
> +                   BluetoothReplyRunnable* aRunnable);
> +
> +  void StopLeScan(int aClientIf, BluetoothReplyRunnable* aRunnable);

Should it be uint32_t since it's uint32_t for StopLeScanInternal?

@@ +32,5 @@
> +                   BluetoothReplyRunnable* aRunnable);
> +
> +  void StopLeScan(int aClientIf, BluetoothReplyRunnable* aRunnable);
> +
> +

remove the redundant space.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1137,5 @@
>    BluetoothReplyRunnable* aRunnable)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  ENSURE_BLUETOOTH_IS_READY_VOID(aRunnable);

I think it will break v1 build since this is v2 only.

@@ +1234,5 @@
>    BluetoothReplyRunnable* aRunnable)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  ENSURE_BLUETOOTH_IS_READY_VOID(aRunnable);

DITTO.

@@ +1270,5 @@
> +
> +  BluetoothGattManager* gatt = BluetoothGattManager::Get();
> +  ENSURE_GATT_MGR_IS_READY_VOID(gatt, aRunnable);
> +
> +  gatt->StopLeScan(aClientIf, aRunnable);

Please group Start/StopLeScan with other GATT functions under BT_API2_V2 flag.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.h
@@ +60,5 @@
> +  virtual void StopDiscoveryInternal(BluetoothReplyRunnable* aRunnable);
> +
> +  virtual void StartLeScanInternal(const nsTArray<nsString>& aServiceUuids,
> +                                   BluetoothReplyRunnable* aRunnable);
> +  virtual void StopLeScanInternal(uint16_t aClientIf,

Suggest to group start/stop le scan with other Gatt client functions

::: dom/bluetooth/bluetooth2/BluetoothAdapter.cpp
@@ +121,5 @@
> +
> +    const BluetoothValue& v = mReply->get_BluetoothReplySuccess().value();
> +
> +    if (v.type() == BluetoothValue::Tuint32_t) {
> +      discoveryHandle->SetGattClientIf(v.get_uint32_t());

How about we pass clientIf to BluetoothDiscoveryHandle::Create and BluetoothDiscoveryHandle::BluetoothDiscoveryHandle directly?
Then we won't need this setter.

@@ +159,5 @@
> +      , mAdapter(aAdapter)
> +      , mClientIf(aClientIf)
> +  {
> +    MOZ_ASSERT(aPromise);
> +    MOZ_ASSERT(aAdapter);

Add MOZ_ASSERT(aClientIf > 0);

@@ +594,5 @@
> +
> +  nsRefPtr<Promise> promise = Promise::Create(global, aRv);
> +  NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
> +
> +

redundant newline.

@@ +1043,5 @@
> +      MOZ_ASSERT(value.type() == BluetoothValue::TArrayOfuint8_t);
> +      advData = value.get_ArrayOfuint8_t();
> +    } else {
> +      BT_WARNING("Receive an unexpected value name '%s'",
> +                                             NS_ConvertUTF16toUTF8(name).get());

nit: personally I would prefer to align NS_* with the start of first param.

@@ +1057,5 @@
> +    // New device, append it to adapter's device array
> +    mDevices.AppendElement(scannedDevice);
> +  } else {
> +    // Existing device, discard temporary discovered device
> +    scannedDevice = mDevices[index];

If it's a existing device, we won't update the device from GattAdv.
Is this the expected behavior?

::: dom/bluetooth/bluetooth2/BluetoothAdapter.h
@@ +190,5 @@
> +   * Bluetooth adapter receives 'LeDeviceFound' Bluetooth signal.
> +   *
> +   * @param aDiscoveryHandle [in] Discovery handle to append.
> +   */
> +  void AppendLeScanHandleArray(BluetoothDiscoveryHandle* aDiscoveryHandle);

nit: How about we use AppendLeScanHandle/RemoveLeScanHandle directly?

@@ +198,5 @@
> +   *
> +   * Remove the BluetoothDiscoveryHandle with the given client
> +   * interface id from LeScan handle array.
> +   *
> +   * @param aClientIf [in] A clientIf which used to start a LeScan.

nit: The

@@ +200,5 @@
> +   * interface id from LeScan handle array.
> +   *
> +   * @param aClientIf [in] A clientIf which used to start a LeScan.
> +   */
> +  void RemoveLeScanHandleFromArray(uint32_t aClientIf);

I think it should be RemoveLeScanHandles since we're actually remove all scan handles for this clientIf.

::: dom/bluetooth/bluetooth2/BluetoothCommon.h
@@ +626,5 @@
> + * EIR Data Type, Advertising Data Type (AD Type) and OOB Data Type Definitions
> + * Please refer to https://www.bluetooth.org/en-us/specification/\
> + * assigned-numbers/generic-access-profile
> + */
> +enum BluetoothGapDataType {

We define the entire set of AD types here but only uses a small subset.
Suggest to define the necessary ones here, we could easily expand this when we need other values.

::: dom/bluetooth/bluetooth2/BluetoothDevice.cpp
@@ +327,5 @@
> +void
> +BluetoothDevice::UpdatePropertiesFromAdvData(const nsTArray<uint8_t>& aAdvData)
> +{
> +  unsigned int offset = 0;
> +  // An AD type data field have at least two bytes. One for the data type, one

This comment is confusing and might be incorrect.
Besides, it seems we cover whole AD structure here other than data field only.
Please revise it for explaining this function properly.

::: dom/bluetooth/bluetooth2/BluetoothDiscoveryHandle.cpp
@@ +38,5 @@
>  }
>  
>  BluetoothDiscoveryHandle::~BluetoothDiscoveryHandle()
>  {
> +  DisconnectFromOwner();

Good catch!

@@ +90,5 @@
> +  nsTArray<nsString> remoteUuids;
> +  aLeDevice->GetUuids(remoteUuids);
> +
> +  // The web API startLeScan() makes the device's adapter start seeking for
> +  // remote LE devices advertising given services.

nit: given service UUIDs.

@@ +104,5 @@
> +
> +  // Dispach 'devicefound 'event only if
> +  //  - the UUIDs of device matchs the given UUIDs.
> +  //  - the given UUIDs is empty.
> +  if (matched || mServiceUuids.IsEmpty() || remoteUuids.IsEmpty()) {

Suggest not to treat remoteUuids.IsEmpty() as a valid result of this filter since we couldn't know if it really matches or not.

@@ +109,5 @@
> +    BluetoothLeDeviceEventInit init;
> +    init.mDevice = aLeDevice;
> +    init.mRssi = aRssi;
> +
> +    // Assign an ArrayBuffer to init.mScanRecord.

Please see my comments in LeDeviceEvent.

::: dom/bluetooth/bluetooth2/BluetoothDiscoveryHandle.h
@@ +66,5 @@
> +   */
> +  uint32_t mGattClientIf;
> +
> +  /**
> +   * A DOMString array of service UUIDs to discovery / scan for.

nit: discover

::: dom/bluetooth/bluetooth2/BluetoothLeDeviceEvent.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 "mozilla/dom/bluetooth/BluetoothLeDeviceEvent.h"

alphabet order.

@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/dom/bluetooth/BluetoothLeDeviceEvent.h"
> +
> +#include "BluetoothDevice.h"

Since we export BluetoothDevice.h, please import it from mozilla/dom/bluetooth/BluetoothDevice.h

@@ +65,5 @@
> +  nsRefPtr<BluetoothLeDeviceEvent> e = new BluetoothLeDeviceEvent(aOwner);
> +  bool trusted = e->Init(aOwner);
> +  e->InitEvent(aType, aEventInitDict.mBubbles, aEventInitDict.mCancelable);
> +  e->mDevice = aEventInitDict.mDevice;
> +  e->mRssi = aEventInitDict.mRssi;

Suggest to pass device, rssi, scanRecord directly to this Constructor in order to make code be simpler in DispatchLeDeviceEvent and here.
Similar as https://dxr.mozilla.org/mozilla-central/source/dom/media/eme/MediaKeyMessageEvent.cpp?from=MediaKeyMessageEvent.cpp&case=true#65-75.

@@ +114,5 @@
> +    return nullptr;
> +  }
> +
> +  e->SetTrusted(trusted);
> +  mozilla::HoldJSObjects(e.get());

Why not do mozilla::HoldJSObejects(this) in BluetoothLeDeviceEvent::BluetoothLeDeviceEvent for any |Constructor| function is called?

::: dom/bluetooth/bluetooth2/BluetoothLeDeviceEvent.h
@@ +6,5 @@
> +
> +#ifndef mozilla_dom_bluetooth_bluetoothledeviceevent_h
> +#define mozilla_dom_bluetooth_bluetoothledeviceevent_h
> +
> +#include "BluetoothCommon.h"

mozilla/dom/bluetooth/BluetoothCommon.h

::: dom/bluetooth/bluetooth2/BluetoothUtils.h
@@ +54,5 @@
> +/**
> +* Generate a random uuid.
> +*
> +* @param aUuidString [out] String to store the generated uuid.
> +*/

nit: put a space before '*' for these lines.

::: dom/bluetooth/bluez/BluetoothDBusService.cpp
@@ +2865,5 @@
> +
> +void
> +BluetoothDBusService::StopLeScanInternal(uint16_t aClientIf,
> +                                         BluetoothReplyRunnable* aRunnable);
> +{

Please group start/stop lescan with other GATT functions in the last section under BT_API_V2 flag.

::: dom/bluetooth/bluez/BluetoothDBusService.h
@@ +89,5 @@
> +  virtual void StartLeScanInternal(const nsTArray<nsString>& aServiceUuids,
> +                                   BluetoothReplyRunnable* aRunnable) override;
> +
> +  virtual void StopLeScanInternal(uint16_t aClientIf,
> +                                  BluetoothReplyRunnable* aRunnable) override;

Please group start/stop lescan with other GATT functions under BT_API_V2 flag.
Attachment #8591495 - Flags: review?(joliu)
- Revise previous patch based on comment 16

Thank Jocelyn for reviewing the patch.
Attachment #8591495 - Attachment is obsolete: true
Attachment #8593320 - Flags: review?(joliu)
(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #16)

Thank you for your time and reviewing effort.
I've uploaded a new patch based on your comments.

> 1) We're using mDevices directly though details remain unclear to me in this
> patch.
> For example, what would happen if we call both startDiscovery and
> startLeScan?
> I'm not sure it's a good idea to use the same array for LeScan.
Agree with you.
Besides, the memory usage of mDevices might also be a problem since the API allows user to scan LE devices for unlimited time. Therefore, I think we should create individual instances for each LeDeviceEvent and release the memory of Device when it's not used by Gaia.

Thank you for point it out.

> 2) 
> Another thing is it doesn't seems that we could support multiple LeScan
> correctly in this patch, we are trying to do this in this patch, right?
Yes. I fixed defect of previous patch based on your comments and tested multiple scan by marionette framework.

> What would happen if app call StartLeScan multiple times, will the current one be replaced?
No. Each 'StartLeScan' would register a clientIf and return multiple discoveryHandles.

> This function doesn't seems to be able to handle multiple le scan requests
> from an adapter?
> For example, if we started a lescan request, then we start another one, it
> will be using another clientIf.
> Then StopLeScan won't stop all lescans for an application but just stop
> BluetoothAdapter::mDiscoveryHandle[0].
Each StartLeScan would register an individual clientIf for scanning task, and unregister it when user call StopLeScan.
StopLeScan takes 'BluetoothDiscoveryHandle' as argument and extracts the clientIf from discoveryHandle to determine which scanning task should be stopped.

> @@ +428,5 @@ > +    // Unreigster client if startLeScan failed
> > +    if (index != sClients->NoIndex) {
> > +      sBluetoothGattClientInterface->UnregisterClient(mClient->mClientIf, nullptr);
> Is it possible that this calling this API returns error? (BT_STATUS!=SUCCESS)
It seems unlikely, but I would say it's possible.
I've revised the previous patch slightly.
When 'UnregisterClient' fails, it would call BT_WARMING() to indicate the fact. However, It can't re-trigger UnregisterClient automatically

> @@ +1057,5 @@
> > +    // New device, append it to adapter's device array
> > +    mDevices.AppendElement(scannedDevice);
> > +  } else {
> > +    // Existing device, discard temporary discovered device
> > +    scannedDevice = mDevices[index];
> 
> If it's a existing device, we won't update the device from GattAdv.
> Is this the expected behavior?
It's a great question.
I'm not sure should we update properties from GattAdv data or not. 

This section has been redesigned in #attachment 8593320 [details] [diff] [review].
The scan results wouldn't be cached in mDevices any more.
Maybe we can discuss this topic based on the changes in #attachment 8593320 [details] [diff] [review].

Regards
Comment on attachment 8593320 [details] [diff] [review]
Implement LE scan related methods and LE device event (v8)

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

Hi Jamin,

Please see my comments below.

Thanks,
Jocelyn

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +52,5 @@
>    { }
>  
>    ~BluetoothGattClient()
>    {
> +    mStartLeScanRunnable = nullptr;

I think these null out operations are actually not necessary.
Could you help to remove them?

@@ +413,5 @@
> +  { }
> +
> +  void Scan() override
> +  {
> +    MOZ_ASSERT(mClient > 0);

nit: insert a newline after this.

@@ +428,5 @@
> +
> +    // Unreigster client if startLeScan failed
> +    size_t index = sClients->IndexOf(mClient->mClientIf, 0 /* Start */,
> +                                     ClientIfComparator());
> +    if (index != sClients->NoIndex && mClient->mClientIf > 0) {

not need to search the client again since we already have mClient.

|if (mClient->mClientIf > 0) {| is enough.

@@ +429,5 @@
> +    // Unreigster client if startLeScan failed
> +    size_t index = sClients->IndexOf(mClient->mClientIf, 0 /* Start */,
> +                                     ClientIfComparator());
> +    if (index != sClients->NoIndex && mClient->mClientIf > 0) {
> +      sBluetoothGattClientInterface->UnregisterClient(mClient->mClientIf, nullptr);

nit: over 80 chars

@@ +456,5 @@
> +
> +    size_t index = sClients->IndexOf(mClientIf, 0 /* Start */,
> +                                     ClientIfComparator());
> +    if (index != sClients->NoIndex) {
> +      sBluetoothGattClientInterface->UnregisterClient(mClientIf, nullptr);

This client won't be removed since you call this directly and didn't pass any result handler.
Same as in |StartLeScanResultHandler::OnError|.

@@ +498,5 @@
> +  sClients->AppendElement(new BluetoothGattClient(appUuidStr, EmptyString()));
> +  nsRefPtr<BluetoothGattClient> client = sClients->ElementAt(index);
> +  client->mStartLeScanRunnable = aRunnable;
> +
> +  if (client->mClientIf > 0) {

why will this happen?

@@ +985,5 @@
>    const BluetoothGattAdvData& aAdvData)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  BluetoothValue properties = InfallibleTArray<BluetoothNamedValue>();

InfallibleTArray<BluetoothNamedValue> properties;

@@ +990,5 @@
> +
> +  nsTArray<uint8_t> advData;
> +  advData.AppendElements(aAdvData.mAdvData, sizeof(aAdvData.mAdvData));
> +
> +  BT_APPEND_NAMED_VALUE(properties.get_ArrayOfBluetoothNamedValue(), "Address", nsString(aBdAddr));

BT_APPEND_NAMED_VALUE(properties, "Address", nsString(aBdAddr));

::: dom/bluetooth/bluetooth2/BluetoothAdapter.cpp
@@ +315,5 @@
>    // We can be null on shutdown, where this might happen
>    NS_ENSURE_TRUE_VOID(bs);
>    bs->UnregisterBluetoothSignalHandler(NS_LITERAL_STRING(KEY_ADAPTER), this);
> +
> +  mLeScanHandleArray.Clear();

not necessary here.

BTW, should we clear it when bluetooth disabled?

@@ +494,5 @@
> +
> +void
> +BluetoothAdapter::RemoveLeScanHandles(uint32_t aClientIf)
> +{
> +  if (mLeScanHandleArray.Length() <= 0) {

This check is not necessary?
It won't enter the loop under this condition, right?

::: dom/bluetooth/bluetooth2/BluetoothAdapter.h
@@ +185,5 @@
>  
> +  /**
> +   * Append a BluetoothDiscoveryHandle to LeScan handle array.
> +   *
> +   * Append a BluetoothDiscoveryHandle for LeScan to notify each handle when

nit: Suggest to remove these two lines since it's covered by the comment of mLeScanHandleArray.

@@ +193,5 @@
> +   */
> +  void AppendLeScanHandle(BluetoothDiscoveryHandle* aDiscoveryHandle);
> +
> +  /**
> +   * Remove a BluetoothDiscoveryHandle from LeScan handle array.

nit: remove this line, it's the same as the description below but less clear.

@@ +195,5 @@
> +
> +  /**
> +   * Remove a BluetoothDiscoveryHandle from LeScan handle array.
> +   *
> +   * Remove the BluetoothDiscoveryHandle with the given client

nit: Remove BluetoothDiscoverHandles of given client interface from LeScan handle array.

::: dom/bluetooth/bluetooth2/BluetoothDevice.cpp
@@ +330,5 @@
> +  unsigned int offset = 0;
> +  // A significant AD structure has at least two bytes. One for the length field
> +  // structure, one for AD type field. According to BT Core Spec. Vol 3 - Ch 11,
> +  // If the Length field is set to zero, then the Data field has zero bytes. It
> +  // only occurs to allow an early termination of the Advertising data.

Per offline discussion, the comment here state that we will handle AD structures that has at least two bytes.
But according to the while loop condition, offset = aAdvData.Length() - 2 is not valid, but it's a valid AD structure which has exactly two bytes.

Please either refine the comment to suit your implementation or revise the implementation.

::: dom/bluetooth/bluetooth2/BluetoothDevice.h
@@ +147,5 @@
>  
> +  /**
> +   * Parse advertising data to update device properties.
> +   *
> +   * Parse 'Advertising Data Type' from inquiry response and set name, uuids and

nit: from 'an' inquiry...
UUIDs

@@ +148,5 @@
> +  /**
> +   * Parse advertising data to update device properties.
> +   *
> +   * Parse 'Advertising Data Type' from inquiry response and set name, uuids and
> +   * COD if they exists in ADV data.

nit: exist

@@ +150,5 @@
> +   *
> +   * Parse 'Advertising Data Type' from inquiry response and set name, uuids and
> +   * COD if they exists in ADV data.
> +   *
> +   * @param aAdvData [in] advertising data which provided by LeScan result.

nit: by the LeScan result

::: dom/bluetooth/bluetooth2/BluetoothDiscoveryHandle.cpp
@@ +106,5 @@
> +    }
> +  }
> +
> +  // Dispach 'devicefound 'event only if
> +  //  - the service UUIDs in scan record matchs the given UUIDs.

nit: in the scan...

::: dom/bluetooth/bluetooth2/BluetoothDiscoveryHandle.h
@@ +42,5 @@
> +  {
> +    return mGattClientIf;
> +  };
> +
> +  inline void SetGattClientIf(uint32_t aClientIf)

This setter is unnecessary?

::: dom/bluetooth/bluetooth2/BluetoothLeDeviceEvent.cpp
@@ +60,5 @@
> +already_AddRefed<BluetoothLeDeviceEvent>
> +BluetoothLeDeviceEvent::Constructor(
> +  mozilla::dom::EventTarget* aOwner,
> +  const nsAString& aType,
> +  const nsRefPtr<BluetoothDevice> aDevice,

why we need to use nsRefPtr for the parameter?

::: dom/bluetooth/bluetooth2/BluetoothLeDeviceEvent.h
@@ +18,5 @@
> +class BluetoothLeDeviceEvent : public Event
> +{
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(BluetoothLeDeviceEvent, Event)

nit: over 80 chars

@@ +28,5 @@
> +  int16_t mRssi;
> +  JS::Heap<JSObject*> mScanRecord;
> +
> +public:
> +  virtual JSObject* WrapObjectInternal(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;

nit: over 80 chars

@@ +33,5 @@
> +
> +  static already_AddRefed<BluetoothLeDeviceEvent>
> +    Constructor(EventTarget* aOwner,
> +                const nsAString& aType,
> +                const nsRefPtr<BluetoothDevice> aDevice,

Use raw pointer here?

::: dom/webidl/BluetoothLeDeviceEvent.webidl
@@ +17,5 @@
> +dictionary BluetoothLeDeviceEventInit : EventInit
> +{
> +  required BluetoothDevice device;
> +  short  rssi = 0;
> +  ArrayBuffer scanRecord;

required?
Attachment #8593320 - Flags: review?(joliu)
- Revise previous patch based on comment 19.

Thank Jocelyn for reviewing the patch.
Attachment #8593320 - Attachment is obsolete: true
Attachment #8593897 - Flags: review?(joliu)
Comment on attachment 8593897 [details] [diff] [review]
Implement LE scan related methods and LE device event (v9)

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

Hi Jamin,

Please see my comments below.

Thanks,
Jocelyn

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ -60,5 @@
> -    mReadRemoteRssiRunnable = nullptr;
> -    mRegisterNotificationsRunnable = nullptr;
> -    mDeregisterNotificationsRunnable = nullptr;
> -  }
> -

Thanks for the cleanup.
Could you also help to add |sClients = nullptr;| in BluetoothGattManager::HandleShutdown()?

@@ +414,5 @@
> +    BT_WARNING("BluetoothGattClientInterface::StartLeScan failed: %d",
> +               (int)aStatus);
> +    MOZ_ASSERT(mClient->mStartLeScanRunnable);
> +
> +    // Unreigster client if startLeScan failed

nit: Unregister

@@ +445,5 @@
> +  void Scan() override
> +  {
> +    DispatchReplySuccess(mRunnable);
> +
> +    // Unreigster client when stopLeScan succeed

Unregister, succeeded

@@ +981,5 @@
> +  nsTArray<uint8_t> advData;
> +  advData.AppendElements(aAdvData.mAdvData, sizeof(aAdvData.mAdvData));
> +
> +  BT_APPEND_NAMED_VALUE(properties, "Address", nsString(aBdAddr));
> +  BT_APPEND_NAMED_VALUE(properties, "Rssi", (int32_t) aRssi);

static_cast<int32_t>(aRssi)

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1171,3 @@
>    sChangeDiscoveryRunnableArray.AppendElement(aRunnable);
>  #else
>    // Missing in bluetooth1

We also need to do the BLUETOOTH_IS_READY check for v1, right?
Maybe add this macro for v1? (after ENSURE_BLUETOOTH_IS_READY)
https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp#571

@@ +1267,3 @@
>    sChangeDiscoveryRunnableArray.AppendElement(aRunnable);
>  #else
>    // Missing in bluetooth1

DITTO.

::: dom/bluetooth/bluetooth2/BluetoothAdapter.cpp
@@ +355,5 @@
>  
>      // Clear saved devices when state changes to disabled
>      if (mState == BluetoothAdapterState::Disabled) {
>        mDevices.Clear();
> +      mLeScanHandleArray.Clear();

Please revise the comment to fit the implementation.

::: dom/bluetooth/bluetooth2/BluetoothAdapter.h
@@ +185,5 @@
>  
> +  /**
> +   * Append a BluetoothDiscoveryHandle to LeScan handle array.
> +   *
> +   * @param aDiscoveryHandle [in] Discovery handle to append.

nit: The discovery handle to be appended.

::: dom/bluetooth/bluetooth2/BluetoothDevice.cpp
@@ +330,5 @@
> +  unsigned int offset = 0;
> +  // A significant AD structure has at least two bytes. One for the length field
> +  // structure, one for AD type field. According to BT Core Spec. Vol 3 - Ch 11,
> +  // If the Length field is set to zero, then the Data field has zero bytes. It
> +  // only occurs to allow an early termination of the Advertising data.

Sorry, personally, I will still suggest to use |while (offset <= aAdvData.Length())| directly.
'A significant AD structure has at least two bytes.' doesn't seem to be matched with the spec.
What spec said was there is a significant part consists of AD structures, and each AD structure have a Length field of one octet, which contains the Length value, and a Data field of Length octets.
This two bytes magic is a bit of confusing and hard to read.

If we want to make sure we won't access illegal address when accessing the array, we could check it also after len == 0 check.

@@ +335,5 @@
> +  while (offset <= (aAdvData.Length() - 2)) {
> +    int len = aAdvData[offset++];
> +    if (len == 0) {
> +      break;
> +    }

Suggest to add a dataLength variable (= len - 1) and use it below.
It will be clearer that we're parsing the data field with the dataLength.

@@ +343,5 @@
> +    switch (type) {
> +      case GAP_INCOMPLETE_UUID16:
> +      case GAP_COMPLETE_UUID16:
> +        while (len > 1) {
> +            // Read 2 byptes from data buffer and compose to 16-bits UUID.

bytes from the data buffer and compose a 16-bits UUID.

@@ +379,5 @@
> +          mUuids.AppendElement(uuidNsString);
> +        }
> +        break;
> +      case GAP_INCOMPLETE_UUID128:
> +      case GAP_COMPLETE_UUID128:

There are some similar codes when parsing UUID16, UUID32, UUID128, is it possible to use one function to parse them all?

@@ +412,5 @@
> +
> +        mName.AssignASCII(deviceName, len - 1);
> +        break;
> +      }
> +      case GAP_COD: {

According to Core Specification Supplement, Part A, section 1.6.1, "The Secure Simple Pairing Out of Band data types shall not be used in EIR or AD packets over the BR/EDR or LE transports and shall be only used over an out-of-band mechanism."

And it seems COD is a type of it, and no need to be parsed here, have you seen LE devices advertised their COD?

::: dom/bluetooth/bluetooth2/BluetoothDiscoveryHandle.cpp
@@ +40,5 @@
>  }
>  
>  BluetoothDiscoveryHandle::~BluetoothDiscoveryHandle()
>  {
> +  DisconnectFromOwner();

I think we also need to unregister the clientIf when discovery handle is going to be destructed.
Note that this client might also be unregistered elsewhere, and since we don't update the clientIf of this class, there might be a chance to unregister the clientIf again.
We might unregister other app's clientIf if we unregister multiple times.
Please make sure we won't encounter this problem here.

@@ +91,5 @@
> +{
> +  MOZ_ASSERT(aLeDevice);
> +
> +  nsTArray<nsString> remoteUuids;
> +  aLeDevice->GetUuids(remoteUuids);

Suggest to early return if remoteUuids.IsEmpty is true to make this logic explicit.

::: dom/bluetooth/bluetooth2/BluetoothGatt.cpp
@@ +161,2 @@
>  
> +    aValue.setInt32(static_cast<int32_t>(v.get_int32_t()));

You will need to revise corresponding value in GattManager, too.
https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothGattManager.cpp#1885

::: dom/bluetooth/bluetooth2/BluetoothLeDeviceEvent.cpp
@@ +60,5 @@
> +already_AddRefed<BluetoothLeDeviceEvent>
> +BluetoothLeDeviceEvent::Constructor(
> +  mozilla::dom::EventTarget* aOwner,
> +  const nsAString& aType,
> +  BluetoothDevice* const aDevice,

Do we need const?

@@ +92,5 @@
> +  e->mDevice = aEventInitDict.mDevice;
> +  e->mRssi = aEventInitDict.mRssi;
> +
> +  const uint8_t* data = aEventInitDict.mScanRecord.Data();
> +  size_t length = aEventInitDict.mScanRecord.Length();

Don't we need to call ComputeLengthAndData before these two lines?

::: dom/bluetooth/bluetooth2/BluetoothLeDeviceEvent.h
@@ +60,5 @@
> +};
> +
> +END_BLUETOOTH_NAMESPACE
> +
> +

nit: remove this redundant line.

::: dom/bluetooth/bluetooth2/BluetoothService.h
@@ +196,5 @@
> +  StopLeScanInternal(uint16_t aClientIf,
> +                     BluetoothReplyRunnable* aRunnable) = 0;
> +
> +  /**
> +   * Starts a Bluetooth LE devices scan.

nit: device
Attachment #8593897 - Flags: review?(joliu)
Does this run on all devices that run against kk base image? E.g. Flame? Do I need any updated drivers?
Flags: needinfo?(jaliu)
Hi Jan,
Yes, I believe so.
However, I only tested this patch on flame-kk with base image v18D [1].

LE scan is based on Bluetooth API v2 [2]. 
You can find the dev patch in Bug 1053673 which can activate Bluetooth API v2 with LE support.

[1] https://intranet.mozilla.org/QA/B2G_Tips_and_Tricks#lastest_OEM_KitKat_Base_Image_:_v18D
[2] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2
Flags: needinfo?(jaliu)
- revise previous patch based on comment 21
- rebase

Thank Jocelyn for reviewing the previous patch.
Attachment #8593897 - Attachment is obsolete: true
Attachment #8597005 - Flags: review?(joliu)
(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #21)
> Comment on attachment 8593897 [details] [diff] [review]
> Implement LE scan related methods and LE device event (v9)

> ::: dom/bluetooth/bluetooth2/BluetoothLeDeviceEvent.cpp
> @@ +60,5 @@
> > +already_AddRefed<BluetoothLeDeviceEvent>
> > +BluetoothLeDeviceEvent::Constructor(
> > +  mozilla::dom::EventTarget* aOwner,
> > +  const nsAString& aType,
> > +  BluetoothDevice* const aDevice,
> 
> Do we need const?

It seems unnecessary to put a 'const' in front of  BluetoothDevice*.

It would cause a compile error at "e->mDevice = aDevice;" since the instance of BluetoothDevice might be modified by accessing the address in e->mDevice.

Therefore, I only put a 'const' in front of aDevice to prevent the modification of the pointer itself.
(In reply to Jamin Liu [:jaliu] from comment #25)
> (In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #21)
> > Comment on attachment 8593897 [details] [diff] [review]
> > Implement LE scan related methods and LE device event (v9)
> 
> > ::: dom/bluetooth/bluetooth2/BluetoothLeDeviceEvent.cpp
> > @@ +60,5 @@
> > > +already_AddRefed<BluetoothLeDeviceEvent>
> > > +BluetoothLeDeviceEvent::Constructor(
> > > +  mozilla::dom::EventTarget* aOwner,
> > > +  const nsAString& aType,
> > > +  BluetoothDevice* const aDevice,
> > 
> > Do we need const?
> 
> It seems unnecessary to put a 'const' in front of  BluetoothDevice*.
> 
> It would cause a compile error at "e->mDevice = aDevice;" since the instance
> of BluetoothDevice might be modified by accessing the address in e->mDevice.
> 
> Therefore, I only put a 'const' in front of aDevice to prevent the
> modification of the pointer itself.

Hi Jamin,

It's not wrong to put this const and it's also right to set this limitation here in this case.
But I can hardly see this in our code base, also, there's EventTarget* aOwner in this function without const which makes this semantic more inconsistent here.
I would suggest remove this 'const', what do you think?

Besides, could you provide an interdiff between v9 and v10?
Bugzilla's interdiff is not working in this case.
An interdiff is appreciated here to make sure I won't miss anything.

Thanks,
jocelyn
Flags: needinfo?(jaliu)
(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #26)
> Hi Jamin,
> 
> It's not wrong to put this const and it's also right to set this limitation
> here in this case.
> But I can hardly see this in our code base, also, there's EventTarget*
> aOwner in this function without const which makes this semantic more
> inconsistent here.
> I would suggest remove this 'const', what do you think?
Sure.
I'll remove the 'const' to keep the code consistent

> Besides, could you provide an interdiff between v9 and v10?
> Bugzilla's interdiff is not working in this case.
> An interdiff is appreciated here to make sure I won't miss anything.

Please refer to  attachment 8597784 [details] [diff] [review].
It's not the real diff file between v9 and v10 since that would be hard to read.
I rebased the code of v9 and use it to generate the diff file with v10.
Flags: needinfo?(jaliu)
Comment on attachment 8597005 [details] [diff] [review]
Implement LE scan related methods and LE device event (v10)

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

Hi Jamin,

We're almost there, thanks for the hard work. ;)
Please see my comments below.

Thanks,
Jocelyn

::: dom/bluetooth/BluetoothCommon.h
@@ +750,5 @@
> +  GAP_INCOMPLETE_UUID128 = 0X06, // Incomplete List of 128-bit Service Class UUIDs
> +  GAP_COMPLETE_UUID128   = 0X07, // Complete List of 128-bit Service Class UUIDs
> +  GAP_SHORTENED_NAME     = 0X08, // Shortened Local Name
> +  GAP_COMPLETE_NAME      = 0X09, // Complete Local Name
> +  GAP_COD                = 0X0D, // Class of Device

nit: Remove COD since it's unused.

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +138,5 @@
>    , mClientIf(0)
>    , mConnId(0)
>    { }
>  
> +  // jamin remove this four lines ?

This has been taken care by Bug 1156229, no need to modify the destructor, you can see the private empty destructor if you rebase, thanks!

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.h
@@ +194,5 @@
> +  virtual void StartLeScanInternal(const nsTArray<nsString>& aServiceUuids,
> +                                   BluetoothReplyRunnable* aRunnable);
> +
> +  virtual void StopLeScanInternal(const nsAString& aScanUuid,
> +                                   BluetoothReplyRunnable* aRunnable);

nit: align with const nsAString& aScanUuid.

::: dom/bluetooth/bluetooth2/BluetoothAdapter.cpp
@@ +641,5 @@
> +    if (uuid.Equals(scanUuid)) {
> +      isScanning = true;
> +      break;
> +    }
> +  }

Can we use aDiscoveryHandle directly instead of using uuid here to make code simpler?

::: dom/bluetooth/bluetooth2/BluetoothAdapter.h
@@ +100,5 @@
>                                              ErrorResult& aRv);
>    already_AddRefed<Promise> StartDiscovery(ErrorResult& aRv);
>    already_AddRefed<Promise> StopDiscovery(ErrorResult& aRv);
> +
> +  already_AddRefed<Promise> StartLeScan(const nsTArray<nsString>& aServiceUuids, ErrorResult& aRv);

nit: over 80 chars.

@@ +101,5 @@
>    already_AddRefed<Promise> StartDiscovery(ErrorResult& aRv);
>    already_AddRefed<Promise> StopDiscovery(ErrorResult& aRv);
> +
> +  already_AddRefed<Promise> StartLeScan(const nsTArray<nsString>& aServiceUuids, ErrorResult& aRv);
> +  already_AddRefed<Promise> StopLeScan(BluetoothDiscoveryHandle& aDiscoveryHandle, ErrorResult& aRv);

nit: over 80 chars.

@@ +190,5 @@
> +   */
> +  void AppendLeScanHandle(BluetoothDiscoveryHandle* aDiscoveryHandle);
> +
> +  /**
> +   * Remove BluetoothDiscoverHandle with the given UUID from LeScan handle

Remove the BluetoothDiscoverHandle...

@@ +197,5 @@
> +   * @param aScanUuid [in] The UUID of the LE scan task.
> +   */
> +  void RemoveLeScanHandle(const nsAString& aScanUuid);
> +
> +

nit: remove this extra new line.

::: dom/bluetooth/bluetooth2/BluetoothDevice.cpp
@@ +316,5 @@
>  
> +void
> +BluetoothDevice::UpdatePropertiesFromAdvData(const nsTArray<uint8_t>& aAdvData)
> +{
> +  // According to BT Core Spec. Vol 3 - Ch 11, advertising data consists of a

nit: advertisement

@@ +331,5 @@
> +    if (len <= 0) {
> +      break;
> +    }
> +
> +    // Length of the data field which is composed by AD type and AD data.

Please also note that AD type (1 byte), AD data (Length-1) byte here.

@@ +340,5 @@
> +
> +    int type = aAdvData[offset++];
> +    if (offset >= aAdvData.Length()) {
> +      break;
> +    }

Suggest to revise as similar as below,

int dataLength = len - 1;
if (offset + dataLength >= aAdvData.Length()) {
  break;
}

int type = aAdvData[offset++];

@@ +354,5 @@
> +        mUuids.Clear();
> +        while (dataLength > 0) {
> +
> +          // The length of uint16_t UUID array
> +          uint8_t len = 0;

len is already used in this function, it's a bit of confusing what it is when I read 'len' at somewhere in this case, please use another naming.

@@ +361,5 @@
> +          } else if(GAP_INCOMPLETE_UUID32 && GAP_COMPLETE_UUID32) {
> +            len = 2;
> +          } else {
> +            len = 8;
> +          }

This if-else section should be outside the while loop since it's only need to be executed once.

@@ +373,5 @@
> +            dataLength -= 2;
> +          }
> +
> +          char uuidStr[36];
> +          if (len == 1) {

nit: use type for the condition here instead of len might be more clear?

@@ +387,5 @@
> +              uuid[7], uuid[6], uuid[5], uuid[4],
> +              uuid[3], uuid[2], uuid[1], uuid[0]);
> +          }
> +          nsString uuidNsString;
> +          uuidNsString.AssignASCII(uuidStr, 36);

nit: Use uuidNsString.AssignLiteral(uuidStr);

@@ +390,5 @@
> +          nsString uuidNsString;
> +          uuidNsString.AssignASCII(uuidStr, 36);
> +
> +          mUuids.AppendElement(uuidNsString);
> +        }

After finish updating uuids, you will need to do |BluetoothDeviceBinding::ClearCachedUuidsValue(this);| to delete the cached value.

@@ +393,5 @@
> +          mUuids.AppendElement(uuidNsString);
> +        }
> +        break;
> +      case GAP_SHORTENED_NAME:
> +        if (!mName.IsEmpty()) break;

Use dataLength instead of len - 1 in this case.

::: dom/bluetooth/bluetooth2/BluetoothDiscoveryHandle.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 "BluetoothDiscoveryHandle.h"

nit: #include "mozilla/dom/bluetooth/BluetoothDiscoveryHandle.h" and put it after #include "mozilla/dom/bluetooth/BluetoothCommon.h"

@@ +41,5 @@
>  }
>  
>  BluetoothDiscoveryHandle::~BluetoothDiscoveryHandle()
>  {
> +  DisconnectFromOwner();

I think we don't need to do |DOMEventTargetHelper::DisconnectFromOwner()| here.
Please pull out the signal distribution part into a function and use it here and in |BluetoothDiscoveryHandle::DisconnectFromOwner()|.

::: dom/bluetooth/bluetooth2/BluetoothDiscoveryHandle.h
@@ +34,3 @@
>    void DispatchDeviceEvent(BluetoothDevice* aDevice);
>  
> +  void DispatchLeDeviceEvent(BluetoothDevice* aLeDevice, int32_t aRssi,

nit: Put int32_T aRssi in an extra line.
Attachment #8597005 - Flags: review?(joliu)
- revise previous patch based on #comment 29
Attachment #8597005 - Attachment is obsolete: true
Attachment #8597784 - Attachment is obsolete: true
Attachment #8599024 - Flags: review?(joliu)
Bugzilla interdiff encountered errors while comparing v10 and v11.
Upload an diff file between v9 and v10 for reviewer.
Comment on attachment 8599024 [details] [diff] [review]
Implement LE scan related methods and LE device event (v11)

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

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +491,5 @@
>    sBluetoothGattClientInterface->UnregisterClient(
>      aClientIf,
>      new UnregisterClientResultHandler(client));
>  }
> +class BluetoothGattManager::StartLeScanResultHandler final

nit: add a newline before this line.

::: dom/bluetooth/bluetooth2/BluetoothAdapter.cpp
@@ +633,5 @@
> +                  : false;
> +
> +  // Reject the request if the LE scan has been stopped.
> +  BT_ENSURE_TRUE_REJECT(!isScanning,
> +                        NS_ERROR_DOM_BLUETOOTH_DONE);

Suggest to remove isScanning and revise as

// Reject the request if there's no ongoing LE Scan using this handle.
BT_ENSURE_TRUE_REJECT(!mLeScanHandleArray.Contains(&aDiscoveryHandle),
                      NS_ERROR_DOM_BLUETOOTH_DONE);

::: dom/bluetooth/bluetooth2/BluetoothDevice.cpp
@@ +321,5 @@
> +  // structure shall have a Length field of one octet, which contains the
> +  // Length value, and a Data field of Length octets.
> +  unsigned int offset = 0;
> +  while (offset < aAdvData.Length()) {
> +    int advDataStructLength = aAdvData[offset++];

nit: dataFieldLength?

@@ +351,5 @@
> +        // The length of uint16_t UUID array
> +        uint8_t len = 0;
> +        if (GAP_INCOMPLETE_UUID16 && GAP_COMPLETE_UUID16) {
> +          len = 1;
> +        } else if(GAP_INCOMPLETE_UUID32 && GAP_COMPLETE_UUID32) {

nit: space after if

@@ +385,5 @@
> +          nsString uuidNsString;
> +          uuidNsString.AssignLiteral(uuidStr);
> +
> +          mUuids.AppendElement(uuidNsString);
> +          BluetoothDeviceBinding::ClearCachedUuidsValue(this);

I will suggest to do it just once after we finish the while loop.

::: dom/bluetooth/bluetooth2/BluetoothDiscoveryHandle.cpp
@@ +34,5 @@
>    MOZ_ASSERT(aWindow);
>  }
>  BluetoothDiscoveryHandle::~BluetoothDiscoveryHandle()
>  {
> +  Shutdown();

Instead of using signal, how about we hold a reference to the adapter directly and do

if (!mLeScanUuid.IsEmpty()) {
  mAdapter->RemoveLeScanHandle(mLeScanUuid);
}

Same as in DisconnectFromOwner.
And we won't need a function for it since it will be quite simple.
Attachment #8599024 - Flags: review?(joliu)
- revise previous patch based on #comment 32

Thank Jocelyn for reviewing the previous patch.
Attachment #8599024 - Attachment is obsolete: true
Attachment #8599025 - Attachment is obsolete: true
Attachment #8599188 - Flags: review?(joliu)
Bugzilla interdiff encountered errors while comparing v11 and v12.
Upload an diff file between v11 and v12 for reviewer.
Comment on attachment 8599188 [details] [diff] [review]
Implement LE scan related methods and LE device event (v12)

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

r=me with comments addressed.
Thanks a lot!

A friendly reminder, please request a DOM peer review for webidls and related implementation.
And make sure it's green on try before landing it.

::: dom/bluetooth/bluetooth2/BluetoothDevice.cpp
@@ +387,5 @@
> +          nsString uuidNsString;
> +          uuidNsString.AssignLiteral(uuidStr);
> +
> +          mUuids.AppendElement(uuidNsString);
> +        }

nit: add a new line here.

::: dom/bluetooth/bluetooth2/BluetoothDiscoveryHandle.cpp
@@ +14,4 @@
>  #include "nsThreadUtils.h"
>  
>  USING_BLUETOOTH_NAMESPACE
>  

NS_IMPL_CYCLE_COLLECTION_INHERITED(BluetoothDiscoveryHandle, DOMEventTargetHelper, mAdapter)

@@ +79,5 @@
> +  const nsAString& aLeScanUuid,
> +  BluetoothAdapter* aAdapter)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aWindow);

MOZ_ASSERT(aAdapter);

::: dom/bluetooth/bluetooth2/BluetoothDiscoveryHandle.h
@@ +6,5 @@
>  
>  #ifndef mozilla_dom_bluetooth_bluetoothdiscoveryhandle_h
>  #define mozilla_dom_bluetooth_bluetoothdiscoveryhandle_h
>  
>  #include "BluetoothCommon.h"

mozilla/dom/bluetooth/BluetoothCommon.h

@@ +21,5 @@
>  
>  class BluetoothDiscoveryHandle final : public DOMEventTargetHelper
>  {
>  public:
>    NS_DECL_ISUPPORTS_INHERITED

NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(BluetoothDiscoveryHandle, DOMEventTargetHelper)
Attachment #8599188 - Flags: review?(joliu) → review+
- revise previous patch based on #comment 35

Thank Jocelyn for reviewing the patch. :)
Attachment #8599188 - Attachment is obsolete: true
Attachment #8599213 - Attachment is obsolete: true
Comment on attachment 8599257 [details] [diff] [review]
Implement LE scan related methods and LE device event (v13), r=joliu

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

Hi Blake,
Would you mind helping to review this patch regarding to WebIDL part?

This bug implements a fundamental feature to seek for remote LE devices advertising given services.
The seeking process is called LE scan. [1] 
P.S. The functionality of 'LE Scan' is similar to 'discovery' which is used to seek for classic devices.

Two methods (startLeScan and stopLescan) are added to BluetoothAdapter2.webidl [2] and BluetoothLeDeviceEvent.webidl [3] is created to deliver the scan results.

BluetoothLeDeviceEvent is carried as the parameter of event handler DiscoveryHandle.ondevicefound [4].
This bug doesn't add any new event handler since 'ondevicefound' is designed to handle both BluetoothDeviceEvent [5] and BluetoothLeDeviceEvent [6].

Thank you very much.
Regards,
Jamin

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothAdapter#startLeScan.28sequence.3CDOMString.3E_aServiceUuids.29

[2] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/BluetoothAdapter2.webidl?from=BluetoothAdapter2.webidl

[3] dom/webidl/BluetoothLeDeviceEvent.webidl

[4] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothDiscoveryHandle

[5] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothDeviceEvent

[6] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothLeDeviceEvent
Attachment #8599257 - Flags: review?(mrbkap)
Attachment #8599257 - Flags: review?(mrbkap) → review+
Thank Blake for reviewing the patch.
- rebased

Thank Jocelyn and Blake for reviewing the patch.

Try results on Treeherder:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=06473903368a
Attachment #8599257 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/887ac5b1c5f9
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S12 (15may)
You need to log in before you can comment on or make changes to this bug.