W3C defines the mapping from system errors to Javascript error names. Add error codes for error handling to Gecko for aligning with W3C spec.

Hi Bruce, According to W3C spec [1], we need to provide a error code mapping mechanism from GATT error code to DOM error code. In this patch, I adding a mapping function in |BluetoothUtils|. Besides, I implement "ReplyRunnable" classes (|BluetoothGattReplyRunnable|, |BluetoothGattVoidReplyRunnable| and |BluetoothGattReplyTaskQueue|) for GATT API to distribute the error replying mechanism between classic bluetooth and GATT(BLE). I also add a |BluetoothGattWriteVoidReplyRunnable| class for separating the "Write" and "Not Write" command based on [2]. For aligning [3], I modify the code in |BluetoothManager| while it reply such error code at first time. Could you help me to review this patch? [1] https://webbluetoothcg.github.io/web-bluetooth/#error-handling [2] https://webbluetoothcg.github.io/web-bluetooth/#Application-Error [3] https://webbluetoothcg.github.io/web-bluetooth/#Insufficient-Encryption-Key-Size Thanks, Tom

Comment on attachment 8714229 [details] [diff] [review] Bug1238842 - Add error codes to Gecko by following W3C spec - dom/bluetooth/bluedroid & dom/bluetooth/common & dom/bluetooth/ipc change - v1 Review of attachment 8714229 [details] [diff] [review]: ----------------------------------------------------------------- Hi Tom, I haven't digest all codes of |BluetoothGattReplyRunnable|, but I notice that lots of codes and data members are duplicated from the base class. Would you please help to refine the design so that we can minimize the duplication? It would be a good practice to just let the derived class handle specialized parts only. Other than |BluetoothGattReplyRunnable|, comments are added inline below. ::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp @@ +3356,5 @@ > DispatchReplySuccess(runnable, BluetoothValue(value)); > } else if (!client->mReadCharacteristicState.mAuthRetry && > (aStatus == GATT_STATUS_INSUFFICIENT_AUTHENTICATION || > + aStatus == GATT_STATUS_INSUFFICIENT_ENCRYPTION || > + aStatus == GATT_STATUS_INSUFFICIENT_ENCRYPTION_KEY_SIZE)) { This modification would impact the current behavior. Suggest to leave this change to another follow-up bug. Probably we could compare the implementation of Android[1][2][3][4] on that follow-up bug as well to make sure our codes work correctly after applying this change. [1] http://androidxref.com/5.1.1_r6/xref/frameworks/base/core/java/android/bluetooth/BluetoothGatt.java#329 [2] http://androidxref.com/5.1.1_r6/xref/frameworks/base/core/java/android/bluetooth/BluetoothGatt.java#389 [3] http://androidxref.com/5.1.1_r6/xref/frameworks/base/core/java/android/bluetooth/BluetoothGatt.java#477 [4] http://androidxref.com/5.1.1_r6/xref/frameworks/base/core/java/android/bluetooth/BluetoothGatt.java#530 @@ +3404,5 @@ > DispatchReplySuccess(runnable); > } else if (!client->mWriteCharacteristicState.mAuthRetry && > (aStatus == GATT_STATUS_INSUFFICIENT_AUTHENTICATION || > + aStatus == GATT_STATUS_INSUFFICIENT_ENCRYPTION || > + aStatus == GATT_STATUS_INSUFFICIENT_ENCRYPTION_KEY_SIZE)) { Ditto. @@ +3468,5 @@ > DispatchReplySuccess(runnable, BluetoothValue(value)); > } else if (!client->mReadDescriptorState.mAuthRetry && > (aStatus == GATT_STATUS_INSUFFICIENT_AUTHENTICATION || > + aStatus == GATT_STATUS_INSUFFICIENT_ENCRYPTION || > + aStatus == GATT_STATUS_INSUFFICIENT_ENCRYPTION_KEY_SIZE)) { Ditto. @@ +3517,5 @@ > DispatchReplySuccess(runnable); > } else if (!client->mWriteDescriptorState.mAuthRetry && > (aStatus == GATT_STATUS_INSUFFICIENT_AUTHENTICATION || > + aStatus == GATT_STATUS_INSUFFICIENT_ENCRYPTION || > + aStatus == GATT_STATUS_INSUFFICIENT_ENCRYPTION_KEY_SIZE)) { Ditto. ::: dom/bluetooth/common/BluetoothGattReplyRunnable.cpp @@ +152,5 @@ > + const BluetoothGattStatus\ > + aErrorGattStatus) > +{ > + mErrorString = aErrorString; > + GattStatustoDOMStatus(aErrorGattStatus, mErrorDOMStatus, true); I am not a fan to use boolean parameters to determine the behavior of functions. Here are some possible alternatives to get rid of it: - Determine whether to assign |InvalidModificationError| before calling |GattStatustoDOMStatus()|. Let |GattStatustoDOMStatus()| always assign |NotSupportedError| while encountering |Application Error|. - Adding one virtual function, for example |bool IsWriteProcedure()|, on |BluetoothGattReplyRunnable| and simply return |false|. Override |IsWriteProcedure()| at |BluetoothGattWriteVoidReplyRunnable| and simply return |true|. And then our error mapping logic can directly check on the boolean value returned by this virtual function. Any other clever way to avoid boolean parameters is encouraged. ::: dom/bluetooth/common/BluetoothGattReplyRunnable.h @@ +32,5 @@ > + > + virtual void ReleaseMembers(); > + > + void SetError(const nsAString& aErrorString, > + const enum BluetoothStatus aErrorStatus = STATUS_FAIL) I guess you need a |virtual| keyword |SetError| on the base class (i.e. |BluetoothReplyRunnable|), no? @@ +39,5 @@ > + mErrorStatus = aErrorStatus; > + mIsGattError = false; > + } > + virtual void SetError(const nsAString& aErrorString, > + const BluetoothGattStatus aErrorGattStatus); How about we just make |BluetoothReplyRunnable::SetError()| be a virtual function, and make it accept three arguments (i.e. error string, bluetooth status, gatt status)? So that we probably can save the burden of almost duplicating the whole base class here. ::: dom/bluetooth/common/BluetoothUtils.cpp @@ +711,5 @@ > > // Reply will be deleted by the runnable after running on main thread > BluetoothReply* reply = > + new BluetoothReply(BluetoothReplyError(STATUS_FAIL, > + static_cast<BluetoothGattStatus>(0), Please avoid using any magic number as much as possible (i.e. |0| in this case.) @@ +728,5 @@ > > // Reply will be deleted by the runnable after running on main thread > BluetoothReply* reply = > + new BluetoothReply(BluetoothReplyError(aStatus, > + static_cast<BluetoothGattStatus>(0), Ditto. @@ +746,5 @@ > + > + // Reply will be deleted by the runnable after running on main thread > + BluetoothReply* reply = > + new BluetoothReply(BluetoothReplyError(aStatus, > + static_cast<BluetoothGattStatus>(0), Ditto. @@ +763,5 @@ > + MOZ_ASSERT(!aErrorStr.IsEmpty()); > + > + // Reply will be deleted by the runnable after running on main thread > + BluetoothReply* reply = > + new BluetoothReply(BluetoothReplyError(static_cast<BluetoothStatus>(0), Ditto. @@ +823,5 @@ > + * Since the current Gecko does not have such timeout mechenism and cannot > + * get the information about ATT bearer, put it into TODO. > + */ > + > + aDOMStatus = NS_ERROR_DOM_NOT_SUPPORTED_ERR; nit: |aDOMStatus| would be assigned values twice in most cases. Although this probably is not the bottleneck of the performance, suggest to consider how the logic could be refined to reduce redundant assignment without breaking the correct flow and the readability. Probably assign the default value at the default case of the switch statement would be one possible choice. Any other alternative to refine it would be appreciated as well. ::: dom/bluetooth/common/BluetoothUtils.h @@ +337,5 @@ > + */ > +void > +DispatchGattReplyError(BluetoothReplyRunnable* aRunnable, > + const enum BluetoothStatus aStatus, > + const nsAString& aErrorStr); Could this function be used in non-GATT cases? Suggest to remove GATT related wordings from the function name and comments if this function can be used in general cases. @@ +353,5 @@ > + */ > +void > +DispatchGattReplyError(BluetoothReplyRunnable* aRunnable, > + const enum BluetoothGattStatus aGattStatus, > + const nsAString& aErrorStr); nit: Suggest to remove GATT related wordings to sync with other dispatch functions. |BluetoothGattStatus| is just one field of |BluetoothReplyError|, and this function handles general |BluetoothReplyRunnable|. @@ +390,5 @@ > + * [1]: https://webbluetoothcg.github.io/web-bluetooth/#error-handling > + */ > +void GattStatustoDOMStatus(const BluetoothGattStatus aGattStatus, > + nsresult& aDOMStatus, > + const bool IsWrite = false); Regarding to the re-usability of this function, it might not be used outside |BluetoothGattReplyRunnable|. Suggest to implement it directly into |BluetoothGattReplyRunnable| for clarity. If it is implemented inside the runnable, probably we have chances to avoid passing boolean parameters (i.e. |IsWrite|, and probably |IsLong| in the near future?) for different operations on the argument list. ::: dom/bluetooth/ipc/BluetoothTypes.ipdlh @@ +95,1 @@ > nsString errorString; nit: Suggest to put the newly added field at the end of the struct if there are no strong reasons to insert it in the middle of that struct.

