* BluetoothDevice: https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothDevice

* Revise BluetoothDevice.webidl according to https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothDevice#BluetoothDevice. * Add [Throws] for fetchUuids()

* Implementation of new BluetoothDevice APIs.

bluedroid won't perform get_remote_services if it is discovering nearby remote devices. Revise FetchUuidsInternal to stop discovery first before calling get_remote_services.

Add NewObjects to fetchUuids

Comment on attachment 8449148 [details] [diff] [review] Bug 1006315 - Patch1(v2): Revise BluetoothDevice.webidl for new WebBluetooth API. Review of attachment 8449148 [details] [diff] [review]: ----------------------------------------------------------------- f=me with comment addressed. Thanks. ::: dom/webidl/BluetoothDevice2.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/. */ > > +enum BluetoothDeviceAttribute Please leave comment to explain that |BluetoothDeviceAttribute| doesn't have "address" since it should never changes after BluetoothDevice is created.

Comment on attachment 8449146 [details] [diff] [review] Bug 1006315 - Patch2 (v2): Implement BluetoothDevice for new WebBluetooth API. Review of attachment 8449146 [details] [diff] [review]: ----------------------------------------------------------------- Please see my comments below. ::: dom/bluetooth2/BluetoothAdapter.cpp @@ +909,5 @@ > new BluetoothVoidReplyRunnable(request); > > nsAutoString address; > aDevice.GetAddress(address); > + uint32_t deviceClass = aDevice.Cod()->GetUint32_t(); Modify function name to |ToUinit32| since I've revised it in bug 1027504. ::: dom/bluetooth2/BluetoothDevice.cpp @@ +26,3 @@ > USING_BLUETOOTH_NAMESPACE > > DOMCI_DATA(BluetoothDevice, BluetoothDevice) Please check if we can remove NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED in BluetoothDevice.h and some of the following macros. You can refer to BluetoothDiscoveryHandle.h/cpp. @@ +50,5 @@ > > +class FetchUuidsTask : public BluetoothReplyRunnable > +{ > + public: > + FetchUuidsTask(nsIDOMDOMRequest* aReq, Remove |aReq| since we'll use promise. Pass nullptr directly into BluetoothReplyRunnable constructor. @@ +70,5 @@ > + > + const InfallibleTArray<nsString>& uuids = v.get_ArrayOfnsString(); > + > + AutoJSAPI jsapi; > + if (!jsapi.Init(mGlobal)) { Use NS_ENSURE_TRUE. Remove |SetError| since only DOMRequest requires error string. @@ +78,5 @@ > + } > + > + JSContext* cx = jsapi.cx(); > + JS::Rooted<JSObject*> jsUuids(cx); > + if (NS_FAILED(nsTArrayToJSArray(cx, uuids, &jsUuids))) { Use |ToJSValue| as in |DispatchAttributeEvent|. @@ +80,5 @@ > + JSContext* cx = jsapi.cx(); > + JS::Rooted<JSObject*> jsUuids(cx); > + if (NS_FAILED(nsTArrayToJSArray(cx, uuids, &jsUuids))) { > + BT_WARNING("Cannot create JS array!"); > + SetError(NS_LITERAL_STRING("BluetoothCreateJSArrayError")); Remove |SetError|. @@ +88,5 @@ > + aValue.setObject(*jsUuids); > + return true; > + } > + > + void ReleaseMembers() Add virtual and MOZ_OVERRIDE as [1]. [1] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothAdapter.cpp#91 @@ +95,5 @@ > + mGlobal = nullptr; > + } > + > +private: > + nsCOMPtr<nsIGlobalObject> mGlobal; Store BluetoothDevice instead of global object only for consistency. @@ +172,5 @@ > + > + nsRefPtr<Promise> promise = new Promise(global); > + > + BluetoothService* bs = BluetoothService::Get(); > + if (!bs) { Use new macro BT_ENSURE_TRUE_REJECT defined in BluetoothCommon.h @@ +183,5 @@ > + promise, > + NS_LITERAL_STRING("FetchUuids"), > + global); > + > + if (NS_FAILED(bs->FetchUuidsInternal(mAddress, result))) { Use BT_ENSURE_TRUE_REJECT. @@ +214,2 @@ > } else { > #ifdef DEBUG Simplify this section as [2]. [2] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothAdapter.cpp#330 @@ +243,5 @@ > +{ > + switch(aType) { > + case BluetoothDeviceAttribute::Cod: > + MOZ_ASSERT(aValue.type() == BluetoothValue::Tuint32_t); > + return mCod->Equals(aValue.get_uint32_t()); return !Equals. @@ +252,5 @@ > + MOZ_ASSERT(aValue.type() == BluetoothValue::Tbool); > + return mPaired != aValue.get_bool(); > + case BluetoothDeviceAttribute::Uuids: { > + MOZ_ASSERT(aValue.type() == BluetoothValue::TArrayOfnsString); > + return mUuids != aValue.get_ArrayOfnsString(); Does it compare strings inside? @@ +282,5 @@ > + > + // BluetoothDeviceAttribute properties > + if (IsDeviceAttributeChanged(type, arr[i].value())) { > + SetPropertyByValue(arr[i]); > + types.AppendElement(arr[i].name()); Use BT_APPEND_ENUM_STRING. @@ +295,5 @@ > +{ > + NS_ENSURE_TRUE_VOID(aTypes.Length()); > + > + AutoJSAPI jsapi; > + jsapi.Init(GetOwner()); NS_ENSURE_TRUE_VOID(jsapi.Init(GetOwner())); ::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp @@ +129,2 @@ > sSetPropertyRunnableArray.Clear(); > sUnbondingRunnableArray.Clear(); nit: reorder them as order of declaration. @@ -468,5 @@ > Run() > { > MOZ_ASSERT(NS_IsMainThread()); > > - if (sRequestedDeviceCountArray.IsEmpty()) { Move this check after FetchUuids task. Make the code flow as following: - fire propertychanged - fetchuuids task - get devices task > check |sRequestedDeviceCountArray.IsEmpty| > ... @@ +403,5 @@ > } > > + // FetchUuids task > + if (!sFetchUuidsRunnableArray.IsEmpty()) { > + // mProps will contains Address and Uuids only nit: mProps 'contains' ... @@ +405,5 @@ > + // FetchUuids task > + if (!sFetchUuidsRunnableArray.IsEmpty()) { > + // mProps will contains Address and Uuids only > + DispatchBluetoothReply(sFetchUuidsRunnableArray[0], > + mProps[1].value(), Leave comment to specify mProps[1] is UUIDs: DispatchBluetoothReply(sFetchUuidsRunnableArray[0], mProps[1].value(), /* UUIDs */ @@ +505,5 @@ > BT_APPEND_NAMED_VALUE(propertiesArray, "Name", propertyValue); > } else if (p.type == BT_PROPERTY_CLASS_OF_DEVICE) { > uint32_t cod = *(uint32_t*)p.val; > propertyValue = cod; > + BT_APPEND_NAMED_VALUE(propertiesArray, "Cod", propertyValue); Append |cod| directly. @@ +1053,5 @@ > + > + bt_bdaddr_t addressType; > + StringToBdAddressType(aDeviceAddress, &addressType); > + ret = sBtInterface->get_remote_services(&addressType); > + nit: remove the newline and similar before other |ENSURE_BT_STATUS_SUCCESS|, since the do-then-check should be logically one operation. ::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.h @@ +37,5 @@ > BluetoothReplyRunnable* aRunnable); > > + virtual nsresult > + FetchUuidsInternal(const nsAString& aDeviceAddress, > + BluetoothReplyRunnable* aRunnable); Add MOZ_OVERRIDE. ::: dom/bluetooth2/bluedroid/BluetoothUtils.cpp @@ +55,5 @@ > aRetBdAddress = NS_ConvertUTF8toUTF16(bdstr); > } > > +void > +UuidToString(bt_uuid_t* aUuid, nsAString& aString) { We can make BluetoothUuid.h/cpp blueZ only since bluedroid only uses two of its functions. Please file a follow-up bug to do this. @@ +61,5 @@ > + > + uint32_t uuid0, uuid4; > + uint16_t uuid1, uuid2, uuid3, uuid5; > + > + memcpy(&uuid0, &(aUuid->uu[0]), 4); Replace 4 with sizeof(uint32_t) and 2 with sizeof(uint16_t). @@ +73,5 @@ > + ntohl(uuid0), ntohs(uuid1), > + ntohs(uuid2), ntohs(uuid3), > + ntohl(uuid4), ntohs(uuid5)); > + > + aString = NS_ConvertUTF8toUTF16(uuidStr); Call |aString.Truncate| to clean it and |aString.AssignLiteral| to set it. ::: dom/bluetooth2/bluez/BluetoothDBusService.h @@ +64,5 @@ > > + virtual nsresult > + FetchUuidsInternal(const nsAString& aDeviceAddress, > + BluetoothReplyRunnable* aRunnable) > + MOZ_OVERRIDE; nit: make MOZ_OVERRIDE follow |BluetoothReplyRunnable* aRunnable)| in the same line. ::: dom/bluetooth2/ipc/BluetoothServiceChildProcess.h @@ +63,5 @@ > MOZ_OVERRIDE; > virtual nsresult > + FetchUuidsInternal(const nsAString& aDeviceAddress, > + BluetoothReplyRunnable* aRunnable) > + MOZ_OVERRIDE; nit: make MOZ_OVERRIDE follow |BluetoothReplyRunnable* aRunnable)| in the same line.

* Addressed review comments

Comment on attachment 8451559 [details] [diff] [review] Bug 1006315 - Patch2 (v3): Implement BluetoothDevice for new WebBluetooth API. Review of attachment 8451559 [details] [diff] [review]: ----------------------------------------------------------------- Please see my comment below. ::: dom/bluetooth2/BluetoothDevice.cpp @@ +28,5 @@ > NS_IMPL_RELEASE_INHERITED(BluetoothDevice, DOMEventTargetHelper) > > +class FetchUuidsTask : public BluetoothReplyRunnable > +{ > + public: nit: do not indent. Also revise following lines. @@ +32,5 @@ > + public: > + FetchUuidsTask(Promise* aPromise, > + const nsAString& aName, > + BluetoothDevice* aDevice) > + : BluetoothReplyRunnable(nullptr /* DOM Request */, aPromise, aName) nit: DOMRequest. @@ +35,5 @@ > + BluetoothDevice* aDevice) > + : BluetoothReplyRunnable(nullptr /* DOM Request */, aPromise, aName) > + , mDevice(aDevice) > + { > + MOZ_ASSERT(aPromise); Add MOZ_ASSERT(aDevice); @@ +202,5 @@ > +bool > +BluetoothDevice::IsDeviceAttributeChanged(BluetoothDeviceAttribute aType, > + const BluetoothValue& aValue) > +{ > + switch(aType) { nit: space before |(aType)|. @@ +220,5 @@ > + if (uuids.Length() != mUuids.Length()) { > + return true; > + } > + for (size_t i = 0; i < uuids.Length(); i++) { > + if (!mUuids[i].Equals(uuids[i])) { Identical uuids may be reordered but we only care about whether it remains. I find operator == compares both length and each element [1]. Maybe we can sort the two arrays before comparing them. [1] http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h?from=nsTArray.h&case=true# @@ +270,5 @@ > + > + AutoJSAPI jsapi; > + NS_ENSURE_TRUE_VOID(jsapi.Init(GetOwner())); > + JSContext* cx = jsapi.cx(); > + JS::Rooted<JS::Value> value(cx); Retrieve |scope| and |global| for compartment as |BluetoothAdapter::DispatchAttributeEvent|. ::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp @@ +45,5 @@ > return result; \ > } \ > } while(0) > > +#define ENSURE_BT_STATUS_SUCCESS(runnable, result, errStr) \ I suggest to rebase on Thomas's changes in bug 1033961. He wraps bluedroid interface for better portability on different backends. @@ +399,5 @@ > + // FetchUuids task > + if (!sFetchUuidsRunnableArray.IsEmpty()) { > + // mProps contains Address and Uuids only > + DispatchBluetoothReply(sFetchUuidsRunnableArray[0], > + mProps[1].value() /* UUids */, nit: either Uuids or UUIDs. ::: dom/bluetooth2/bluedroid/BluetoothUtils.cpp @@ +74,5 @@ > + ntohs(uuid2), ntohs(uuid3), > + ntohl(uuid4), ntohs(uuid5)); > + > + aString.Truncate(); > + //aString.AssignLiteral(NS_ConvertUTF8toUTF16(uuidStr).get()); Remove this line.

(In reply to Ben Tian [:btian] from comment #9) > Comment on attachment 8451559 [details] [diff] [review] > Bug 1006315 - Patch2 (v3): Implement BluetoothDevice for new WebBluetooth > API. > > Review of attachment 8451559 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please see my comment below. > > ::: dom/bluetooth2/BluetoothDevice.cpp > @@ +28,5 @@ > > NS_IMPL_RELEASE_INHERITED(BluetoothDevice, DOMEventTargetHelper) > > > > +class FetchUuidsTask : public BluetoothReplyRunnable > > +{ > > + public: > > nit: do not indent. Also revise following lines. > > @@ +32,5 @@ > > + public: > > + FetchUuidsTask(Promise* aPromise, > > + const nsAString& aName, > > + BluetoothDevice* aDevice) > > + : BluetoothReplyRunnable(nullptr /* DOM Request */, aPromise, aName) > > nit: DOMRequest. > > @@ +35,5 @@ > > + BluetoothDevice* aDevice) > > + : BluetoothReplyRunnable(nullptr /* DOM Request */, aPromise, aName) > > + , mDevice(aDevice) > > + { > > + MOZ_ASSERT(aPromise); > > Add MOZ_ASSERT(aDevice); > > @@ +202,5 @@ > > +bool > > +BluetoothDevice::IsDeviceAttributeChanged(BluetoothDeviceAttribute aType, > > + const BluetoothValue& aValue) > > +{ > > + switch(aType) { > > nit: space before |(aType)|. > > @@ +220,5 @@ > > + if (uuids.Length() != mUuids.Length()) { > > + return true; > > + } > > + for (size_t i = 0; i < uuids.Length(); i++) { > > + if (!mUuids[i].Equals(uuids[i])) { > > Identical uuids may be reordered but we only care about whether it remains. > I find operator == compares both length and each element [1]. Maybe we can > sort the two arrays before comparing them. > > [1] > http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray. > h?from=nsTArray.h&case=true# > > @@ +270,5 @@ > > + > > + AutoJSAPI jsapi; > > + NS_ENSURE_TRUE_VOID(jsapi.Init(GetOwner())); > > + JSContext* cx = jsapi.cx(); > > + JS::Rooted<JS::Value> value(cx); > > Retrieve |scope| and |global| for compartment as > |BluetoothAdapter::DispatchAttributeEvent|. > According the comment of |Init|, it will enter the correct compartment for us. http://dxr.mozilla.org/mozilla-central/source/dom/base/ScriptSettings.h#140 > ::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp > @@ +45,5 @@ > > return result; \ > > } \ > > } while(0) > > > > +#define ENSURE_BT_STATUS_SUCCESS(runnable, result, errStr) \ > > I suggest to rebase on Thomas's changes in bug 1033961. He wraps bluedroid > interface for better portability on different backends. I will rebase based on that after it is landed.

* Make uuids a sorted set. * Address review comments.

(In reply to jocelyn [:jocelyn] from comment #10) > > @@ +270,5 @@ > > > + > > > + AutoJSAPI jsapi; > > > + NS_ENSURE_TRUE_VOID(jsapi.Init(GetOwner())); > > > + JSContext* cx = jsapi.cx(); > > > + JS::Rooted<JS::Value> value(cx); > > > > Retrieve |scope| and |global| for compartment as > > |BluetoothAdapter::DispatchAttributeEvent|. > > > According the comment of |Init|, it will enter the correct compartment for > us. > http://dxr.mozilla.org/mozilla-central/source/dom/base/ScriptSettings.h#140 Please open a follow-up bug to remove them from BluetoothManager and BluetoothAdapter as well.

Comment on attachment 8452226 [details] [diff] [review] Bug 1006315 - Patch2 (v4): Implement BluetoothDevice for new WebBluetooth API. Review of attachment 8452226 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment addressed. Thanks! ::: dom/bluetooth2/BluetoothDevice.cpp @@ +123,4 @@ > } else if (name.EqualsLiteral("Paired")) { > mPaired = value.get_bool(); > } else if (name.EqualsLiteral("UUIDs")) { > mUuids = value.get_ArrayOfnsString(); Add comment: // We assume the received uuids array is sorted without duplicate items. If it's not, we require additional processing before assigning it directly. @@ +128,5 @@ > } else { > nsCString warningMsg; > warningMsg.AssignLiteral("Not handling device property: "); > warningMsg.Append(NS_ConvertUTF16toUTF8(name)); > BT_WARNING(warningMsg.get()); Simplify to: BT_WARNING("Not handling device property: %s", NS_ConvertUTF16toUTF8(name).get()); @@ +172,5 @@ > > void > BluetoothDevice::Notify(const BluetoothSignal& aData) > { > BT_LOGD("[D] %s: %s", __FUNCTION__, NS_ConvertUTF16toUTF8(aData.name()).get()); Remove |__FUNCTION__| since BT_LOGD prints it already. @@ +216,5 @@ > + return mPaired != aValue.get_bool(); > + case BluetoothDeviceAttribute::Uuids: { > + MOZ_ASSERT(aValue.type() == BluetoothValue::TArrayOfnsString); > + const InfallibleTArray<nsString>& uuids = aValue.get_ArrayOfnsString(); > + // uuids should be a sorted set Revise comment: // We assume the received uuids array is sorted without duplicate items. If it's not, we require additional processing before comparing it directly. ::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp @@ +470,5 @@ > + nsAutoString uuid; > + bt_uuid_t* pUuid = (bt_uuid_t*)p.val + j; > + UuidToString(pUuid, uuid); > + > + if (!uuids.Contains(uuid)) { Leave comment to note we filter out duplicate uuids here.

Above review comments have been addressed. Thanks for your time, Ben.

Comment on attachment 8451556 [details] [diff] [review] Bug 1006315 - Patch1(v3): Revise BluetoothDevice.webidl for new WebBluetooth API. f=btian Hi Boris, In this bug, we revised BluetoothDevice.webidl[1] and implemented them. Changes are divided into two patches, webidl and implementation. Could you help us review these two patches? Patch1: webidl change Patch2: * Implement fetchUuids functionality to retrieve latest Uuid information of remote device. * Using CoD class implemented by bug 1027504 for Cod attribute * Fire attributechanged event for device attributes * Remove redundant code Thanks, Jocelyn [1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothDevice#BluetoothDevice (We will add [NewObject, Throws] into fetchUuids() on this webpage.)

Comment on attachment 8451556 [details] [diff] [review] Bug 1006315 - Patch1(v3): Revise BluetoothDevice.webidl for new WebBluetooth API. f=btian This needs some documentation that explains what these various bits do (e.g. how does fetchUuids interact with .uuids? Is the array it's resolved with the same as the array .uuids returns? etc). r=me with that added.

Comment on attachment 8452871 [details] [diff] [review] Bug 1006315 - Patch2 (v5): Implement BluetoothDevice for new WebBluetooth API. r=btian r=me on the DOM-facing bits, but someone more familiar with the bluetoothservice impls should review those parts.

* Add comments to fetchUuids as bz suggested.

Thanks for reviewing the DOM-facing part, Boris. This implementation patch has already been reviewed by Ben, one of our BT module peers. * Rebased

* qfold patch2 into patch1 since patch1 can't be compiled alone.