* Set properties of BluetoothAdapter Promise<void> setName(DOMString aName); Promise<void> setDiscoverable(boolean aDiscoverable);

This bug is depends on Bug1016196 for Promise support in BluetoothReplyRunnable.

* Revise BluetoothAdapter webidl according to https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothAdapter#BluetoothAdapter.

* Revise SetName and SetDiscoverable to use Promise instead of DOM request.

Upload Gaia patch for testing purpose.

Comment on attachment 8435656 [details] [diff] [review] Bug 1006310 - Patch1: Revise setName and setDiscoverable method in BluetoothAdapter.webidl Review of attachment 8435656 [details] [diff] [review]: ----------------------------------------------------------------- f=me with comment addressed. Thanks. ::: dom/webidl/BluetoothAdapter2.webidl @@ +86,5 @@ > // Fired when attributes of BluetoothAdapter changed > attribute EventHandler onattributechanged; > > + Promise setName(DOMString aName); > + Promise setDiscoverable(boolean aDiscoverable); Please add comment to note they are |Promise<void>|.

Hi Boris, This patch revise two methods in BluetoothAdapter.webidl for our new Web Bluetoth API[1]. Would you mind to review this webidl change? Thanks, Jocelyn [1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothAdapter#BluetoothAdapter)

Comment on attachment 8436614 [details] [diff] [review] Bug 1006310 - Patch1(v2): Revise setName and setDiscoverable method in BluetoothAdapter.webidl. f=btian Please don't just ask me to review IDL in isolation next time. How the implementation matches up with the IDL is somewhat important. For example, the IDL here says these methods never return null but the implementation returns null in two places. Is the IDL or the implementation wrong? I would expect the implementation; it should be throwing an exception instead, which bindings will convert to a rejected promise. The implementation also rejects promises with strings. This is generally rather frowned upon; you should be rejecting with an Error instance. In fact, on trunk the implementation shouldn't even compile because MaybeReject was tightened up to not accept string arguments. I guess r=me on the IDL (except that it probably needs [Throws]), but please fix the implementation....

Comment on attachment 8435660 [details] [diff] [review] Bug 1006310 - Patch2: Revise SetName and SetDiscoverable to use Promise r- on this bit. Oh, and the comments were just about SetName(), but SetDiscoverable() has the same issues. In addition to those, is doing SetDiscoverable() with the existing boolean _really_ something that should reject the promise? That seems pretty odd, given that rejecting a promise is supposed to be equivalent to throwing an exception; I can't think of any API that would throw an exception on a set to the existing value.

(In reply to Boris Zbarsky [:bz] from comment #8) > Comment on attachment 8436614 [details] [diff] [review] > Bug 1006310 - Patch1(v2): Revise setName and setDiscoverable method in > BluetoothAdapter.webidl. f=btian > > Please don't just ask me to review IDL in isolation next time. How the > implementation matches up with the IDL is somewhat important. Understood and sorry for that. Thanks for the comment. :) > > For example, the IDL here says these methods never return null but the > implementation returns null in two places. Is the IDL or the implementation > wrong? I would expect the implementation; it should be throwing an > exception instead, which bindings will convert to a rejected promise. Will fix the implementation with through exception instead of return nullptr directly. > > The implementation also rejects promises with strings. This is generally > rather frowned upon; you should be rejecting with an Error instance. In > fact, on trunk the implementation shouldn't even compile because MaybeReject > was tightened up to not accept string arguments. We are awared of this change and already start to fix our current implementation. It's my fault that I only sync with the reviewer offline but didn't remove the review request on bugzilla. > > I guess r=me on the IDL (except that it probably needs [Throws]), but please > fix the implementation.... This bug is blocked by Bug1016196, I probably gonna wait for Bug1016196 to fix passing string while rejecting Promise. Then I will fix this bug's implementation patch. Thanks a lot for your valuable comments. Since webidl and its implementation are highly related, may I ask for your feedback/review again after the implementation patch addressed above comments? Thanks, Jocelyn

(In reply to jocelyn [:jocelyn] from comment #10) > (In reply to Boris Zbarsky [:bz] from comment #8) > > Comment on attachment 8436614 [details] [diff] [review] > > Bug 1006310 - Patch1(v2): Revise setName and setDiscoverable method in > > BluetoothAdapter.webidl. f=btian > > > > Please don't just ask me to review IDL in isolation next time. How the > > implementation matches up with the IDL is somewhat important. > Understood and sorry for that. Thanks for the comment. :) > > > > > For example, the IDL here says these methods never return null but the > > implementation returns null in two places. Is the IDL or the implementation > > wrong? I would expect the implementation; it should be throwing an > > exception instead, which bindings will convert to a rejected promise. > Will fix the implementation with through exception instead of return nullptr > directly. > typo here. Will fix the implementation with throwing exception instead of returning nullptr directly. > > > > The implementation also rejects promises with strings. This is generally > > rather frowned upon; you should be rejecting with an Error instance. In > > fact, on trunk the implementation shouldn't even compile because MaybeReject > > was tightened up to not accept string arguments. > We are awared of this change and already start to fix our current > implementation. > It's my fault that I only sync with the reviewer offline but didn't remove > the review request on bugzilla. > > > > > I guess r=me on the IDL (except that it probably needs [Throws]), but please > > fix the implementation.... > This bug is blocked by Bug1016196, I probably gonna wait for Bug1016196 to > fix passing string while rejecting Promise. Then I will fix this bug's > implementation patch. > > Thanks a lot for your valuable comments. > Since webidl and its implementation are highly related, may I ask for your > feedback/review again after the implementation patch addressed above > comments? > > Thanks, > Jocelyn

(In reply to Boris Zbarsky [:bz] from comment #9) > Comment on attachment 8435660 [details] [diff] [review] > Bug 1006310 - Patch2: Revise SetName and SetDiscoverable to use Promise > > r- on this bit. > > Oh, and the comments were just about SetName(), but SetDiscoverable() has > the same issues. > > In addition to those, is doing SetDiscoverable() with the existing boolean > _really_ something that should reject the promise? That seems pretty odd, > given that rejecting a promise is supposed to be equivalent to throwing an > exception; I can't think of any API that would throw an exception on a set > to the existing value. The idea behind our design is, when user receives a resolved promise for setName(), (setDiscovery as well) we think he/she will expect the value to be changed. And we will always have both an onattributechanged event and a resolved promise for success operations. We design as this way since we think that's more reasonable to us as a user. Is our idea make sense to you? What will you suggest on this case?

The common guideline I've heard about Promise-based APIs is that they should reject in exactly the situations in which a synchronous API would throw. Would you expect a sync setDiscoverable() to throw if setting to the already-existing value?

* add [Throws] and set r=bz

* Rebased and addressed bz's review comments

Comment on attachment 8441991 [details] [diff] [review] Bug 1006310 - Patch2(v2): Revise SetName and SetDiscoverable to use Promise Review of attachment 8441991 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth2/BluetoothAdapter.cpp @@ +454,2 @@ > if (mName.Equals(aName)) { > + promise->MaybeResolve(JS::UndefinedHandleValue); Why return JS::UndefinedHandleValue here? @@ +461,5 @@ > BluetoothNamedValue property(NS_LITERAL_STRING("Name"), value); > + > + BluetoothService* bs = BluetoothService::Get(); > + if (!bs) { > + promise->MaybeReject(NS_ERROR_DOM_INVALID_STATE_ERR); NS_ERROR_NOT_AVAILABLE as http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothAdapter.cpp#685 @@ +466,5 @@ > + return promise.forget(); > + } > + > + nsRefPtr<BluetoothReplyRunnable> result = > + new BluetoothVoidReplyRunnable(nullptr /* DOMRequest */, promise); Also pass method name to track resolve/reject of promise as http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothAdapter.cpp#712 new BluetoothVoidReplyRunnable(nullptr /* DOMRequest */, promise, NS_LITERAL_STRING("SetName")); @@ +497,4 @@ > > + BluetoothService* bs = BluetoothService::Get(); > + if (!bs) { > + promise->MaybeReject(NS_ERROR_DOM_OPERATION_ERR); Ditto. @@ +502,4 @@ > } > + > + nsRefPtr<BluetoothReplyRunnable> result = > + new BluetoothVoidReplyRunnable(nullptr /* DOMRequest */, promise); Ditto. ::: dom/bluetooth2/BluetoothAdapter.h @@ +69,5 @@ > { > aName = mName; > } > > + nsString GetPath() const Remove object path related code from BluetoothAdapter and BluetoothDevice since only blueZ requires it. ::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp @@ -897,5 @@ > BT_WARNING("Failed to dispatch to main thread!"); > } > BT_LOGR("Error"); > } > - nit: Keep newline here @@ +1112,5 @@ > prop.type = BT_PROPERTY_ADAPTER_SCAN_MODE; > } else if (propName.EqualsLiteral("DiscoverableTimeout")) { > prop.type = BT_PROPERTY_ADAPTER_DISCOVERY_TIMEOUT; > } else { > BT_LOGR("Warning: Property type is not supported yet, type: %d", prop.type); Print |propName| instead of |prop.type| since it's unassigned yet. @@ +1115,5 @@ > } else { > BT_LOGR("Warning: Property type is not supported yet, type: %d", prop.type); > + DispatchBluetoothReply(aRunnable, BluetoothValue(), > + NS_LITERAL_STRING(ERR_SET_PROPERTY)); > + nit: remove this newline. @@ +1139,5 @@ > } else { > BT_LOGR("SetProperty but the property cannot be recognized correctly."); > + DispatchBluetoothReply(aRunnable, BluetoothValue(), > + NS_LITERAL_STRING(ERR_SET_PROPERTY)); > + nit: remove this newline.

Hi Ben, I used JS::UndefinedHandleValue for JSVAL_VOID. Other review comments are addressed in Patch2(v3). Thanks, Jocelyn

Comment on attachment 8442664 [details] [diff] [review] Bug 1006310 - Patch2(v3): Revise SetName and SetDiscoverable to use Promise Review of attachment 8442664 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment addressed. Thanks. ::: dom/bluetooth2/BluetoothAdapter.cpp @@ +306,5 @@ > BluetoothAdapter::Notify(const BluetoothSignal& aData) > { > InfallibleTArray<BluetoothNamedValue> arr; > > BT_LOGD("[A] %s: %s", __FUNCTION__, NS_ConvertUTF16toUTF8(aData.name()).get()); nit: This line exceeds 80 characters. @@ +310,5 @@ > BT_LOGD("[A] %s: %s", __FUNCTION__, NS_ConvertUTF16toUTF8(aData.name()).get()); > > BluetoothValue v = aData.value(); > if (aData.name().EqualsLiteral("DeviceFound")) { > + nsRefPtr<BluetoothDevice> device = BluetoothDevice::Create(GetOwner(), aData.value()); nit: This line exceeds 80 characters. @@ +317,5 @@ > init.mBubbles = false; > init.mCancelable = false; > init.mDevice = device; > nsRefPtr<BluetoothDeviceEvent> event = > BluetoothDeviceEvent::Constructor(this, NS_LITERAL_STRING("devicefound"), init); nit: This line exceeds 80 characters. @@ +451,2 @@ > if (mName.Equals(aName)) { > + promise->MaybeResolve(JS::UndefinedHandleValue); Leave comment to explain that we use |JS::UndefinedHandleValue| as JSVAL_VOID since this method is of type Promise<void>. @@ +466,5 @@ > + nsRefPtr<BluetoothReplyRunnable> result = > + new BluetoothVoidReplyRunnable(nullptr /* DOMRequest */, > + promise, > + NS_LITERAL_STRING("SetName")); > + if(NS_FAILED(bs->SetProperty(BluetoothObjectType::TYPE_ADAPTER, nit: space after |if|. @@ +489,2 @@ > if (aDiscoverable == mDiscoverable) { > + promise->MaybeResolve(JS::UndefinedHandleValue); Ditto. @@ +503,5 @@ > + nsRefPtr<BluetoothReplyRunnable> result = > + new BluetoothVoidReplyRunnable(nullptr /* DOMRequest */, > + promise, > + NS_LITERAL_STRING("SetDiscoverable")); > + if(NS_FAILED(bs->SetProperty(BluetoothObjectType::TYPE_ADAPTER, Ditto. ::: dom/bluetooth2/BluetoothDevice.cpp @@ +169,5 @@ > MOZ_ASSERT(NS_IsMainThread()); > MOZ_ASSERT(aWindow); > > nsRefPtr<BluetoothDevice> device = > + new BluetoothDevice(aWindow, aValue); nit: Wrap this into one line. ::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp @@ +1114,5 @@ > } else if (propName.EqualsLiteral("DiscoverableTimeout")) { > prop.type = BT_PROPERTY_ADAPTER_DISCOVERY_TIMEOUT; > } else { > + BT_LOGR("Warning: Property type is not supported yet, type: %s", > + propName.get()); Use |NS_ConvertUTF16toUTF8(propName).get()| since |propName| is of type nsString, not nsCString. @@ +1148,5 @@ > > int ret = sBtInterface->set_adapter_property(&prop); > if (ret != BT_STATUS_SUCCESS) { > + ReplyStatusError(aRunnable, ret, NS_LITERAL_STRING(ERR_SET_PROPERTY)); > + sSetPropertyRunnableArray.RemoveElementAt(0); Use |RemoveElement(aRunnable)|.

* Addressed latest review comments.

Comment on attachment 8442715 [details] [diff] [review] Bug 1006310 - Patch2: Revise SetName and SetDiscoverable to use Promise. r=btian Hi Boris, I have addressed your review comments in comment8 and comment9. Changes: * Throw an exception instead of return nullptr directly. (Also added [Throws] into webidl.) * Reject promise with an error instead of string. * Resolved promise for setting the existing value. Would you mind to review this patch again? Thanks, Jocelyn

Comment on attachment 8442715 [details] [diff] [review] Bug 1006310 - Patch2: Revise SetName and SetDiscoverable to use Promise. r=btian >+ // Use JS::UndefinedHandleValue as JSVAL_VOID Please don't mention JSVAL_VOID. It's deprecated. Just say that this is a Promise<void> so it needs to be resolved with "undefined". Same for the other place this comment appears. r=me

Thanks for the review!

* Rebased

https://hg.mozilla.org/integration/b2g-inbound/rev/3d821db0a262 https://hg.mozilla.org/integration/b2g-inbound/rev/3a9209d7df39

https://hg.mozilla.org/mozilla-central/rev/3d821db0a262 https://hg.mozilla.org/mozilla-central/rev/3a9209d7df39