Last Comment Bug 774114 - B2G RIL: Refactor the way of handling RIL_REQUEST_{ENTER_*, CHANGE_*} responses
: B2G RIL: Refactor the way of handling RIL_REQUEST_{ENTER_*, CHANGE_*} responses
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla17
Assigned To: José Antonio Olivera Ortega [:jaoo]
:
:
Mentors:
Depends on: 780063
Blocks: 772357
  Show dependency treegraph
 
Reported: 2012-07-15 13:54 PDT by José Antonio Olivera Ortega [:jaoo]
Modified: 2012-08-17 04:28 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP v1 (9.65 KB, patch)
2012-07-15 14:13 PDT, José Antonio Olivera Ortega [:jaoo]
philipp: feedback+
Details | Diff | Splinter Review
Error log (1.60 KB, text/plain)
2012-08-01 06:45 PDT, José Antonio Olivera Ortega [:jaoo]
allstars.chh: feedback+
Details
WIP (14.87 KB, patch)
2012-08-07 14:22 PDT, José Antonio Olivera Ortega [:jaoo]
no flags Details | Diff | Splinter Review
v1 (16.05 KB, patch)
2012-08-09 03:23 PDT, José Antonio Olivera Ortega [:jaoo]
no flags Details | Diff | Splinter Review
v2 (17.23 KB, patch)
2012-08-10 06:45 PDT, José Antonio Olivera Ortega [:jaoo]
philipp: feedback+
Details | Diff | Splinter Review
v3 (19.32 KB, patch)
2012-08-13 04:34 PDT, José Antonio Olivera Ortega [:jaoo]
philipp: feedback+
Details | Diff | Splinter Review
v4 (19.46 KB, patch)
2012-08-13 15:14 PDT, José Antonio Olivera Ortega [:jaoo]
philipp: review+
Details | Diff | Splinter Review
v5 (18.94 KB, patch)
2012-08-14 10:22 PDT, José Antonio Olivera Ortega [:jaoo]
philipp: checkin+
Details | Diff | Splinter Review

Description José Antonio Olivera Ortega [:jaoo] 2012-07-15 13:54:55 PDT
Here we have to deal with:
- Reuse the `options` object.
- Improve the error handling for those RIL request types.
- Rename s/result/sucess.

The need comes form comment #2 in bug 772357.
Comment 1 José Antonio Olivera Ortega [:jaoo] 2012-07-15 14:13:32 PDT
Created attachment 642418 [details] [diff] [review]
WIP v1

This is a WIP patch. It needs to be tested on a phone. Just need some feedback before continuing.
Comment 2 Philipp von Weitershausen [:philikon] 2012-07-16 15:16:58 PDT
Comment on attachment 642418 [details] [diff] [review]
WIP v1

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

Thanks! This is going in the right direction, but it's still missing an important piece to actually work (see below.)

::: dom/system/gonk/ril_consts.js
@@ +219,5 @@
>  const GECKO_ERROR_RADIO_NOT_AVAILABLE = "RadioNotAvailable";
>  const GECKO_ERROR_GENERIC_FAILURE = "GenericFailure";
>  const GECKO_ERROR_REQUEST_NOT_SUPPORTED = "RequestNotSupported";
>  const GECKO_ERROR_ILLEGAL_SIM_OR_ME = "IllegalSIMorME";
> +const GECKO_ERROR_PASSWORD_INCORRECT = "PasswordIncorrect";

Nit: "IncorrectPassword" sounds better to me...

