1006310 - Implement set properties of BluetoothAdapter

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

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

Comment 1

[Ben Tian (inactive)]

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

Comment 2

[Jocelyn Liu [:jocelyn] [:joliu]]

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

Comment 3

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1006310 - Patch1: Revise setName and setDiscoverable method in BluetoothAdapter.webidl

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

Comment 4

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1006310 - Patch2: Revise SetName and SetDiscoverable to use Promise

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

Comment 5

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Dev] Gaia patch for SetName and SetDiscoverable

Upload Gaia patch for testing purpose.

Comment 6

[Ben Tian (inactive)]

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>|.

Comment 7

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1006310 - Patch1(v2): Revise setName and setDiscoverable method in BluetoothAdapter.webidl. f=btian

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 8

[Boris Zbarsky [:bzbarsky]]

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 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Jocelyn Liu [:jocelyn] [:joliu]]

(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

Comment 11

[Jocelyn Liu [:jocelyn] [:joliu]]

(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

Comment 12

[Jocelyn Liu [:jocelyn] [:joliu]]

(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?

Comment 13

[Boris Zbarsky [:bzbarsky]]

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?

Comment 14

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Bug 1006310 - Patch1: Revise setName and setDiscoverable method in BluetoothAdapter.webidl. f=btian, r=bz

* add [Throws] and set r=bz

Comment 15

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1006310 - Patch2(v2): Revise SetName and SetDiscoverable to use Promise

* Rebased and addressed bz's review comments

Comment 16

[Ben Tian (inactive)]

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.

Comment 17

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1006310 - Patch2(v3): Revise SetName and SetDiscoverable to use Promise

Hi Ben,

I used JS::UndefinedHandleValue for JSVAL_VOID.
Other review comments are addressed in Patch2(v3).

Thanks,
Jocelyn

Comment 18

[Ben Tian (inactive)]

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)|.

Comment 19

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1006310 - Patch2: Revise SetName and SetDiscoverable to use Promise. r=btian

* Addressed latest review comments.

Comment 20

[Jocelyn Liu [:jocelyn] [:joliu]]

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 21

[Boris Zbarsky [:bzbarsky]]

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

Comment 22

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Bug 1006310 - Patch2: Revise SetName and SetDiscoverable to use Promise. r=btian, r=bz

Thanks for the review!

Comment 23

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Bug 1006310 - Patch2: Revise SetName and SetDiscoverable to use Promise. r=btian, r=bz

* Rebased

Comment 24

[Ben Tian (inactive)]

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

Comment 25

[Ed Morley [:emorley]]

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