Closed Bug 809726 Opened 7 years ago Closed 7 years ago

B2G RIL: Exporting contacts to SIM card

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: allstars.chh, Assigned: edgar)

References

Details

Attachments

(3 files, 16 obsolete files)

4.91 KB, patch
Details | Diff | Splinter Review
12.22 KB, patch
Details | Diff | Splinter Review
3.35 KB, patch
Details | Diff | Splinter Review
Support for exporting Contacts to SIM card.
Assignee: nobody → allstars.chh
Summary: B2G RIL: Export to SIM Contact. → B2G RIL: Exporting contacts to SIM card
Depends on: 821584
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;
> };
Attachment #697009 - Attachment is patch: true
Attachment #697009 - Attachment description: Part 1: IDL for exporting contacts to SIM, v1 → WIP Part 1: IDL for exporting contacts to SIM, v1
Attachment #697010 - Attachment description: Part 2: Modify ContactManager, v1 → WIP Part 2: Modify ContactManager, v1
Attached patch WIP Part 3: Support in RIL, v1 (obsolete) — Splinter Review
Assignee: allstars.chh → echen
Blocks: 827253
WIP patch v2.

ContactManager part will be handle in bug 827253
Attachment #697009 - Attachment is obsolete: true
Attachment #697010 - Attachment is obsolete: true
Attachment #697011 - Attachment is obsolete: true
1). Correct typo. (s/alpaId/alphaId)
Attachment #699181 - Attachment is obsolete: true
1). Fix some stupid error. :(
Attachment #700256 - Attachment is obsolete: true
Attachment #700927 - Flags: feedback?(allstars.chh)
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.
Attachment #700927 - Flags: feedback?(allstars.chh)
1). Address review comment #9.

Thanks for your feedback, Yoshi.
Attachment #700927 - Attachment is obsolete: true
Attachment #701046 - Flags: feedback?(allstars.chh)
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();

??
Attachment #701046 - Flags: feedback?(allstars.chh)
Whiteboard: mon11
blocking-b2g: --- → shira+
Whiteboard: mon11 → [mno11]
(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.
Attachment #701046 - Attachment is obsolete: true
Attachment #702179 - Flags: feedback?
Attachment #702179 - Flags: feedback? → feedback?(allstars.chh)
(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.
Attachment #702179 - Flags: feedback?(allstars.chh) → feedback+
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.
Attachment #702179 - Attachment is obsolete: true
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
blocking-b2g: shira+ → ---
Whiteboard: [mno11]
Address comment #25
Attachment #703254 - Attachment is obsolete: true
1). Rebase.
2). Address comment #24.
Attachment #703256 - Attachment is obsolete: true
1). Error handling in findFreeICCContact()
Attachment #704412 - Attachment is obsolete: true
Attachment #704398 - Flags: review?(allstars.chh)
Attachment #704439 - Flags: review?(allstars.chh)
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.
Attachment #704398 - Flags: review?(allstars.chh) → review+
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
Attachment #704439 - Flags: review?(allstars.chh)
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
Attachment #704398 - Attachment is obsolete: true
1). Move the ICC IO related code into another patch.
2). Address comment #29.
Attachment #704439 - Attachment is obsolete: true
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.
Attachment #704742 - Flags: review+
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.
Attachment #704741 - Flags: review+
No longer blocks: 804679
Blocks: 847820
Blocks: 1157082
You need to log in before you can comment on or make changes to this bug.