::: dom/system/gonk/ril_worker.js
@@ +1930,5 @@
> +   * Helper for processing responses of functions such as enterICC* and changeICC*.
> +   */
> +  _processEnterAndChangeICCResponses: function _processEnterAndChangeICCResponses(options) {
> +    options.success = options.rilRequestError == 0 ? true : false;
> +    if (options.success == false) {

if (!options.success)

(here and twice below)

@@ +2786,5 @@
>    if (DEBUG) debug("iccStatus: " + JSON.stringify(iccStatus));
>    this._processICCStatus(iccStatus);
>  };
>  RIL[REQUEST_ENTER_SIM_PIN] = function REQUEST_ENTER_SIM_PIN(length, options) {
> +  options.type = "iccunlockcardlock";

This won't work. The message.type going in will be something like "enterICCPIN" (see RadioInterfaceLayer::unlockCardLock). We need to synchronize the incoming and outgoing ones. I think the best would actually be to simplify the incoming ones in RadioInterfaceLayer::unlockCardLock by moving the switch() statement into ril_worker.js.
Comment 3 Kai-Chih Hu [:khu] 2012-07-18 22:49:04 PDT
Looks like Jaoo is working on this. So, just assign Jaoo as the owner. If Jaoo is not handling this case, please assign to nobody. Thanks!
Comment 4 Dietrich Ayala (:dietrich) 2012-07-31 13:29:02 PDT
Nice to have according to Philikon, so not blocking. Parent bug is blocking though.
Comment 5 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-07-31 23:53:56 PDT
Comment on attachment 642418 [details] [diff] [review]
WIP v1

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

::: dom/system/gonk/ril_consts.js
@@ +224,1 @@
>  

Hi, jaoo
On emulator, some ICC operations are not supported, so the error code would be ERROR_REQUEST_NOT_SUPPORTED. How do you think if you add another error message for that error code ERROR_REQUEST_NOT_SUPPORTED here?

Thanks
Comment 6 José Antonio Olivera Ortega [:jaoo] 2012-08-01 03:59:07 PDT
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #5)
> Comment on attachment 642418 [details] [diff] [review]
> WIP v1
> 
> Review of attachment 642418 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_consts.js
> @@ +224,1 @@
> >  
> 
> Hi, jaoo
> On emulator, some ICC operations are not supported, so the error code would
> be ERROR_REQUEST_NOT_SUPPORTED. How do you think if you add another error
> message for that error code ERROR_REQUEST_NOT_SUPPORTED here?
> 
> Thanks

That error code is already included (see [1]) in ril_const.js file. Please, let me know if I am misunderstanding what you mean.
Comment 7 José Antonio Olivera Ortega [:jaoo] 2012-08-01 03:59:53 PDT
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #6)
> (In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #5)
> > Comment on attachment 642418 [details] [diff] [review]
> > WIP v1
> > 
> > Review of attachment 642418 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/ril_consts.js
> > @@ +224,1 @@
> > >  
> > 
> > Hi, jaoo
> > On emulator, some ICC operations are not supported, so the error code would
> > be ERROR_REQUEST_NOT_SUPPORTED. How do you think if you add another error
> > message for that error code ERROR_REQUEST_NOT_SUPPORTED here?
> > 
> > Thanks
> 
> That error code is already included (see [1]) in ril_const.js file. Please,
> let me know if I am misunderstanding what you mean.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_consts.js#232
Comment 8 José Antonio Olivera Ortega [:jaoo] 2012-08-01 06:45:48 PDT
Created attachment 647929 [details]
Error log

Has you tested this functionality on otoro? It seems It does not work at all. I mean every time the user enters the PIN for unlocking the SIM the RIL returns error '6' that is RIL_E_REQUEST_NOT_SUPPORTED. That's strange because valid errors are SUCCESS, RADIO_NOT_AVAILABLE, GENERIC_FAILURE and PASSWORD_INCORRECT.
Comment 9 Dietrich Ayala (:dietrich) 2012-08-01 11:14:22 PDT
This bug was marked blocking-, because Philikon said it's a nice-to-have, which means it doesn't block the release, and shouldn't be worked on.

However, it's also blocking network depersonalization, which *is* blocking the release.

Yoshi and Jaoo, can you clarify whether this is required work for network depersonalization?
Comment 10 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-08-01 20:09:23 PDT
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #8)
> Created attachment 647929 [details]
> Error log
> 
> Has you tested this functionality on otoro? It seems It does not work at
> all. I mean every time the user enters the PIN for unlocking the SIM the RIL
> returns error '6' that is RIL_E_REQUEST_NOT_SUPPORTED. That's strange
> because valid errors are SUCCESS, RADIO_NOT_AVAILABLE, GENERIC_FAILURE and
> PASSWORD_INCORRECT.
Hi, jaoo
I'll test this on otoro, 
will reply you tomorrow.
Comment 11 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-08-01 20:12:37 PDT
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #6)
> That error code is already included (see [1]) in ril_const.js file. Please,
> let me know if I am misunderstanding what you mean.

Oh, yes
it's already in the code,
sorry for confusion.
Comment 12 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-08-02 19:57:49 PDT
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #8)
> Has you tested this functionality on otoro? It seems It does not work at
> all. I mean every time the user enters the PIN for unlocking the SIM the RIL
> returns error '6' that is RIL_E_REQUEST_NOT_SUPPORTED. That's strange
> because valid errors are SUCCESS, RADIO_NOT_AVAILABLE, GENERIC_FAILURE and
> PASSWORD_INCORRECT.

Hi Jaoo
I can reproduce your problem, will file a bug and be working on it now!
Thanks for your notification.
Comment 13 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-08-02 20:04:24 PDT
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #12)
> Hi Jaoo
> I can reproduce your problem, will file a bug and be working on it now!
> Thanks for your notification.

Hi, jaoo
I just file the bug 780063,
since your patch already deals with the refactoring stuff, 
I'll just add AID in my patch and try not to influence yours.

Thanks
Comment 14 José Antonio Olivera Ortega [:jaoo] 2012-08-07 14:22:57 PDT
Created attachment 649810 [details] [diff] [review]
WIP
Comment 15 José Antonio Olivera Ortega [:jaoo] 2012-08-09 03:23:06 PDT
Created attachment 650491 [details] [diff] [review]
v1
Comment 16 José Antonio Olivera Ortega [:jaoo] 2012-08-10 06:45:38 PDT
Created attachment 650867 [details] [diff] [review]
v2

Rebasing patch.
Comment 17 Philipp von Weitershausen [:philikon] 2012-08-12 18:29:22 PDT
Comment on attachment 650867 [details] [diff] [review]
v2

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

I like this a lot. Apart from two points below, all of my comments are merely cosmetic.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +947,5 @@
>  
>    handleICCGetCardLock: function handleICCGetCardLock(message) {
> +    let messageType = message.success ? "RIL:GetCardLock:Return:OK" :
> +                                        "RIL:GetCardLock:Return:KO";
> +    ppmm.sendAsyncMessage(messageType, message);

I feel like we can simplify the return messages a lot. All "RIL:*:Return:OK" and "RIL:*:Return:KO" messages, respectively, have the same handler. So why even have different types of messages. I would do something like this:

  // invoke this for rilMessageType == "iccUnlockCardLock" etc.
  handleICCLockResult: function handleICCLockResult(message) {
    ppmm.sendAsyncMessage("RIL:CardLockResult", message);
  },

Then in RILContentHelper you would do this:

  case "RIL:CardLockResult":
    if (msg.json.success) {
      this.fireRequestSuccess(msg.json.requestId, msg.json);
    } else {
      this.fireRequestError(msg.json.requestId, msg.json);
    }

which seems a lot simpler to me and saves a lot of lines of code. :)

@@ +1678,1 @@
>      this.worker.postMessage(message);

Two things:

(a) Because `rilMessageType` corresponds to methods on the `RIL` object in ril_worker.js and we prefer camelCase, I would suggest "iccUnlockCardLock" or something like that for `rilMessageType`. You need to adjust the code in ril_worker.js for that, of course, as well as other uses of this message and other similar messages.

(b) With the removal of all `case` blocks, the code is a bit awkward to read now. I would recommend putting two lines at the bottom inside the `switch` block below the four `case` statements. Although, I'm almost tempted to say that we should let ril_worker.js take care of the error case and have RadioInterfaceLayer.js just play dumb switchboard.

::: dom/system/gonk/ril_worker.js
@@ +2089,5 @@
> +   /**
> +   * Helper for processing responses of functions such as enterICC* and changeICC*.
> +   */
> +  _processEnterAndChangeICCResponses: function _processEnterAndChangeICCResponses(length, options) {
> +    options.success = options.rilRequestError == 0 ? true : false;

Simpler:

  options.success = options.rilRequestError == 0;

@@ +3379,5 @@
>    datacall.rilMessageType = "datacallstatechange";
>    this.sendDOMMessage(datacall);
>  };
>  RIL[REQUEST_QUERY_FACILITY_LOCK] = function REQUEST_QUERY_FACILITY_LOCK(length, options) {
> +  options.success = options.rilRequestError == 0 ? true : false;

See above.

@@ +3386,3 @@
>    }
>  
> +  options.enabled = Buf.readUint32List()[0] == 0 ? false : true;

This can fail if there was an error and length is 0. I suggest:

  if (length) {
    options.enabled = Buf.readUint32List()[0] == 0;
  }

This explicitly leaves `options.enabled` undefined if there was an error, which seems semantically correct to me.

@@ +3390,3 @@
>  };
>  RIL[REQUEST_SET_FACILITY_LOCK] = function REQUEST_SET_FACILITY_LOCK(length, options) {
> +  options.success = options.rilRequestError == 0 ? true : false;

See above.

@@ +3392,5 @@
> +  options.success = options.rilRequestError == 0 ? true : false;
> +  if (!options.success) {
> +    options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];
> +  }
> +  options.retryCount = length ? Buf.readUint32List()[0] : -1;

Same problem as above: guard against length being 0.
Comment 18 José Antonio Olivera Ortega [:jaoo] 2012-08-13 04:34:30 PDT
Created attachment 651323 [details] [diff] [review]
v3

Addressing latest comments.
Comment 19 José Antonio Olivera Ortega [:jaoo] 2012-08-13 04:38:38 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #17)
> Comment on attachment 650867 [details] [diff] [review]
> v2
> 
> Review of attachment 650867 [details] [diff] [review]:
> -----------------------------------------------------------------

> (b) With the removal of all `case` blocks, the code is a bit awkward to read
> now. I would recommend putting two lines at the bottom inside the `switch`
> block below the four `case` statements. 

Done.

> Although, I'm almost tempted to say
> that we should let ril_worker.js take care of the error case and have
> RadioInterfaceLayer.js just play dumb switchboard.

I don't understand what you mean here. I see want you mean about moving the error case to ril_worker.js but I do not catch the latter. Could you clarify that please?
Comment 20 Philipp von Weitershausen [:philikon] 2012-08-13 10:44:19 PDT
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #19)
> > (b) With the removal of all `case` blocks, the code is a bit awkward to read
> > now. I would recommend putting two lines at the bottom inside the `switch`
> > block below the four `case` statements. 
> 
> Done.

I don't see it done in your latest patch.

> > Although, I'm almost tempted to say
> > that we should let ril_worker.js take care of the error case and have
> > RadioInterfaceLayer.js just play dumb switchboard.
> 
> I don't understand what you mean here. I see want you mean about moving the
> error case to ril_worker.js but I do not catch the latter. Could you clarify
> that please?

Basically, make RadioInterfaceLayer.js look like this:

  getCardLock: function getCardLock(message) {
    message.rilMessageType = "iccGetCardLock";
    this.worker.postMessage(message);
  },

and handle the error case in ril_worker.js.
Comment 21 Philipp von Weitershausen [:philikon] 2012-08-13 10:45:03 PDT
Comment on attachment 651323 [details] [diff] [review]
v3

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1640,1 @@
>                                 requestId: message.requestId});

