Support for exporting Contacts to SIM card.

Comment on attachment 697009 [details] [diff] [review] WIP Part 1: IDL for exporting contacts to SIM, v1 ># HG changeset patch ># User Edgar Chen <echen@mozilla.com> ># Date 1356338969 -28800 ># Node ID 095c8fe6cd9093ec10047bee8d2f1a85563ed31b ># Parent 6f27b04897f24d02297a78738ca7e737dc86ae35 >Bug 809726 - Part 1: IDL for exporting contacts to SIM > >diff --git a/dom/interfaces/contacts/nsIDOMContactManager.idl b/dom/interfaces/contacts/nsIDOMContactManager.idl >--- a/dom/interfaces/contacts/nsIDOMContactManager.idl >+++ b/dom/interfaces/contacts/nsIDOMContactManager.idl >@@ -29,10 +29,17 @@ interface nsIDOMContactManager : nsISupp > nsIDOMDOMRequest clear(); > > nsIDOMDOMRequest save(in nsIDOMContact contact); > > nsIDOMDOMRequest remove(in nsIDOMContact contact); > > nsIDOMDOMRequest getSimContacts(in DOMString type); > >+ nsIDOMDOMRequest addSimContact(in DOMString type, >+ in nsIDOMContact contact); >+ >+ nsIDOMDOMRequest updateSimContact(in DOMString type, >+ in unsigned long index, >+ in nsIDOMContact contact); >+ > attribute nsIDOMEventListener oncontactchange; > };

WIP patch v2. ContactManager part will be handle in bug 827253

1). Correct typo. (s/alpaId/alphaId)

