1) Create dom/bluetooth2 folder for API refinement and copy dom/bluetooth to dom/bluetooth2. 2) Modify dom/webidl/Bluetooth*.webidl and moz.build to build dom/bluetooth2 folder based on additional build flag. Jocelyn, please help on this bug. Thanks.

Comment on attachment 8421522 [details] [diff] [review] (local patch) Bug 1009347 Patch5: Set BT API version to V2 Hi Shuang, May I ask that is this patch looks ok to you? Or there's a more suitable point to insert this build flag? Please let me know if you have any feedback. Thanks, Jocelyn

Comment on attachment 8421515 [details] [diff] [review] Bug 1009347 Patch1: Make copy of dom/bluetooth for API refinement Review of attachment 8421515 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

Comment on attachment 8421516 [details] [diff] [review] Bug 1009347 Patch2: Using new bluetooth webidls based on bluetooth API version build flag Review of attachment 8421516 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

Comment on attachment 8421517 [details] [diff] [review] Bug 1009347 Patch3: Using dom/bluetooth2 based on bluetooth API version build flag Review of attachment 8421517 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

Comment on attachment 8421518 [details] [diff] [review] Bug 1009347 Patch4: Change binding headers in bluetooth2 for using new webidls Review of attachment 8421518 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

Comment on attachment 8421516 [details] [diff] [review] Bug 1009347 Patch2: Using new bluetooth webidls based on bluetooth API version build flag Marcos, can you help review our webidl patches for WebBluetooth API refinement? This bug is to create another folder dom/bluetooth2 for the new API and build it according to additional build flag MOZ_B2G_BT_API_V2 (by default OFF). Two patches for review: - Patch2 is to create webidl files for the new API and build them according to build flag, and - Patch3 is to build dom/bluetooth2 when the build flag is set. Let us know for any question. Thanks.