See comment 20 about moving the error handling in these methods to ril_worker.js.

::: dom/system/gonk/ril_worker.js
@@ +3396,5 @@
> +    options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];
> +  }
> +  if (length) {
> +    options.retryCount = length ? Buf.readUint32List()[0] : -1;
> +  }

Sorry, my previous review comment was bad. You're already guarding against `length` being 0. In fact, wrapping this in an `if (length)` block means that we never set `options.retryCount` to -1.
Comment 22 José Antonio Olivera Ortega [:jaoo] 2012-08-13 15:14:52 PDT
Created attachment 651543 [details] [diff] [review]
v4

Addressing latest comments.
Comment 23 José Antonio Olivera Ortega [:jaoo] 2012-08-13 15:20:41 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #20)
> Basically, make RadioInterfaceLayer.js look like this:
> 
>   getCardLock: function getCardLock(message) {
>     message.rilMessageType = "iccGetCardLock";
>     this.worker.postMessage(message);
>   },
> 
> and handle the error case in ril_worker.js.

Gotcha!, thanks for your comments and suggestions.
Comment 24 Philipp von Weitershausen [:philikon] 2012-08-14 08:20:26 PDT
Comment on attachment 651543 [details] [diff] [review]
v4

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

Awesome. r=me

(Just address the whitespace nits locally and feel free to commit this. Thanks!)

