Closed Bug 1032755 Opened 7 years ago Closed 7 years ago

Follow up of Bug 1031253: Revise BluetoothReplyRunnable to allow Promise to reject with some subtype of 'Error'.

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S3 (29aug)

People

(Reporter: yrliou, Assigned: jaliu)

References

Details

(Whiteboard: webbt-api, [p=1])

Attachments

(2 files, 8 obsolete files)

9.89 KB, patch
Details | Diff | Splinter Review
7.35 KB, patch
Details | Diff | Splinter Review
This is a follow up bug of bug 1031253.
Need to revise BluetoothReplyRunnable to support nsresult eror.
See Also: → 1036233
Summary: Follow up of Bug 1031253: Revise BluetoothReplyRunnable to support nsresult error → Follow up of Bug 1031253: Revise BluetoothReplyRunnable to allow Promise to reject with some subtype of 'Error'.
Assignee: nobody → jaliu
Whiteboard: webbt-api → webbt-api, [p=1]
Target Milestone: --- → 2.1 S2 (15aug)
P.S. According to the description of Bug 1016560, promise should reject with exception.
Attachment #8466964 - Flags: review?(btian)
A few issues here:

1)  Why are you manually doing ToJSValue on the DOMException?  Why not just MaybeReject(*domException)?

2)  Is there a reason not to just add a new nsresult code representing this case and reject with it?  You can map it to a string of your choosing (and an exception .name of your choosing) in dom/base/domerr.msg.
Also, we could just add a helper on Promise to reject with a particular kind of exception and message string if that would be useful.  I'm all in favor of making it easy to provide useful rejection messages!
Comment on attachment 8466964 [details] [diff] [review]
Return a DOMException with error message when BluetoothReplyRunnable rejects the promise. (v1)

Please revise based on bz's comment first. Thanks.
Attachment #8466964 - Flags: review?(btian)
Blocks: 1036233
(In reply to On vacation Aug 5-18.  Please do not request review. from comment #2)
> A few issues here:
> 
> 1)  Why are you manually doing ToJSValue on the DOMException?  Why not just
> MaybeReject(*domException)?
> 
> 2)  Is there a reason not to just add a new nsresult code representing this
> case and reject with it?  You can map it to a string of your choosing (and
> an exception .name of your choosing) in dom/base/domerr.msg.

Hi bz,
Thank you for your comments.

1)
I created a JSValue which represents DOMException since the MaybeReject() can only accept 'JS::Value' or 'nsresult' as parameter.

- void MaybeReject(JSContext* aCx, JS::Handle<JS::Value> aValue)
- void MaybeReject(nsresult aArg)

However, I'm going to add new nsresult codes and reject with it in my next patch as you mentioned at #comment 2 (2). The 'JS::Value' will be replaced by 'nsresult'.

2)
I think it's a great advise and I will uploaded a new patch based on it.

I didn't add nsresult for Bluetooth since the error messages are mapping to different Bluedroid status. Sometimes the Bluedroid status may not be an 'Error', it seems a little odd to put it into a error list. For example, 'BT_STATUS_DONE' represent "request already completed". Sometimes 'BT_STATUS_DONE' is treated as an Error, but most time it's not.
I will try to find a proper way to handle this case.
Blocks: 1049452
No longer blocks: 1036233
> I created a JSValue which represents DOMException since the MaybeReject() can only accept
> 'JS::Value' or 'nsresult' as parameter.

Ah.  We should fix that.  A followup bug would be great.
Attachment #8468333 - Flags: review?(btian)
Attachment #8469177 - Flags: review?(btian)
Comment on attachment 8469177 [details] [diff] [review]
(part 1) Add NS_ERROR_MODULE_DOM_BLUETOOTH to nsError list and allow it to be used to create dom exception. (v2)

Hi Dave,

This is Jamin from Bluetooth team.
Bluetooth team is working on API refinement and we plan to return Promise object for every asynchronous Bluetooth API.