(In reply to jocelyn [:jocelyn] from comment #6) > Comment on attachment 8421522 [details] [diff] [review] > (local patch) Bug 1009347 Patch5: Set BT API version to V2 > > Hi Shuang, > > May I ask that is this patch looks ok to you? > Or there's a more suitable point to insert this build flag? > Please let me know if you have any feedback. Add API flag is a little unusual, please add explanation when requesting review to mwu or glandium. For now, I think it's ok to use it.

Comment on attachment 8421516 [details] [diff] [review] Bug 1009347 Patch2: Using new bluetooth webidls based on bluetooth API version build flag Review of attachment 8421516 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/BluetoothAdapter2.webidl @@ +27,5 @@ > + // current track length (ms) > + long long duration = -1; > + // playing time (ms) > + long long position = -1; > + // one of 'STOPPED'/'PLAYING'/'PAUSED'/'FWD_SEEK'/'REV_SEEK'/'ERROR' You should create an enum for the playStatus, maybe call it PlayState. Make sure the states are in lower-case. { PlayState playState; } enum PlayState { "stopped", "playing", ...}; @@ +46,5 @@ > + > + // array of type DOMString[] > + [GetterThrows] > + readonly attribute any uuids; > + I guess there is some reason why you can't use the types in the comments above. @@ +65,5 @@ > + // Fired when remote devices query current media play status > + attribute EventHandler onrequestmediaplaystatus; > + > + [NewObject, Throws] > + DOMRequest setName(DOMString name); Just wondering, but given that we now have Promises, can't we start deprecating the use of DOMRequest? @@ +81,5 @@ > + DOMRequest unpair(DOMString deviceAddress); > + [NewObject, Throws] > + DOMRequest getPairedDevices(); > + [NewObject, Throws] > + DOMRequest getConnectedDevices(unsigned short serviceUuid); Aren't UUIDs strings? Applies globally for other serviceUuids. ::: 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/. */ > + > +interface BluetoothDevice : EventTarget { It's a bit strange that this implements EventTarget but doesn't have any EventHandler attributes? I would have expected some "attribute EventHandler onfoo;" listed in the IDL. @@ +17,5 @@ > + readonly attribute any uuids; > + > + // array of type DOMString[] > + [Throws] > + readonly attribute any services; It's not clear what kind of services will be returned? Is it a bunch of IDs or actual service names? ::: dom/webidl/BluetoothManager2.webidl @@ +4,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +interface BluetoothManager : EventTarget { > + [Throws] > + readonly attribute boolean enabled; Seems a bit mean to throw when checking an attribute. @@ +13,5 @@ > + > + [Throws] > + boolean isConnected(unsigned short aProfile); > + [NewObject, Throws] > + DOMRequest? getDefaultAdapter(); Why does this return null? I think this should always return a DOMRequest that results in an error if there is no default adapter.

Sorry Marcos I didn't say it clear. This patch only creates a copy of existing bluetooth API webidl files (BluetoothManager.webidl, BluetoothAdapter.webidl, BluetoothDevice.idl). What we want to do is fork them first in this bug and start the refinement [1] based on them. We'd like to request for your review as this patch modifies files under dom/webidl/ and dom/ folders. Please let us know for any suggestion. Thanks. [1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2 (In reply to Marcos Caceres [:marcosc] from comment #13) > Comment on attachment 8421516 [details] [diff] [review] > Bug 1009347 Patch2: Using new bluetooth webidls based on bluetooth API > version build flag > > Review of attachment 8421516 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/webidl/BluetoothAdapter2.webidl > @@ +27,5 @@ > > + // current track length (ms) > > + long long duration = -1; > > + // playing time (ms) > > + long long position = -1; > > + // one of 'STOPPED'/'PLAYING'/'PAUSED'/'FWD_SEEK'/'REV_SEEK'/'ERROR' > > You should create an enum for the playStatus, maybe call it PlayState. Make > sure the states are in lower-case. > > { > PlayState playState; > } > > enum PlayState { "stopped", "playing", ...}; > > @@ +46,5 @@ > > + > > + // array of type DOMString[] > > + [GetterThrows] > > + readonly attribute any uuids; > > + > > I guess there is some reason why you can't use the types in the comments > above. > > @@ +65,5 @@ > > + // Fired when remote devices query current media play status > > + attribute EventHandler onrequestmediaplaystatus; > > + > > + [NewObject, Throws] > > + DOMRequest setName(DOMString name); > > Just wondering, but given that we now have Promises, can't we start > deprecating the use of DOMRequest? > > @@ +81,5 @@ > > + DOMRequest unpair(DOMString deviceAddress); > > + [NewObject, Throws] > > + DOMRequest getPairedDevices(); > > + [NewObject, Throws] > > + DOMRequest getConnectedDevices(unsigned short serviceUuid); > > Aren't UUIDs strings? Applies globally for other serviceUuids. > > ::: 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/. */ > > + > > +interface BluetoothDevice : EventTarget { > > It's a bit strange that this implements EventTarget but doesn't have any > EventHandler attributes? I would have expected some "attribute EventHandler > onfoo;" listed in the IDL. > > @@ +17,5 @@ > > + readonly attribute any uuids; > > + > > + // array of type DOMString[] > > + [Throws] > > + readonly attribute any services; > > It's not clear what kind of services will be returned? Is it a bunch of IDs > or actual service names? > > ::: dom/webidl/BluetoothManager2.webidl > @@ +4,5 @@ > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > + > > +interface BluetoothManager : EventTarget { > > + [Throws] > > + readonly attribute boolean enabled; > > Seems a bit mean to throw when checking an attribute. > > @@ +13,5 @@ > > + > > + [Throws] > > + boolean isConnected(unsigned short aProfile); > > + [NewObject, Throws] > > + DOMRequest? getDefaultAdapter(); > > Why does this return null? I think this should always return a DOMRequest > that results in an error if there is no default adapter.

Comment on attachment 8421516 [details] [diff] [review] Bug 1009347 Patch2: Using new bluetooth webidls based on bluetooth API version build flag Review of attachment 8421516 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the misunderstanding, I guess then LGTM.

Comment on attachment 8421517 [details] [diff] [review] Bug 1009347 Patch3: Using dom/bluetooth2 based on bluetooth API version build flag Review of attachment 8421517 [details] [diff] [review]: ----------------------------------------------------------------- LGTM

We need to change other moz.build files which import bluetooth headers

Accidentally include webidl changes in patch6 , remove it

Comment on attachment 8421515 [details] [diff] [review] Bug 1009347 Patch1: Make copy of dom/bluetooth for API refinement Review of attachment 8421515 [details] [diff] [review]: ----------------------------------------------------------------- r+ since this is just a copy of dom/bluetooth. Please ensure there won't be any build failure happening, and next time please write a short summary of the patch to explain what the patch is going to change.

Do "hg cp dom/bluetooth dom/bluetooth2" for making a development environment for new bluetooth API.

1. hg cp BluetoothManager, BluetoothDevice, BluetoothAdapter webidls for new api 2. conditionally adding webidls based on MOZ_B2G_BT_API_V2 flag

Switching dom/bluetooth and dom/bluetooth2 based on MOZ_B2G_BT_API_V2

Change included binding header files for BT manager, adapter, and device (ex: BluetoothAdapterBinding.h -> BluetoothAdapter2Binding.h)

Try Server: https://tbpl.mozilla.org/?tree=Try&rev=eb6695e223ce

update commit log for reviewer name

update reviewer name in the commit log

Cancel the checkin request for running more build options on try server

try server: https://tbpl.mozilla.org/?tree=Try&rev=29307b493981

Clean 'checkin-needed' as patch 2 requires DOM peer review.

Comment on attachment 8424683 [details] [diff] [review] [Final] Bug 1009347 Patch2: Using new bluetooth webidls based on bluetooth API version build flag, f=btian, r=marcosc, r=bz Boris, can you review our WEBIDL change for WebBluetooth API refinement? This patch creates a copy of existing bluetooth webidl files for us to work based on them. The whole bug intends to create another folder dom/bluetooth2 for the new API and build it according to additional build flag MOZ_B2G_BT_API_V2 (by default OFF).

Comment on attachment 8424683 [details] [diff] [review] [Final] Bug 1009347 Patch2: Using new bluetooth webidls based on bluetooth API version build flag, f=btian, r=marcosc, r=bz r=me

https://hg.mozilla.org/integration/b2g-inbound/rev/7e5b760b4fdf https://hg.mozilla.org/integration/b2g-inbound/rev/71df0f6dface https://hg.mozilla.org/integration/b2g-inbound/rev/1d21153bfe47 https://hg.mozilla.org/integration/b2g-inbound/rev/1887733a9549

https://hg.mozilla.org/mozilla-central/rev/7e5b760b4fdf https://hg.mozilla.org/mozilla-central/rev/71df0f6dface https://hg.mozilla.org/mozilla-central/rev/1d21153bfe47 https://hg.mozilla.org/mozilla-central/rev/1887733a9549