Last Comment Bug 731786 - B2G RIL: Support SIM cards that require PIN codes
: B2G RIL: Support SIM cards that require PIN codes
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla15
Assigned To: Yoshi Huang[:allstars.chh]
:
Mentors:
Depends on: 751005 756375 757512
Blocks: b2g-ril 756886
  Show dependency treegraph
 
Reported: 2012-02-29 14:11 PST by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-06-14 19:53 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
IDL (2.40 KB, patch)
2012-04-11 23:59 PDT, Yoshi Huang[:allstars.chh]
no flags Details | Diff | Review
IDL v2 (2.00 KB, patch)
2012-04-13 01:31 PDT, Yoshi Huang[:allstars.chh]
philipp: review-
Details | Diff | Review
IDL v3 (4.34 KB, patch)
2012-04-18 09:10 PDT, Yoshi Huang[:allstars.chh]
philipp: review+
jonas: superreview+
Details | Diff | Review
[WIP] implementation (17.11 KB, patch)
2012-05-02 04:48 PDT, Yoshi Huang[:allstars.chh]
no flags Details | Diff | Review
[WIP] Part2 (v2) : implementation (20.65 KB, patch)
2012-05-03 00:13 PDT, Yoshi Huang[:allstars.chh]
no flags Details | Diff | Review
IDL v4 (4.38 KB, patch)
2012-05-08 15:49 PDT, Yoshi Huang[:allstars.chh]
philipp: review+
Details | Diff | Review
Part 2: implementation (29.09 KB, patch)
2012-05-08 15:50 PDT, Yoshi Huang[:allstars.chh]
no flags Details | Diff | Review
Part2 : implementation, v6 (29.78 KB, patch)
2012-05-16 20:38 PDT, Yoshi Huang[:allstars.chh]
philipp: review+
Details | Diff | Review
Part 2: Implementations. v7 (29.53 KB, patch)
2012-05-18 03:40 PDT, Yoshi Huang[:allstars.chh]
no flags Details | Diff | Review
Part 2: Implementations. v7 (29.53 KB, patch)
2012-05-18 03:47 PDT, Yoshi Huang[:allstars.chh]
philipp: review+
Details | Diff | Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-29 14:11:44 PST
Some prepaid SIM cards need a PIN to unlock them.  In current b2g, those SIM cards have a permanent "Connecting..." status.

*Unlocking* SIM cards is another interesting, but lower priority, issue.
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-03-01 00:58:56 PST
We need an implementation bit here for RIL, but also an API piece that's not RIL-specific.  I'm fine with morphing this into the RIL work.
Comment 2 Yoshi Huang[:allstars.chh] 2012-04-11 23:59:13 PDT
Created attachment 614288 [details] [diff] [review]
IDL

Currently I add those API inside WebMobileConnection, Bug 729173
Philikon, can you kindly help to review that and give me some feedback? 

Great thanks.
Comment 3 Philipp von Weitershausen [:philikon] 2012-04-12 00:31:46 PDT
Comment on attachment 614288 [details] [diff] [review]
IDL

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

Good start! Please see comments below.

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +58,5 @@
> +   *  Enter PIN.
> +   *  TODO: return bool to indicate success or failure?
> +   *        or return remaining retry counts?
> +   *
> +   *  TODO: Should add a callback to prevent blocking content process?

These methods cannot be synchronous! Also, instead of taking callbacks, we typically return a DOMRequest where the caller can attach event listeners.

@@ +60,5 @@
> +   *        or return remaining retry counts?
> +   *
> +   *  TODO: Should add a callback to prevent blocking content process?
> +   */
> +  bool enterPin(in DOMString pin);

So there are basically a lot of enter* and change* methods here. I would like to collapse them because CDMA has different kinds of locks and speccing the API to a particular radio technology doesn't seem like a good idea.

How about:

  DOMRequest unlockCard(in DOMString lockType,
                        in DOMString value);

  DOMRequest changeCardLock(in DOMString lockType,
                            in DOMString newValue,
                            [optional] in DOMString oldValue);

This way we can also extend this API later to support other facility locks, like you mention below.

@@ +95,5 @@
> +   * Although we can use oncardstatechange to know if the SIM is locked or not,
> +   * But I think this API provides the functionality that user can query the SIM
> +   * card lock directly.
> +   */
> +  bool getIccLockEnabled();