According to Bug 1016560, promise should reject with Exception object.
bz suggested me to add error messages to dom/base/domerr.msg a #Comment 2.

I added NS_ERROR_MODULE_DOM_BLUETOOTH to nsError list at Attachment #8469177 [details] [diff].
Ben Tian would review it from Bluetooth perspective and I'm looking for a reviewer from different perspective.

Since bz is on vacation, would you mind to take look and give me some feedback?
I noticed you reviewed a similar patch for NS_ERROR_MODULE_DOM_FILESYSTEM at Bug 910412.

Thank you.
Attachment #8469177 - Flags: review?(dhylands)
The error messages which defined at ErrorList.h are mapping to the error status from Bluetooth stack.

Please refer to hardware/libhardware/include/hardware/bluetooth.h.
- BT_STATUS_FAIL,
- BT_STATUS_NOT_READY,
- BT_STATUS_NOMEM,
- BT_STATUS_BUSY,
- BT_STATUS_DONE,
- BT_STATUS_UNSUPPORTED,
- BT_STATUS_PARM_INVALID,
- BT_STATUS_UNHANDLED,
- BT_STATUS_AUTH_FAILURE,
- BT_STATUS_RMT_DEV_DOWN,
- BT_STATUS_AUTH_REJECTED
Comment on attachment 8469177 [details] [diff] [review]
(part 1) Add NS_ERROR_MODULE_DOM_BLUETOOTH to nsError list and allow it to be used to create dom exception. (v2)

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

This looks fine to me, but I think that a DOM peer should really review this.
Attachment #8469177 - Flags: review?(dhylands) → feedback+
Comment on attachment 8469177 [details] [diff] [review]
(part 1) Add NS_ERROR_MODULE_DOM_BLUETOOTH to nsError list and allow it to be used to create dom exception. (v2)

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

f=me with nit addressed. Please request a DOM peer review. Thanks.

::: dom/base/DOMException.cpp
@@ +83,5 @@
> +  BtDoneError              = 0,
> +  BtUnsupportedError       = 0,
> +  BtParmInvalidError       = 0,
> +  BtUnhandledError         = 0,
> +  BtAuthFailError          = 0,

nit: |BtAuthFailureError| to be consistent with error name.

@@ +85,5 @@
> +  BtParmInvalidError       = 0,
> +  BtUnhandledError         = 0,
> +  BtAuthFailError          = 0,
> +  BtRmtDevDownError        = 0,
> +  BtRejectedError          = 0,

|BtAuthRejectedError|.
Attachment #8469177 - Flags: review?(btian) → feedback+
Comment on attachment 8469763 [details] [diff] [review]
(part 1) Add NS_ERROR_MODULE_DOM_BLUETOOTH to nsError list and allow it to be used to create dom exception. (v3), f=dhylands, btian

Hi Olli,

This is Jamin from Bluetooth team.
Bluetooth team is working on API refinement and we plan to return Promise object for every asynchronous Bluetooth API.

According to Bug 1016560, promise should reject with Exception object.
bz suggested me to add error messages to dom/base/domerr.msg a #Comment 2.

I added NS_ERROR_MODULE_DOM_BLUETOOTH to nsError list at Attachment #8469763 [details] [diff].
Ben has reviewed it from Bluetooth perspective and I'm looking for a reviewer from DOM perspective.

Since bz is on vacation, would you mind to take a look and review the patch?
Thank you.
Attachment #8469763 - Flags: review?(bugs)
Comment on attachment 8469763 [details] [diff] [review]
(part 1) Add NS_ERROR_MODULE_DOM_BLUETOOTH to nsError list and allow it to be used to create dom exception. (v3), f=dhylands, btian