::: dom/system/gonk/ril_worker.js
@@ +816,5 @@
> +        this.enterICCPUK2(options);
> +        break;
> +
> +
> +      default:

Style nit: excessive whitespace ;)

(Same thing a few times below.)

@@ +930,5 @@
>        Buf.writeString(options.aid ? options.aid : this.aid);
>      }
>      Buf.sendParcel();
>    },
> +

Nit: you introduced a trailing space here.
Comment 25 José Antonio Olivera Ortega [:jaoo] 2012-08-14 10:22:55 PDT
Created attachment 651814 [details] [diff] [review]
v5
Comment 26 Philipp von Weitershausen [:philikon] 2012-08-15 09:51:04 PDT
Comment on attachment 651814 [details] [diff] [review]
v5

https://hg.mozilla.org/integration/mozilla-inbound/rev/cddb8d3836df
Comment 27 Ryan VanderMeulen [:RyanVM] 2012-08-15 18:45:29 PDT
https://hg.mozilla.org/mozilla-central/rev/cddb8d3836df
Comment 28 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-08-16 04:09:17 PDT
Hi, Jaoo:

Can you also help to update the comments in nsIDOMMobileConnection.idl ? 
For example, you change the name 'result' to 'success' in response, right?

Gaia team has told me they cannot get the 'result' value in latest gecko build.