1). Fix some stupid error. :(

If you want to test add new SIM Contact, you can apply below patches which add a new button in Contacts Setting. [1][2][3] After pressing export button, Contacts Settings will try to add a new contact with name='add1', number='0912345671' into SIM card. *Note: these patches are for test purpose only. [1] https://github.com/EdgarChen/mozilla-central/commit/28b52420cfc1a1281c47e4199bfe1fc9601f453b [2] https://github.com/EdgarChen/mozilla-central/commit/4d918d5c288fe36d8e236d29cf357a1deb5b455e [3] https://github.com/EdgarChen/gaia/commit/46d51530d103974439b75390b304c80196a8f560

Comment on attachment 700927 [details] [diff] [review] WIP, Support exporting contact in RIL, v4 Review of attachment 700927 [details] [diff] [review]: ----------------------------------------------------------------- Seems the code should work, but I'd like to make the code more have better structure, also more comments on functions prototype. ::: dom/system/gonk/nsIRadioInterfaceLayer.idl @@ +146,5 @@ > + * > + * @param errorMsg > + * error message from RIL. > + */ > + void receiveUpdateResult(in DOMString errorMsg); function should have a better naming. Also why no contactType in parameters? When two different contactType uses the same callback, how do you tell? @@ +360,5 @@ > + * Update ICC Contact > + * > + * @param contactType One of the values below. > + * "ADN" (Abbreviated Dialling Numbers) > + * @param contact The new contact record. Alignment on comments. Also the comment for contact, "The contact will be updated." @@ +365,5 @@ > + * @param callback A nsIRILContactCallback object. > + */ > + void updateICCContact(in DOMString contactType, > + in jsval contact, > + in nsIRILContactUpdateCallback callback); You didn't change the UUID of nsIRadioInterfaceLayer. ::: dom/system/gonk/ril_worker.js @@ +1206,5 @@ > Buf.writeString(options.pathId); > Buf.writeUint32(options.p1); > Buf.writeUint32(options.p2); > Buf.writeUint32(options.p3); > + if (options.datawriter) { dataWriter, @@ +1207,5 @@ > Buf.writeUint32(options.p1); > Buf.writeUint32(options.p2); > Buf.writeUint32(options.p3); > + if (options.datawriter) { > + options.datawriter(options); The way you pass recordSize to dataWriter is mystery. @@ +8469,5 @@ > }, > > /** > + * Update EF with type 'Linear Fixed'. > + */ add comments for parameters. @@ +9223,5 @@ > + * @param fileId EF id of the ADN. > + * @param successCb Callback to be called when success. > + * @param errorCb Callback to be called when error. > + */ > + getADNFreeRecordId: function getADNFreeRecordId(fileId, onsuccess, onerror) { This function and updateADN should be put together with getADN. @@ +9341,5 @@ > + ICCIOHelper.updateLinearFixedEF({fileId: fileId, > + recordNumber: contact.recordId, > + callback: callback.bind(this), > + onerror: onerror}, > + datawriter.bind(this)); alignment. @@ +9611,5 @@ > } > }, > > /** > + * Helper function to find free contact period. @@ +9652,5 @@ > + */ > + updateICCContact: function updateICCContact(appType, contactType, contact, successCb, errorCb) { > + > + // Update ICC contact > + let updateContact = function updateContact(appType, contactType, contact, successCb, errorCb) { updateContact inside updateICCContact ? @@ +9671,5 @@ > + if (contact.recordId) { > + updateContact(appType, contactType, contact, successCb, errorCb); > + } else { > + // Find free record first. > + this.findFreeICCContact( findFreeICCContact should not be done inside this function. Please don't try to make this function more complicated, make it SIMPLER!! Try to make your function does one thing and does it well.

1). Address review comment #9. Thanks for your feedback, Yoshi.

Comment on attachment 701046 [details] [diff] [review] WIP, Support exporting contact in RIL, v5 Review of attachment 701046 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/nsIRadioInterfaceLayer.idl @@ +146,5 @@ > + * > + * @param errorMsg > + * error message from RIL. > + * @param contactType > + * Type of the contact, i.e. ADN, FDN. FDN? @@ +360,5 @@ > in nsIRILContactCallback callback); > + > + /** > + * Update ICC Contact > + * We need to add more documentations on this. When will it insert, when will it update? ::: dom/system/gonk/ril_worker.js @@ +1207,5 @@ > Buf.writeUint32(options.p1); > Buf.writeUint32(options.p2); > Buf.writeUint32(options.p3); > + if (options.dataWriter) { > + options.dataWriter(options.recordSize); Again, mysterious recordSize. @@ +1363,5 @@ > + > + if (!this.appType) { > + options.rilMessageType = "icccontactupdate"; > + options.errorMsg = GECKO_ERROR_REQUEST_NOT_SUPPORTED; > + this.sendDOMMessage(options); onerror(GECKO_ERROR_REQUEST_NOT_SUPPORTED); seems much simpler. @@ +1366,5 @@ > + options.errorMsg = GECKO_ERROR_REQUEST_NOT_SUPPORTED; > + this.sendDOMMessage(options); > + } > + > + if (options.contact.recordId) { Could you add comments here to explain what we're trying to do ? @@ +1386,5 @@ > + options.contactType, > + options.contact, > + onsuccess, > + onerror); > + }.bind(this), This function is long enough to make people confused, also I see some lines are repeating ICCContactHelper.updateICCContact( this.appType, options.contactType, options.contact, onsuccess, onerror); Do you think you can make this function more shorter and more easy to read? @@ +8914,5 @@ > + } > + > + function callback(options) { > + // Consume Buf > + Buf.readString(); ??

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #11) > Comment on attachment 701046 [details] [diff] [review] > WIP, Support exporting contact in RIL, v5 > > Review of attachment 701046 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/ril_worker.js > @@ +8914,5 @@ > > + } > > + > > + function callback(options) { > > + // Consume Buf > > + Buf.readString(); > > ?? The parcel of SIM_IO solicited response is as below: RIL Worker: Parcel (size 24): 0,0,0,0,187,0,0,0,0,0,0,0,144,0,0,0,0,0,0,0,255,255,255,255 There are '255,255,255,255' in data field, so I think we may need to call some read function to consume Buf. Thanks.

Thanks a lot for the feedback, Yoshi. :) I have made a new patch to address review comment #11.

(In reply to Edgar Chen [:edgar][:echen] from comment #12) > (In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #11) > > Comment on attachment 701046 [details] [diff] [review] > > WIP, Support exporting contact in RIL, v5 > > > > Review of attachment 701046 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: dom/system/gonk/ril_worker.js > > @@ +8914,5 @@ > > > + } > > > + > > > + function callback(options) { > > > + // Consume Buf > > > + Buf.readString(); > > > > ?? > The parcel of SIM_IO solicited response is as below: > RIL Worker: Parcel (size 24): > 0,0,0,0,187,0,0,0,0,0,0,0,144,0,0,0,0,0,0,0,255,255,255,255 > > There are '255,255,255,255' in data field, so I think we may need to call > some read function to consume Buf. > Thanks. Is it defined in spec?

Comment on attachment 702179 [details] [diff] [review] WIP, Support exporting contact in RIL, v6 Review of attachment 702179 [details] [diff] [review]: ----------------------------------------------------------------- This patch is 10 times better, and easier to read and understand. ::: dom/system/gonk/RadioInterfaceLayer.js @@ +610,5 @@ > message.contacts); > } > break; > + case "icccontactupdate": > + if (!this._contactsCallbacks) { I wonder is it okay here, you use the same callbacks map for read and update. ::: dom/system/gonk/nsIRadioInterfaceLayer.idl @@ +366,5 @@ > + > + /** > + * Update ICC Contact. > + * > + * This method allows two operation: update the existed contact or s/existed/existing/ ::: dom/system/gonk/ril_worker.js @@ +1198,5 @@ > * String containing the PIN2. > * @param [optional] aid > * AID value. > + * @param dataWriter > + * The writer function for data which shall be wrote into Buf. s/wrote/written/ @@ +1212,5 @@ > + if (dataWriter) { > + dataWriter(options.p3); > + } else { > + Buf.writeString(options.data || null); > + } This is much better, but I still look forward to a better way. @@ +1363,5 @@ > + RIL.sendDOMMessage(options); > + }.bind(this); > + > + if (!this.appType) { > + onerror(GECKO_ERROR_REQUEST_NOT_SUPPORTED); I guess you should return here. @@ +8426,5 @@ > options.command = ICC_COMMAND_READ_RECORD; > options.p1 = options.recordNumber || 1; // Record number > options.p2 = READ_RECORD_ABSOLUTE_MODE; > options.p3 = options.recordSize; > + RIL.iccIO(options, null); you don't have to provide null here, since it will be undefined. @@ +8436,5 @@ > * Load next record from current record Id. > */ > loadNextRecord: function loadNextRecord(options) { > options.p1++; > + RIL.iccIO(options, null); ditto @@ +8489,5 @@ > + * The number of the record shall be updated. > + * @param callback [optional] > + * The callback function shall be called when the record is updated. > + * @param onerror [optional] > + * The callback function shall be called when failure. It's better if we put these two function objects at last. @@ +8904,5 @@ > + } > + > + function callback(options) { > + // Consume Buf > + Buf.readString(); As I said before.

You might need to move IDL to another patch.

Comment on attachment 702179 [details] [diff] [review] WIP, Support exporting contact in RIL, v6 Review of attachment 702179 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/nsIRadioInterfaceLayer.idl @@ +141,5 @@ > +[scriptable, function, uuid(ab954d56-12a1-4c6b-8753-14ad5664111d)] > +interface nsIRILContactUpdateCallback : nsISupports > +{ > + /** > + * Called when updating an ICC contact is finished. How do you think "Callback function to be called when an ICC contact is updated." @@ +148,5 @@ > + * error message from RIL. > + * @param contactType > + * Type of the contact, ADN. > + */ > + void onUpdateResult(in DOMString errorMsg, maybe simpler onUpdated @@ +366,5 @@ > + > + /** > + * Update ICC Contact. > + * > + * This method allows two operation: update the existed contact or s/method/function/, s/operation/operations/

Comment on attachment 702179 [details] [diff] [review] WIP, Support exporting contact in RIL, v6 Review of attachment 702179 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/nsIRadioInterfaceLayer.idl @@ +148,5 @@ > + * error message from RIL. > + * @param contactType > + * Type of the contact, ADN. > + */ > + void onUpdateResult(in DOMString errorMsg, or onSuccess.

Comment on attachment 702179 [details] [diff] [review] WIP, Support exporting contact in RIL, v6 Review of attachment 702179 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +8484,5 @@ > + * Update EF with type 'Linear Fixed'. > + * > + * @param fileId > + * The file to operate on, one of the ICC_EF_* constants. > + * @param recordNumber [optional] Can this really be optional? @@ +8491,5 @@ > + * The callback function shall be called when the record is updated. > + * @param onerror [optional] > + * The callback function shall be called when failure. > + * @param data [optional] > + * The data shall be upated into the record. ?? I don't see any data in your code

Comment on attachment 702179 [details] [diff] [review] WIP, Support exporting contact in RIL, v6 Review of attachment 702179 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +1212,5 @@ > + if (dataWriter) { > + dataWriter(options.p3); > + } else { > + Buf.writeString(options.data || null); > + } Should we really move dataWriter outside options? Since only UPDATE_BINARY and UPDATE_RECORD will write data string, maybe we could check the command before writing data if (commmand == UPDATE_BINARY || commmand == UPDATE_RECORD) { Buf.writeString(null); } else { if (dataWriter) // call dataWriter else Buf.writeString(data); } In that case you could move dataWriter into options. Also, do we need to add 'data' as the parameters of dataWriter? @@ +8493,5 @@ > + * The callback function shall be called when failure. > + * @param data [optional] > + * The data shall be upated into the record. > + * @param dataWriter [optional] > + * The writer function for data which shall be updated into the record. The comments seems not so clear. @@ +8896,5 @@ > + * @param onsuccess Callback to be called when success. > + * @param onerror Callback to be called when error. > + */ > + updateADN: function updateADN(fileId, contact, onsuccess, onerror) { > + function dataWriter(recordSize) { As I said above, should we also add data as parameters? @@ +8898,5 @@ > + */ > + updateADN: function updateADN(fileId, contact, onsuccess, onerror) { > + function dataWriter(recordSize) { > + GsmPDUHelper.writeAlphaIdDiallingNumber(recordSize, > + contact.alphaId, contact could be null, right? @@ +8915,5 @@ > + ICCIOHelper.updateLinearFixedEF({fileId: fileId, > + recordNumber: contact.recordId, > + callback: callback.bind(this), > + onerror: onerror}, > + dataWriter.bind(this)); People may find it hard to understand why you cannot move dataWriter into the object. @@ +9643,5 @@ > + * > + * @param appType CARD_APPTYPE_SIM or CARD_APPTYPE_USIM. > + * @param contactType "ADN". > + * @param successCb Callback to be called when success. > + * @param errorCb Callback to be called when error. onsuccess and onerror. @@ +9674,5 @@ > + * @param appType CARD_APPTYPE_SIM or CARD_APPTYPE_USIM. > + * @param contactType "ADN" > + * @param contact The contact will be added. > + * @param successCb Callback to be called when success. > + * @param errorCb Callback to be called when error. ditto @@ +9693,5 @@ > + * @param appType CARD_APPTYPE_SIM or CARD_APPTYPE_USIM. > + * @param contactType "ADN". > + * @param contact The contact will be updated. > + * @param successCb Callback to be called when success. > + * @param errorCb Callback to be called when error. ditto @@ +9742,5 @@ > + * Update USIM contact. > + * > + * @param contact The contact will be updated. > + * @param successCb Callback to be called when success. > + * @param errorCb Callback to be called when error. ditto. @@ +9760,5 @@ > + * Update SIM contact. > + * > + * @param contact The new contact record. > + * @param successCb Callback to be called when success. > + * @param errorCb Callback to be called when error. ditto.

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #14) > (In reply to Edgar Chen [:edgar][:echen] from comment #12) > > (In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #11) > > > Comment on attachment 701046 [details] [diff] [review] > > > WIP, Support exporting contact in RIL, v5 > > > > > > Review of attachment 701046 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > ::: dom/system/gonk/ril_worker.js > > > @@ +8914,5 @@ > > > > + } > > > > + > > > > + function callback(options) { > > > > + // Consume Buf > > > > + Buf.readString(); > > > > > > ?? > > The parcel of SIM_IO solicited response is as below: > > RIL Worker: Parcel (size 24): > > 0,0,0,0,187,0,0,0,0,0,0,0,144,0,0,0,0,0,0,0,255,255,255,255 > > > > There are '255,255,255,255' in data field, so I think we may need to call > > some read function to consume Buf. > > Thanks. > > Is it defined in spec? From the sepc, the response of UPDATE_RECORD has no output data. I believe '255,255,255,255' is from the implementation of Parcel which indicate the output data is null. (Android also try to read output data when processing the response of UPDATE_RECORD) Thanks

1). Move IDL to another patch. 2). Address review commend #17.

1). Move IDL to another patch 2). Address review comment #15, comment #19, comment #20.

Comment on attachment 703256 [details] [diff] [review] Part 2: Support exporting contact in RIL, v7 Review of attachment 703256 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +1203,1 @@ > */ Again dataWriter should be put near 'data' parameters to make people easy to understand 'data' and 'dataWriter' are related. @@ +1217,5 @@ > + Buf.writeString(options.data || null); > + } > + } else { > + Buf.writeString(null); > + } So many lines, if-check but only try to write a string, We could move these code to a local function, so we could simply call dataWriter(options.data); here @@ +8497,5 @@ > + * The callback function shall be called when the record is updated. > + * @param onerror [optional] > + * The callback function shall be called when failure. > + */ > + updateLinearFixedEF: function updateLinearFixedEF(options) { I saw you check if recordId exists when calling this function, but can we add a check for recordNumber here? To make this function more correct to use. Otherwise this function will fail and may spend hours to figure it out. @@ +8891,5 @@ > /** > + * Update ICC ADN. > + * > + * @param fileId EF id of the ADN. > + * @param contact The contact will be updated. We should also mention that contact needs to have 'recordId' property. @@ +8900,5 @@ > + function dataWriter(recordSize) { > + GsmPDUHelper.writeAlphaIdDiallingNumber( > + recordSize, > + contact.alphaId ? contact.alphaId : "", > + contact.number ? contact.number : ""); These two lines are wrong. @@ +9754,5 @@ > + updateUSimContact: function updateUSimContact(contact, onsuccess, onerror) { > + let gotPbrCb = function gotPbrCb(pbr) { > + if (pbr.adn) { > + ICCRecordHelper.updateADN(pbr.adn.fileId, contact, onsuccess, onerror); > + } else { // error }

Comment on attachment 703254 [details] [diff] [review] Part 1: Add new interface in nsIRadioInterface, v1 Review of attachment 703254 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/nsIRadioInterfaceLayer.idl @@ +379,5 @@ > + * > + * This function allows two operations: update the existing contact or > + * insert a new contact. > + * If the contact has 'recordId' property, the corresponding record will be > + * updated. If not, the contact will be inserted into a free record. I think 'into a free record' could be removed

Address comment #25

1). Rebase. 2). Address comment #24.

1). Error handling in findFreeICCContact()

Comment on attachment 704439 [details] [diff] [review] Part 2: Support exporting contact in RIL, v9 Review of attachment 704439 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +1358,5 @@ > + * @param contactType "ADN". > + * @param contact The contact will be updated. > + * @param requestId Request id from RadioInterfaceLayer. > + */ > + updateICCContact: function updateICCContact(options) { Could contact possibly be null? What happens if contact is undefined or null? @@ +1378,5 @@ > + } > + > + // If contact has 'recordId' property, updates corresponding record. > + // If not, inserts the contact into a free record. > + if (options.contact.recordId) { What happened here is contact is null/undefined? @@ +8912,5 @@ > + updateADN: function updateADN(fileId, contact, onsuccess, onerror) { > + function dataWriter(recordSize) { > + GsmPDUHelper.writeAlphaIdDiallingNumber(recordSize, > + contact.alphaId, > + contact.number); ditto

Comment on attachment 704398 [details] [diff] [review] Part 1: Add new interface in nsIRadioInterface, v2 Review of attachment 704398 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/nsIRadioInterfaceLayer.idl @@ +146,5 @@ > + * > + * @param errorMsg > + * error message from RIL. > + * @param contactType > + * Type of the contact, ADN. This sentence is incomplete.

Comment on attachment 704439 [details] [diff] [review] Part 2: Support exporting contact in RIL, v9 Review of attachment 704439 [details] [diff] [review]: ----------------------------------------------------------------- Since you still got problems in ICC IO, I would suggest you move those ICC IO related code into one part and ContactHelper to another part, or file another bug if necessary. ::: dom/system/gonk/ril_worker.js @@ +8486,5 @@ > + * The number of the record shall be updated. > + * @param callback [optional] > + * The callback function shall be called when the record is updated. > + * @param onerror [optional] > + * The callback function shall be called when failure. Still you didn't provide comments for data nor dataWriter. If you think the complexity of ICC IO is too much, you could file another bug for handling UPDATE_RECORD/BINARY

Comment on attachment 704439 [details] [diff] [review] Part 2: Support exporting contact in RIL, v9 Review of attachment 704439 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +1194,5 @@ > * Arbitrary integer parameters for the command. > * @param data > * String parameter for the command. > + * @param [optional] dataWriter > + * The function for writing string parameter for UPDATE command. For simplicity, we could just use dataWriter for now, if caller doesn't provide it, we use Buf.writeString(null) as default, this should also simplify updateLinearFixedEF.

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #29) > Comment on attachment 704439 [details] [diff] [review] > Part 2: Support exporting contact in RIL, v9 > > Review of attachment 704439 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/ril_worker.js > @@ +1358,5 @@ > > + * @param contactType "ADN". > > + * @param contact The contact will be updated. > > + * @param requestId Request id from RadioInterfaceLayer. > > + */ > > + updateICCContact: function updateICCContact(options) { > > Could contact possibly be null? > What happens if contact is undefined or null? > > @@ +1378,5 @@ > > + } > > + > > + // If contact has 'recordId' property, updates corresponding record. > > + // If not, inserts the contact into a free record. > > + if (options.contact.recordId) { > > What happened here is contact is null/undefined? I will add a check for contact. Thanks > > @@ +8912,5 @@ > > + updateADN: function updateADN(fileId, contact, onsuccess, onerror) { > > + function dataWriter(recordSize) { > > + GsmPDUHelper.writeAlphaIdDiallingNumber(recordSize, > > + contact.alphaId, > > + contact.number); > > ditto contact already be checked in below to avoid unexpected value. if (!contact || !contact.recordId) { if (onerror) { onerror(GECKO_ERROR_REQUEST_NOT_SUPPORTED); } return; }

1). Address comment #30