>+/* Non-standard Bluetooth DOM errors. */
>+DOM4_MSG_DEF(BtFailError,         "Fail",  NS_ERROR_DOM_BLUETOOTH_FAIL)
>+DOM4_MSG_DEF(BtNotReadyError,     "Not ready",  NS_ERROR_DOM_BLUETOOTH_NOT_READY)
>+DOM4_MSG_DEF(BtNoMemError,        "No memory",  NS_ERROR_DOM_BLUETOOTH_NOMEM)
>+DOM4_MSG_DEF(BtBusyError,         "Device busy with another command",  NS_ERROR_DOM_BLUETOOTH_BUSY)
>+DOM4_MSG_DEF(BtDoneError,         "Request already done",  NS_ERROR_DOM_BLUETOOTH_DONE)
>+DOM4_MSG_DEF(BtUnsupportedError,  "Unsupported",  NS_ERROR_DOM_BLUETOOTH_UNSUPPORTED)
>+DOM4_MSG_DEF(BtParmInvalidError,  "Invalid parameter",  NS_ERROR_DOM_BLUETOOTH_PARM_INVALID)
Parm sounds a bit odd. why not Param. same with _PARM
Attachment #8469763 - Flags: review?(bugs) → review+
Comment on attachment 8468333 [details] [diff] [review]
(part 2) Return a DOMException with error message when BluetoothReplyRunnable rejects the promise. (v2)

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

Please see my questions below.