Thanks
Comment 29 José Antonio Olivera Ortega [:jaoo] 2012-08-16 04:33:49 PDT
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #28)
> Hi, Jaoo:
> 
> Can you also help to update the comments in nsIDOMMobileConnection.idl ? 

Sure, That's on my TODO list. I have also made some changes in current implementation for unclocking the SIM when it needs to be unlocked.
Comment 30 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-08-17 04:28:54 PDT
Comment on attachment 651814 [details] [diff] [review]
v5

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

Hi, Jaoo 
I found another problem in this patch,

::: dom/system/gonk/RILContentHelper.js
@@ +569,5 @@
> +        if (msg.json.success) {
> +          this.fireRequestSuccess(msg.json.requestId, msg.json);
> +        } else {
> +          this.fireRequestError(msg.json.requestId, msg.json);
> +        }

Hi, Jaoo
Here it might have problems,

if you check the IDL for nsIDOMDOMRequest, for fireError, the arguments is DOMString (error message)

  void fireError(in nsIDOMDOMRequest request, in DOMString error);

whereas in success, it's a jsval

  void fireSuccess(in nsIDOMDOMRequest request, in jsval result);

So in this implementation, 
you change it to 'when pin is wrong, then onerror shall be called', 
but the problem is you only can get errorMessags(so in this patch, it should be
  this.fireRequestError(msg.json.requestId, msg.json.errorMsg);
) in onerror callback,

which might cause problems in gaia, 
for example in your PR
https://github.com/mozilla-b2g/gaia/pull/3507/files

you cannot get retryCount

Can you check that again ?

Note You need to log in before you can comment on or make changes to this bug.