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.

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.

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 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.)

(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

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.

(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

(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});

(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. :)

Address to philikon's comments and suggestions.

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. */

Hi, philikon Your comments addressed, Thanks for your suggestion.

Comment on attachment 616165 [details] [diff] [review] IDL v3 Looks good to me. Over to Jonas for superreview!

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 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.

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.

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

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

(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!

(In reply to Fabrice Desré [:fabrice] from comment #18) Hi Fabrice, Great thanks to your explaination, I'll try. :) thanks

(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.

(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.

(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.

Refactoring DOMRequestHelper.jsm by Kanru and philikon's suggestions. thanks~~

update comments, s/type/lockType/

Implementations, including refactoring in DOMRequestHelper.jsm.

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 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 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.

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 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.

(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 .

Rebase and address to philikon's last review comments, thanks, Philikon!

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.

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 on attachment 625056 [details] [diff] [review] Part 2: Implementations. v7 \o/