::: dom/bluetooth2/BluetoothReplyRunnable.cpp
@@ +82,5 @@
>    // Promise
>    if (mPromise) {
>      BT_API2_LOGR("<%s>", NS_ConvertUTF16toUTF8(mName).get());
>  
> +    if (!mErrorStatus) {

Ensure |mErrorStatus| is zero by default.

@@ +83,5 @@
>    if (mPromise) {
>      BT_API2_LOGR("<%s>", NS_ConvertUTF16toUTF8(mName).get());
>  
> +    if (!mErrorStatus) {
> +      nsresult rv = NS_ERROR_GENERATE_SUCCESS(NS_ERROR_MODULE_DOM_BLUETOOTH, mErrorStatus);

nit: this line is too long.

Why not |NS_ERROR_GENERATE_FAILURE| but |NS_ERROR_GENERATE_SUCCESS|?

::: dom/bluetooth2/BluetoothReplyRunnable.h
@@ +40,3 @@
>    {
> +    mErrorStatus = BT_STATUS_FAIL;
> +    mErrorString = aErrorString;

Call |setError(BT_STATUS_FAIL, aErrorString)| directly.

@@ +41,5 @@
> +    mErrorStatus = BT_STATUS_FAIL;
> +    mErrorString = aErrorString;
> +  }
> +
> +  void SetError(const uint8_t aErrorStatus, const nsAString& aErrorString)

How about make this function:

  void SetError(const nsAString& aErrorString, const uint8_t aErrorStatus = SUCCESS);

so you don't need another function?

::: dom/bluetooth2/bluedroid/BluetoothUtils.cpp
@@ +12,5 @@
>  #include "BluetoothUtils.h"
>  #include "jsapi.h"
>  #include "mozilla/Scoped.h"
>  #include "mozilla/dom/bluetooth/BluetoothTypes.h"
> +#include "mozilla/dom/DOMException.h"

Why is the header file required here?

@@ +192,5 @@
> +                       const bt_status_t aStatusCode)
> +{
> +  // Reply will be deleted by the runnable after running on main thread
> +  BluetoothReply* reply;
> +  if (aStatusCode == 0) {

|aStatusCode != BT_STATUS_SUCCESS| is safer if BT_STATUS_SUCCESS becomes non-zero value.

::: dom/bluetooth2/bluedroid/BluetoothUtils.h
@@ +46,5 @@
>  
>  void
> +DispatchBluetoothReply(BluetoothReplyRunnable* aRunnable,
> +                       const BluetoothValue& aValue,
> +                       const bt_status_t aStatusCode);

Can both |aStatusCode| and |aErrorStr| be passed in? If not, why?
Attachment #8468333 - Flags: review?(btian)
Thank you for your comments.
I've uploaded a new patch based on your comments.

(In reply to Ben Tian [:btian] from comment #17)
> Comment on attachment 8468333 [details] [diff] [review]
> ::: dom/bluetooth2/bluedroid/BluetoothUtils.h
> @@ +46,5 @@
> >  
> >  void
> > +DispatchBluetoothReply(BluetoothReplyRunnable* aRunnable,
> > +                       const BluetoothValue& aValue,
> > +                       const bt_status_t aStatusCode);
> 
> Can both |aStatusCode| and |aErrorStr| be passed in? If not, why?

I think it's a reasonable option, but personally I'd prefer not to do so.

We plan to reject promise with DOMException which contains error message and both |aStatusCode| and |aErrorStr| can provide that message.
If we pass 'aStatusCode' in DispatchBluetoothReply(), the pre-defined error string will be associated to DOMException automatically.

I think we need to support dispatching BluetoothReply with |aStatusCode| first, since it's easier to use.
Then, maybe we should remove or extend |DispatchBluetoothReply(..., const nsAString& aErrorStr)|, depends on whether we plan to support "customized error string" in bluetooth2.
Thank you for reviewing the patch. :)

(In reply to Olli Pettay [:smaug] from comment #16)
> Comment on attachment 8469763 [details] [diff] [review]
> >+DOM4_MSG_DEF(BtParmInvalidError,  "Invalid parameter",  NS_ERROR_DOM_BLUETOOTH_PARM_INVALID)
> Parm sounds a bit odd. why not Param. same with _PARM

Yeah, that's true.
It's a dilemma between 'Readability' and 'Consistency', since Bluedroid use the enum value 'BT_STATUS_PARM_INVALID' at hardware/libhardware/include/hardware/bluetooth.h.

Hi, Ben
Could you give me some advices on this topic?
Flags: needinfo?(btian)
Comment on attachment 8471467 [details] [diff] [review]
(part 2) Return a DOMException with error message when BluetoothReplyRunnable rejects the promise. (v3)

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

::: dom/bluetooth2/BluetoothReplyRunnable.cpp
@@ +5,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "base/basictypes.h"
>  #include "BluetoothReplyRunnable.h"
> +#include "BluetoothUtils.h"

Why is the header required?

@@ +20,5 @@
>                                                 Promise* aPromise,
>                                                 const nsAString& aName)
>    : mDOMRequest(aReq)
>    , mPromise(aPromise)
> +  , mErrorStatus(BT_STATUS_SUCCESS)

Remove this init since the if-condition in |FireErrorString| should be removed.

@@ +83,5 @@
>    // Promise
>    if (mPromise) {
>      BT_API2_LOGR("<%s>", NS_ConvertUTF16toUTF8(mName).get());
>  
> +    if (!mErrorStatus) {

Remove the whole if-condition since 1) |mErrorStatus| is never |BT_STATUS_SUCCESS| here since |setError| is always called before and 2) |mErrorStatus| by default is BT_STATUS_FAIL after |setError| is called.

  if (mPromise) {
    BT_API2_LOGR(...);

    nsresult rv =
      NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM_BLUETOOTH, mErrorStatus);
    mPromise->MaybeReject(rv);
  }

::: dom/bluetooth2/bluedroid/BluetoothUtils.cpp
@@ +191,5 @@
> +                       const bt_status_t aStatusCode)
> +{
> +  // Reply will be deleted by the runnable after running on main thread
> +  BluetoothReply* reply;
> +  if (aStatusCode == BT_STATUS_SUCCESS) {

I believe it's |aStatus != BT_STATUS_SUCCESS|.

::: dom/bluetooth2/bluedroid/BluetoothUtils.h
@@ +44,5 @@
>                         const BluetoothValue& aValue,
>                         const nsAString& aErrorStr);
>  
>  void
> +DispatchBluetoothReply(BluetoothReplyRunnable* aRunnable,

Leave comment to explain difference of what DOMRequest error and Promise rejection get between these 2 functions.
Attachment #8471467 - Flags: review?(btian)
Thank you for your comments.

(In reply to Ben Tian [:btian] from comment #21)
> Comment on attachment 8471467 [details] [diff] [review]
> (part 2) Return a DOMException with error message when
> BluetoothReplyRunnable rejects the promise. (v3)
> 
> Review of attachment 8471467 [details] [diff] [review]:
> @@ +20,5 @@
> >                                                 Promise* aPromise,
> >                                                 const nsAString& aName)
> >    : mDOMRequest(aReq)
> >    , mPromise(aPromise)
> > +  , mErrorStatus(BT_STATUS_SUCCESS)
> 
> Remove this init since the if-condition in |FireErrorString| should be
> removed.
I prefer to initial this member variable to avoid undefined value.
Although this variable should be initialed by SetError() with our current codebase, I want to ensure that every member variables are initialized after constructor was called.

As you mentioned at #Comment 23, @@ +83,5 @@, we don't have to use "if (!mErrorStatus)" as condition.
I'd like to change the initial value from 'BT_STATUS_SUCCESS' to 'BT_STATUS_FAIL', since we take BT_STATUS_FAIL as a default error.

> @@ +83,5 @@
> >    // Promise
> >    if (mPromise) {
> >      BT_API2_LOGR("<%s>", NS_ConvertUTF16toUTF8(mName).get());
> >  
> > +    if (!mErrorStatus) {
> 
> Remove the whole if-condition since 1) |mErrorStatus| is never
> |BT_STATUS_SUCCESS| here since |setError| is always called before and 2)
> |mErrorStatus| by default is BT_STATUS_FAIL after |setError| is called.
> 
>   if (mPromise) {
>     BT_API2_LOGR(...);
> 
>     nsresult rv =
>       NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM_BLUETOOTH, mErrorStatus);
>     mPromise->MaybeReject(rv);
>   }

> ::: dom/bluetooth2/bluedroid/BluetoothUtils.h
> @@ +44,5 @@
> >                         const BluetoothValue& aValue,
> >                         const nsAString& aErrorStr);
> >  
> >  void
> > +DispatchBluetoothReply(BluetoothReplyRunnable* aRunnable,
> 
> Leave comment to explain difference of what DOMRequest error and Promise
> rejection get between these 2 functions.
That's a great idea.
Thanks.
(In reply to Jamin Liu [:jaliu] from comment #23)
> I prefer to initial this member variable to avoid undefined value.
> Although this variable should be initialed by SetError() with our current
> codebase, I want to ensure that every member variables are initialized after
> constructor was called.

Then also initialize |mErrorString|.
Flags: needinfo?(btian)
(In reply to Ben Tian [:btian] from comment #24)
> (In reply to Jamin Liu [:jaliu] from comment #23)
> > I prefer to initial this member variable to avoid undefined value.
> > Although this variable should be initialed by SetError() with our current
> > codebase, I want to ensure that every member variables are initialized after
> > constructor was called.
> 
> Then also initialize |mErrorString|.

Per offline discuss with Ben, we decide to take empty string as default value of |mErrorString|.
Since the default constructor of nsString would set itself as an empty string, I don't add |mErrorString| to initialization list.

I will rebase this patch after Bug 1050126 land since it defined Bluedroid status codes as an enumeration in BluetoothCommon.h.
Comment on attachment 8472876 [details] [diff] [review]
(part 2) Return a DOMException with error message when BluetoothReplyRunnable rejects the promise. (v4)

Clear r? flag and will review rebased patch.
Attachment #8472876 - Flags: review?(btian)
* rebase.
* replace 'bt_status_t' by 'BluetoothStatus' since Bug 1050126 has landed.
Attachment #8472876 - Attachment is obsolete: true
Attachment #8476415 - Flags: review?(btian)
Comment on attachment 8476415 [details] [diff] [review]
(part 2) Return a DOMException with error message when BluetoothReplyRunnable rejects the promise. (v5)

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

r=me with comment addressed. Thanks.

::: dom/bluetooth2/BluetoothReplyRunnable.h
@@ +8,5 @@
>  #define mozilla_dom_bluetooth_bluetoothreplyrunnable_h__
>  
>  #include "mozilla/Attributes.h"
>  #include "BluetoothCommon.h"
> +#include "BluetoothInterface.h"

Remove the redundant header since it's useless.

::: dom/bluetooth2/bluedroid/BluetoothUtils.h
@@ +41,5 @@
> +/**
> + * Dispatch Bluetooth reply to main thread. The reply will contain an error
> + * string if the request fails.
> + *
> + * This function mainly designed for Bluetooth APIs which return 'DOMRequest'.

'is' mainly designed. Here and below.

Replace 'APIs' with 'methods.' Or you can just write 'for DOMRequest-based methods'.

@@ +43,5 @@
> + * string if the request fails.
> + *
> + * This function mainly designed for Bluetooth APIs which return 'DOMRequest'.
> + * If aErrorStr is not empty, the DOMRequest property 'error.name' would be
> + * updated to aErrorStr right before the callback function 'onerror' fired.

'is' fired.

@@ +45,5 @@
> + * This function mainly designed for Bluetooth APIs which return 'DOMRequest'.
> + * If aErrorStr is not empty, the DOMRequest property 'error.name' would be
> + * updated to aErrorStr right before the callback function 'onerror' fired.
> + *
> + * @param aRunnable  the Bluetooth Reply Runnable which use to reply the

which is used. I suggest to just write 'the runnable to reply the bluetooth request' and the following.

@@ +62,5 @@
> + * Dispatch Bluetooth reply to main thread. The reply will contain an error
> + * status if the request fails.
> + *
> + * This function mainly designed for Bluetooth APIs which return 'Promise'.
> + * If aStatusCode is not STATUS_SUCCESS, the Promise would reject with an

would be rejected

@@ +64,5 @@
> + *
> + * This function mainly designed for Bluetooth APIs which return 'Promise'.
> + * If aStatusCode is not STATUS_SUCCESS, the Promise would reject with an
> + * Exception object.
> + * The ns error used by Exception will be associated with given status code.

Replace 'given status code' with aStatusCode.

@@ +66,5 @@
> + * If aStatusCode is not STATUS_SUCCESS, the Promise would reject with an
> + * Exception object.
> + * The ns error used by Exception will be associated with given status code.
> + * The name and messege of Exception are defined in dom/base/domerr.msg and
> + * will be filled automaticlly during promise rejection.

typo: automatic'a'lly
Attachment #8476415 - Flags: review?(btian) → review+
(In reply to Jamin Liu [:jaliu] from comment #20)
> Thank you for reviewing the patch. :)
> 
> (In reply to Olli Pettay [:smaug] from comment #16)
> > Comment on attachment 8469763 [details] [diff] [review]
> > >+DOM4_MSG_DEF(BtParmInvalidError,  "Invalid parameter",  NS_ERROR_DOM_BLUETOOTH_PARM_INVALID)
> > Parm sounds a bit odd. why not Param. same with _PARM
> 
> Yeah, that's true.
> It's a dilemma between 'Readability' and 'Consistency', since Bluedroid use
> the enum value 'BT_STATUS_PARM_INVALID' at
> hardware/libhardware/include/hardware/bluetooth.h.
> 
> Hi, Ben
> Could you give me some advices on this topic?

Per offline discussion with Ben, we both agree to keep the naming of enum value 'BT_STATUS_PARM_INVALID'.

There is a similar enumeration of Bluedroid status has been defined in Bug 1038591 recently.
It used 'STATUS_PARM_INVALID' to represent "Invalid parameter", therefore, we decide to follow it for being consistent.
* Add r=.. ,f=.. to commit message.
* Convert from git format to hg format.
Attachment #8469763 - Attachment is obsolete: true
It looks fine on try server.
https://tbpl.mozilla.org/?tree=Try&rev=d604c1115349

P.S. Try server wouldn't verify the correctness of bluetooth2. It's only been used in preventing regression of bluetooth1.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6c6f7dcc4c72
https://hg.mozilla.org/mozilla-central/rev/e8efd2cf56e8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
You need to log in before you can comment on or make changes to this bug.