1122376 - [B2G][RIL] Support SIM contact dialling number exceed 20 digits

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[John Dai[:jdai]]

According to TS 151.011 clause 10.5.1 and TS 131.102 clause 4.4.2.3 EF_ADN |Dialling Number/SSC String| field,

If the telephone number or SSC is longer than 20 digits, the first 20 digits are stored in this data item and the remainder is stored in an associated record in the EF EXT1. The record is identified by the Extension1 Record Identifier. If ADN/SSC require less than 20 digits, excess nibbles at the end of the data item shall be set to 'F'.

Comment 1

[John Dai[:jdai]]

Attachment: Part 1: Read SIM contact dialling number exceed 20 digits.

Hi Edgar,

May I have your feedback for this approach?

Thanks

Comment 2

[Edgar Chen [:edgar]]

Comment on attachment 8563854 [details] [diff] [review]
Part 1: Read SIM contact dialling number exceed 20 digits.

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

Thanks for your work, per off-line discussion, let's have better modularization for reading ext stuff.
And please see my comments below.

::: dom/system/gonk/ril_worker.js
@@ +10424,5 @@
>        contact = {alphaId: alphaId,
>                   number: number};
>      }
> +    if(extRecordNumber != 0xff) {
> +      this.readExtensionNumber(extFileId, extRecordNumber, contact);

Move reading ext stuff in |readADNLike|, leave this help function just decoding raw data.

@@ +10436,5 @@
> +   * @param  recordNumber The number of the record shall be loaded.
> +   * @param  contact      The contact will be updated.
> +   *
> +   */
> +  readExtensionNumber: function(fileId, recordNumber, contact) {

Move this function to |ICCRecordHelper|.

@@ +15470,5 @@
>  
>      switch (contactType) {
>        case GECKO_CARDCONTACT_TYPE_ADN:
>          if (!this.hasDfPhoneBook(appType)) {
> +          ICCRecordHelper.updateADNLike(ICC_EF_ADN, ICC_EF_EXT1, contact, null, onsuccess, onerror);

I guess this change should not belong to this part.

@@ +15484,5 @@
>          if (!ICCUtilsHelper.isICCServiceAvailable("FDN")) {
>            onerror(CONTACT_ERR_CONTACT_TYPE_NOT_SUPPORTED);
>            break;
>          }
> +        ICCRecordHelper.updateADNLike(ICC_EF_FDN, ICC_EF_EXT2, contact, pin2, onsuccess, onerror);

Ditto

@@ +15748,5 @@
>      let updateAdnCb = function() {
>        this.updateSupportedPBRFields(pbr, contact, onsuccess, onerror);
>      }.bind(this);
>  
> +    this.context.ICCRecordHelper.updateADNLike(pbr.adn.fileId, pbr.ext1.fileId, contact, null,

Ditto.

Comment 3

[John Dai[:jdai]]

Attachment: Part 1: Read SIM contact dialling number exceed 20 digits.(V2)

Comment 4

[John Dai[:jdai]]

Attachment: Part 2: Write SIM contact dialling number exceed 20 digits.

Comment 5

[John Dai[:jdai]]

Attachment: Part 3: Update marionette testcase.

Comment 6

[John Dai[:jdai]]

Attachment: [external/qemu] pull request 132

Comment 7

[Edgar Chen [:edgar]]

Comment on attachment 8569056 [details] [diff] [review]
Part 1: Read SIM contact dialling number exceed 20 digits.(V2)

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

Please see my comments below, thank you.

::: dom/system/gonk/ril_consts.js
@@ +13,5 @@
>   * limitations under the License.
>   */
>  
>  // Set to true to debug all RIL layers
> +this.DEBUG_ALL = true;

Remember to remove this change in formal patch. :)

@@ +595,5 @@
>  // See TS 151.011 clause 10.5.1 EF_ADN, 'Dialling Number'.
>  this.ADN_MAX_NUMBER_DIGITS = 20;
> +// Maximum size of BCD numbers in EXT.
> +// See TS 151.011 clause 10.5.10 EF_EXT1, 'Extension data'.
> +this.EXTENSION_MAX_BCD_NUMBER_BYTES = 10;

s/EXTENSION/EXT/

::: dom/system/gonk/ril_worker.js
@@ +10410,5 @@
>      let alphaId = this.readAlphaIdentifier(alphaLen);
>  
>      let number = this.readNumberWithLength();
>  
> +    // Skip CCP unused octets

// Skip unused octet, CCP

@@ +12877,5 @@
>    /**
>     * Read ICC ADN like EF, i.e. EF_ADN, EF_FDN.
>     *
> +   * @param fileId      EF id of the ADN, FDN or SDN.
> +   * @param extFileId   EF Extension id.

EF id of the EXT.

@@ +12894,5 @@
>        let contact =
>          this.context.ICCPDUHelper.readAlphaIdDiallingNumber(options.recordSize);
>        if (contact) {
> +        if(contact.extRecordNumber != 0xff) {
> +          this.readExtension(extFileId, contact.extRecordNumber, updateContactNumber);

If reading EXT is necessary, should we put subsequent process, e.g. putting pushing contact and loading next adn record, in the callback?

@@ +12903,1 @@
>          contacts.push(contact);

contacts.push({
  alphaId: contact.alphaId,
  number: contact.number
});

@@ +12919,5 @@
>      }
>  
>      let contacts = [];
>      ICCIOHelper.loadLinearFixedEF({fileId: fileId,
> +                                   extFileId: extFileId,

Don't need to pass extFileId to loadLinearFixedEF.

@@ +13383,5 @@
> +   * @param  recordNumber        The number of the record shall be loaded.
> +   * @param  updateContactNumber Update contact number function
> +   *
> +   */
> +  readExtension: function(fileId, recordNumber, updateContactNumber) {

s/updateContactNumber/onsuccess/ and have another onerror callback for error handling (readADNLike needs to handle error case).

@@ +13387,5 @@
> +  readExtension: function(fileId, recordNumber, updateContactNumber) {
> +    function callback(options){
> +      let Buf = this.context.Buf;
> +      let length = Buf.readInt32();
> +      let ext1RecordType = this.context.GsmPDUHelper.readHexOctet();

s/ext1RecordType/recordType/

@@ +13402,5 @@
> +            }
> +            // +1 to skip Identifier
> +            Buf.seekIncoming((EXTENSION_MAX_BCD_NUMBER_BYTES + 1) * Buf.PDU_HEX_OCTET_SIZE);
> +            Buf.readStringDelimiter(length);
> +            updateContactNumber(number);

Trigger onerror callback and bail-out earlier here.

@@ +13426,5 @@
> +
> +    this.context.ICCIOHelper.loadLinearFixedEF({
> +      fileId: fileId,
> +      recordNumber: recordNumber,
> +      callback: callback.bind(this),

Passing onerror callback into loadLinearFixedEF.

@@ +15567,5 @@
>      let gotAdnCb = function gotAdnCb(contacts) {
>        this.readSupportedPBRFields(pbr, contacts, onsuccess, onerror);
>      }.bind(this);
>  
> +    this.context.ICCRecordHelper.readADNLike(pbr.adn.fileId, pbr.ext1.fileId, gotAdnCb, onerror);

EF_EXT is optional, so pbr could contain no ext1 information.

Comment 8

[Edgar Chen [:edgar]]

Comment on attachment 8569057 [details] [diff] [review]
Part 2: Write SIM contact dialling number exceed 20 digits.

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

We should consider more different cases for updating and have better error handling. Please see my below comments.

::: dom/system/gonk/ril_worker.js
@@ +10450,5 @@
>      GsmPDUHelper.writeHexOctet(0xff);
> +    if (extRecordNumber) {
> +      GsmPDUHelper.writeHexOctet(extRecordNumber);
> +    } else {
> +      GsmPDUHelper.writeHexOctet(0xff);

If the original EXT extRecordNumber has an valid value and we overwrite it to 0xff, we should clean corresponding recordNumber in EF_EXT, otherwise we will get leak in EF_EXT.

@@ +12967,5 @@
>        onerror: onerror
>      });
>    },
>  
> +  updateADNLikeWithExtension: function(fileId, extFileId, contact, pin2, onsuccess, onerror) {

An ICC card may not contain EF_EXT because EF_EXT is optional.
For the icc card doesn't support EXT, we should just truncate the number, instead of reporting failure if the number needs to be wrote into EF_EXT.

And move this utility function into `ICCContactHelper`.

@@ +13473,5 @@
> +      }
> +     onsuccess(0xff);
> +    }.bind(this);
> +
> +    ICCRecordHelper.findFreeRecordId(fileId, successCb, errorCb);

this.findFreeRecordId(...)

@@ +13483,5 @@
> +   * @param  extFileId EF Extension id.
> +   * @param  recordId  EF record id of the ADN or FDN.
> +   * @param  onsuccess Callback to be called when success.
> +   */
> +  getADNLikeExtensionRecordNumber: function(fileId, extFileId, recordId, onsuccess) {

onerror callback?

@@ +13499,5 @@
> +
> +      if(extRecordNumber != 0xff) {
> +        onsuccess(extRecordNumber);
> +      } else {
> +        this.findFreeExtension(extFileId, onsuccess);

It seems we could just call findFreeRecordId directly, then we don't have to introduce |findFreeExtension| which looks redundant to me.

@@ +13506,5 @@
> +
> +    this.context.ICCIOHelper.loadLinearFixedEF({
> +      fileId: fileId,
> +      recordNumber: recordId,
> +      callback: callback.bind(this),

Error handling for loadLinearFixedEF.

@@ +13516,5 @@
> +   * @param  fileId       EF Extension id.
> +   * @param  recordNumber The number of the record shall be updated.
> +   * @param  number       Dialling Number to be written.
> +   */
> +  updateExtension: function(fileId, recordNumber, number) {

Put |updateExtension| along with |readExtension|.

@@ +15899,5 @@
>  
> +    this.context.ICCRecordHelper.updateADNLikeWithExtension(pbr.adn.fileId,
> +                                                pbr.ext1.fileId,
> +                                                contact, null,
> +                                                updateAdnCb, onerror);

1. Nit: Indentation
2. And EF_EXT is optional, so pbr could contain no ext1 information.

Comment 9

[Edgar Chen [:edgar]]

Comment on attachment 8569084 [details] [review]
[external/qemu] pull request 132

Looks good, and I left some comments on github.

Comment 10

[Edgar Chen [:edgar]]

Comment on attachment 8569061 [details] [diff] [review]
Part 3:  Update marionette testcase.

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

Per sync, you are going to split this patch into two part (for reading and updating). So I cancel the feedback first. Please feel free to send f? or r? to me when patches are ready. :)

Comment 11

[John Dai[:jdai]]

Attachment: Part 1: Read SIM contact dialling number exceed 20 digits.(V3)

Comment 12

[John Dai[:jdai]]

Attachment: Part 2: Write SIM contact dialling number exceed 20 digits.(V2)

Comment 13

[John Dai[:jdai]]

Attachment: Part 3: Marionette testcase.(V2)

Comment 14

[John Dai[:jdai]]

Attachment: Part 4: xpcshell-test testcase.

Comment 15

[John Dai[:jdai]]

Attachment: Part 1: Read SIM contact dialling number exceed 20 digits.(V4)

Per sync, merge patches into read part.

Comment 16

[John Dai[:jdai]]

Attachment: Part 2: Write SIM contact dialling number exceed 20 digits.(V3)

Per sync, merge patches into write part.

Comment 17

[Edgar Chen [:edgar]]

Comment on attachment 8575143 [details] [diff] [review]
Part 1: Read SIM contact dialling number exceed 20 digits.(V4)

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

The overall architecture is in the right direction, give r+.
But please address the comments below before requesting formal review. Thank you.

::: dom/icc/tests/marionette/test_icc_contact.js
@@ +52,5 @@
>        // Get ICC contact for checking new contact
>        return aIcc.readContacts(aType)
>          .then((aResult) => {
> +          // There are 6 SIM contacts which are harded in emulator
> +          is(aResult.length, 7);

Should this belong to part2?

::: dom/system/gonk/ril_worker.js
@@ +10317,3 @@
>      GsmPDUHelper.writeHexOctet(0xff);
> +    if (extRecordNumber) {
> +      GsmPDUHelper.writeHexOctet(extRecordNumber);

Should this belong to part2?

@@ +12875,4 @@
>     * @param onsuccess   Callback to be called when success.
>     * @param onerror     Callback to be called when error.
>     */
> +  readADNLike: function(fileId, extFileId, onsuccess, onerror) {

The changes in this function looks dazzling. Could we make it simpler and easier to understand?

@@ +12879,4 @@
>      let ICCIOHelper = this.context.ICCIOHelper;
>  
>      function callback(options) {
> +      let updateNextContactRecord = function() {

updateNextContactRecord doesn't really "update" next contact, but just load next record. And it is easy to get confused with updateContactRecord.

@@ +13380,5 @@
> +
> +  /**
> +   * Read Extension Number from TS 151.011 clause 10.5.10, TS 31.102, clause 4.4.2.4
> +   * @param  fileId              EF Extension id
> +   * @param  recordNumber        The number of the record shall be loaded.

Nit: Remove redundant whitespace.
     - Just one ws between `@param` and `recordNumber`.
     - Don't need to have so many ws between `recordNumber` and `The number ...`.

@param recordNumber  The number of the record shall be loaded.

@@ +13385,5 @@
> +   * @param  onsuccess           Callback to be called when success.
> +   * @param  onerror             Callback to be called when error.
> +   */
> +  readExtension: function(fileId, recordNumber, onsuccess, onerror) {
> +    function callback(options){

Nit: One ws before `{`

@@ +13389,5 @@
> +    function callback(options){
> +      let Buf = this.context.Buf;
> +      let length = Buf.readInt32();
> +      let recordType = this.context.GsmPDUHelper.readHexOctet();
> +      let number = "";

Nit: Add one empty line here.

@@ +13392,5 @@
> +      let recordType = this.context.GsmPDUHelper.readHexOctet();
> +      let number = "";
> +      // TS 31.102, clause 4.4.2.4 EFEXT1
> +      // Case 1, Extension1 record is additional data
> +      if(recordType & 0x02) {

Nit: One ws between `if` and `(`

@@ +13403,5 @@
> +            }
> +            // +1 to skip Identifier
> +            Buf.seekIncoming((EXT_MAX_BCD_NUMBER_BYTES + 1) * Buf.PDU_HEX_OCTET_SIZE);
> +            Buf.readStringDelimiter(length);
> +            onerror(number);

Don't have to pass |number| to onerror callback.

@@ +13429,5 @@
> +    this.context.ICCIOHelper.loadLinearFixedEF({
> +      fileId: fileId,
> +      recordNumber: recordNumber,
> +      callback: callback.bind(this),
> +      onerror: onerror.bind(this)

Do not |.bind(this)| here, otherwise you change the |this| of onerror callback.

@@ +15344,5 @@
>      switch (contactType) {
>        case GECKO_CARDCONTACT_TYPE_ADN:
>          if (!this.hasDfPhoneBook(appType)) {
> +          ICCRecordHelper.readADNLike(ICC_EF_ADN,
> +          (ICCUtilsHelper.isICCServiceAvailable("EXT1")) ? ICC_EF_EXT1 : null,

Nit: indentation.

@@ +15355,5 @@
>          if (!ICCUtilsHelper.isICCServiceAvailable("FDN")) {
>            onerror(CONTACT_ERR_CONTACT_TYPE_NOT_SUPPORTED);
>            break;
>          }
> +        ICCRecordHelper.readADNLike(ICC_EF_FDN, ICC_EF_EXT2, onsuccess, onerror);

Should we do |isICCServiceAvailable| checking for EXT2, too?

@@ +15363,5 @@
>            onerror(CONTACT_ERR_CONTACT_TYPE_NOT_SUPPORTED);
>            break;
>          }
>  
> +        ICCRecordHelper.readADNLike(ICC_EF_SDN, ICC_EF_EXT3, onsuccess, onerror);

Ditto.

@@ +15574,3 @@
>      let gotAdnCb = function gotAdnCb(contacts) {
>        this.readSupportedPBRFields(pbr, contacts, onsuccess, onerror);
>      }.bind(this);

Nit: Add one empty line here.

::: dom/system/gonk/tests/test_ril_worker_icc_ICCContactHelper.js
@@ +349,5 @@
>      record.readPBR = function readPBR(onsuccess, onerror) {
>        onsuccess(JSON.parse(JSON.stringify(aTestData.pbrs)));
>      };
>  
> +    record.readADNLike = function readADNLike(fileId, extFileId, onsuccess, onerror) {

Since we add some new logic in readADNLike, please also add some tests for it.

::: dom/system/gonk/tests/test_ril_worker_icc_ICCRecordHelper.js
@@ +763,5 @@
> +      do_print("number:"+number);
> +      equal(number, expectedExtensionNumber);
> +    };
> +
> +   record.readExtension(0x6f4a, 1, updateContactRecord, updateContactRecord);

|updateContactRecord| isn't a good naming here.
And with this setup we are unable to check if onsuccess/onerror callback is filed as expected.

@@ +765,5 @@
> +    };
> +
> +   record.readExtension(0x6f4a, 1, updateContactRecord, updateContactRecord);
> +  }
> +  //Invalid record type 0x01

s/Invalid/Unsupported/

Comment 18

[Edgar Chen [:edgar]]

Comment on attachment 8575145 [details] [diff] [review]
Part 2: Write SIM contact dialling number exceed 20 digits.(V3)

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

Overall architecture is in the right direction, so give f+.
But there are still many places need to be addressed. please address them before requesting formal review.
Thank you.

::: dom/icc/tests/marionette/test_icc_contact.js
@@ +70,5 @@
>      });
>  }
>  
> +function testChangeContact(aIcc, aType, aData, aExpect, aPin2) {
> +  log("testChangeContact: type=" + aType + ", pin2=" + aPin2);

Print all the param.

@@ +73,5 @@
> +function testChangeContact(aIcc, aType, aData, aExpect, aPin2) {
> +  log("testChangeContact: type=" + aType + ", pin2=" + aPin2);
> +  let iccId = aIcc.iccInfo.iccid;
> +
> +  return aIcc.readContacts(aType)

Why we have to readContacts first?

@@ +78,5 @@
> +    .then((aResult) => {
> +      let contact = aResult[aData.index];
> +      contact.tel[0].value = aData.value;
> +
> +      return aIcc.updateContact(aType, contact, aPin2)

s/contact/aContact/

@@ +90,5 @@
> +              ok(false, "Cannot get " + aType + " contacts: " + aError.name);
> +            })
> +        }, (aError) => {
> +          if (aType === "fdn" && aPin2 === undefined) {
> +            ok(aError.name === "SimPin2",

is(aError.name, GECKO_ERROR_SIM_PIN2, ...);

@@ +103,4 @@
>  // Start tests
>  startTestCommon(function() {
>    let icc = getMozIcc();
> +  const TEST_DATA = [

Move TEST_DATA to global scrope.

@@ +108,5 @@
> +      data: {
> +        index: 0,
> +        name: "Mozilla",
> +        value: "9876543210987654321001234"},
> +      expect: {

If we expect the contact should be updated as what we give in |data|, we still need |expect|, couldn't we just use |data| as expected result?

@@ +121,5 @@
> +        value: "98765432109876543210998877665544332211001234"},
> +      expect: {
> +        index: 1,
> +        name: "Saßê黃",
> +        value: "9876543210987654321099887766554433221100"}

Is any reason that data.value and expect.value are different?

@@ +151,4 @@
>  
>    // Test read adn contacts
> +  promise = testReadContacts(icc, "adn")
> +  // Test add adn contacts

Nit: indentation.

e.g.
promise = testReadContacts(icc, "adn")
  // ...
  .then(...)
  .then(...);

@@ +176,5 @@
> +
> +  for (let i = 0; i < TEST_DATA.length; i++) {
> +    let test_data = TEST_DATA[i];
> +    promise = promise.then(() =>
> +      testChangeContact(icc, "fdn", test_data.data, test_data.expect, "0000"));

Merge three `for` into one and remember to add some comments.

e.g.
for (let i = 0; i < TEST_DATA.length; i++) {
  promise = promise
    // ...
    .then(() => testChangeContact(...))
    // ...
    .then(() => testChangeContact(...))
    // ...
    .then(() => testChangeContact(...));

}

::: dom/system/gonk/ril_worker.js
@@ +10299,5 @@
>     *
> +   * @param recordSize          The size of linear fixed record.
> +   * @param alphaId             Alpha Identifier to be written.
> +   * @param number              Dialling Number to be written.
> +   * @param extRecordNumber     The record identifier of EXT.

Nit: Two ws between param name and description is enough

@@ +13437,5 @@
>    },
>  
> +  /**
> +   * Update Extension.
> +   * @param  fileId       EF Extension id.

Nit: Just one ws between `@param` and param name, here and elsewhere.

@@ +13441,5 @@
> +   * @param  fileId       EF Extension id.
> +   * @param  recordNumber The number of the record shall be updated.
> +   * @param  number       Dialling Number to be written.
> +   * @param  onsuccess    Callback to be called when success.
> +   * @param  onerror      Callback to be called when error.

Nit: Just one ws between `@param` and param name.

@@ +13444,5 @@
> +   * @param  onsuccess    Callback to be called when success.
> +   * @param  onerror      Callback to be called when error.
> +   */
> +  updateExtension: function(fileId, recordNumber, number, onsuccess, onerror) {
> +    let dataWriter = function dataWriter(recordSize) {

let dataWrite = function(recordSize) {
  ...
};

@@ +13461,5 @@
> +      // Write trailing 0xff of Extension data.
> +      for (let i = 0; i < EXT_MAX_BCD_NUMBER_BYTES - numLen; i++) {
> +        GsmPDUHelper.writeHexOctet(0xff);
> +      }
> +      // Write trailing 0xff of Identifier.

// Write trailing 0xff for Identifier. ?

@@ +13467,5 @@
> +      Buf.writeStringDelimiter(strLen);
> +    }
> +
> +    this.context.ICCIOHelper.updateLinearFixedEF({
> +     fileId: fileId,

Nit: indentation.

@@ +13481,5 @@
> +   * @param  recordNumber The number of the record shall be updated.
> +   * @param  onsuccess    Callback to be called when success.
> +   * @param  onerror      Callback to be called when error.
> +   */
> +  cleanExtension: function(fileId, recordNumber, onsuccess, onerror) {

I think this could be a generic function to clean one specific record in a file, so please have a generic name for it.

@@ +13512,5 @@
> +   * @param  recordId  EF record id of the ADN or FDN.
> +   * @param  onsuccess Callback to be called when success.
> +   * @param  onerror   Callback to be called when error.
> +   */
> +  getADNLikeExtensionRecordNumber: function(fileId, extFileId, recordId, onsuccess, onerror) {

s/recordId/recordNumber/

@@ +13520,5 @@
> +      let alphaLen = options.recordSize - ADN_FOOTER_SIZE_BYTES;
> +
> +      Buf.seekIncoming(alphaLen * Buf.PDU_HEX_OCTET_SIZE);
> +      // Skip numLen, BCD Number, CCP octets.
> +      Buf.seekIncoming((ADN_MAX_BCD_NUMBER_BYTES + 2) * Buf.PDU_HEX_OCTET_SIZE);

If we know that the data is located in the last byte of a record, couldn't we just seek to the last byte and read it?

@@ +16104,5 @@
> +   * @param  pin2      PIN2 is required when updating ICC_EF_FDN.
> +   * @param  onsuccess Callback to be called when success.
> +   * @param  onerror   Callback to be called when error.
> +   */
> +  updateADNLikeWithExtension: function(fileId, extFileId, contact, pin2, onsuccess, onerror) {

I think there are still some spaces we can improve this function to make it simpler and easier to understand. Please try to make the logic straightforward and sometime you can use the `Closures` to help to cache something. :)

@@ +16115,5 @@
> +                     contact.number.substring(numStart)
> +                     .replace(/[^0-9*#,]/g, "")
> +                     .replace(/\*/g, "a")
> +                     .replace(/\#/g, "b")
> +                     .replace(/\,/g, "c");

|writeNumberWithLength| also has similar codes as above, it seems it's worth to put them into a utility function.

@@ +16148,5 @@
> +            );
> +          }
> +        }, onerror);
> +    } else {
> +      ICCRecordHelper.getADNLikeExtensionRecordNumber(fileId, extFileId, contact.recordId,

No matter need extension or not, we need to get extension number, could we just have one |getADNLikeExtensionRecordNumber|?

e.g.

ICCRecordHelper.getADNLikeExtensionRecordNumber(.... ,
  (extRecordNumber) => {
      if (isNeedExtension) {
        ...
      } else {
        ...
      }
  }, onerror);

::: dom/system/gonk/tests/test_ril_worker_icc_ICCContactHelper.js
@@ +435,5 @@
>          }]);
>        }
>      };
>  
> +    recordHelper.updateADNLike = function(fileId, extRecordNumber, contact, pin2, onsuccess, onerror) {

check extRecordNumber, too.

@@ -582,5 @@
>      do_print("Test update RUIM fdn contacts with enhanced phone book");
>      do_test(CARD_APPTYPE_RUIM, GECKO_CARDCONTACT_TYPE_FDN, contact, "1234",
>              null, true);
>    }
> -

Keep this empty line.

::: dom/system/gonk/tests/test_ril_worker_icc_ICCPDUHelper.js
@@ +378,4 @@
>  
>    let contactR = helper.readAlphaIdDiallingNumber(recordSize);
>    equal(contactW.alphaId, contactR.alphaId);
>    equal(contactW.number, contactR.number);

check contactR.extRecordNumber.

@@ +390,3 @@
>    contactR = helper.readAlphaIdDiallingNumber(recordSize);
>    equal(contactUCS2.alphaId, contactR.alphaId);
>    equal(contactUCS2.number, contactR.number);

Ditto.

@@ +410,3 @@
>    contactR = helper.readAlphaIdDiallingNumber(recordSize);
>    equal(contactR.alphaId, "AAAAAAAAABBBBBBBBB");
>    equal(contactR.number, "12345678901234567890");

Ditto.

@@ +418,3 @@
>    contactR = helper.readAlphaIdDiallingNumber(recordSize);
>    equal(contactR.alphaId, "AAAAAAAAABBBBBBBBB");
>    equal(contactR.number, "+12345678901234567890");

Ditto.

::: dom/system/gonk/tests/test_ril_worker_icc_ICCRecordHelper.js
@@ +732,5 @@
>  
>  /**
> + *  Verify ICCRecordHelper.updateADNLikeWithExtension
> + */
> +add_test(function test_update_adn_like_with_extension() {

This test should belong to test_ril_worker_icc_ICCContactHelper.js.

And please help to add some test to check the logic of clean/update extension.

@@ +739,5 @@
> +  let ril = context.RIL;
> +  let record = context.ICCRecordHelper;
> +  let io = context.ICCIOHelper;
> +  let pdu = context.ICCPDUHelper;
> +  let buf = context.Buf;

io, pdu and buf are unused variable.

@@ +742,5 @@
> +  let pdu = context.ICCPDUHelper;
> +  let buf = context.Buf;
> +
> +  ril.appType = CARD_APPTYPE_SIM;
> +  let fileId, extFileId;

Nit: One declaration per line.

@@ +746,5 @@
> +  let fileId, extFileId;
> +
> +  function do_test(fileId, extFileId, contact, expectedExtRecordNumber) {
> +    do_print("Verify ICCRecordHelper.updateADNLikeWithExtension");
> +    //Override

// Override

@@ +749,5 @@
> +    do_print("Verify ICCRecordHelper.updateADNLikeWithExtension");
> +    //Override
> +    record.getADNLikeExtensionRecordNumber = function() {
> +      equal(0x6f4a, expectedExtRecordNumber);
> +    }

missing ;

@@ +752,5 @@
> +      equal(0x6f4a, expectedExtRecordNumber);
> +    }
> +    record.updateADNLike = function() {
> +      equal(0xff, expectedExtRecordNumber);
> +    }

Ditto.

@@ +754,5 @@
> +    record.updateADNLike = function() {
> +      equal(0xff, expectedExtRecordNumber);
> +    }
> +    record.updateADNLikeWithExtension(fileId, extFileId, contact);
> +  }

Ditto.

@@ +759,5 @@
> +
> +  fileId = ICC_EF_ADN;
> +  extFileId = ICC_EF_EXT1;
> +  do_test(fileId, extFileId,
> +    {recordId: 1, alphaId: "test", number: ""}, 0xff);

Nit: Indentation.

@@ +825,5 @@
> +/**
> + *  Verify ICCRecordHelper.updateExtension
> + */
> +add_test(function test_update_extension() {
> +  const recordSize = 13;

Nit: Use ALL_CAPS for constants

@@ +827,5 @@
> + */
> +add_test(function test_update_extension() {
> +  const recordSize = 13;
> +  const recordNumber = 1;
> +  const number = "01234567890123456789";

Nit: one space here.

@@ +835,5 @@
> +  let ril = context.RIL;
> +  let recordHelper = context.ICCRecordHelper;
> +  let buf = context.Buf;
> +  let ioHelper = context.ICCIOHelper;
> +  let count = 0;

unused variable.

@@ +864,5 @@
> +
> +      // pathId.
> +      if (ril.appType == CARD_APPTYPE_SIM || ril.appType == CARD_APPTYPE_RUIM) {
> +        equal(this.readString(),
> +                  EF_PATH_MF_SIM + EF_PATH_DF_TELECOM);

Nit: indentation, aligned with `this.readString()`

@@ +867,5 @@
> +        equal(this.readString(),
> +                  EF_PATH_MF_SIM + EF_PATH_DF_TELECOM);
> +      } else{
> +        equal(this.readString(),
> +                  EF_PATH_MF_SIM + EF_PATH_DF_TELECOM + EF_PATH_DF_PHONEBOOK);

Ditto.

@@ +882,5 @@
> +
> +      // data.
> +      let strLen = this.readInt32();
> +      // Extension record
> +      let data = pduHelper.readHexOctet();

s/data/recordType/

@@ +887,5 @@
> +
> +      equal(data, 0x02);
> +      equal(pduHelper.readHexOctet(), 10);
> +      equal(
> +        pduHelper.readSwappedNibbleExtendedBcdString(expectedNumber.length-1),

Nit: space around operator.

@@ +902,5 @@
> +      }
> +    };
> +
> +    recordHelper.updateExtension(fileId, recordNumber, number);
> +  }

Nit: One empty line here.

@@ +921,5 @@
> +/**
> + *  Verify ICCRecordHelper.cleanExtension
> + */
> +add_test(function test_clean_extension() {
> +  const recordSize = 13;

Nit: Use ALL_CAPS for constants

@@ +931,5 @@
> +  let ril = context.RIL;
> +  let recordHelper = context.ICCRecordHelper;
> +  let buf = context.Buf;
> +  let ioHelper = context.ICCIOHelper;
> +  let count = 0;

unused variable.

@@ +959,5 @@
> +      equal(this.readInt32(), fileId);
> +
> +      // pathId.
> +      equal(this.readString(),
> +                  EF_PATH_MF_SIM + EF_PATH_DF_TELECOM);

Nit: indentation.

@@ +996,5 @@
> +  do_test(ICC_EF_EXT1, "01234567890123456789");
> +  do_test(ICC_EF_EXT2, "01234567890123456789");
> +
> +  run_next_test();
> +});

Nit: One empty line here.

@@ +1001,5 @@
> +/**
> + *  Verify ICCRecordHelper.getADNLikeExtensionRecordNumber
> + */
> +add_test(function test_get_adn_like_extension_record_number() {
> +  const recordSize = 0x20;

Nit: Use ALL_CAPS for constants and add one empty line here.

Comment 19

[John Dai[:jdai]]

Attachment: Part 1: Read SIM contact dialling number exceed 20 digits.(V5)

Address comment 17.

Comment 20

[John Dai[:jdai]]

Attachment: Part 2: Write SIM contact dialling number exceed 20 digits.(V4)

Address comment 18.

Comment 21

[Edgar Chen [:edgar]]

Comment on attachment 8579072 [details] [diff] [review]
Part 1: Read SIM contact dialling number exceed 20 digits.(V5)

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

Please see my comments below. Thank you.

::: dom/system/gonk/ril_consts.js
@@ +621,5 @@
> +// See TS 151.011 clause 10.5.10 EF_EXT1, 'Extension data'.
> +this.EXT_MAX_BCD_NUMBER_BYTES = 10;
> +// Maximum digits of the Dialling Number in EXT.
> +// See TS 151.011 clause 10.5.10 EF_EXT1, 'Extension data'.
> +this.EXT_MAX_NUMBER_DIGITS = 20;

Nit: Add one empty line here.

::: dom/system/gonk/ril_worker.js
@@ +12871,5 @@
>  
>      function callback(options) {
> +      let loadNextContactRecord = function() {
> +        if (options.p1 < options.totalRecords) {
> +          ICCIOHelper.loadNextRecord(options);

Bail-out early,

if (options.p1 < options.totalRecords) {
  ICCIOHelper.loadNextRecord(options);
  return;
}

.....

@@ +12898,2 @@
>  
> +        if(extFileId && contact.extRecordNumber != 0xff) {

Nit: Add white space between `if` and `(`.

@@ +12900,5 @@
> +          this.readExtension(extFileId, contact.extRecordNumber, (number) => {
> +            if (number) {
> +              record.number += number;
> +            }
> +            return loadNextContactRecord();

Don't need to return, just loadNextContactRecord(); seems enough.

@@ +12906,1 @@
>          }

return here?

@@ +13392,5 @@
> +            }
> +            // +1 to skip Identifier
> +            Buf.seekIncoming((EXT_MAX_BCD_NUMBER_BYTES + 1) * Buf.PDU_HEX_OCTET_SIZE);
> +            Buf.readStringDelimiter(length);
> +            return onerror();

onerror();
return;

::: dom/system/gonk/tests/test_ril_worker_icc_ICCRecordHelper.js
@@ +508,5 @@
> +  let worker = newUint8Worker();
> +  let context = worker.ContextPool._contexts[0];
> +  let helper = context.GsmPDUHelper;
> +  let record = context.ICCRecordHelper;
> +  let buf    = context.Buf;

Nit: Don't need to align `=` with previous line.

@@ +509,5 @@
> +  let context = worker.ContextPool._contexts[0];
> +  let helper = context.GsmPDUHelper;
> +  let record = context.ICCRecordHelper;
> +  let buf    = context.Buf;
> +  let io     = context.ICCIOHelper;

Ditto.

@@ +512,5 @@
> +  let buf    = context.Buf;
> +  let io     = context.ICCIOHelper;
> +  let ril = context.RIL;
> +
> +  ril.appType = CARD_APPTYPE_SIM;

Remove this line given that we also do this in line #561.

@@ +525,5 @@
> +        helper.writeHexOctet(parseInt(rawEF.substr(i, 2), 16));
> +      }
> +
> +      // Write string delimiter
> +      buf.writeStringDelimiter(rawEF.length * 2);

Nit: Add one empty line here.

@@ +539,5 @@
> +    io.loadNextRecord = function fakeLoadNextRecord(options) {
> +      ifLoadEF = true;
> +    };
> +
> +    ok(!ifLoadEF);

Having |ifLoadEF| checking is good, but the check here is meaningless.
I guess what you wanna do is,

let ifLoadEF = false;
let successCb = function(contacts) {
  ....
  ifLoadEF = true;
}

record.readADNLike(..., successCb, ...);
ok(ifLoadEF);

And |options.totalRecords| don't have to be 2 actually, unless you wanna test multiple record, but it seems not the case.

@@ +546,5 @@
> +      onsuccess("1234");
> +    }
> +
> +    let successCb = function successCb(contacts) {
> +      equal(contacts[0].number, expectedNumber);

It seems we won't do this equal checking actually, because |loadNextRecord| doesn't trigger onsuccess call back.

@@ +558,5 @@
> +    record.readADNLike(fileId, extFileId, successCb, errorCb);
> +  }
> +
> +  ril.appType = CARD_APPTYPE_SIM;
> +  do_test(ICC_EF_ADN, ICC_EF_EXT1,"436f6e74616374303031ffffffffffffffff0b8199887766554433221100ff01",

1. Please add one more test passing |null| as extFileId.
2. Looks like we don't really need to pass ICC_EF_ADN, we could just hard code them in do_test().
3. And please help to add some comments to explain the raw EF and what case we want to test.

@@ +560,5 @@
> +
> +  ril.appType = CARD_APPTYPE_SIM;
> +  do_test(ICC_EF_ADN, ICC_EF_EXT1,"436f6e74616374303031ffffffffffffffff0b8199887766554433221100ff01",
> +          0x01,"998877665544332211001234");
> +  do_test(ICC_EF_ADN, ICC_EF_EXT1,"436f6e74616374303031ffffffffffffffff0b8199887766554433221100ffff",

Ditto

@@ +804,5 @@
> +  let worker = newUint8Worker();
> +  let context = worker.ContextPool._contexts[0];
> +  let helper = context.GsmPDUHelper;
> +  let record = context.ICCRecordHelper;
> +  let buf    = context.Buf;

Nit: Don't need to align `=` with previous line.

@@ +805,5 @@
> +  let context = worker.ContextPool._contexts[0];
> +  let helper = context.GsmPDUHelper;
> +  let record = context.ICCRecordHelper;
> +  let buf    = context.Buf;
> +  let io     = context.ICCIOHelper;

Ditto

@@ +826,5 @@
> +      }
> +    };
> +
> +    let successCb = function successCb(number) {
> +      do_print("number:"+number);

Nit: Space around operator.

@@ +832,5 @@
> +    };
> +
> +    let errorCb = function errorCb() {
> +      if (expectedExtensionNumber == null) {
> +        ok(true);

ok(expectedExtensionNumber == null);

@@ +836,5 @@
> +        ok(true);
> +      }
> +    };
> +    record.readExtension(0x6f4a, 1, successCb, errorCb);
> +  }

Nit: Add one empty line here.

@@ +837,5 @@
> +      }
> +    };
> +    record.readExtension(0x6f4a, 1, successCb, errorCb);
> +  }
> +  //Unsupported record type 0x01

Add one white space after `//`.

e.g.
// Test unsupported record type 0x01.

Comment 22

[Edgar Chen [:edgar]]

Comment on attachment 8579073 [details] [diff] [review]
Part 2: Write SIM contact dialling number exceed 20 digits.(V4)

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

Please see my comments below. Thank you.

::: dom/icc/tests/marionette/test_icc_contact.js
@@ +7,5 @@
> +const TEST_DATA = [{
> +    data: {
> +      index: 0,
> +      name: "Mozilla",
> +      value: "9876543210987654321001234"},

s/value/number/, here and elsewhere.

@@ +17,5 @@
> +      name: "Saßê黃",
> +      //Unsupported extension chain
> +      value: "98765432109876543210998877665544332211001234"},
> +    expect: {
> +      value: "9876543210987654321099887766554433221100"}

Move the comment here and revise it a bit.
e.g. // We don't support extension chain for now.

@@ +145,5 @@
>  
>    // Test read adn contacts
> +  promise = testReadContacts(icc, "adn")
> +  // Test add adn contacts
> +  .then(() => testAddContact(icc, "adn"))

Nit: indentation.

e.g.
promise = testReadContacts(icc, "adn")
  // ...
  .then(...)
  .then(...);

::: dom/system/gonk/ril_worker.js
@@ +7018,5 @@
>  
>    /**
> +   * Convert string to a GSM extended BCD string
> +   */
> +  stringToExtendedBcd: function(number) {

s/number/string/ maybe.

@@ +7019,5 @@
>    /**
> +   * Convert string to a GSM extended BCD string
> +   */
> +  stringToExtendedBcd: function(number) {
> +    let numStart = number[0] == "+" ? 1 : 0;

This utility function is to cover string to extneded BCD code, it should not care about the string contains `+` or not.

@@ +10330,1 @@
>      GsmPDUHelper.writeHexOctet(0xff);

Nit: Add one empty line here.

@@ +10493,5 @@
>      let GsmPDUHelper = this.context.GsmPDUHelper;
>  
>      if (number) {
>        let numStart = number[0] == "+" ? 1 : 0;
> +      number = GsmPDUHelper.stringToExtendedBcd(number);

number = number.substring(0, numStart) +
         GsmPDUHelper.stringToExtendedBcd(number.substring(numStart));

@@ +13462,5 @@
> +      GsmPDUHelper.writeHexOctet(0x02);
> +      GsmPDUHelper.writeHexOctet(numLen);
> +      GsmPDUHelper.writeSwappedNibbleBCD(number);
> +      // Write trailing 0xff of Extension data.
> +      for (let i = 0; i < EXT_MAX_BCD_NUMBER_BYTES - numLen; i++) {

What if numLen > EXT_MAX_BCD_NUMBER_BYTES? I guess we should have better error handling for that.

@@ +13492,5 @@
> +    let dataWriter = function dataWriter(recordSize) {
> +      let GsmPDUHelper = this.context.GsmPDUHelper;
> +      // Write String length
> +      let strLen = recordSize * 2;
> +      let Buf = this.context.Buf;

Nit: Reorder the sequence a bit.

let GsmPDUHelper = ...
let Buf = ...
// Write String length
let strLen = recordSize * 2;

@@ +13518,5 @@
> +   * @param  recordNumber  EF record id of the ADN or FDN.
> +   * @param  onsuccess Callback to be called when success.
> +   * @param  onerror   Callback to be called when error.
> +   */
> +  getADNLikeExtensionRecordNumber: function(fileId, extFileId, recordNumber, onsuccess, onerror) {

Don't need |extFileId|.

@@ +13525,5 @@
> +      let length = Buf.readInt32();
> +      let alphaLen = options.recordSize - ADN_FOOTER_SIZE_BYTES;
> +
> +      // Skip alphaLen, numLen, BCD Number, CCP octets.
> +      Buf.seekIncoming((alphaLen + 13) * Buf.PDU_HEX_OCTET_SIZE);

Buf.seekIncoming((options.recordSize -1) * Buf.PDU_HEX_OCTET_SIZE);

@@ +15907,5 @@
> +                                      contact, null,
> +                                      updateAdnCb, onerror);
> +    } else {
> +      this.context.ICCRecordHelper.updateADNLike(pbr.adn.fileId,
> +                                                  0xff,

Nit: Indentation.

@@ +16117,5 @@
> +  updateADNLikeWithExtension: function(fileId, extFileId, contact, pin2, onsuccess, onerror) {
> +    let ICCRecordHelper = this.context.ICCRecordHelper;
> +    let numStart = contact.number[0] == "+" ? 1 : 0;
> +    let number = this.context.GsmPDUHelper.stringToExtendedBcd(contact.number);
> +    let extContactNumber = number.substring(ADN_MAX_NUMBER_DIGITS + numStart,

s/extContactNumber/extNumber/

And maybe

let extNumber = number.substr(numStart + ADN_MAX_NUMBER_DIGITS,
                              EXT_MAX_NUMBER_DIGITS);

@@ +16123,5 @@
> +                                          EXT_MAX_NUMBER_DIGITS);
> +    let isNeedExtension = contact.number.substring(numStart).length >
> +                          ADN_MAX_NUMBER_DIGITS;
> +
> +    let updateADNLike = function(extRecordNumber) {

Put the declaration of a local variables as near to their use as possible. e.g. in onsuccess callback of |ICCRecordHelper.getADNLikeExtensionRecordNumbe|

@@ +16124,5 @@
> +    let isNeedExtension = contact.number.substring(numStart).length >
> +                          ADN_MAX_NUMBER_DIGITS;
> +
> +    let updateADNLike = function(extRecordNumber) {
> +      return ICCRecordHelper.updateADNLike(fileId, extRecordNumber,

Async function usually doesn't return anything, the result will be carried in callback function.

@@ +16129,5 @@
> +                                            contact, pin2, onsuccess, onerror)
> +    }
> +
> +    let updateExtension = function(extRecordNumber, extContactNumber) {
> +      return ICCRecordHelper.updateExtension(extFileId, extRecordNumber,

Ditto.

@@ +16137,5 @@
> +    }
> +
> +    ICCRecordHelper.getADNLikeExtensionRecordNumber(fileId, extFileId, contact.recordId,
> +      (extRecordNumber) => {
> +        if (isNeedExtension) {

Couldn't we just reuse extNumber?

e.g.
if (extNumber) {
  ...

@@ +16138,5 @@
> +
> +    ICCRecordHelper.getADNLikeExtensionRecordNumber(fileId, extFileId, contact.recordId,
> +      (extRecordNumber) => {
> +        if (isNeedExtension) {
> +          if (extRecordNumber != 0xff) {

Let's bail-out early which can provide a better readability.

e.g.

if (....) {
  if (....) {
    ...
    return;
  }

  ...
  return;
}

.....

::: dom/system/gonk/tests/test_ril_worker_icc_ICCContactHelper.js
@@ +795,5 @@
> +
> +  ril.appType = CARD_APPTYPE_SIM;
> +
> +  let fileId = ICC_EF_ADN;
> +  let extFileId = ICC_EF_EXT1;

If we always pass same value as argument into |do_test|, why don't just move them into do_test scope. then do_test won't need these two arguments as argument.

@@ +809,5 @@
> +      equal(extRecordNumber, expectedExtRecordNumber);
> +    }
> +
> +    record.updateExtension = function(fileId, recordNumber, number, onsuccess, onerror) {
> +      onsuccess();

updateADNLikeWithExtension() contains some different logic and flow, e.g. for some cases, updateExtension won't be called actually ...etc.

Always triggering onsuccess callback cannot really check if the result is what we expect to get. Please enhance this test to make it can cover all different cases.

@@ +817,5 @@
> +      onsuccess(0x02);
> +    }
> +
> +    record.cleanEFRecord = function(fileId, recordNumber, onsuccess, onerror) {
> +      onsuccess();

Ditto. Always triggering onsuccess callback cannot really check if the result is what we expect to get. Please enhance this test to make it can cover all different cases.

@@ +824,5 @@
> +    record.updateADNLikeWithExtension(fileId, extFileId, contact);
> +  }
> +
> +  do_test(fileId, extFileId,
> +          {recordId: 1, alphaId: "test", number: "001122334455667788991234"}, 0x01, 0x01);

Add some comments about what case we want to test.

::: dom/system/gonk/tests/test_ril_worker_icc_ICCRecordHelper.js
@@ +640,5 @@
>      }
>    };
>  
>    fileId = ICC_EF_ADN;
> +  record.updateADNLike(fileId, 0xff,

Please also add checking for extRecordNumber should be equal to 0xff.

@@ +856,5 @@
> +/**
> + *  Verify ICCRecordHelper.updateExtension
> + */
> +add_test(function test_update_extension() {
> +  const RECORDSIZE = 13;

s/RECORDSIZE/RECORD_SIZE/, same for other consts.

@@ +945,5 @@
> +  do_test(ICC_EF_EXT2, "01234567890123456789");
> +
> +  ril.appType = CARD_APPTYPE_RUIM;
> +  do_test(ICC_EF_EXT1, "01234567890123456789");
> +  do_test(ICC_EF_EXT2, "01234567890123456789");

Nit: One empty line here.

@@ +974,5 @@
> +    options.p3 = RECORDSIZE;
> +    ril.iccIO(options);
> +  };
> +
> +  function do_test(fileId, expectedNumber) {

Don't need |expectedNumber|.

@@ +1023,5 @@
> +  }
> +
> +  ril.appType = CARD_APPTYPE_SIM;
> +  do_test(ICC_EF_EXT1, "01234567890123456789");
> +  do_test(ICC_EF_EXT2, "01234567890123456789");

These two tests are almostly testing same thing. It seems having one test for cleanEFRecord is enough.

@@ +1067,5 @@
> +    let errorCb = function errorCb(errorMsg) {
> +      do_print("Reading ADNLike failed, msg = " + errorMsg);
> +      ok(false);
> +    };
> +    record.getADNLikeExtensionRecordNumber(0x6f3a, ICC_EF_EXT1, 1, successCb, errorCb);

Replace 0x6f3a with ICC_EF_XXX.

@@ +1070,5 @@
> +    };
> +    record.getADNLikeExtensionRecordNumber(0x6f3a, ICC_EF_EXT1, 1, successCb, errorCb);
> +  }
> +
> +  do_test("436f6e74616374303031ffffffffffffffff0b8199887766554433221100ff01", "01");

Add some comments to explain the rawEF.

Comment 23

[John Dai[:jdai]]

Attachment: Part 1: Read SIM contact dialling number exceed 20 digits.(V6)

Address comment 21.

Comment 24

[John Dai[:jdai]]

Attachment: Part 2: Write SIM contact dialling number exceed 20 digits.(V5)

Address comment 22.

Comment 25

[Edgar Chen [:edgar]]

Comment on attachment 8593801 [details] [diff] [review]
Part 1: Read SIM contact dialling number exceed 20 digits.(V6)

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

We are almost there and I would like to see a revised version again.
Please see my comments below. Thank you.

::: dom/system/gonk/ril_worker.js
@@ +12081,5 @@
> +      let loadNextContactRecord = function() {
> +        if (options.p1 < options.totalRecords) {
> +          ICCIOHelper.loadNextRecord(options);
> +          return;
> +        } else {

You already bail-out early, don't need to put codes in else block.

e.g.

if ( ... ) {
  ....
  return;
}

.....

@@ +12092,5 @@
> +          if (onsuccess) {
> +            onsuccess(contacts);
> +          }
> +        }
> +      }.bind(this);

Nit: Use arrow function instead.

@@ +12627,5 @@
> +
> +    this.context.ICCIOHelper.loadLinearFixedEF({
> +      fileId: fileId,
> +      recordNumber: recordNumber,
> +      callback: callback.bind(this),

Use arrow function instead.

e.g.

let callback = (options) => { 
  ... 
};

@@ +14781,5 @@
> +    if (pbr.ext1) {
> +      ICCRecordHelper.readADNLike(pbr.adn.fileId, pbr.ext1.fileId, gotAdnCb, onerror);
> +    } else {
> +      ICCRecordHelper.readADNLike(pbr.adn.fileId, null, gotAdnCb, onerror);
> +    }

ICCRecordHelper.readADNLike(pbr.adn.fileId, 
  (pbr.ext1) ? pbr.ext1.fileId : null, gotAdnCb, onerror);

::: dom/system/gonk/tests/test_ril_worker_icc_ICCRecordHelper.js
@@ +511,5 @@
> +  let record = context.ICCRecordHelper;
> +  let buf = context.Buf;
> +  let io = context.ICCIOHelper;
> +  let ril = context.RIL;
> +  ril.appType = CARD_APPTYPE_SIM;

It seems we can remove this line, we already do this in line #553.

@@ +548,5 @@
> +      ok(false);
> +    };
> +
> +    record.readADNLike(ICC_EF_ADN, extFileId, successCb, errorCb);
> +  }

Nit: one empty line here

@@ +560,5 @@
> +  // Unsupport extension
> +  do_test(null,"436f6e74616374303031ffffffffffffffff0b8199887766554433221100ffff",
> +          0xff, "99887766554433221100");
> +  // Empty contact
> +  do_test(null,"436f6e74616374303031ffffffffffffffffffffffffffffffffffffffffffff",

"Empty contact" is confused, it has |alphaId| information actually.
Please revise the comment a bit.

@@ +832,5 @@
> +    };
> +
> +    let errorCb = function errorCb() {
> +      ok(expectedExtensionNumber == null);
> +    };

Nit: add one empty line here.

@@ +838,5 @@
> +  }
> +
> +  // Test unsupported record type 0x01
> +  do_test("010a10325476981032547698ff", "");
> +  // Test invalid length c1

s/c1/0xc1/

Comment 26

[Edgar Chen [:edgar]]

Comment on attachment 8593802 [details] [diff] [review]
Part 2: Write SIM contact dialling number exceed 20 digits.(V5)

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

Still have room to improve the xpcshell test.
Please see my comments, thank you.

::: dom/icc/tests/marionette/test_icc_contact.js
@@ +16,5 @@
> +      index: 1,
> +      name: "Saßê黃",
> +      // We don't support extension chain for now.
> +      number: "98765432109876543210998877665544332211001234"},
> +    expect: {

Move the comment to expect result, so we can know why expect to get this value.

@@ +142,4 @@
>  // Start tests
>  startTestCommon(function() {
>    let icc = getMozIcc();
> +  let promise = Promise.resolve();

testReadContacts returns a promise already, don't need Promise.resolve().

@@ +146,3 @@
>  
>    // Test read adn contacts
> +  promise = testReadContacts(icc, "adn")

let promise = testReadContacts(icc, "adn")

::: dom/system/gonk/ril_worker.js
@@ +6231,5 @@
> +    return string.replace(/[^0-9*#,]/g, "")
> +                 .replace(/\*/g, "a")
> +                 .replace(/\#/g, "b")
> +                 .replace(/\,/g, "c");
> +  },

Nit: One empty line here.

@@ +12148,5 @@
>     * @param pin2        PIN2 is required when updating ICC_EF_FDN.
>     * @param onsuccess   Callback to be called when success.
>     * @param onerror     Callback to be called when error.
>     */
> +  updateADNLike: function(fileId, extRecordNumber, contact, pin2, onsuccess, onerror) {

Please also update the comments to add a new param, "extRecordNumber".

@@ +12225,5 @@
>        }
>  
>        if (options.p1 < options.totalRecords) {
>          ICCIOHelper.loadNextRecord(options);
> +        return;

This change should not belong to this patch. Please remove this return.

@@ +12667,5 @@
> +      let Buf = this.context.Buf;
> +      Buf.writeInt32(strLen);
> +
> +      if (number.length > EXT_MAX_NUMBER_DIGITS) {
> +        number = number.substring(0, EXT_MAX_NUMBER_DIGITS);

Please add log about we do this handling is because we don't support extension chain currently.

@@ +12687,5 @@
> +
> +    this.context.ICCIOHelper.updateLinearFixedEF({
> +      fileId: fileId,
> +      recordNumber: recordNumber,
> +      dataWriter: dataWriter.bind(this),

Nit: Use arrow function.

@@ +12719,5 @@
> +
> +    this.context.ICCIOHelper.updateLinearFixedEF({
> +     fileId: fileId,
> +     recordNumber: recordNumber,
> +     dataWriter: dataWriter.bind(this),

Nit: Use arrow function.

@@ +12749,5 @@
> +
> +    this.context.ICCIOHelper.loadLinearFixedEF({
> +      fileId: fileId,
> +      recordNumber: recordNumber,
> +      callback: callback.bind(this),

Nit: Use arrow function.

@@ +15121,5 @@
> +                                      contact, null,
> +                                      updateAdnCb, onerror);
> +    } else {
> +      this.context.ICCRecordHelper.updateADNLike(pbr.adn.fileId,
> +                                                  0xff, contact,

Nit: Indentation.

@@ +15338,5 @@
> +                                  EXT_MAX_NUMBER_DIGITS);
> +
> +    let updateADNLike = function(extRecordNumber) {
> +      ICCRecordHelper.updateADNLike(fileId, extRecordNumber,
> +                                            contact, pin2, onsuccess, onerror)

Nit: Indentation.

And put `let updateADNLike =` into success callback function of |getADNLikeExtensionRecordNumber()|.

@@ +15343,5 @@
> +    }
> +
> +    let updateExtension = function(extRecordNumber, extNumber) {
> +      ICCRecordHelper.updateExtension(extFileId, extRecordNumber,
> +                                              extNumber,

Ditto.

@@ +15352,5 @@
> +    ICCRecordHelper.getADNLikeExtensionRecordNumber(fileId, contact.recordId,
> +      (extRecordNumber) => {
> +        let isNeedExtension = contact.number.substring(numStart).length >
> +                              ADN_MAX_NUMBER_DIGITS;
> +        if (isNeedExtension) {

if (extNumber) {

@@ +15356,5 @@
> +        if (isNeedExtension) {
> +          if (extRecordNumber != 0xff) {
> +            updateExtension(extRecordNumber, extNumber);
> +            return;
> +          }

Nit: One empty line here.

@@ +15361,5 @@
> +          ICCRecordHelper.findFreeRecordId(extFileId,
> +            (extRecordNumber) => updateExtension(extRecordNumber, extNumber),
> +            (errorMsg) => {
> +              if (DEBUG) {
> +                this.context.debug(errorMsg + " find Free Extension fileId:" + extFileId);

debug("Couldn't find free extension record Id for " + extFileId + ": " + errorMsg);

Hmm, this remind me one thing. icc.updateContact now returns the updated mozContact (bug 943234), I am wondering does the returned mozContact is correct in such case. Could you help to check about this? Thank you.

@@ +15366,5 @@
> +              }
> +              updateADNLike(0xff);
> +            });
> +          return;
> +        } else {

You already bail-out early, don't need else block.

::: dom/system/gonk/tests/test_ril_worker_icc_ICCContactHelper.js
@@ +811,5 @@
> +    }
> +
> +    record.updateExtension = function(fileId, recordNumber, number, onsuccess, onerror) {
> +      if (recordNumber != 0xff) {
> +        free_records[recordNumber] = {};

I like the idea about having |free_records|, but there is room to improve this idea.

1. s/free_records/ext_records/
2. How about do following checks instead?

if (recordNumber > ext_record.length) {
  onerror(...);
  return;
}

ext_record[recordNumber - 1] = number;
onsuccess();

@@ +820,5 @@
> +    }
> +
> +    record.findFreeRecordId = function(fileId, onsuccess, onerror) {
> +      // The 1st element, free_records[0], is null.
> +      if (free_records.length > MAX_RECORDS) {

I don't quite understand this.

Could we just check is there any "free" element in ext_record?

@@ +829,5 @@
> +      onsuccess(free_records.length++);
> +    }
> +
> +    record.cleanEFRecord = function(fileId, recordNumber, onsuccess, onerror) {
> +      if (recordNumber != 0xff) {

Ditto, maybe following check,

if (recordNumber > ext_record.length) {
  onerror(...);
  return;
}
 
ext_record[recordNumber - 1] = null;
onsuccess();

@@ +837,5 @@
> +        ok(false);
> +      }
> +    }
> +
> +    contactHelper.updateADNLikeWithExtension(fileId, extFileId, contact);

Just contactHelper.updateADNLikeWithExtension(ICC_EF_ADN, ICC_EF_EXT1, contact).

And we have to test the onsuccess and onerrro passed into updateExtension() will be triggered correctly.

@@ +838,5 @@
> +      }
> +    }
> +
> +    contactHelper.updateADNLikeWithExtension(fileId, extFileId, contact);
> +  }

Nit: One empty space here.

::: dom/system/gonk/tests/test_ril_worker_icc_ICCRecordHelper.js
@@ +933,5 @@
> +        this.readInt32();
> +      }
> +    };
> +
> +    recordHelper.updateExtension(fileId, RECORD_NUMBER, NUMBER);

Hmm, on a second review, this test isn't enough to cover all situation.
1. We have to test the onsuccess and onerrro passed into updateExtension() will be triggered correctly
2. Test case for extNumber.length > EXT_MAX_NUMBER_DIGITS.

Sorry for didn't point this out during previous review.

@@ +973,5 @@
> +    options.p3 = RECORD_SIZE;
> +    ril.iccIO(options);
> +  };
> +
> +  function do_test(fileId, expectedNumber) {

Don't need |cleanEFRecord| for the test of cleanEFRecord.

@@ +1017,5 @@
> +        this.readInt32();
> +      }
> +    };
> +
> +    recordHelper.cleanEFRecord(fileId, RECORD_NUMBER);

And we should have to test the onsuccess and onerrror callback passed into cleanEFRecord() will be triggered correctly.

@@ +1066,5 @@
> +      do_print("Reading ADNLike failed, msg = " + errorMsg);
> +      ok(false);
> +    };
> +
> +    record.getADNLikeExtensionRecordNumber(ICC_EF_ADN, ICC_EF_EXT1, 1, successCb, errorCb);

getADNLikeExtensionRecordNumber doesn't take ICC_EF_EXT1 actually, but the test case is passed?

It seem we need a way to the ensure the onsuccess or onerror is really tirggered as expected (I think the test in part 1 is also need to do so).

Comment 27

[John Dai[:jdai]]

Attachment: Part 1: Read SIM contact dialling number exceed 20 digits.(V7)

Address comment 25.

Comment 28

[John Dai[:jdai]]

Attachment: Part 2: Write SIM contact dialling number exceed 20 digits.(V6)

Address comment 26.

Comment 29

[John Dai[:jdai]]

> @@ +15361,5 @@
> > +          ICCRecordHelper.findFreeRecordId(extFileId,
> > +            (extRecordNumber) => updateExtension(extRecordNumber, extNumber),
> > +            (errorMsg) => {
> > +              if (DEBUG) {
> > +                this.context.debug(errorMsg + " find Free Extension fileId:" + extFileId);
> 
> debug("Couldn't find free extension record Id for " + extFileId + ": " +
> errorMsg);
> 
> Hmm, this remind me one thing. icc.updateContact now returns the updated
> mozContact (bug 943234), I am wondering does the returned mozContact is
> correct in such case. Could you help to check about this? Thank you.
> 
No, it didn't return updated mozContact. I filed a bug 1161438 to handle this.

Comment 30

[John Dai[:jdai]]

Attachment: Part 1: Read SIM contact dialling number exceed 20 digits.(V8)

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

Comment 31

[John Dai[:jdai]]

Attachment: Part 2: Write SIM contact dialling number exceed 20 digits.(V7)

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

Comment 32

[Edgar Chen [:edgar]]

Comment on attachment 8615158 [details] [diff] [review]
Part 1: Read SIM contact dialling number exceed 20 digits.(V8)

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

::: dom/icc/tests/marionette/manifest.ini
@@ +4,5 @@
>  qemu = true
>  
>  [test_icc_contact_read.js]
>  [test_icc_contact_add.js]
> +[test_icc_contact_update.js]

You missed to add this file into patch.

::: dom/system/gonk/ril_worker.js
@@ +12836,5 @@
> +   * @param number        Dialling Number to be written.
> +   * @param onsuccess     Callback to be called when success.
> +   * @param onerror       Callback to be called when error.
> +   */
> +  updateExtension: function(fileId, recordNumber, number, onsuccess, onerror) {

This should belong to part 2.

@@ +12880,5 @@
> +   * @param recordNumber  The number of the record shall be updated.
> +   * @param onsuccess     Callback to be called when success.
> +   * @param onerror       Callback to be called when error.
> +   */
> +  cleanEFRecord: function(fileId, recordNumber, onsuccess, onerror) {

Ditto.

@@ +12911,5 @@
> +   * @param  recordNumber  EF record id of the ADN or FDN.
> +   * @param  onsuccess Callback to be called when success.
> +   * @param  onerror   Callback to be called when error.
> +   */
> +  getADNLikeExtensionRecordNumber: function(fileId, recordNumber, onsuccess, onerror) {

Ditto.

@@ +14999,5 @@
>      switch (contactType) {
>        case GECKO_CARDCONTACT_TYPE_ADN:
>          if (!this.hasDfPhoneBook(appType)) {
> +          if (ICCUtilsHelper.isICCServiceAvailable("EXT1")) {
> +            this.updateADNLikeWithExtension(ICC_EF_ADN, ICC_EF_EXT1,

Ditto.

@@ +15003,5 @@
> +            this.updateADNLikeWithExtension(ICC_EF_ADN, ICC_EF_EXT1,
> +                                            contact, null,
> +                                            updateContactCb, onerror);
> +          } else {
> +            ICCRecordHelper.updateADNLike(ICC_EF_ADN, 0xff,

Ditto.

@@ +15021,5 @@
>            onerror(CONTACT_ERR_CONTACT_TYPE_NOT_SUPPORTED);
>            break;
>          }
> +        if (ICCUtilsHelper.isICCServiceAvailable("EXT2")) {
> +          this.updateADNLikeWithExtension(ICC_EF_FDN, ICC_EF_EXT2,

Ditto.

@@ +15025,5 @@
> +          this.updateADNLikeWithExtension(ICC_EF_FDN, ICC_EF_EXT2,
> +                                          contact, pin2,
> +                                          updateContactCb, onerror);
> +        } else {
> +          ICCRecordHelper.updateADNLike(ICC_EF_FDN,

Ditto.

@@ +15298,5 @@
>        }, onerror);
>      }.bind(this);
>  
> +    if (pbr.ext1) {
> +      this.updateADNLikeWithExtension(pbr.adn.fileId, pbr.ext1.fileId,

Ditto.

@@ +15301,5 @@
> +    if (pbr.ext1) {
> +      this.updateADNLikeWithExtension(pbr.adn.fileId, pbr.ext1.fileId,
> +                                      contact, null, updateAdnCb, onerror);
> +    } else {
> +      this.context.ICCRecordHelper.updateADNLike(pbr.adn.fileId, 0xff, contact,

Ditto.

::: dom/system/gonk/tests/test_ril_worker_icc_ICCRecordHelper.js
@@ +827,5 @@
> +      }
> +    };
> +
> +    let successCb = function successCb(number) {
> +      do_print("number:" + number);

Nit: s/number:/extension number:/

Comment 33

[Edgar Chen [:edgar]]

Comment on attachment 8615158 [details] [diff] [review]
Part 1: Read SIM contact dialling number exceed 20 digits.(V8)

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

::: dom/icc/tests/marionette/manifest.ini
@@ +4,5 @@
>  qemu = true
>  
>  [test_icc_contact_read.js]
>  [test_icc_contact_add.js]
> +[test_icc_contact_update.js]

Please ignore my previous comment, it should belong to part 2.

Comment 34

[John Dai[:jdai]]

Attachment: Part 1: Read SIM contact dialling number exceed 20 digits.(V9)

- Addressing comment #32.

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

Comment 35

[John Dai[:jdai]]

Attachment: Part 2: Write SIM contact dialling number exceed 20 digits.(V8)

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

Comment 36

[Edgar Chen [:edgar]]

Comment on attachment 8622300 [details] [diff] [review]
Part 1: Read SIM contact dialling number exceed 20 digits.(V9)

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

Nice, thank you.

::: dom/system/gonk/ril_worker.js
@@ +12666,5 @@
>                                     onerror: onerror});
>    },
> +
> +  /**
> +   * Read Extension Number from TS 151.011 clause 10.5.10, TS 31.102, clause 4.4.2.4

Nit: One empty line here.

Comment 37

[Edgar Chen [:edgar]]

Comment on attachment 8622305 [details] [diff] [review]
Part 2: Write SIM contact dialling number exceed 20 digits.(V8)

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

Please see my comments below, thank you.

::: dom/icc/tests/marionette/test_icc_contact_add.js
@@ +27,5 @@
>      tel: [{value: "01234567890123456789"}, {value: "123456"}, {value: "123"}],
>      email:[{value: "[e-mail]"}],
>    }];
>  
> +

Nit: Remove this empty line.

@@ +35,5 @@
>  
>    return aIcc.updateContact(aType, contact, aPin2)
>      .then((aResult) => {
>        is(aResult.name[0], aMozContact.name[0]);
> +      // Maximum ADN digits of the Dialling Number is 20, and maximum Extension is 20.

Nit:
s/Maximum ADN digits/Maximum digits/
s/maximum Extension/maximum digits of Extension/

@@ +36,5 @@
>    return aIcc.updateContact(aType, contact, aPin2)
>      .then((aResult) => {
>        is(aResult.name[0], aMozContact.name[0]);
> +      // Maximum ADN digits of the Dialling Number is 20, and maximum Extension is 20.
> +      is(aResult.tel[0].value, aMozContact.tel[0].value);

`is(aResult.tel[0].value, aMozContact.tel[0].value.substring(0, 40))` maybe?
 
And since we don't support extension chain for now, please also add test for this.

@@ +47,5 @@
>          .then((aResult) => {
>            let contact = aResult[aResult.length - 1];
>  
>            is(contact.name[0], aMozContact.name[0]);
> +          // Maximum ADN digits of the Dialling Number is 20, and maximum Extension is 20.

Ditto.

@@ +48,5 @@
>            let contact = aResult[aResult.length - 1];
>  
>            is(contact.name[0], aMozContact.name[0]);
> +          // Maximum ADN digits of the Dialling Number is 20, and maximum Extension is 20.
> +          is(contact.tel[0].value, aMozContact.tel[0].value);

Ditto.

::: dom/icc/tests/marionette/test_icc_contact_update.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +MARIONETTE_TIMEOUT = 60000;

60000 probably isn't enough. SIM IO takes time, please increase the timeout value to prevent intermittent script timeout when try server is busy.

@@ +41,5 @@
> +    expect: {
> +      number: "+99887766554433221100"}
> +  }];
> +
> +function testChangeContact(aIcc, aType, aContactId, aMozContact, aExpect, aPin2) {

s/testChangeContact/testUpdateContact/

@@ +46,5 @@
> +  log("testChangeContact: type=" + aType +
> +      ", mozContact=" + JSON.stringify(aMozContact) +
> +      ", expect=" + aExpect.value + ", pin2=" + aPin2);
> +
> +  let cacheContact;

Don't have to do caching original contacts and recovering for each sub-test.
We could just cache original contacts at the very beginning of test. and do recovering after all sub-tests is done.

@@ +53,5 @@
> +
> +  return aIcc.readContacts(aType)
> +    .then((aResult) => {
> +      cacheContact = aResult[aContactId - 1];
> +    return aIcc.updateContact(aType, contact, aPin2)

Nit: Indentation.

::: dom/system/gonk/ril_worker.js
@@ +9579,5 @@
>     *
> +   * @param recordSize       The size of linear fixed record.
> +   * @param alphaId          Alpha Identifier to be written.
> +   * @param number           Dialling Number to be written.
> +   * @param extRecordNumber  The record identifier of EXT.

Nit: s/EXT/the EXT/

@@ +9601,3 @@
>      GsmPDUHelper.writeHexOctet(0xff);
> +
> +    if (extRecordNumber) {

We have to consider the case that extRecordNumber is 0x0.
 
GsmPDUHelper.writeHexOctet((extRecordNumber != null) ? extRecordNumber : 0xff);

@@ +12223,5 @@
>    /**
>     * Update ICC ADN like EFs, like EF_ADN, EF_FDN.
>     *
> +   * @param fileId          EF id of the ADN or FDN.
> +   * @param extRecordNumber The EXT record identifier.

Nit: The record identifier of the EXT.

@@ +12746,5 @@
>      });
>    },
> +
> +  /**
> +   * Update Extension.

Nit: One empty line here.

@@ +12747,5 @@
>    },
> +
> +  /**
> +   * Update Extension.
> +   * @param fileId        EF Extension id.

Nit: EF id of the EXT.

@@ +12822,5 @@
> +    });
> +  },
> +
> +  /**
> +   * Get ADNLike extension record number.

Nit: One empty line here.

@@ +12824,5 @@
> +
> +  /**
> +   * Get ADNLike extension record number.
> +   * @param  fileId    EF id of the ADN or FDN.
> +   * @param  recordNumber  EF record id of the ADN or FDN.

Nit: Indentation

@param fileId       EF id of the ADN or FDN.
@param recordNumber EF record id of the ADN or FDN.
....

@@ +15449,5 @@
> +  /**
> +   * Update ICC ADN like EFs with Extension, like EF_ADN, EF_FDN.
> +   *
> +   * @param  fileId    EF id of the ADN or FDN.
> +   * @param  extFileId EF Extension id.

Nit: EF id of the EXT.

@@ +15462,5 @@
> +
> +    if (contact.number) {
> +      let numStart = contact.number[0] == "+" ? 1 : 0;
> +      let number = contact.number.substring(0, numStart) +
> +                    this.context.GsmPDUHelper.stringToExtendedBcd(

Nit: Indentation.

@@ +15469,5 @@
> +                                EXT_MAX_NUMBER_DIGITS);
> +    }
> +
> +    ICCRecordHelper.getADNLikeExtensionRecordNumber(fileId, contact.recordId,
> +      (extRecordNumber) => {

Nit: Indentation
 
ICCRecordHelper.getADNLikeExtensionRecordNumber(fileId, contact.recordId,
	                                        (extRecordNumber) => {
  ....
});

@@ +15470,5 @@
> +    }
> +
> +    ICCRecordHelper.getADNLikeExtensionRecordNumber(fileId, contact.recordId,
> +      (extRecordNumber) => {
> +        let updateADNLikeCb = (extRecordNumber) => {

This isn't a callback function of updateADNLike actually, just `updateADNLike` is enough.

@@ +15472,5 @@
> +    ICCRecordHelper.getADNLikeExtensionRecordNumber(fileId, contact.recordId,
> +      (extRecordNumber) => {
> +        let updateADNLikeCb = (extRecordNumber) => {
> +          ICCRecordHelper.updateADNLike(fileId, extRecordNumber,
> +                                        contact, pin2, (updatedContact) => {

Nit: Indentation
 
ICCRecordHelper.updateADNLike(fileId, extRecordNumber, contact,
	                      pin2, (updatedContact) => {
  ...
});

@@ +15475,5 @@
> +          ICCRecordHelper.updateADNLike(fileId, extRecordNumber,
> +                                        contact, pin2, (updatedContact) => {
> +                                          if (extNumber && extRecordNumber != 0xff) {
> +                                            updatedContact.number = updatedContact.number.concat(extNumber);
> +                                            onsuccess(updatedContact);

if (extNumber && extRecordNumber != 0xff) {
  updatedContact.number = updatedContact.number.concat(extNumber);
}
 
onsuccess(updatedContact);

@@ +15480,5 @@
> +                                            return;
> +                                          }
> +                                          onsuccess(updatedContact);
> +                                        }, onerror)
> +        }

Missing semicolon after function assigned to a variable.

@@ +15482,5 @@
> +                                          onsuccess(updatedContact);
> +                                        }, onerror)
> +        }
> +
> +        let updateExtensionCb = (extRecordNumber) => {

This isn't a callback function of updateExtensionCb actually, just `updateExtension` is enough.

@@ +15484,5 @@
> +        }
> +
> +        let updateExtensionCb = (extRecordNumber) => {
> +          ICCRecordHelper.updateExtension(extFileId, extRecordNumber,
> +                                                  extNumber,

Nit: Indentation.

@@ +15487,5 @@
> +          ICCRecordHelper.updateExtension(extFileId, extRecordNumber,
> +                                                  extNumber,
> +                                                  () => updateADNLikeCb(extRecordNumber),
> +                                                  () => updateADNLikeCb(0xff));
> +        }

Missing semicolon after function assigned to a variable.
 
And one empty line here.

@@ +15498,5 @@
> +          ICCRecordHelper.findFreeRecordId(extFileId,
> +            (extRecordNumber) => updateExtensionCb(extRecordNumber),
> +            (errorMsg) => {
> +              if (DEBUG) {
> +                this.context.debug(errorMsg + " couldn't find free extension fileId:" + extFileId);

this.context.debug("Couldn't find free extension record Id for " + extFileId + ": " + errorMsg);

@@ +15503,5 @@
> +              }
> +              updateADNLikeCb(0xff);
> +            });
> +          return;
> +        }

Nit: One empty line here.

::: dom/system/gonk/tests/test_ril_worker_icc_ICCContactHelper.js
@@ +462,4 @@
>      };
>  
> +    recordHelper.getADNLikeExtensionRecordNumber = function(fileId, recordNumber, onsuccess, onerror) {
> +      onsuccess(0x01);

Have a const for extension record number in ADN.

@@ +469,5 @@
> +      onsuccess();
> +    };
> +
> +    recordHelper.findFreeRecordId = function(fileId, onsuccess, onerror) {
> +      onsuccess(0x01);

Have a const for free record id.

@@ +587,5 @@
>        alphaId:  "test4",
>        number:   "123456",
>        anr:      ["+654321"]
> +    },
> +    // a contact number over 20 digits.

Please also add a test for the length of contact number is over 40 digits.

@@ +657,5 @@
>    let context = worker.ContextPool._contexts[0];
>    let recordHelper = context.ICCRecordHelper;
>    let contactHelper = context.ICCContactHelper;
>  
> +

Nit: Remove this empty line.

@@ +851,5 @@
> +  let contactHelper = context.ICCContactHelper;
> +  ril.appType = CARD_APPTYPE_SIM;
> +  // Correct record Id starts from 1, so put a null element at index 0.
> +  // ext_records contains data at index 1, and it only has 1 free record at index 2.
> +  let ext_records = [null, {}, null];

Storing a string for no-empty records, instead of storing an object.

@@ +894,5 @@
> +      ext_records[recordNumber] = null;
> +      onsuccess();
> +    }
> +
> +    let successCb = function successCb(extRecordNumber) {

If my understand correct, the success callback of updateADNLikeWithExtension is carrying `updated contact`, not extension record number.

@@ +915,5 @@
> +  do_test({recordId: 1, alphaId: "test", number: "001122334455667788995678"}, 0xff, 0x02, "5678");
> +  // Update extension record with no free extension record.
> +  do_test({recordId: 1, alphaId: "test", number: "001122334455667788994321"}, 0xff, 0xff);
> +  // Update extension record with clean previous extension record.
> +  do_test({recordId: 1, alphaId: "test", number: "00112233445566778899"}, 0x01, 0xff);

How can we make sure the |cleanEFRecord| is really called?

::: dom/system/gonk/tests/test_ril_worker_icc_ICCPDUHelper.js
@@ +469,5 @@
>  
>    let contactR = helper.readAlphaIdDiallingNumber(recordSize);
>    equal(writtenContact.alphaId, contactR.alphaId);
>    equal(writtenContact.number, contactR.number);
> +  equal(contactR.extRecordNumber, 0xff);

Nit: equal(0xff, contactR.extRecordNumber);

@@ +484,4 @@
>    contactR = helper.readAlphaIdDiallingNumber(recordSize);
>    equal(writtenContact.alphaId, contactR.alphaId);
>    equal(writtenContact.number, contactR.number);
> +  equal(contactR.extRecordNumber, 0xff);

Ditto

@@ +509,4 @@
>    contactR = helper.readAlphaIdDiallingNumber(recordSize);
>    equal(writtenContact.alphaId, contactR.alphaId);
>    equal(writtenContact.number, contactR.number);
> +  equal(contactR.extRecordNumber, 0xff);

Ditto

@@ +519,4 @@
>    contactR = helper.readAlphaIdDiallingNumber(recordSize);
>    equal(writtenContact.alphaId, contactR.alphaId);
>    equal(writtenContact.number, contactR.number);
> +  equal(contactR.extRecordNumber, 0xff);

Ditto.

::: dom/system/gonk/tests/test_ril_worker_icc_ICCRecordHelper.js
@@ +920,5 @@
> +
> +      // pin2.
> +      equal(this.readString(), null);
> +
> +      if (!ril.v5Legacy) {

Don't need this, v5Legacy is deprecated already (see bug 999300).

@@ +927,5 @@
> +      }
> +    };
> +
> +    let successCb = function successCb() {
> +      ok(true);

This isn't enough to ensure that onsuccess passed into updateExtension() will be triggered correctly.

I think we need use following way,

let isSuccess = false;
let successCb = function() {
  isSuccess = true;
}

recordHelper.updateExtension(fileId, RECORD_NUMBER, number, successCb, ...);
ok(isSuccess);

@@ +966,5 @@
> +  let ril = context.RIL;
> +  let recordHelper = context.ICCRecordHelper;
> +  let buf = context.Buf;
> +  let ioHelper = context.ICCIOHelper;
> +  ril.appType = CARD_APPTYPE_SIM;

Put this along with |do_test|, just follow same style as the one in test_update_extension.

@@ +988,5 @@
> +      this.readInt32();
> +
> +      // command.
> +      equal(this.readInt32(), ICC_COMMAND_UPDATE_RECORD);
> +      //do_print("ICC_COMMAND_UPDATE_RECORD:"+this.readInt32());

I guess this is for debugging, please remember to remove it.

@@ +992,5 @@
> +      //do_print("ICC_COMMAND_UPDATE_RECORD:"+this.readInt32());
> +
> +      // fileId.
> +      equal(this.readInt32(), fileId);
> +      //do_print("fileId:"+this.readInt32());

Ditto.

@@ +1018,5 @@
> +
> +      // pin2.
> +      equal(this.readString(), null);
> +
> +      if (!ril.v5Legacy) {

Don't need this.

@@ +1025,5 @@
> +      }
> +    };
> +
> +    let successCb = function successCb() {
> +      ok(true);

Having a way to ensure the onsuccess callback will be triggered correctly.

@@ +1033,5 @@
> +      do_print("clean EF Record failed, msg = " + errorMsg);
> +      ok(false);
> +    };
> +
> +    function writeRecord() {

What is this for?

@@ +1046,5 @@
> +      // Write string delimiter
> +      buf.writeStringDelimiter(rawEF.length);
> +    }
> +
> +    recordHelper.cleanEFRecord(ICC_EF_EXT1, RECORD_NUMBER, successCb, errorCb);

s/ICC_EF_EXT1/fileId?

@@ +1086,5 @@
> +      }
> +    };
> +
> +    let successCb = function successCb(number) {
> +      equal(number, expectedRecordNumber);

Ditto, having a way to ensure the onsuccess callback will be triggered correctly.

Comment 38

[John Dai[:jdai]]

Attachment: Part 1: Read SIM contact dialling number exceed 20 digits.(Final), r=echen

Addressing comment #36

Comment 39

[John Dai[:jdai]]

Attachment: Part 2: Write SIM contact dialling number exceed 20 digits.(V9)

- Addressing comment #37.

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

Comment 40

[Edgar Chen [:edgar]]

Comment on attachment 8623447 [details] [diff] [review]
Part 2: Write SIM contact dialling number exceed 20 digits.(V9)

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

Sorry for the late response, please see my comments below. Thank you.

::: dom/icc/tests/marionette/test_icc_contact_update.js
@@ +73,5 @@
> +      });
> +}
> +
> +function revertContact(aIcc, aContactId, aContact, aType, aPin2) {
> +  log("removeContact: contactId=" + aContactId +

s/removeContact/revertContact/

@@ +92,5 @@
> +  return icc.readContacts("adn")
> +    .then((aResult) => {
> +      adnContacts = aResult;
> +
> +      return icc.readContacts("fdn")

return icc.readContacts("adn")
  .then((aResult) => {
    adnContacts = aResult;
  })
  .then(() => icc.readContacts("fdn"))
  .then((aResult) => {
    fdnContacts = aResult;
  })
  ....

@@ +102,5 @@
> +      for (let i = 0; i < TEST_UPDATE_DATA.length; i++) {
> +        let test_data = TEST_UPDATE_DATA[i];
> +
> +        // Test update adn contacts
> +        return testUpdateContact(icc, "adn", test_data.id, test_data.data,

if returning here, only the first item in TEST_UPDATE_DATA will be executed.

@@ +118,5 @@
> +          let test_data = TEST_UPDATE_DATA[i];
> +          let adnContact = adnContacts[test_data.id - 1];
> +          let fdnContact = fdnContacts[test_data.id - 1];
> +
> +          return revertContact(icc, test_data.id, adnContact, "adn")

For the recovery, it seems to me that we could just pass the cached contact (which is already a mozContact instance) to |icc.updateContact()|.
And also if returning here, we won't go through all the item actually.

::: dom/system/gonk/ril_worker.js
@@ +15461,5 @@
> +    if (contact.number) {
> +      let numStart = contact.number[0] == "+" ? 1 : 0;
> +      let number = contact.number.substring(0, numStart) +
> +                   this.context.GsmPDUHelper.stringToExtendedBcd(
> +                   contact.number.substring(numStart));

Nit: Indentation.

let number = contact.number.substring(0, numStart) +
	     this.context.GsmPDUHelper.stringToExtendedBcd(
	       contact.number.substring(numStart));

@@ +15471,5 @@
> +                                                    (extRecordNumber) => {
> +        let updateADNLike = (extRecordNumber) => {
> +          ICCRecordHelper.updateADNLike(fileId, extRecordNumber, contact,
> +                                        pin2, (updatedContact) => {
> +                                          if (extNumber && extRecordNumber != 0xff) {

Nit: Indentation.

ICCRecordHelper.updateADNLike(fileId, extRecordNumber, contact,
	                      pin2, (updatedContact) => {
  if (extNumber && extRecordNumber != 0xff) {
    ...
  }
}, onerror);

@@ +15476,5 @@
> +                                            updatedContact.number = updatedContact.number.concat(extNumber);
> +                                          }
> +                                          onsuccess(updatedContact);
> +                                        }, onerror);
> +        }

Missing semicolon after function assigned to a variable.

@@ +15482,5 @@
> +        let updateExtension = (extRecordNumber) => {
> +          ICCRecordHelper.updateExtension(extFileId, extRecordNumber,  extNumber,
> +                                          () => updateADNLike(extRecordNumber),
> +                                          () => updateADNLike(0xff));
> +        }

Missing semicolon after function assigned to a variable

::: dom/system/gonk/tests/test_ril_worker_icc_ICCContactHelper.js
@@ +859,5 @@
> +  let contactHelper = context.ICCContactHelper;
> +  ril.appType = CARD_APPTYPE_SIM;
> +  // Correct record Id starts from 1, so put a null element at index 0.
> +  // ext_records contains data at index 1, and it only has 1 free record at index 2.
> +  let ext_records = [null, 0x01, null];

Nit: s/0x01/"notFree"/ which makes code clearer.

@@ +905,5 @@
> +      onsuccess();
> +    }
> +
> +    let successCb = function successCb(updatedContact) {
> +      do_print("JJ updatedContact:"+JSON.stringify(updatedContact));

I guess this is for debugging, please remember to remove it.

::: dom/system/gonk/tests/test_ril_worker_icc_ICCRecordHelper.js
@@ +1042,5 @@
> +      // AID. Ignore because it's from modem.
> +      this.readInt32();
> +    };
> +
> +    recordHelper.readExtension(fileId, RECORD_NUMBER, successCb, errorCb);

Why testing readExtension here? Should this belong to part 1?

Comment 41

[John Dai[:jdai]]

Attachment: Part 2: Write SIM contact dialling number exceed 20 digits.(V10)

Edgar, may I have your review again?

Comment 42

[Edgar Chen [:edgar]]

Comment on attachment 8650811 [details] [diff] [review]
Part 2: Write SIM contact dialling number exceed 20 digits.(V10)

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

Nice, just few Nit. Thank you.

::: dom/icc/tests/marionette/test_icc_contact_update.js
@@ +65,5 @@
> +            is(contact.tel[0].value, aExpect.number);
> +          }
> +
> +          is(contact.id, aIcc.iccInfo.iccid + aContactId);
> +        })

Missing a semicolon here.

@@ +106,5 @@
> +        let fdnContact = fdnContacts[test_data.id - 1];
> +
> +        // Test update adn contacts
> +        promise = promise.then(() => testUpdateContact(icc, "adn", test_data.id,
> +                                      test_data.data, test_data.expect))

Nit: Indentation, align the arguments.

promise = promise.then(() => testUpdateContact(icc, "adn", test_data.id,
                                               test_data.data, test_data.expect))

@@ +109,5 @@
> +        promise = promise.then(() => testUpdateContact(icc, "adn", test_data.id,
> +                                      test_data.data, test_data.expect))
> +          // Test update fdn contacts
> +          .then(() => testUpdateContact(icc, "fdn", test_data.id, test_data.data,
> +                                 test_data.expect))

Ditto.

@@ +112,5 @@
> +          .then(() => testUpdateContact(icc, "fdn", test_data.id, test_data.data,
> +                                 test_data.expect))
> +          // Test update fdn contacts without passing pin2
> +          .then(() => testUpdateContact(icc, "fdn", test_data.id, test_data.data,
> +                                 test_data.expect, "0000"))

Ditto.

::: dom/system/gonk/ril_worker.js
@@ +14903,5 @@
> +
> +    if (contact.number) {
> +      let numStart = contact.number[0] == "+" ? 1 : 0;
> +      let number = contact.number.substring(0, numStart) +
> +            this.context.GsmPDUHelper.stringToExtendedBcd(

Nit: Indentation, align with contact.number.substring(..), like

let number = contact.number.substring(0, numStart) +
	     this.context.GsmPDUHelper.stringToExtendedBcd(
	       contact.number.substring(numStart));

Comment 43

[John Dai[:jdai]]

Attachment: Part 2: Write SIM contact dialling number exceed 20 digits.(Final), r=echen

Addressing comment #42

Comment 44

[John Dai[:jdai]]

Try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c115dba1905e

Comment 45

[John Dai[:jdai]]

Attachment: Part 1: Read SIM contact dialling number exceed 20 digits.(Final), r=echen

Fixed test_icc_contact_add.js can't run all of testcase.

Comment 46

[John Dai[:jdai]]

Attachment: Part 1: Read SIM contact dialling number exceed 20 digits.(Final), r=echen

Wrong patch of previous comment.

Comment 47

[John Dai[:jdai]]

Attachment: Part 2: Write SIM contact dialling number exceed 20 digits.(Final), r=echen

Fixed test_icc_contact_add.js can't run all of testcase.

Comment 48

[John Dai[:jdai]]

Try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=acd1da52524f

Comment 49

[John Dai[:jdai]]

Attachment: [external/qemu] pull request 159

Comment 50

[John Dai[:jdai]]

Try link:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3c2f5a5e423

Comment 51

[John Dai[:jdai]]

I saw MNW has many error, and it would be better to update a newer try result. Cancel this checkin-needed.

Comment 52

[John Dai[:jdai]]

Attachment: Part 1: Read SIM contact dialling number exceed 20 digits.(Final), r=echen

Add reviewer name.

Comment 53

[John Dai[:jdai]]

Attachment: Part 2: Write SIM contact dialling number exceed 20 digits.(Final), r=echen

1. Added reviewer name and rebased.
2. Increased marionette-web api timeout time.
3. Fixed test_icc_contact_add.js early-return.

Comment 54

[John Dai[:jdai]]

Try link:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1eb1acf32f91

Comment 55

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/23bd00cf364f
https://hg.mozilla.org/integration/b2g-inbound/rev/211edf7e53aa

Comment 56

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

https://hg.mozilla.org/mozilla-central/rev/23bd00cf364f
https://hg.mozilla.org/mozilla-central/rev/211edf7e53aa

Comment 57

[Edgar Chen [:edgar]]

b2g-ics:
https://github.com/mozilla-b2g/platform_external_qemu/commit/12ff7481566587aa4198cf1287598acb3a999973

Comment 58

[Edgar Chen [:edgar]]

b2g-kitkat:
https://github.com/mozilla-b2g/platform_external_qemu/commit/e002268b30ce69725e60fdc827a19d729cce7396