1140259 - [B2G][RIL] Handle new error codes introduced in RIL version 10

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar]]

+++ This bug was initially created as a follow-up of Bug #1113054 +++

There are some new error codes introduced in version 10 [1], we should map them into proper error string.

[1] https://github.com/android/platform_hardware_ril/blob/lollipop-release/include/telephony/ril.h#L78-L103

Comment 1

[Edgar Chen [:edgar]]

ril_consts contains some error codes which are not lived in standard AOSP ril.h [1]. I guess they are partner's specific errors.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_consts.js?from=ril_consts.js&case=true#232-243

Comment 2

[Edgar Chen [:edgar]]

Attachment: Part 1: RIL error for version 10, v1

Comment 3

[Edgar Chen [:edgar]]

Attachment: Part 2: [RIL] Handle options.errorMsg in processParcel() and map unknown error code to "UnspecifiedError", v1

Comment 4

[Edgar Chen [:edgar]]

Attachment: Part 2: [RIL] Handle options.errorMsg in processParcel() and map unknown error code to "UnspecifiedError", v2

Comment 5

[Edgar Chen [:edgar]]

Attachment: Part 2: [RIL] Handle options.errorMsg in processParcel() and map unknown error code to "UnspecifiedError", v3

Comment 6

[Edgar Chen [:edgar]]

Comment on attachment 8575188 [details] [diff] [review]
Part 1: RIL error for version 10, v1

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

In this part, I add new error code introduced in ril v10 (note that error code 16 was already added in bug 1139063).
And also remove the partner's specific error code given that we didn't use them actually and they are conflicted with standard error code.

Hi Hinsyi, may I have your review? Thank you.

Comment 7

[Edgar Chen [:edgar]]

Comment on attachment 8576478 [details] [diff] [review]
Part 2: [RIL] Handle options.errorMsg in processParcel() and map unknown error code to "UnspecifiedError", v3

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

Changes in this part:
1. Handle errorMsg in proccessParcel().
2. Map unknown error code to "UnspecifiedError".

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8576478 [details] [diff] [review]
Part 2: [RIL] Handle options.errorMsg in processParcel() and map unknown error code to "UnspecifiedError", v3

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

r=me with comments addressed, thank you!

::: dom/system/gonk/ril_worker.js
@@ +5718,1 @@
>    if ((!options.success || this.IMEI == null) && !options.errorMsg) {

The condition is dazzling. We could simplify it as |if (this.IMEI == null && options.success)|. 
Since we are here, please modify it, too.

@@ +5834,5 @@
>  
>    this._updateNetworkSelectionMode(selectionMode);
>  };
>  RilObject.prototype[REQUEST_SET_NETWORK_SELECTION_AUTOMATIC] = function REQUEST_SET_NETWORK_SELECTION_AUTOMATIC(length, options) {
> +  if (!options.errorMsg) {

In most cases, we still use "options.rilRequestError." Can we keep using that term? 
Alternatively, I will also be okay if we replace all existing "options.rilRequestError" with "options.errorMsg." Just want to have consistency.

@@ +5841,4 @@
>    this.sendChromeMessage(options);
>  };
>  RilObject.prototype[REQUEST_SET_NETWORK_SELECTION_MANUAL] = function REQUEST_SET_NETWORK_SELECTION_MANUAL(length, options) {
> +  if (!options.errorMsg) {

ditto.

@@ +5847,4 @@
>    this.sendChromeMessage(options);
>  };
>  RilObject.prototype[REQUEST_QUERY_AVAILABLE_NETWORKS] = function REQUEST_QUERY_AVAILABLE_NETWORKS(length, options) {
> +  if (!options.errorMsg) {

ditto.

Comment 9

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> Comment on attachment 8576478 [details] [diff] [review]
> Part 2: [RIL] Handle options.errorMsg in processParcel() and map unknown
> error code to "UnspecifiedError", v3
> 
> Review of attachment 8576478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed, thank you!
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +5718,1 @@
> >    if ((!options.success || this.IMEI == null) && !options.errorMsg) {
> 
> The condition is dazzling. We could simplify it as |if (this.IMEI == null &&
> options.success)|. 
> Since we are here, please modify it, too.

Okay, will do in next version.

> 
> @@ +5834,5 @@
> >  
> >    this._updateNetworkSelectionMode(selectionMode);
> >  };
> >  RilObject.prototype[REQUEST_SET_NETWORK_SELECTION_AUTOMATIC] = function REQUEST_SET_NETWORK_SELECTION_AUTOMATIC(length, options) {
> > +  if (!options.errorMsg) {
> 
> In most cases, we still use "options.rilRequestError." Can we keep using
> that term? 
> Alternatively, I will also be okay if we replace all existing
> "options.rilRequestError" with "options.errorMsg." Just want to have
> consistency.

Agree with you, I will use |rilRequestError| in this bug to have consistency. 
In fact, I already have a patch to deprecate all rilRequestError usage in bug 991582. Let's replace |rilRequestError| with |errorMsg| there.
Thank you.

Comment 10

[Edgar Chen [:edgar]]

Attachment: Part 2: [RIL] Handle options.errorMsg in processParcel() and map unknown error code to "UnspecifiedError", v4, r=hsinyi

Address review comment #8.
- if (this.IMEI == null && options.success)
- Using |rilRequestError| to have consistency.

Comment 11

[Edgar Chen [:edgar]]

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f65ad3d67786

Comment 12

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/rev/5cf436417378
https://hg.mozilla.org/integration/b2g-inbound/rev/5825affccf38

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5cf436417378
https://hg.mozilla.org/mozilla-central/rev/5825affccf38