1161438 - [B2G][RIL] Should return the updated contact after icc.updateICCContact succeed.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[John Dai[:jdai]]

When we exporting contact to SIM, some contact information has been lost. Therefore, we should return the updated contact which is actually stored in SIM.

Comment 1

[John Dai[:jdai]]

Attachment: Part1: Exporting contact to SIM should return updated contact.

Comment 2

[John Dai[:jdai]]

Attachment: Part2: Marionette testcase.

Comment 3

[John Dai[:jdai]]

Attachment: Part3: Xpcshell testcase.

Comment 4

[John Dai[:jdai]]

Attachment: Part2: Marionette testcase.(V2)

Comment 5

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

[Tracking Requested - why for this release]:

Comment 6

[Edgar Chen [:edgar]]

Comment on attachment 8603177 [details] [diff] [review]
Part1: Exporting contact to SIM should return updated contact.

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

Per offline discussion, I think we should do more in this bug, otherwise we can not fix this completely, thank you.

::: dom/system/gonk/ril_worker.js
@@ +9705,5 @@
>     * @param recordSize  The size of linear fixed record.
>     * @param alphaId     Alpha Identifier to be written.
>     * @param number      Dialling Number to be written.
>     */
> +  writeAlphaIdDiallingNumber: function(contact, recordSize, alphaId, number) {

What about returning the written |alphaId| and |number|?

@@ +9863,5 @@
>  
>      return number;
>    },
>  
> +  writeNumberWithLength: function(number, contact) {

Ditto, what about returning the written |number|?

@@ +15153,5 @@
>      } else if (field === USIM_PBR_ANR0) {
>        let anr = Array.isArray(contact.anr) ? contact.anr[0] : null;
> +      ICCRecordHelper.updateANR(pbr, contact.recordId, anr, null, () => {
> +        if (contact.anr && Array.isArray(contact.anr)) {
> +          contact.anr = contact.anr.slice(0,1);

anr could be truncated if it is too long and we don't support EF_EXT now.

@@ -15161,5 @@
>     * @param onerror       Callback to be called when error.
>     */
>    updateContactFieldType2: function(pbr, contact, field, onsuccess, onerror) {
>      let ICCRecordHelper = this.context.ICCRecordHelper;
> -

Keep this empty line.

Comment 7

[Edgar Chen [:edgar]]

Comment on attachment 8603179 [details] [diff] [review]
Part3: Xpcshell testcase.

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

Since I think there will be more utility to be modified, we probably need to do more changes in xpcshell tests. Thank you.

Comment 8

[Edgar Chen [:edgar]]

Comment on attachment 8603274 [details] [diff] [review]
Part2: Marionette testcase.(V2)

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

Please see my comments below and remember to rebase patch for bug 1159622. Thank you.

::: dom/icc/tests/marionette/test_icc_contact.js
@@ +13,5 @@
> +  },
> +  // a contact with email but without anr.
> +  {
> +    name: ["add2"],
> +    tel: [{value: "01234567890123456789"}],

I think we should also have some tests for the case that number is truncated.

@@ +66,5 @@
>      .then((aResult) => {
> +      is(aResult.name[0], aMozContact.name[0]);
> +      is(aResult.tel[0].value, aMozContact.tel[0].value);
> +      ok(aResult.tel.length == 1);
> +      ok(!aResult.email);

I think we shouldn't hardcode to check the result, maybe compare the result with the expected contact? And I think it's worth to have a utility function that helps to compare two contacts.

@@ +76,4 @@
>  
> +          is(aResult[index].name[0], aMozContact.name[0]);
> +          is(aResult[index].tel[0].value, aMozContact.tel[0].value);
> +          is(aResult[index].id, aIcc.iccInfo.iccid + aResult.length);

Ditto.

Comment 9

[John Dai[:jdai]]

Attachment: Part1: Exporting contact to SIM should return updated contact.(V2)

Address comment 6.

Comment 10

[John Dai[:jdai]]

Attachment: Part2: Marionette testcase.(V3)

Address comment 6.

Comment 11

[John Dai[:jdai]]

Attachment: Part3: Xpcshell testcase. (V2)

Address comment 6.

Comment 12

[John Dai[:jdai]]

Attachment: Part2: Marionette testcase.(V3-rebase)

Comment 14

[Edgar Chen [:edgar]]

Comment on attachment 8606165 [details] [diff] [review]
Part1: Exporting contact to SIM should return updated contact.(V2)

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

::: dom/system/gonk/ril_worker.js
@@ +796,5 @@
>     * @param pin2          PIN2 is required for updating FDN.
>     * @param requestId     Request id from RadioInterfaceLayer.
>     */
>    updateICCContact: function(options) {
> +    let onsuccess = function onsuccess(contact) {

s/contact/updatedContact/

@@ +9384,5 @@
>     *
> +   * @param  numOctets Number of total octets to be writen, including trailing
> +   *                   0xff.
> +   * @param  str       String to be written. Could be null.
> +   * @return str       The string has been written.

Don't have to specify a variable name in the documentation for return and one empty line between @param and @return. Here and elsewhere.

/**
 * ...
 *
 * @param ...
 * @param ...
 *
 * @return The string has been written into Buf.
 */

@@ +9425,5 @@
>        GsmPDUHelper.writeHexOctet(0xff);
>      }
> +
> +    if (str) {
> +      return str.substring(0, j);

Some character needs two octets, using number of octects to get substring is wrong actually.

@@ +9427,5 @@
> +
> +    if (str) {
> +      return str.substring(0, j);
> +    }
> +    return;

return str ? str.substring( ... ) : null;

@@ +9440,5 @@
> +   *         Total number of octets to be written. This includes the length of
> +   *         alphaId and the length of trailing unused octets(0xff).
> +   * @param  str
> +   *         String to be written.
> +   * @return str

Ditto

@@ +9709,5 @@
>     *
> +   * @param  recordSize  The size of linear fixed record.
> +   * @param  alphaId     Alpha Identifier to be written.
> +   * @param  number      Dialling Number to be written.
> +   * @return contact     The updated contact.

Ditto and `@return An object contains the alphaId and number that have been written into Buf.`

@@ +9720,5 @@
>      let strLen = recordSize * 2;
>      Buf.writeInt32(strLen);
>  
>      let alphaLen = recordSize - ADN_FOOTER_SIZE_BYTES;
> +    let contactAlphaId = this.writeAlphaIdentifier(alphaLen, alphaId);

s/contactAlphaId/writtenAlphaId/

@@ +9721,5 @@
>      Buf.writeInt32(strLen);
>  
>      let alphaLen = recordSize - ADN_FOOTER_SIZE_BYTES;
> +    let contactAlphaId = this.writeAlphaIdentifier(alphaLen, alphaId);
> +    let contactNumber = this.writeNumberWithLength(number);

s/contactNumber/writtenNumber/

@@ +9773,5 @@
> +   *         Total number of octets to be written. This includes the length of
> +   *         alphaId and the length of trailing unused octets(0xff).
> +   * @param  alphaId
> +   *         Alpha Identifier to be written.
> +   * @return alphaId

Ditto

@@ +9779,5 @@
>     * Unused octets will be written as 0xff.
>     */
>    writeAlphaIdentifier: function(numOctets, alphaId) {
>      if (numOctets === 0) {
>        return;

return null;

@@ +9876,5 @@
>  
> +  /**
> +   * Write Number with Length
> +   * @param  number The value to be written.
> +   * @return number The number has been written.

Ditto.

/**
 * ........
 *
 * @param ...
 *
 * @return ...
 */

@@ +9904,5 @@
>        // Write trailing 0xff of Dialling Number.
>        for (let i = 0; i < ADN_MAX_BCD_NUMBER_BYTES - numLen; i++) {
>          GsmPDUHelper.writeHexOctet(0xff);
>        }
> +      return number.replace(/a/g, "*")

Just cache the original truncated number and return it, instead of doing a replace again.

@@ +9912,5 @@
>        // +1 for numLen
>        for (let i = 0; i < ADN_MAX_BCD_NUMBER_BYTES + 1; i++) {
>          GsmPDUHelper.writeHexOctet(0xff);
>        }
> +      return;

return null;

@@ +12316,5 @@
>     * @param onsuccess   Callback to be called when success.
>     * @param onerror     Callback to be called when error.
>     */
>    updateADNLike: function(fileId, contact, pin2, onsuccess, onerror) {
> +    let updateContact;

s/updateContact/updatedContact/

@@ +12570,5 @@
>        let strLen = recordSize * 2;
>        Buf.writeInt32(strLen);
>  
>        if (fileType == ICC_USIM_TYPE1_TAG) {
> +        email = ICCPDUHelper.writeStringTo8BitUnpacked(recordSize, email);

writtenEmail = ...

@@ +12674,5 @@
>  
>        // EF_AAS record Id. Unused for now.
>        GsmPDUHelper.writeHexOctet(0xff);
>  
> +      number = this.context.ICCPDUHelper.writeNumberWithLength(number);

writtenNumber = .....

@@ +14821,5 @@
>    updateICCContact: function(appType, contactType, contact, pin2, onsuccess, onerror) {
>      let ICCRecordHelper = this.context.ICCRecordHelper;
>      let ICCUtilsHelper = this.context.ICCUtilsHelper;
>  
> +    let modifyContact = (updatedContact) => {

s/modifyContact/updateContactCb/

@@ +14822,5 @@
>      let ICCRecordHelper = this.context.ICCRecordHelper;
>      let ICCUtilsHelper = this.context.ICCUtilsHelper;
>  
> +    let modifyContact = (updatedContact) => {
> +      updatedContact.pbrIndex = contact.pbrIndex;

Handle |pbrIndex| in updateUSimContact().

@@ +14823,5 @@
>      let ICCUtilsHelper = this.context.ICCUtilsHelper;
>  
> +    let modifyContact = (updatedContact) => {
> +      updatedContact.pbrIndex = contact.pbrIndex;
> +      updatedContact.recordId = contact.recordId;

Handle |recordId| in updateADNLike().

@@ +15131,3 @@
>        let field = USIM_PBR_FIELDS[fieldIndex];
>        fieldIndex += 1;
> +      if (fieldEntry) {

This check is a bit tricky.
Why not putting the contact field collecting into the success callback of updateContactField()?

@@ +15185,5 @@
>    updateContactFieldType1: function(pbr, contact, field, onsuccess, onerror) {
>      let ICCRecordHelper = this.context.ICCRecordHelper;
>  
>      if (field === USIM_PBR_EMAIL) {
> +      ICCRecordHelper.updateEmail(pbr, contact.recordId, contact.email, null, (email) => {

s/email/updatedEmail/

@@ +15186,5 @@
>      let ICCRecordHelper = this.context.ICCRecordHelper;
>  
>      if (field === USIM_PBR_EMAIL) {
> +      ICCRecordHelper.updateEmail(pbr, contact.recordId, contact.email, null, (email) => {
> +        onsuccess((email) ? {email: email} : null);

I think `onsuccess({email: updatedEmail})` is enough.

@@ +15192,4 @@
>      } else if (field === USIM_PBR_ANR0) {
>        let anr = Array.isArray(contact.anr) ? contact.anr[0] : null;
> +      ICCRecordHelper.updateANR(pbr, contact.recordId, anr, null, (anr) => {
> +        onsuccess((anr) ? {anr: anr} : null);

Hmm, this seems not correct, currently we expect anr is an array...
I am not sure using array is still a good idea for returning an updated contact, because things probably become much complicated if we support more than one anr. So I am also thinking of not using array for anr, but anr0, anr1, ... etc.

@@ +15244,5 @@
>  
>        // Case 2.
>        if (field === USIM_PBR_EMAIL) {
> +        ICCRecordHelper.updateEmail(pbr, recordId, contact.email, contact.recordId, (email) => {
> +          onsuccess((email) ? {email: email} : null);

Ditto.

@@ +15250,4 @@
>        } else if (field === USIM_PBR_ANR0) {
>          let anr = Array.isArray(contact.anr) ? contact.anr[0] : null;
> +        ICCRecordHelper.updateANR(pbr, recordId, anr, contact.recordId, (anr) => {
> +          onsuccess((anr) ? {anr: anr} : null);

Ditto.

@@ +15284,4 @@
>        }.bind(this);
>  
>        if (field === USIM_PBR_EMAIL) {
> +        ICCRecordHelper.updateEmail(pbr, recordId, contact.email, contact.recordId, (email) => {

s/email/updatedEmail/

@@ +15287,5 @@
> +        ICCRecordHelper.updateEmail(pbr, recordId, contact.email, contact.recordId, (email) => {
> +          if (email) {
> +            contactField.email = email;
> +          }
> +          updateCb();

Just `updateCb({email: updatedEmail});`

@@ +15294,5 @@
> +        ICCRecordHelper.updateANR(pbr, recordId, contact.anr[0], contact.recordId, (anr) => {
> +          if (anr) {
> +            contactField.anr = anr;
> +          }
> +          updateCb();

Ditto.

Comment 15

[Edgar Chen [:edgar]]

Comment on attachment 8606804 [details] [diff] [review]
Part2: Marionette testcase.(V3-rebase)

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

As mention in comment #8, we don't support EF_EXT currently, we should also have some tests for the case that number is truncated due to over 20 digits.

::: dom/icc/tests/marionette/test_icc_contact_add.js
@@ +10,2 @@
>      tel: [{value: "0912345678"}],
>      email:[]

Remove email maybe?

@@ +26,3 @@
>    }];
>  
> +

Nit: Remove this empty line.

@@ +33,5 @@
>    return aIcc.updateContact(aType, contact, aPin2)
>      .then((aResult) => {
>        is(aResult.name[0], aMozContact.name[0]);
>        is(aResult.tel[0].value, aMozContact.tel[0].value);
> +      ok(aResult.tel.length == 1);

Please add some comments about why we expect tel.length is always 1.

@@ +34,5 @@
>      .then((aResult) => {
>        is(aResult.name[0], aMozContact.name[0]);
>        is(aResult.tel[0].value, aMozContact.tel[0].value);
> +      ok(aResult.tel.length == 1);
> +      ok(!aResult.email);

Same here.

Comment 16

[Edgar Chen [:edgar]]

Comment on attachment 8606167 [details] [diff] [review]
Part3: Xpcshell testcase. (V2)

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

Please also revise the xpcshell tests for writeAlphaIdDiallingNumber() in test_ril_worker_icc_ICCPDUHelper.js.
Thank you.

::: dom/system/gonk/tests/test_ril_worker_icc_ICCContactHelper.js
@@ +507,5 @@
> +        equal(aContact.email, updatedContact.email);
> +      }
> +
> +      if (updatedContact.hasOwnProperty('anr')) {
> +        equal(aContact.anr[0], updatedContact.anr);

updatedContact.anr should be an array as well.

@@ +663,5 @@
>                    indexInIAP: 1}}]);
>        }
>      };
>  
> +    let successCb = function(updateContact) {

s/updateContact/updatedContact/

@@ +664,5 @@
>        }
>      };
>  
> +    let successCb = function(updateContact) {
> +      ok(updateContact.email == null);

equal(updatedContact.email, null);

@@ +665,5 @@
>      };
>  
> +    let successCb = function(updateContact) {
> +      ok(updateContact.email == null);
> +      ok(updateContact.anr == null);

anr should be an array.

::: dom/system/gonk/tests/test_ril_worker_icc_ICCPDUHelper.js
@@ +92,5 @@
>      }];
>  
>    for (let i = 0; i < test_data.length; i++) {
>      let test = test_data[i];
> +    let writeStr = iccHelper.writeICCUCS2String(alphaLen, test.data);

It will be good if you could add some test data for the corner case, e.g. the string is too long to be written completely.

@@ +264,5 @@
>  
>    // \u000c is GSM extended alphabet, 2 octets.
>    // \u00a3 is GSM alphabet, 1 octet.
>    let str = "\u000c\u00a3";
> +  writeStr = iccHelper.writeStringTo8BitUnpacked(3, str);

I guess you miss to verify the |writeStr|.

@@ +269,4 @@
>    equal(iccHelper.read8BitUnpackedToString(3), str);
>  
>    str = "\u00a3\u000c";
> +  writeStr = iccHelper.writeStringTo8BitUnpacked(3, str);

Ditto

@@ +274,5 @@
>  
>    // 2 GSM extended alphabets cost 4 octets, but maximum is 3, so only the 1st
>    // alphabet can be written.
>    str = "\u000c\u000c";
> +  writeStr = iccHelper.writeStringTo8BitUnpacked(3, str);

Ditto

@@ +339,5 @@
>    let ffLen = 2;
>  
>    // Removal
> +  let writeAlphaId = iccHelper.writeAlphaIdentifier(10, null);
> +  ok(writeAlphaId == null);

equal(writeAlphaId, null);

@@ +376,2 @@
>    helper.writeHexOctet(0xff); // dummy octet.
> +  ok(writeAlphaId == null);

equal(writeAlphaId, null);

@@ +573,5 @@
>  
>    function test(number, expectedNumber) {
>      expectedNumber = expectedNumber || number;
> +    let writeNumber = iccHelper.writeNumberWithLength(number);
> +    equal(writeNumber, expectedNumber);

It will be good if you could add some tests for the case that length of number is over ADN_MAX_NUMBER_DIGITS.

Comment 17

[John Dai[:jdai]]

Attachment: Part1: Exporting contact to SIM should return updated contact.(V3)

Addressed review comment 14.

Comment 18

[John Dai[:jdai]]

Attachment: Part2: Marionette testcase.(V4)

Addressed review comment 15.

Comment 19

[John Dai[:jdai]]

Attachment: Part3: Xpcshell testcase. (V3)

Addressed review comment 16.

Comment 20

[John Dai[:jdai]]

Attachment: Part1: Exporting contact to SIM should return updated contact.(V4)

Comment 21

[John Dai[:jdai]]

Attachment: Part3: Xpcshell testcase. (V4)

Comment 22

[Edgar Chen [:edgar]]

Comment on attachment 8608076 [details] [diff] [review]
Part1: Exporting contact to SIM should return updated contact.(V4)

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

We are almost there. Please check my comments below, thank you.

::: dom/system/gonk/ril_worker.js
@@ -9385,5 @@
>     * Write GSM 8-bit unpacked octets.
>     *
> -   * @param numOctets   Number of total octets to be writen, including trailing
> -   *                    0xff.
> -   * @param str         String to be written. Could be null.

Nit: Just keep original indentation.

@@ -9435,5 @@
> -   * @param numOctets
> -   *        Total number of octets to be written. This includes the length of
> -   *        alphaId and the length of trailing unused octets(0xff).
> -   * @param str
> -   *        String to be written.

Nit: Just keep original indentation

@@ -9702,5 @@
>     * Write Alpha Identifier and Dialling number from TS 151.011 clause 10.5.1
>     *
> -   * @param recordSize  The size of linear fixed record.
> -   * @param alphaId     Alpha Identifier to be written.
> -   * @param number      Dialling Number to be written.

Nit: Just keep original indentation.

@@ -9762,5 @@
> -   * @param numOctets
> -   *        Total number of octets to be written. This includes the length of
> -   *        alphaId and the length of trailing unused octets(0xff).
> -   * @param alphaId
> -   *        Alpha Identifier to be written.

Nit: Just keep original indentation

@@ +9783,5 @@
>     * Unused octets will be written as 0xff.
>     */
>    writeAlphaIdentifier: function(numOctets, alphaId) {
>      if (numOctets === 0) {
>        return;

return null;

@@ +9878,5 @@
>      return number;
>    },
>  
> +  /**
> +   * Write Number with Length

Nit: One empty line here.

@@ +9901,5 @@
>        let numDigits = number.length - numStart;
>        if (numDigits > ADN_MAX_NUMBER_DIGITS) {
>          number = number.substring(0, ADN_MAX_NUMBER_DIGITS + numStart);
>          numDigits = number.length - numStart;
>        }

let writtenNumber = number.substring(0, numStart) +
                    number.substring(numStart)
                          .replace(/[^0-9*#,]/g, "");

let numDigits = writtenNumber.length - numStart;
if (numDigits > ADN_MAX_NUMBER_DIGITS) {
  writtenNumber = writtenNumber.substring(0, ADN_MAX_NUMBER_DIGITS + numStart);
  numDigits = writtenNumber.length - numStart;
}

....

this.writeDiallingNumber(writtenNumber.replace(/\*/g, "a")
                                      .replace(/\#/g, "b")
                                      .replace(/\,/g, "c"));

....

@@ +9911,5 @@
>        // Write trailing 0xff of Dialling Number.
>        for (let i = 0; i < ADN_MAX_BCD_NUMBER_BYTES - numLen; i++) {
>          GsmPDUHelper.writeHexOctet(0xff);
>        }
> +      return writtenNumber.substring(0, ADN_MAX_NUMBER_DIGITS + numStart);

Then you don't return writtenNumber directly, don't need to do substring() again.

@@ +15202,5 @@
> +      ICCRecordHelper.updateANR(pbr, contact.recordId, anr, null,
> +        (updatedANR) => {
> +          // ANR could have multiple files. If we support more than one anr,
> +          // we will save it as anr0, anr1,...etc.
> +          onsuccess((updatedANR) ? {anr: [updatedANR]} : null);

It seems `onsuccess({anr: [updatedANR]})` is enough.

@@ +15264,5 @@
> +        ICCRecordHelper.updateANR(pbr, recordId, anr, contact.recordId,
> +          (updatedANR) => {
> +            // ANR could have multiple files. If we support more than one anr,
> +            // we will save it as anr0, anr1,...etc.
> +            onsuccess((updatedANR) ? {anr: [updatedANR]} : null);

Ditto.

@@ +15294,4 @@
>      let successCb = function successCb(recordId) {
>        let updateCb = function updateCb() {
> +        this.updateContactFieldIndexInIAP(pbr, contact.recordId, field, recordId, () => {
> +          onsuccess(contactField);

let updateCb = function updateCb(contactField) { ...

@@ +15307,5 @@
> +        ICCRecordHelper.updateANR(pbr, recordId, contact.anr[0], contact.recordId,
> +          (updatedANR) => {
> +            // ANR could have multiple files. If we support more than one anr,
> +            // we will save it as anr0, anr1,...etc.
> +            updateCb((updatedANR) ? {anr: [updatedANR]} : null);

It seems `onsuccess({anr: [updatedANR]})` is enough.

Comment 23

[Edgar Chen [:edgar]]

Comment on attachment 8607938 [details] [diff] [review]
Part2: Marionette testcase.(V4)

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

Mostly looks good, but as per offline discussion, please add one more test for the case that the tel is over 20 digits. Thank you.

Comment 24

[Edgar Chen [:edgar]]

Comment on attachment 8608077 [details] [diff] [review]
Part3: Xpcshell testcase. (V4)

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

Looks good, but per comment #16, please also revise the xpcshell tests for writeAlphaIdDiallingNumber() in test_ril_worker_icc_ICCPDUHelper.js.

::: dom/system/gonk/tests/test_ril_worker_icc_ICCPDUHelper.js
@@ +625,5 @@
>    test("(1)23-456+789", "123456789");
>  
>    test("++(01)2*3-4#5,6+7(8)9*0#1,", "+012*34#5,6789*0#1,");
>  
> +  test("012345678901234567890123456789", "01234567890123456789");

Add comments for this test and one empty line here.

Comment 25

[John Dai[:jdai]]

Attachment: Part1: Exporting contact to SIM should return updated contact.(V5)

Address comment 22.

Comment 26

[John Dai[:jdai]]

Attachment: Part2: Marionette testcase.(V5)

Addressing comment 23.

Comment 27

[John Dai[:jdai]]

Attachment: Part3: Xpcshell testcase. (V5)

Addressing comment 24.

Comment 28

[John Dai[:jdai]]

Comment on attachment 8610361 [details] [diff] [review]
Part1: Exporting contact to SIM should return updated contact.(V5)

Hi Edgar, Can you help me review this patch again? Thanks.

Comment 29

[John Dai[:jdai]]

Comment on attachment 8610362 [details] [diff] [review]
Part2: Marionette testcase.(V5)

Hi Edgar, Can you help me review this patch again? Thanks.

Comment 30

[John Dai[:jdai]]

Comment on attachment 8610363 [details] [diff] [review]
Part3: Xpcshell testcase. (V5)

Hi Edgar, Can you help me review this patch again? Thanks.

Comment 31

[Edgar Chen [:edgar]]

Comment on attachment 8610361 [details] [diff] [review]
Part1: Exporting contact to SIM should return updated contact.(V5)

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

Nice, thank you.

::: dom/system/gonk/ril_worker.js
@@ +9371,5 @@
>     * Write GSM 8-bit unpacked octets.
>     *
> +   * @param numOctets Number of total octets to be writen, including trailing
> +   *                  0xff.
> +   * @param str       String to be written. Could be null.

Nit: Keep original indentation.

Comment 32

[Edgar Chen [:edgar]]

Comment on attachment 8610362 [details] [diff] [review]
Part2: Marionette testcase.(V5)

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

::: dom/icc/tests/marionette/test_icc_contact_add.js
@@ +45,5 @@
>          .then((aResult) => {
>            let contact = aResult[aResult.length - 1];
>  
>            is(contact.name[0], aMozContact.name[0]);
> +          is(contact.tel[0].value, aMozContact.tel[0].value.substring(0, 20));

Add comments about why doing substring here.

Comment 33

[Edgar Chen [:edgar]]

Comment on attachment 8610363 [details] [diff] [review]
Part3: Xpcshell testcase. (V5)

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

Thank you.

Comment 34

[John Dai[:jdai]]

Attachment: Part1: Exporting contact to SIM should return updated contact.(Final), r=echen

Addressed comment 31.

Comment 35

[John Dai[:jdai]]

Attachment: Part2: Marionette testcase.(Final), r=echen

Addressed comment 32.

Comment 36

[John Dai[:jdai]]

Attachment: Part3: Xpcshell testcase. (Final), r=echen

Comment 37

[John Dai[:jdai]]

Try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f49ffd2fb3a&exclusion_profile=false

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/714567bf0da3
https://hg.mozilla.org/integration/b2g-inbound/rev/0559bbc36596
https://hg.mozilla.org/integration/b2g-inbound/rev/a44a427b81e3

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/714567bf0da3
https://hg.mozilla.org/mozilla-central/rev/0559bbc36596
https://hg.mozilla.org/mozilla-central/rev/a44a427b81e3