1). Move the ICC IO related code into another patch. 2). Address comment #29.

1). Move the ICC IO related code into this patch. 2). Address comment #32. 3). Open a bug for UPDATE_BINARY. (bug 833168)

Comment on attachment 704742 [details] [diff] [review] Part 3: Support UPDATE command, v1 Review of attachment 704742 [details] [diff] [review]: ----------------------------------------------------------------- I think this part should be part 2, since those functions in your part 3 depend on this patch. ::: dom/system/gonk/ril_worker.js @@ +1192,5 @@ > * String type, check the 'pathid' parameter from TS 27.007 +CRSM. > * @param p1, p2, p3 > * Arbitrary integer parameters for the command. > + * @param [optional] dataWriter > + * The function for writing string parameter for the UPDATE command. s/UPDATE/ICC_COMMAND_UPDATE_*/ or you could say UPDATE_RECORD or UPDATE_BINARY. @@ +1197,1 @@ > * @param pin2 Could you also help to add '[optional]' for this parameter? @@ +8433,5 @@ > + * @param fileId > + * The file to operate on, one of the ICC_EF_* constants. > + * @param recordNumber > + * The number of the record shall be updated. > + * @param dataWriter this could be optional, right ? @@ +8434,5 @@ > + * The file to operate on, one of the ICC_EF_* constants. > + * @param recordNumber > + * The number of the record shall be updated. > + * @param dataWriter > + * The function for writing string parameter for UPDATE command. s/UPDATE/ICC_COMMAND_UPDATE_*/ @@ +8451,5 @@ > + let cb = options.callback; > + options.callback = function callback(options) { > + options.callback = cb; > + options.command = ICC_COMMAND_UPDATE_RECORD; > + options.p1 = options.recordNumber; // Record number we could remove the comment since it's so obvious.

Comment on attachment 704741 [details] [diff] [review] Part 2: Support exporting contact, v10 Review of attachment 704741 [details] [diff] [review]: ----------------------------------------------------------------- This patch should be part 3. ::: dom/system/gonk/ril_worker.js @@ +8866,5 @@ > + } > + > + if (!contact || !contact.recordId) { > + if (onerror) { > + onerror(GECKO_ERROR_REQUEST_NOT_SUPPORTED); we should use a better error message here. @@ +9730,5 @@ > + if (pbr.adn) { > + ICCRecordHelper.updateADN(pbr.adn.fileId, contact, onsuccess, onerror); > + } else { > + if (onerror) { > + onerror(GECKO_ERROR_REQUEST_NOT_SUPPORTED); you should use the same error message in readUSimContact.

1). Address comment #37

1). Address comment #38.

Rebase

https://hg.mozilla.org/integration/mozilla-inbound/rev/7890512ebda6 https://hg.mozilla.org/integration/mozilla-inbound/rev/bbbfde578294 https://hg.mozilla.org/integration/mozilla-inbound/rev/3c5e676ed5b0

https://hg.mozilla.org/mozilla-central/rev/7890512ebda6 https://hg.mozilla.org/mozilla-central/rev/bbbfde578294 https://hg.mozilla.org/mozilla-central/rev/3c5e676ed5b0