Hi Bruce, Sorry for the late reply and thanks for your feedback. In this patch, I reduced the redundant code in |BluetoothGattReplyRunnable| by modifying part of member functions and variables from the private to the protected in |BluetoothReplyRunnable| class. The remaining modification and the reply for the comments are added inline below. (In reply to Bruce Sun [:brsun] from comment #2) > ::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp > @@ +3356,5 @@ > > DispatchReplySuccess(runnable, BluetoothValue(value)); > > } else if (!client->mReadCharacteristicState.mAuthRetry && > > (aStatus == GATT_STATUS_INSUFFICIENT_AUTHENTICATION || > > + aStatus == GATT_STATUS_INSUFFICIENT_ENCRYPTION || > > + aStatus == GATT_STATUS_INSUFFICIENT_ENCRYPTION_KEY_SIZE)) { > > This modification would impact the current behavior. Suggest to leave this > change to another follow-up bug. Probably we could compare the > implementation of Android[1][2][3][4] on that follow-up bug as well to make > sure our codes work correctly after applying this change. Sure, I removed all the modifications in |BluetoothGattManager.cpp| and added "TODO" comments in |BluetoothReplyRunnable|. > ::: dom/bluetooth/common/BluetoothGattReplyRunnable.cpp > @@ +152,5 @@ > > + const BluetoothGattStatus\ > > + aErrorGattStatus) > > +{ > > + mErrorString = aErrorString; > > + GattStatustoDOMStatus(aErrorGattStatus, mErrorDOMStatus, true); > Here are some possible alternatives to get rid of it: > - Determine whether to assign |InvalidModificationError| before calling > |GattStatustoDOMStatus()|. Let |GattStatustoDOMStatus()| always assign > |NotSupportedError| while encountering |Application Error|. > - Adding one virtual function, for example |bool IsWriteProcedure()|, on > |BluetoothGattReplyRunnable| and simply return |false|. Override > |IsWriteProcedure()| at |BluetoothGattWriteVoidReplyRunnable| and simply > return |true|. And then our error mapping logic can directly check on the > boolean value returned by this virtual function. > > Any other clever way to avoid boolean parameters is encouraged. OK, I removed the argument and added a private member function, called |IsWrite()|, to let the error mapping function to decide whether is write type or not in BluetoothGattReplyRunnable class. > ::: dom/bluetooth/common/BluetoothGattReplyRunnable.h > @@ +32,5 @@ > > + > > + virtual void ReleaseMembers(); > > + > > + void SetError(const nsAString& aErrorString, > > + const enum BluetoothStatus aErrorStatus = STATUS_FAIL) > > I guess you need a |virtual| keyword |SetError| on the base class (i.e. > |BluetoothReplyRunnable|), no? > > @@ +39,5 @@ > > + mErrorStatus = aErrorStatus; > > + mIsGattError = false; > > + } > > + virtual void SetError(const nsAString& aErrorString, > > + const BluetoothGattStatus aErrorGattStatus); > How about we just make |BluetoothReplyRunnable::SetError()| be a virtual > function, and make it accept three arguments (i.e. error string, bluetooth > status, gatt status)? So that we probably can save the burden of almost > duplicating the whole base class here. Since I have alreadt created another class(|BluetoothGattReplyRunnable|) for dealing with W3C error mapping problem, I would like to keep it in the |BluetoothGattReplyRunnable| for specialization rather than handle it in |BluetoothReplyRunnable|. However, I changed the function name to |SetDOMError| for avoiding name hiding problem (Another |SetError()| has inherited from |BluetoothReplyRunnable|). > ::: dom/bluetooth/common/BluetoothUtils.cpp > @@ +711,5 @@ > > > > // Reply will be deleted by the runnable after running on main thread > > BluetoothReply* reply = > > + new BluetoothReply(BluetoothReplyError(STATUS_FAIL, > > + static_cast<BluetoothGattStatus>(0), > > Please avoid using any magic number as much as possible (i.e. |0| in this > case.) For avoiding these magic numbers, I change my modification in |BluetoothReply.ipdlh|. I union the error status, since we only need one kind of error status in a reply. Thus, we don't need to set useless parameter in |BluetoothReplyError|. > @@ +823,5 @@ > > + * Since the current Gecko does not have such timeout mechenism and cannot > > + * get the information about ATT bearer, put it into TODO. > > + */ > > + > > + aDOMStatus = NS_ERROR_DOM_NOT_SUPPORTED_ERR; > nit: |aDOMStatus| would be assigned values twice in most cases. Although > this probably is not the bottleneck of the performance, suggest to consider > how the logic could be refined to reduce redundant assignment without > breaking the correct flow and the readability. Probably assign the default > value at the default case of the switch statement would be one possible > choice. Any other alternative to refine it would be appreciated as well. Sure. > ::: dom/bluetooth/common/BluetoothUtils.h > @@ +337,5 @@ > > + */ > > +void > > +DispatchGattReplyError(BluetoothReplyRunnable* aRunnable, > > + const enum BluetoothStatus aStatus, > > + const nsAString& aErrorStr); > > Could this function be used in non-GATT cases? Suggest to remove GATT > related wordings from the function name and comments if this function can be > used in general cases. OK. Thanks Tom

Comment on attachment 8719690 [details] [diff] [review] Bug 1238842 - Add error codes to Gecko by following W3C spec - dom/bluetooth/common & dom/binding/ipc change - v2 Review of attachment 8719690 [details] [diff] [review]: ----------------------------------------------------------------- Hi Tom, Basically the logic is good. But there are still some duplicated codes in it. Please refer to comments inline below. ::: dom/bluetooth/common/BluetoothGattReplyRunnable.cpp @@ +10,5 @@ > +#include "DOMRequest.h" > +#include "mozilla/dom/ScriptSettings.h" > +#include "mozilla/dom/bluetooth/BluetoothTypes.h" > +#include "mozilla/dom/Promise.h" > +#include "nsServiceManagerUtils.h" If |nsIDOMDOMRequest| is forward declared, is "DOMRequest.h" still a necessary header for |BluetoothGattReplyRunnable|? Suggest to refine the included files to see if we can reduce the list. @@ +25,5 @@ > +{ > +} > + > +void > +BluetoothGattReplyRunnable::GattStatustoDOMStatus(const BluetoothGattStatus\ Except for macros, using a back slash for line continuation is seldom in our code base. Please put the whole |const BluetoothGattStatus aGattStatus| on a new line. nit: GattStatusToDOMStatus @@ +44,5 @@ > + aDOMStatus = NS_ERROR_DOM_INVALID_MODIFICATION_ERR; > + break; > + case GATT_STATUS_ATTRIBUTE_NOT_LONG: > + /** > + * TODO : nit: Currently there are more than one style for TODO items in this patch (ex. |TODO :| with immediate line break, |ToDo:| with continuous comments, etc.) Would you mind syncing the style for TODO comments? @@ +49,5 @@ > + * While receiving |GATT_STATUS_ATTRIBUTE_NOT_LONG|, we need to retry the > + * step without using 'Long' sub-procedure (e.g. Read Blob Request) based on > + * W3C reuqirements. If it fails again, convert the error status to > + * |NS_ERROR_DOM_INVALID_MODIFICATION_ERR| > + * else |NS_ERROR_DOM_NOT_SUPPORTED_ERR|. nit: The sentience is a little bit weird. Any words missing before the word |else|? @@ +50,5 @@ > + * step without using 'Long' sub-procedure (e.g. Read Blob Request) based on > + * W3C reuqirements. If it fails again, convert the error status to > + * |NS_ERROR_DOM_INVALID_MODIFICATION_ERR| > + * else |NS_ERROR_DOM_NOT_SUPPORTED_ERR|. > + * Since the Gecko cannot decide to use the specific type of Read Request, nit: decide |what| to use? decide |how| to use? or other suitable words? @@ +52,5 @@ > + * |NS_ERROR_DOM_INVALID_MODIFICATION_ERR| > + * else |NS_ERROR_DOM_NOT_SUPPORTED_ERR|. > + * Since the Gecko cannot decide to use the specific type of Read Request, > + * convert all conditions into |NOT_SUPPORTED| and take the other error code > + * convert be the TODO. nit: This sentience is a little bit weird. Would you minding rephrasing it again? @@ +53,5 @@ > + * else |NS_ERROR_DOM_NOT_SUPPORTED_ERR|. > + * Since the Gecko cannot decide to use the specific type of Read Request, > + * convert all conditions into |NOT_SUPPORTED| and take the other error code > + * convert be the TODO. > + */ Should |aDOMStatus| be assigned to a valid value before breaking this switch case? @@ +61,5 @@ > + case GATT_STATUS_INSUFFICIENT_ENCRYPTION_KEY_SIZE: > + /** > + * In W3C requirement, UA SHOULD attempt to increase the security level of > + * the connection while receiving those error statuses. If it fails or UA > + * doesn't suuport return the SECURITY ERROR. nit: Suggest to add one empty line here if there would be two paragraphs. @@ +66,5 @@ > + * ToDo: Since we have handled retry as |GATT_STATUS_INSUFFICIENT_AUTHENTICATION| > + * and |GATT_STATUS_INSUFFICIENT_ENCRYPTION| happened in |BluetoothGattManager|, > + * we should handle retry as |GATT_STATUS_INSUFFICIENT_ENCRYPTION_KEY_SIZE|, too. > + * Finally, take all of them have done the retry and only translate > + * error messages to |NS_ERROR_DOM_SECURITY_ERR| here. nit: Is the sentience, |Finally ... here.|, part of TODO items? Suppose we have done it in this patch, right? @@ +75,5 @@ > + aDOMStatus = NS_ERROR_DOM_SECURITY_ERR; > + break; > + default: > + // Check whether 'aGattStatus' is the Application Error or not > + if ((aGattStatus >= 0x80 && aGattStatus <= 0x9f) && IsWrite()) { 0x80 and 0x9f are magic numbers. Please use some pre-defined values or enums for them. @@ +81,5 @@ > + } else { > + aDOMStatus = NS_ERROR_DOM_NOT_SUPPORTED_ERR; > + } > + break; > + } After comparing the switch cases in this patch with the error mapping of the spec[1], I found that it is not very intuitive to verify whether our error mapping is exactly the same as the spec. Suggest to explicitly list all error cases which are explicitly listed on the spec. Probably using |if| statement here is more suitable than |switch| statement here because the values for application errors are not a single value. [1] https://webbluetoothcg.github.io/web-bluetooth/#error-handling @@ +86,5 @@ > +} > + > +void > +BluetoothGattReplyRunnable::SetDOMError(const nsAString& aErrorString, > + const BluetoothGattStatus\ Ditto. Avoid using the back slash for line continuation here. @@ +94,5 @@ > + GattStatustoDOMStatus(aErrorGattStatus, mErrorDOMStatus); > +} > + > +nsresult > +BluetoothGattReplyRunnable::FireErrorString() There are many duplicated logics here. Suggest to reuse what base class already have as many as we can. For example, we initialize |mErrorDOMStatus| as |NS_OK| during the constructor, and then we only handle GATT specific cases here: ================================================================================ if (!mPromise || NS_SUCCEEDED(mErrorDOMStatus)) { return BluetoothReplyRunnable::FireErrorString(); } /* do GATT specific handling */ ================================================================================ The above example could be refined further, but the idea is to reuse what we already have. Prefer DRY over WET[1]. [1] https://en.wikipedia.org/wiki/Don%27t_repeat_yourself @@ +124,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +BluetoothGattReplyRunnable::Run() Ditto. Suggest to reuse what we already have. For example: ================================================================================ if (mReply->type() != BluetoothReply::TBluetoothReplyError || mReply->get_BluetoothReplyError().errorStatus().type() != BluetoothErrorStatus::TBluetoothGattStatus) { return BluetoothReplyRunnable::Run(); } /* do GATT specific handling */ ================================================================================ Again, the above example could be refined further, but the idea is to reuse what we already have. Probably retrieving the first block of |if| statement into one virtual function could be another solution. So that we don't have to call |ParseSuccessfulReply()|, |FireErrorString()|, |FireReplySuccess()|, |ReleaseMembers()|, etc. here. @@ +133,5 @@ > + AutoSafeJSContext cx; > + JS::Rooted<JS::Value> v(cx, JS::UndefinedValue()); > + > + nsresult rv; > + if (BluetoothReplyRunnable::mReply->type() == |BluetoothReplyRunnable::| prefix here seems redundant? ::: dom/bluetooth/common/BluetoothGattReplyRunnable.h @@ +26,5 @@ > +public: > + NS_DECL_NSIRUNNABLE > + > + BluetoothGattReplyRunnable(nsIDOMDOMRequest* aReq, > + Promise* aPromise = nullptr); The parameter of the constructor is not so aligned with our usage: - Suppose there are no chances to use |BluetoothGattReplyRunnable| with |nsIDOMDOMRequest|, suggest to remove it from the parameter list. - Suppose |Promise| would be needed in (almost?) all cases, using |nullptr| as the default parameter doesn't save too much typing time while using this runnable. Suggest to remove the default value |nullptr| of |aPromise|. @@ +34,5 @@ > + > +protected: > + virtual ~BluetoothGattReplyRunnable() {} > + > + void GattStatustoDOMStatus(const BluetoothGattStatus aGattStatus, nit: GattStatusToDOMStatus(...) @@ +43,5 @@ > + return false; > + } > + > + nsresult mErrorDOMStatus; > + bool mIsGattError; Suppose this runnable is specialized for GATT only, should we still need |mIsGattError| for checking something if we can reuse the base class for all general stuffs? @@ +47,5 @@ > + bool mIsGattError; > + > +private: > + virtual void OnSuccessFired() {} > + virtual void OnErrorFired() {} The implementation of |OnSuccessFired| and |OnErrorFired| are the same as the base class. Suggest to override specialized functions only when necessary. @@ +49,5 @@ > +private: > + virtual void OnSuccessFired() {} > + virtual void OnErrorFired() {} > + > + virtual nsresult FireErrorString(); |FireErrorString| at |BluetoothReplyRunnable| is not virtual. Shouldn't it be virtual at the base class? @@ +56,5 @@ > +class BluetoothGattVoidReplyRunnable : public BluetoothGattReplyRunnable > +{ > +public: > + BluetoothGattVoidReplyRunnable(nsIDOMDOMRequest* aReq, > + Promise* aPromise = nullptr) Ditto. The same comments for the parameter list of the constructor as |BluetoothGattReplyRunnable|. @@ +69,5 @@ > + return true; > + } > +}; > + > +class BluetoothGattWriteVoidReplyRunnable : public BluetoothGattVoidReplyRunnable This runnable probably is not a common use case for using W3C APIs. Suggest not to put this runnable in this file, and suggest to let this runnable be implemented only after the corresponding write function has been implemented. @@ +73,5 @@ > +class BluetoothGattWriteVoidReplyRunnable : public BluetoothGattVoidReplyRunnable > +{ > +public: > + BluetoothGattWriteVoidReplyRunnable(nsIDOMDOMRequest* aReq, > + Promise* aPromise = nullptr) Ditto. The same comments for the parameter list of the constructor as |BluetoothGattReplyRunnable|. ::: dom/bluetooth/common/BluetoothReplyRunnable.h @@ +34,5 @@ > > void SetReply(BluetoothReply* aReply); > > + virtual void SetError(const nsAString& aErrorString, > + const enum BluetoothStatus aErrorStatus = STATUS_FAIL) The default parameter value of a virtual function is not inherited by derived classes. How the default parameter value works solely depends on the static type of the pointer/reference. Suggest to remove the default value (i.e. STATUS_FAIL) of the virtual function, or to leave the original function untouched as non-virtual one and create one another virtual function for your usage.

Hi Bruce, Thanks for your comments! I revised my patch based on them. In this patch, I removed the |BluetoothGattWriteVoidReplyRunnable|, rewrote the comments on |GattStatusToDOMStatus(...)|, and removed the redundant code. Please help me to review the patch again. Thanks, Tom

Comment on attachment 8721140 [details] [diff] [review] Bug 1238842 - Add error codes to Gecko by following W3C spec - dom/bluetooth/common & dom/binding/ipc change - v3 Review of attachment 8721140 [details] [diff] [review]: ----------------------------------------------------------------- Hi Tom, Thanks for your great efforts. Your patch is getting more and more elegant. Kindly refer to the comments inline below. ::: dom/bluetooth/common/BluetoothGattReplyRunnable.cpp @@ +42,5 @@ > + aDOMStatus = NS_ERROR_DOM_INVALID_MODIFICATION_ERR; > + break; > + case GATT_STATUS_ATTRIBUTE_NOT_LONG: > + /** > + * ToDo : nit: |ToDo:| or |ToDo :|? Either one is OK, but syncing the style would be good. @@ +44,5 @@ > + case GATT_STATUS_ATTRIBUTE_NOT_LONG: > + /** > + * ToDo : > + * While receiving |GATT_STATUS_ATTRIBUTE_NOT_LONG|, we need to check > + * whether having used 'Long' sub-procedure or not. nit: |whether 'Long' sub-procedure has been used or not.| @@ +47,5 @@ > + * While receiving |GATT_STATUS_ATTRIBUTE_NOT_LONG|, we need to check > + * whether having used 'Long' sub-procedure or not. > + * If we have used 'Long' sub-procedure, we need to retry the step > + * without using 'Long' sub-procedure (e.g. Read Blob Request) based on > + * W3C reuqirements. If it fails again, convert the error status to nit: From the spec, the statement would be something like |...If it fails again due to the length of the value being written, convert...|. @@ +49,5 @@ > + * If we have used 'Long' sub-procedure, we need to retry the step > + * without using 'Long' sub-procedure (e.g. Read Blob Request) based on > + * W3C reuqirements. If it fails again, convert the error status to > + * |NS_ERROR_DOM_INVALID_MODIFICATION_ERR|. > + * If we have not used 'Long' sub-procedure, switch the error status to nit: s/switch/convert/ @@ +73,5 @@ > + break; > + case GATT_STATUS_INSUFFICIENT_AUTHORIZATION: > + aDOMStatus = NS_ERROR_DOM_SECURITY_ERR; > + break; > + default: Please add all cases which are explicitly list on the W3C spec (ex. 'Invalid Handle', 'Invalid PDU', 'Read Not Permitted', 'Write Not Permitted', etc). Those cases could use the same |aDOMStatus| assignment below for simplicity, but the default case here should only represent 'Anything else' to me. @@ +93,5 @@ > +{ > + MOZ_ASSERT(mReply); > + > + if (!(mPromise && (mReply->get_BluetoothReplyError().errorStatus().type() == > + BluetoothErrorStatus::TBluetoothGattStatus))) { Although |!(a && b)| is the same as |!a || !b| by De Morgan's laws, I would suggest using |if (!mPromise || !(...))| here. So that we can have a clear and intuitive idea that the rest of the codes below don't need to check the value of |mPromise| again. @@ +98,5 @@ > + return BluetoothReplyRunnable::FireErrorString(); > + } > + > + // Promise > + if (mPromise) { Suppose |mPromise| should always have a non-nullptr value here. Right? @@ +100,5 @@ > + > + // Promise > + if (mPromise) { > + nsresult rv = NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM, > + mErrorDOMStatus); After looking at the refinement of the patch, one curious thing come up to my mind. Do we really need |SetDOMError()| and |mErrorDOMStatus| in |BluetoothGattReplyRunnable|? I am not sure the re-usability of this member function and this member data. Why should we calculate the |nsresult| in advance and save it to |mErrorDOMStatus|? Could we call |GattStatusToDOMStatus()| directly inside |FireErrorString()| to generate a proper |nsresult| value on-demand? If we can calculate the value only when necessary, probably we can further save the burden of using |ParseErrorStatus()| and keep more codes in |BluetoothReplyRunnable| untouched. @@ +119,5 @@ > + return; > + } > + > + SetDOMError(mReply->get_BluetoothReplyError().errorString(), > + mReply->get_BluetoothReplyError().errorStatus()); Ditto. Shouldn't |errorStatus()->get_BluetoothGattStatus()| be used? ::: dom/bluetooth/common/BluetoothGattReplyRunnable.h @@ +26,5 @@ > +public: > + BluetoothGattReplyRunnable(Promise* aPromise); > + > + virtual void SetDOMError(const nsAString& aErrorString, > + const BluetoothGattStatus aErrorGattStatus); |SetDOMError()| is declared as |public| and |virtual|. What would be the use cases to call/override this function from other place? @@ +31,5 @@ > + > +protected: > + virtual ~BluetoothGattReplyRunnable() {} > + > + virtual void ParseErrorStatus(); Add |override| keyword on this function. @@ +35,5 @@ > + virtual void ParseErrorStatus(); > + > + void GattStatusToDOMStatus(const BluetoothGattStatus aGattStatus, > + nsresult& aDOMStatus); > + nsresult FireErrorString(); Although |FireErrorString()| is a virtual function due to the declaration in the base class, adding |virtual| keyword here would make codes more clear. Please also add |override| keyword on this function as well. ::: dom/bluetooth/common/BluetoothReplyRunnable.cpp @@ +133,5 @@ > +{ > + MOZ_ASSERT(mReply); > + > + SetError(mReply->get_BluetoothReplyError().errorString(), > + mReply->get_BluetoothReplyError().errorStatus()); My blindly guess is that |errorStatus()->get_BluetoothStatus()| would be necessary. Could |errorStatus()| be used here directly? ::: dom/bluetooth/common/BluetoothReplyRunnable.h @@ +49,5 @@ > virtual bool ParseSuccessfulReply(JS::MutableHandle<JS::Value> aValue) = 0; > > + virtual void ParseErrorStatus(); > + > + nsresult FireReplySuccess(JS::Handle<JS::Value> aVal); Does |FireReplySuccess| need to be declared as |protected| in our cases? @@ +73,5 @@ > nsString mErrorString; > + > +private: > + virtual void OnSuccessFired(); > + virtual void OnErrorFired(); nit: In order to sync the coding style of this file, putting function declarations before member data declarations would be better.

Hi Bruce, Thanks for your suggestions. I learned a lot from them. In this patch, I removed useless codes(|DispatchReplyError(aRunnable, aStatus, aErrorStr)|, |SetDOMError()|), modified comments at |GattStatusToDOMStatus()| in |BluetoothGattReplyRunnable.cpp| and dealt with conditions that has not been concerned before. The other modification and comments for reply are added inline below. (In reply to Bruce Sun [:brsun] from comment #6) > @@ +100,5 @@ > > + > > + // Promise > > + if (mPromise) { > > + nsresult rv = NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM, > > + mErrorDOMStatus); > After looking at the refinement of the patch, one curious thing come up to > my mind. Do we really need |SetDOMError()| and |mErrorDOMStatus| in > |BluetoothGattReplyRunnable|? I am not sure the re-usability of this member > function and this member data. Why should we calculate the |nsresult| in > advance and save it to |mErrorDOMStatus|? Could we call > |GattStatusToDOMStatus()| directly inside |FireErrorString()| to generate a > proper |nsresult| value on-demand? If we can calculate the value only when > necessary, probably we can further save the burden of using > |ParseErrorStatus()| and keep more codes in |BluetoothReplyRunnable| > untouched. Yes, we don't need |SetDOMError()| and |mErrorDOMStatus|. I removed them in this patch. However, one of purpose of the |ParseErrorStatus()| in |BluetoothGattReplyRunnable| is to call the function in base class when it's not Promise or type is incorrect. Thus, we cannot remove it unless we can find other way to handle it. > @@ +119,5 @@ > > + return; > > + } > > + > > + SetDOMError(mReply->get_BluetoothReplyError().errorString(), > > + mReply->get_BluetoothReplyError().errorStatus()); > > Ditto. Shouldn't |errorStatus()->get_BluetoothGattStatus()| be used? It should be |errorStatus().get_BluetoothGattStatus()|. It isn't necessary to exist, but having it makes the codes much readable. I addressed it in this patch. > ::: dom/bluetooth/common/BluetoothReplyRunnable.h > @@ +49,5 @@ > > virtual bool ParseSuccessfulReply(JS::MutableHandle<JS::Value> aValue) = 0; > > > > + virtual void ParseErrorStatus(); > > + > > + nsresult FireReplySuccess(JS::Handle<JS::Value> aVal); > > Does |FireReplySuccess| need to be declared as |protected| in our cases? > No, sorry for that. I declared it as |private| in this patch. Thanks, Tom

Comment on attachment 8721940 [details] [diff] [review] Bug 1238842 - Add error codes to Gecko by following W3C spec - dom/bluetooth/common & dom/binding/ipc change - v4 Review of attachment 8721940 [details] [diff] [review]: ----------------------------------------------------------------- Hi Tom, Thanks for your great efforts, some comments are added inline below. Pretty much the patch is good enough, but I would like to check the final version again if you don't mind. Kindly help to address the suggestions if they make sense to you. ::: dom/bluetooth/common/BluetoothGattReplyRunnable.cpp @@ +92,5 @@ > +BluetoothGattReplyRunnable::FireErrorString() > +{ > + MOZ_ASSERT(mReply); > + > + if (!mPromise || (mReply->get_BluetoothReplyError().errorStatus().type() != You'd better check |mReply.type() == BluetoothReply::TBluetoothReplyError| here before calling |mReply->get_BluetoothReplyError()|. Otherwise we might have trouble if |FireErrorString()| is called due to |ParseSuccessulReply()| fails. @@ +108,5 @@ > + return NS_OK; > +} > + > +void > +BluetoothGattReplyRunnable::ParseErrorStatus() nit: Suggest not to override this function if there is no special handling for GATT cases. Suppose the current implementation of |BluetoothReplyRunnable::ParseErrorStatus()| could do the job well, right? ::: dom/bluetooth/common/BluetoothGattReplyRunnable.h @@ +39,5 @@ > + { > + return false; > + } > + > + nsresult mErrorDOMStatus; Suggest to remove |mErrorDOMStatus| if it isn't used anymore. ::: dom/bluetooth/common/BluetoothReplyRunnable.h @@ +69,5 @@ > BluetoothStatus mErrorStatus; > nsString mErrorString; > + > +private: > + virtual bool ParseSuccessfulReply(JS::MutableHandle<JS::Value> aValue) = 0; Some private member data are modified as protected. Some protected member function are modified as private. Although |FireErrorString| should be changed from private to protected, I am not sure whether others are mandatory to solve this bug or not. Suggest not to touch unrelated codes if it is not necessary for the bug. It would make maintainers more easily to understand the evolution of our codes by using one single bug to address one single issue only.

Hi Bruce, Thanks for your review and suggestions. I addressed them in this patch. Could you help me to review this patch again? Thanks, Tom

Comment on attachment 8722372 [details] [diff] [review] Bug 1238842 - Add error codes to Gecko by following W3C spec - dom/bluetooth/common & dom/binding/ipc change - v5 Review of attachment 8722372 [details] [diff] [review]: ----------------------------------------------------------------- Hi Tom, Thanks for the efforts. r=me if the following comments would be addressed. ::: dom/bluetooth/common/BluetoothGattReplyRunnable.cpp @@ +98,5 @@ > + BluetoothErrorStatus::TBluetoothGattStatus) { > + return BluetoothReplyRunnable::FireErrorString(); > + } > + > + BluetoothGattStatus aGattError = nit: |aGattError| seems not a suitable name for a local variable. The 'a' prefix is for arguments of functions, not for local variables. How about |status| or |gattStatus| or some name else? ::: dom/bluetooth/common/BluetoothGattReplyRunnable.h @@ +31,5 @@ > + > +private: > + virtual nsresult FireErrorString() override; > + > + nsresult GattStatusToDOMStatus(const BluetoothGattStatus aGattStatus); The return type |nsresult| is confusing. Probably maintainers would suspect that this function would return |NS_OK|/|NS_FAIL| when the conversion succeeds/fails by simple glancing on its prototype. Suggest to use the original design to have one input parameter and one output parameter for the conversion function. @@ +43,5 @@ > +class BluetoothGattVoidReplyRunnable : public BluetoothGattReplyRunnable > +{ > +public: > + BluetoothGattVoidReplyRunnable(Promise* aPromise) > + : BluetoothGattReplyRunnable(aPromise) {} nit: Indentation of the initializer list[1]. [1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes ::: dom/bluetooth/common/BluetoothReplyRunnable.cpp @@ +128,5 @@ > {} > +void > +BluetoothReplyRunnable::ParseErrorStatus() > +{ > + MOZ_ASSERT(mReply); nit: Use assertion on the type of |mReply| if this function couldn't work well due to unexpected types. ::: dom/bluetooth/common/BluetoothReplyRunnable.h @@ +55,2 @@ > private: > + void ParseErrorStatus(); nit: If this function is override-able by design, let it be a virtual function.

Thanks for the review, Bruce. I addressed review comments in this patch. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7548f3b465c&filter-tier=1&filter-tier=2&filter-tier=3

Comment on attachment 8722888 [details] [diff] [review] [Final]Bug 1238842 - Add error codes to Gecko by following W3C spec. r=brsun Review of attachment 8722888 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/common/BluetoothGattReplyRunnable.cpp @@ +111,5 @@ > + domStatus); > + > + nsresult rv = > + NS_ERROR_GENERATE_FAILURE( > + NS_ERROR_MODULE_DOM, domStatus); nit: Could these lines be merged into one single line without exceeding 80-character boundary? ::: dom/bluetooth/common/BluetoothGattReplyRunnable.h @@ +45,5 @@ > +{ > +public: > + BluetoothGattVoidReplyRunnable(Promise* aPromise) > + : BluetoothGattReplyRunnable(aPromise) {} > + ~BluetoothGattVoidReplyRunnable() {} nit: Indentation with two spaces.

(In reply to Bruce Sun [:brsun] from comment #12) Sorry for that, I fixed them in this patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb50d4d434bfbb237390f7f78261123a17fafa2a Bug 1238842 - Add error codes to Gecko by following W3C spec. r=brsun