809726 - B2G RIL: Exporting contacts to SIM card

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Support for exporting Contacts to SIM card.

Comment 1

[Edgar Chen [:edgar]]

Attachment: WIP Part 1: IDL for exporting contacts to SIM, v1

Comment 2

[Edgar Chen [:edgar]]

Comment on attachment 697009 [details] [diff] [review]
WIP Part 1: IDL for exporting contacts to SIM, v1

># HG changeset patch
># User Edgar Chen <[e-mail]>
># 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;
> };

Comment 3

[Edgar Chen [:edgar]]

Attachment: WIP Part 2: Modify ContactManager, v1

Comment 4

[Edgar Chen [:edgar]]

Attachment: WIP Part 3: Support in RIL, v1

Comment 5

[Edgar Chen [:edgar]]

Attachment: WIP, Support exporting contact in RIL, v2

WIP patch v2.

ContactManager part will be handle in bug 827253

Comment 6

[Edgar Chen [:edgar]]

Attachment: WIP, Support exporting contact in RIL, v3

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

Comment 7

[Edgar Chen [:edgar]]

Attachment: WIP, Support exporting contact in RIL, v4

1). Fix some stupid error. :(

Comment 8

[Edgar Chen [:edgar]]

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 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 10

[Edgar Chen [:edgar]]

Attachment: WIP, Support exporting contact in RIL, v5

1). Address review comment #9.

Thanks for your feedback, Yoshi.

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][: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();

??

Comment 12

[Edgar Chen [:edgar]]

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

Comment 13

[Edgar Chen [:edgar]]

Attachment: WIP, Support exporting contact in RIL, v6

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

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

You might need to move IDL to another patch.

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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 18

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 21

[Edgar Chen [:edgar]]

(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

Comment 22

[Edgar Chen [:edgar]]

Attachment: Part 1: Add new interface in nsIRadioInterface, v1

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

Comment 23

[Edgar Chen [:edgar]]

Attachment: Part 2: Support exporting contact in RIL, v7

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

Comment 24

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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 25

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 26

[Edgar Chen [:edgar]]

Attachment: Part 1: Add new interface in nsIRadioInterface, v2

Address comment #25

Comment 27

[Edgar Chen [:edgar]]

Attachment: Part 2: Support exporting contact in RIL, v8

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

Comment 28

[Edgar Chen [:edgar]]

Attachment: Part 2: Support exporting contact in RIL, v9

1). Error handling in findFreeICCContact()

Comment 29

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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 30

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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 31

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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 32

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 33

[Edgar Chen [:edgar]]

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

Comment 34

[Edgar Chen [:edgar]]

Attachment: Part 1: Add new interface in nsIRadioInterface, v3, r=allstars.chh

1). Address comment #30

Comment 35

[Edgar Chen [:edgar]]

Attachment: Part 2: Support exporting contact, v10

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

Comment 36

[Edgar Chen [:edgar]]

Attachment: Part 3: Support UPDATE command, v1

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

Comment 37

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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 38

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 39

[Edgar Chen [:edgar]]

Attachment: Part 2: Support UPDATE command, v2, r=allstars.chh (former Part 3)

1). Address comment #37

Comment 40

[Edgar Chen [:edgar]]

Attachment: Part 3: Support exporting contact, v11, r=allstars.chh (former Part 2)

1). Address comment #38.

Comment 41

[Edgar Chen [:edgar]]

Attachment: Part 1: Add new interface in nsIRadioInterface, v4, r=allstars.chh

Rebase

Comment 42

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 43

[Ryan VanderMeulen [:RyanVM]]

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