DOMRequest getCardLockEnabled(in DOMString lockType);

@@ +100,5 @@
> +
> +  /**
> +   * Lock/Unlock the SIM lock
> +   */
> +  bool setIccLockEnabled(in bool lock);

DOMRequest setCardLockEnabled(in DOMString lockType, in bool enabled);

@@ +114,5 @@
> +
> +  /**
> +   * SIM card informations, i.e. MSISDN
> +   */
> +  readonly attribute jsvalue iccInfo;

Scope creep! Please remove. (Also, if we need it, we should expose the MSISDN directly as a string and not call it MSISDN because that's a GSM/UMTS specific.)
Comment 4 Yoshi Huang[:allstars.chh] 2012-04-12 20:58:31 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #3)
> Comment on attachment 614288 [details] [diff] [review]
> IDL
> 

Philikon, thanks for your review,

I got some questions inlined
 
> So there are basically a lot of enter* and change* methods here. I would
> like to collapse them because CDMA has different kinds of locks and speccing
> the API to a particular radio technology doesn't seem like a good idea.
> 
> How about:
> 
>   DOMRequest unlockCard(in DOMString lockType,
>                         in DOMString value);

Okay, then possible values for lockType would be 'pin', 'pin2'


>   DOMRequest changeCardLock(in DOMString lockType,
>                             in DOMString newValue,
>                             [optional] in DOMString oldValue);
> 
> This way we can also extend this API later to support other facility locks,
> like you mention below.

Possible values for lockType: 'pin', 'pin2', 'puk', 'puk2'.

And my questions :
1. Why in changeCardLock, newValue comes before oldValue ? 

   Specially when entering PUK for changing PIN.
   It should look lile changeCardLock('puk', "puk code", "new pin code")

2. Also why oldValue is optional? 

3. These 2 APIs are used to supply/change pin, pin2, ..etc, 
   not for facility locks, because they are more a little bit complicated, 
   see below.

> DOMRequest getCardLockEnabled(in DOMString lockType);
>
Philikon, for this lockType, do you mean 'pin' only? Or do you mean those facility locks ?
See TS 27.007 7.4 for facilitiy locks.

If it's for 'pin', then should we need to add a parameter 'lockType'? Because other codes pin2, puk
seems doesn't have this feature. Or it's the idea that you mentioned earlier we generalize this API 
to more than one radio technology? 

If it's for facility locks, then it should be

DOMRequest queryFacilitiyLock(in DOMString facility, in DOMString password, in short serviceClass);

Facility locks needs another two parameters, password and serviceClass.

Since there is password, then we also need an API for changing that password.

DOMRequest changeFacilityPassword(in DOMString facility, in DOMString oldValue, in DOMString newValue);

In this case, I put the oldValue before the newValue.

Thanks
Comment 5 Philipp von Weitershausen [:philikon] 2012-04-12 23:36:18 PDT
You make good points about the different arguments. For instance, to unlock the PUK, one needs to supply a new PIN. (It's not possible to change the PUK, I think.) So maybe we should make the API even more flexible:

  DOMRequest getCardLock(in DOMString lockType);
  DOMRequest setCardLock(in jsval info);
  DOMRequest unlockCardLock(in jsval info);

So you would do something like:

  n.m.getCardLock("pin");
  n.m.setCardLock({"type": "pin",
                   "old": "...",
                   "new": "..."});

Or

  n.m.unlockCardLock({"type": "puk",
                      "puk": "...",
                      "pin": "..."});

I did some more research on facility locks. It seems they are very different from card locks. They don't manipulate the state on the ICC but interact with the network provider. So I don't think we should conflate them into the same API. Also, from the looks of it, the facility locks are pretty esoteric features, so I think we can ignore them for now.
Comment 6 Yoshi Huang[:allstars.chh] 2012-04-13 00:16:32 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #5)
>   DOMRequest getCardLock(in DOMString lockType);
>   DOMRequest setCardLock(in jsval info);
>   DOMRequest unlockCardLock(in jsval info);
> 
Philikon, thanks for your comments.

So now when I try to unlock pin, pin2, puk, and puk2 
I could use 
  unlockCardLock( {"type": "pin|pin2|puk|puk2",
                   "value": "...",
                   "newValue": "...", /* For puk and puk2 only*/
                  });

When I try to change pin and pin2 
I could use

  setCardLock( { "type": "pin|pin2",
                 "old": "...",
                 "new": "..." });

And When I try to know if the sim is locked 
I use

  getCardLock("pin");


Finally I want to enable/disable the sim lock
I use
  setCardLock({"type" : "pin",
               "password": "...",
               "enable": true|false };

Is my understanding correct?
           
thanks
Comment 7 Philipp von Weitershausen [:philikon] 2012-04-13 00:45:17 PDT
(In reply to Yoshi Huang[:yoshi] from comment #6)
> So now when I try to unlock pin, pin2, puk, and puk2 
> I could use 
>   unlockCardLock( {"type": "pin|pin2|puk|puk2",
>                    "value": "...",
>                    "newValue": "...", /* For puk and puk2 only*/
>                   });

"newValue" would be the new PIN value for "puk" and "puk2", so I was going to suggest calling that parameter "pin" or better "newPin".

> When I try to change pin and pin2 
> I could use
> 
>   setCardLock( { "type": "pin|pin2",
>                  "old": "...",
>                  "new": "..." });

or maybe

  setCardLock({type: "pin",
               pin: "...",
               newPin: "..."});

> And When I try to know if the sim is locked 
> I use
> 
>   getCardLock("pin");

Correct.

> Finally I want to enable/disable the sim lock
> I use
>   setCardLock({"type" : "pin",
>                "password": "...",
>                "enable": true|false };

I think I have a slight preference for "enabled" (adjective) rather than the verb. Also, why "password" all of a sudden? Isn't it the PIN, at least in this case?

  setCardLock({type: "pin",
               pin: "...",
               enabled: false});
Comment 8 Yoshi Huang[:allstars.chh] 2012-04-13 00:52:38 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #7)
> Also, why "password" all of a sudden? Isn't it the PIN, at least in
> this case?
> 
>   setCardLock({type: "pin",
>                pin: "...",
>                enabled: false});

Yes in this case it's pin.
Sorry, it was my fault yesterday. For enabling SIM Lock, we need to provide 'pin' code for this. (Whereas for getting the lock is enabled or nor, we don't have to provide a password/pin.)

Great thanks for your suggestions and comments. :)
Comment 9 Yoshi Huang[:allstars.chh] 2012-04-13 01:31:32 PDT
Created attachment 614700 [details] [diff] [review]
IDL v2

Address to philikon's comments and suggestions.
Comment 10 Philipp von Weitershausen [:philikon] 2012-04-17 20:55:39 PDT
Comment on attachment 614700 [details] [diff] [review]
IDL v2

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

You need to change the interface's UUID when you modify it!

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +58,5 @@
> +   *  Get SIM is locked or not when phone power-up.
> +   *
> +   *  TS 27.007, clause 7.4 Facility Command "SC".
> +   *  @param lockType: 'pin'
> +   */

A couple of things to make this API documentation more useful:

* s/SIM/ICC/

* The reference to a GSM spec is not very helpful in a technology-agnostic DOM API.

* These methods are not just about the PIN lock but for any lock. Please phrase your documentation accordingly. You can use the PIN lock as an example.

* Please document what the DOMRequest will return.

We already discussed all of this in this bug, but everybody else will not have this context when they read the code. And this is not only Mozilla contributors, this API will be on the *web*. Please keep this in mind. We're writing this code not just for us, but for a lot of other people.

Here's my take on this:

/**
 * Find out about the status of an ICC lock (e.g. the PIN lock).
 *
 * @param lockType
 *        Identifies the lock type, e.g. "pin" for the PIN lock.
 *
 * @return a DOM Request. The request's result will be an object containing information
 *         about the specified lock's status, e.g. {lockType: "pin", enabled: true}.
 */

@@ +69,5 @@
> +   *  @param value: Code for 'pin', 'pin2', 'puk' and 'puk2'.
> +   *  [optional]
> +   *    @param newPin: When the type is 'puk' or 'puk2',
> +   *                   this parameter will be the new pin or pin2.
> +   */

Same points from above apply. My suggestion:

/**
 * Unlock a card lock.
 *
 * @param info
 *        An object containing the information necessary to unlock
 *        the given lock. At a minimum, this object must have a
 *        "lockType" attribute which specifies the type of lock, e.g.
 *        "pin" for the PIN lock. Other attributes are dependent on
 *        the lock type.
 *
 * Examples:
 *
 * (1) Unlocking the PIN:
 *
 *   unlockCardLock({type: "pin",
 *                   pin: "..."});
 *
 * (2) Unlocking the PUK and supplying a new PIN:
 *
 *   unlockCardLock({type: "puk",
 *                   puk: "...",
 *                   newPin: "..."});
 *
 * @return a DOM Request.
 */

@@ +83,5 @@
> +   *                   This parameter will be the new value of pin or pin2.
> +   *    @param enabled: Used when trying to enable SIM lock.
> +   *                    This parameter will be a boolean to enable/disable 
> +   *                    the lock.
> +   */

Same points from above apply. My suggestion:

/**
 * Modify the state of a card lock.
 *
 * @param info
 *        An object containing information about the lock and
 *        how to modify its state. At a minimum, this object
 *        must have a "lockType" attribute which specifies the
 *        type of lock, e.g. "pin" for the PIN lock. Other
 *        attributes are dependent on the lock type.
 *
 * Examples:
 *
 * (1) Disabling the PIN lock:
 *
 *   setCardLock({type: "pin",
 *                pin: "...",
 *                enabled: false});
 *
 * (2) Changing the PIN:
 *
 *   setCardLock({type: "pin",
 *                pin: "...",
 *                newPin: "..."});
 *
 * @return a DOM Request.
 */
Comment 11 Yoshi Huang[:allstars.chh] 2012-04-18 09:10:48 PDT
Created attachment 616165 [details] [diff] [review]
IDL v3

Hi, philikon
Your comments addressed,
Thanks for your suggestion.
Comment 12 Philipp von Weitershausen [:philikon] 2012-04-18 09:21:29 PDT
Comment on attachment 616165 [details] [diff] [review]
IDL v3

Looks good to me. Over to Jonas for superreview!
Comment 13 Yoshi Huang[:allstars.chh] 2012-04-26 22:56:24 PDT
Comment on attachment 616165 [details] [diff] [review]
IDL v3

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

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +64,5 @@
> +   *         The request's result will be an object containing 
> +   *         information about the specified lock's status,
> +   *         e.g. {lockType: "pin", enabled: true}.
> +   */
> +  DOMRequest getCardLock(in DOMString lockType);

should be nsIDOMDOMRequest

@@ +109,5 @@
> +   *       lockType:  "pin",
> +   *       result:    true
> +   *     }
> +   */
> +  DOMRequest unlockCardLock(in jsval info);

^^^^^^^^^ ditto

@@ +156,5 @@
> +   *       lockType:  "pin",
> +   *       result:    true
> +   *     }
> +   */
> +  DOMRequest setCardLock(in jsval info);

^^^^^^^^^ ditto
Comment 14 Jonas Sicking (:sicking) 2012-05-01 15:06:02 PDT
Comment on attachment 616165 [details] [diff] [review]
IDL v3

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

Looks good. Though we need to document somewhere exactly what the various JSON objects will contain. Once we push this to a spec that will need to be specced as part of it.
Comment 15 Yoshi Huang[:allstars.chh] 2012-05-02 04:48:15 PDT
Created attachment 620257 [details] [diff] [review]
[WIP] implementation

Hi,philikon:
  Can you help to give me some feedback about using nsIDOMDOMRequest in MobileConnection? The util function to maintain dom requests inside RILContentHelper.js is from DOMRequestHelper.jsm, but in order to use that I have to call DOMRequestHelper.initHelper with a 'window' object in init, which we don't have this method in RILContentHelper.js. Kanru has suggested me to refactor DOMRequestHelper, but I guess that will be another follow-ups.
Also the IPC message name "OK" and "KO" is from ContactManager.

thanks.
Comment 16 [:fabrice] Fabrice Desré 2012-05-02 07:48:53 PDT
You must implement nsIDOMGlobalPropertyInitializer in RILContentHelper.js to get a window object that can be used with the DOMRequestHelper.

See for instance https://mxr.mozilla.org/mozilla-central/source/dom/base/Webapps.js#28
Comment 17 Yoshi Huang[:allstars.chh] 2012-05-02 08:09:27 PDT
Hi Fabrice
Thanks for the pointer and tips.
In addition to implement nsIDOMGlobalPropertyInitializer, Should I also modify the manifest 

https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.manifest#7
to change the category to JavaScript-navigator-property ?

Also in my patch, RILContentHelper.js, the three interfaces getCardLock, setCardLock unlockCardLock all have a parameter 'window', I am also wondering in this case is it still correct to call DOMRequestHelper.initHelper in init()?  In these 3 interfaces, will the 'window' in each interface refer to different context?

thanks
Comment 18 [:fabrice] Fabrice Desré 2012-05-02 08:54:26 PDT
(In reply to Yoshi Huang[:yoshi] from comment #17)
> Hi Fabrice
> Thanks for the pointer and tips.
> In addition to implement nsIDOMGlobalPropertyInitializer, Should I also
> modify the manifest 
> 
> https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/
> RadioInterfaceLayer.manifest#7
> to change the category to JavaScript-navigator-property ?

No, since RILContentHelper is not exposed directly as a property of the navigator object.

But you have to find a way to get a window reference if you want to use DOMRequest. We choose to not use them when implementing the XPCOM service for the settings API for this very reason.

> Also in my patch, RILContentHelper.js, the three interfaces getCardLock,
> setCardLock unlockCardLock all have a parameter 'window', I am also
> wondering in this case is it still correct to call
> DOMRequestHelper.initHelper in init()?  In these 3 interfaces, will the
> 'window' in each interface refer to different context?

 I don't know the call contexts well enough to answer that!
Comment 19 Yoshi Huang[:allstars.chh] 2012-05-02 09:06:11 PDT
(In reply to Fabrice Desré [:fabrice] from comment #18)
Hi Fabrice,
   Great thanks to your explaination, I'll try.  :) 
   thanks
Comment 20 Philipp von Weitershausen [:philikon] 2012-05-02 10:38:23 PDT
(In reply to Fabrice Desré [:fabrice] from comment #16)
> You must implement nsIDOMGlobalPropertyInitializer in RILContentHelper.js to
> get a window object that can be used with the DOMRequestHelper.

No no no, no nsIDOMGlobalPropertyInitializer needed! RILContentHelper is a content process wide singleton.
Comment 21 Philipp von Weitershausen [:philikon] 2012-05-02 10:43:49 PDT
(In reply to Yoshi Huang[:yoshi] from comment #17)
> Also in my patch, RILContentHelper.js, the three interfaces getCardLock,
> setCardLock unlockCardLock all have a parameter 'window', I am also
> wondering in this case is it still correct to call
> DOMRequestHelper.initHelper in init()?  In these 3 interfaces, will the
> 'window' in each interface refer to different context?

DOMRequestIpcHelper doesn't fit the object model here since it expects to extend a nsIDOMGlobalPropertyInitializer. But the whole request management code as well as some of the window expiration management could be reused if it was refactored a bit.
Comment 22 Kan-Ru Chen [:kanru] (UTC+8) 2012-05-02 19:12:33 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #21)
> DOMRequestIpcHelper doesn't fit the object model here since it expects to
> extend a nsIDOMGlobalPropertyInitializer. But the whole request management
> code as well as some of the window expiration management could be reused if
> it was refactored a bit.

Yep, that is my point.
Comment 23 Yoshi Huang[:allstars.chh] 2012-05-03 00:13:55 PDT
Created attachment 620606 [details] [diff] [review]
[WIP] Part2 (v2) : implementation

Refactoring DOMRequestHelper.jsm by Kanru and philikon's suggestions.
thanks~~
Comment 24 Yoshi Huang[:allstars.chh] 2012-05-08 15:49:08 PDT
Created attachment 622179 [details] [diff] [review]
IDL v4

update comments, s/type/lockType/
Comment 25 Yoshi Huang[:allstars.chh] 2012-05-08 15:50:28 PDT
Created attachment 622180 [details] [diff] [review]
Part 2: implementation

Implementations, including refactoring in DOMRequestHelper.jsm.
Comment 26 Yoshi Huang[:allstars.chh] 2012-05-08 16:26:28 PDT
Hi philikon,
After unlocking the pin code, the radio state will be changed from RADIO_STATE_SIM_LOCKED_OR_ABSENT to RADIO_STATE_READY, but this event(READY) will be ignored, it bails out in http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#2724 

I think we should revise the handler for UNSOLICITED_RESPONSE_RADIO_STATE_CHANGED a little bit,
should I file a bug for this?


thanks
Comment 27 Philipp von Weitershausen [:philikon] 2012-05-14 14:09:43 PDT
Comment on attachment 622180 [details] [diff] [review]
Part 2: implementation

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

Great job! A few comments below, incl one question for Mounir.

::: dom/base/DOMRequestHelper.jsm
@@ +79,3 @@
>    initHelper: function(aWindow, aMessages) {
> +    this.initMessageListener(aMessages);
> +    this.initRequests();

I'm not exactly sure why these are two separate methods, but ok.

I think we should also factor out the cleanup (unregistering the message listeners).

::: dom/system/gonk/RILContentHelper.js
@@ +12,5 @@
>  
>  var RIL = {};
>  Cu.import("resource://gre/modules/ril_consts.js", RIL);
>  
> +const DEBUG = true; // set to true to see debug messages

brzzzt :)

@@ +111,5 @@
>      throw Components.Exception("Not implemented", Cr.NS_ERROR_NOT_IMPLEMENTED);
>    },
>  
> +  getCardLock: function getCardLock(window, lockType) {
> +    let request = Services.DOMRequest.createRequest(window);

Mounir has said in previous reviews for WebMobileConnection that we need to check for window == null here. If it's indeed null, then we should notify an error on the DOMRequest on the next event loop tick:

  Services.tm.currentThread.dispatch(function () {
    Services.DOMRequest.fireError(request, ...);
  }, Ci.nsIThread.DISPATCH_NORMAL);

This should be done here and everywhere else we deal with window objects. Though I'm not sure how the DOMRequestService will deal with window being null, to be honest... Like, can you even have a DOMRrequest without a window? Maybe we should just abort with an exception? Pinging Mounir for some feedback/guidance here.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1274,5 @@
> +  unlockCardLock: function unlockCardLock(message) {
> +    if (message.lockType == "pin") {
> +      this.worker.postMessage({type: "enterICCPIN",
> +                               pin: message.pin,
> +                               requestId: message.requestId});

A `switch()` statement would make this easier to read (remember, we're not in C ;)). But it can be even simpler (and more efficient). Just reuse the `message` object:

  switch(message.lockType) {
    case "pin2":
      message.type = "enterICCPIN2"
      break;
    ...
    default:
      ppmm.sendAsyncMessage("RIL:UnlockCardLock:Return:KO",
                            {errorMsg: "Unsupported Card Lock.",
                             requestId: requestId});
      return;
  }
  this.worker.postMessage(message);

@@ +1296,5 @@
> +                             requestId: requestId});
> +    }
> +  },
> +
> +  setCardLock: function setCardLock(message) {

Same thing here.

@@ +1321,5 @@
> +    } else { // Enable/Disable pin lock.
> +      let facility = RIL.ICC_CB_FACILITY_SIM, password;
> +      let serviceClass = RIL.ICC_SERVICE_CLASS_VOICE |
> +                         RIL.ICC_SERVICE_CLASS_DATA  |
> +                         RIL.ICC_SERVICE_CLASS_FAX;

I feel like this logic should be pushed to ril_worker.js. Let's keep RadioInterfaceLayer.js as the simple message exchange that it is. Just amend the message with the right `type` and send it off to ril_worker.js...

::: dom/system/gonk/ril_worker.js
@@ +2277,5 @@
> +                       cardLock: {
> +                         lockType: "pin",
> +                         result: options.rilRequestError == 0 ? true : false,
> +                         retryCount: length ? Buf.readUint32List()[0] : -1,
> +                         requestId: options.requestId}});

A single flat object that serves as both the message and the cardLock information is more efficient.
Comment 28 Mounir Lamouri (:mounir) 2012-05-15 00:34:05 PDT
Comment on attachment 622180 [details] [diff] [review]
Part 2: implementation

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

You will actually get a crash if you try to create a DOMRequest object with a null window. I guess here throwing an exception would be the way to go then.
Comment 29 Yoshi Huang[:allstars.chh] 2012-05-16 20:38:45 PDT
Created attachment 624636 [details] [diff] [review]
Part2  : implementation, v6

Thanks to philikon's and mounir's suggestions.
This patch has been revised according to 

Address to philikon's review comments: 
1. Using switch to simplify code
2. Move implementations from RadioInterface to ril_worker
3. Simplify message layout 
4. Refactor DOMRequestHelper, this time adds a function removeMessageListener

Address to mounir's feedbacks.
1. throw exception when GetOwner returns null (done in RILContentHelper)


thanks
Comment 30 Philipp von Weitershausen [:philikon] 2012-05-17 13:12:39 PDT
Comment on attachment 624636 [details] [diff] [review]
Part2  : implementation, v6

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

Excellent work! Just one minor nit below. r=me with that.

As per our new policy, this needs tests. However, the test harness isn't quite ready yet, so let's do them in a follow up. In fact we need tests all of MobileConnection. Can you file a bug for this, please? Thank you!

::: dom/system/gonk/ril_worker.js
@@ +879,5 @@
> +                               password: "", // For query no need to provide pin.
> +                               serviceClass: ICC_SERVICE_CLASS_VOICE |
> +                                             ICC_SERVICE_CLASS_DATA  |
> +                                             ICC_SERVICE_CLASS_FAX,
> +                               requestId: options.requestId});

Can you reuse the `options` object instead of creating a new one, please?

@@ +918,5 @@
> +                             password: options.pin,
> +                             serviceClass: ICC_SERVICE_CLASS_VOICE |
> +                                           ICC_SERVICE_CLASS_DATA  |
> +                                           ICC_SERVICE_CLASS_FAX,
> +                             requestId: options.requestId});

Same here. Here it's more obvious that the new object copies over several attributes from `options`, so we definitely want to reuse it.
Comment 31 Yoshi Huang[:allstars.chh] 2012-05-17 23:17:45 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #30)
> Comment on attachment 624636 [details] [diff] [review]
> Part2  : implementation, v6
> 
> Review of attachment 624636 [details] [diff] [review]:
> -----------------------------------------------------------------
> As per our new policy, this needs tests. However, the test harness isn't
> quite ready yet, so let's do them in a follow up. In fact we need tests all
> of MobileConnection. Can you file a bug for this, please? Thank you!
>
Sure, I just file a Bug 756375 for this .
Comment 32 Yoshi Huang[:allstars.chh] 2012-05-18 03:40:55 PDT
Created attachment 625055 [details] [diff] [review]
Part 2: Implementations. v7

Rebase and address to philikon's last review comments, 
thanks, Philikon!
Comment 33 Yoshi Huang[:allstars.chh] 2012-05-18 03:46:18 PDT
Comment on attachment 625055 [details] [diff] [review]
Part 2: Implementations. v7

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

::: dom/system/gonk/ril_worker.js
@@ +2728,5 @@
> +                       lockType: "pin",
> +                       result: options.rilRequestError == 0 ? true : false,
> +                       retryCount: length ? Buf.readUint32List()[0] : -1,
> +                       requestId: options.requestId});
> +

found an extra line, 
I'll revise this again now.
Comment 34 Yoshi Huang[:allstars.chh] 2012-05-18 03:47:24 PDT
Created attachment 625056 [details] [diff] [review]
Part 2: Implementations. v7
Comment 35 Alexandre LISSY :gerard-majax 2012-05-20 08:29:29 PDT
I've hacked a bit in Gaia with those patches, I have hacksish pin code unlock on my Free Mobile SIM, network is working, I've been able to call my Android cell :)
Comment 36 Philipp von Weitershausen [:philikon] 2012-05-21 13:43:26 PDT
Comment on attachment 625056 [details] [diff] [review]
Part 2: Implementations. v7

\o/
Comment 37 Philipp von Weitershausen [:philikon] 2012-05-21 14:08:30 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe33852b68c7
Comment 38 Ed Morley [:emorley] 2012-05-22 06:31:29 PDT
https://hg.mozilla.org/mozilla-central/rev/fe33852b68c7

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