842458 - B2G RIL: support FDN for updateICCContact

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

In Bug 809726 we support updating ADN to SIM, now we'd also support FDN as well.

Comment 1

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

Attachment: Part 1: Add pin2 for updateICCContact.

Comment 2

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

Attachment: Part 2: implementations for updating FDN.

Comment 3

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

Attachment: Part 3: xpcshell tests for updateFdn and updateICCContact.

Comment 4

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

Attachment: Part 2: implementations for updating FDN. v2

Comment 5

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

Attachment: Part 3: xpcshell tests for updateFdn and updateICCContact. v2

Comment 6

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

Comment on attachment 718332 [details] [diff] [review]
Part 2: implementations for updating FDN. v2

Cancelling r? as I found I missed some changes as well.

Comment 7

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

Comment on attachment 718333 [details] [diff] [review]
Part 3: xpcshell tests for updateFdn and updateICCContact. v2

ditto

Comment 8

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

Attachment: Part 3: ICCRecordHelper.getFreeRecordId.

Comment 9

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

Attachment: Part 4: xpcshell tests for updateFdn and updateICCContact. v3

Comment 10

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

Comment on attachment 718310 [details] [diff] [review]
Part 1: Add pin2 for updateICCContact.

Switch r? to Vicamo as Hsinyi is busy.

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 718310 [details] [diff] [review]
Part 1: Add pin2 for updateICCContact.

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

::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ +268,3 @@
>  interface nsIRadioInterfaceLayer : nsISupports
>  {
>    const unsigned short CALL_STATE_UNKNOWN = 0;

Sorry but bug 834160 was landed yesterday, and this patch has to be rebased.

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 718332 [details] [diff] [review]
Part 2: implementations for updating FDN. v2

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

::: dom/system/gonk/ril_worker.js
@@ +10130,5 @@
>      switch (contactType) {
>        case "ADN":
>          switch (appType) {
>            case CARD_APPTYPE_SIM:
> +            ICCRecordHelper.updateADNLike(ICC_EF_ADN, contact, "", onsuccess, onerror);

|null| please

@@ +10328,5 @@
>     */
>    updateUSimContact: function updateUSimContact(contact, onsuccess, onerror) {
>      let gotPbrCb = function gotPbrCb(pbr) {
>        if (pbr.adn) {
> +        ICCRecordHelper.updateADNLike(pbr.adn.fileId, contact, "", onsuccess, onerror);

ditto.

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 720541 [details] [diff] [review]
Part 3: ICCRecordHelper.getFreeRecordId.

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

::: dom/system/gonk/ril_worker.js
@@ +10704,5 @@
> +
> +      while (readLen < octetLen) {
> +        let octet = GsmPDUHelper.readHexOctet();
> +        readLen++;
> +        if (octet != 0xff) {

3GPP TS 31.102 section 4.4.2.3 "EF adn" has:

>   - Length of BCD number/SSC contents.
> Contents:
> -  ... When an ADN/SSC has extension, it is indicated by the extension 1 identifier being unequal to 'FF'.

In the other words, when there is no extension, the extension 1 identifier equals to 'FF'.  So the code here recognizes an occupied, no extension ADN record as an empty one.  Doesn't it?

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 720541 [details] [diff] [review]
Part 3: ICCRecordHelper.getFreeRecordId.

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

Just found you're searching for |octet != 0xFF|, not |octet == 0xFF|.

Comment 15

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

Attachment: Part 1: Add pin2 for updateICCContact. v2

rebase

Comment 16

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

Attachment: Part 2: implementations for updating FDN. v3

replaced "" with null.

Comment 17

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

Attachment: Part 3: ICCRecordHelper.getFreeRecordId. v2

add r=vicamo

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 720542 [details] [diff] [review]
Part 4: xpcshell tests for updateFdn and updateICCContact. v3

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

Great! Those overridden functions make me feel that we do have to complete emulator ICC supports to test these features. :(

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +2033,5 @@
> +  let contact = {recordId: 1, alphaId: "test", number: "123456"};
> +  contactType = "ADN";
> +  contactHelper.updateICCContact(CARD_APPTYPE_SIM, contactType, contact, "");
> +  contactType = "FDN";
> +  contactHelper.updateICCContact(CARD_APPTYPE_SIM, contactType, contact, "1111");

Always abstract test function and make it able for further tests:

  function do_test(aContact, aContaType, aFileId, aPin2) {
    record.updateADNLike = function (fileId, contact, pin2, onsuccess, onerror) {
      do_check_eq(fileId, aFileId);
      do_check_eq(contact.alphaId, aContact.alphaId);
      do_check_eq(contact.number, aContact.number);
      do_check_eq(pin2, aPin2);
    };
    contactHelper.updateICCContact(CARD_APPTYPE_SIM, aContaType, aContact, aPin2);
  }

  let contact = {recordId: 1, alphaId: "test", number: "123456"};
  do_test(contact, "ADN", ICC_EF_ADN, null);
  do_test(contact, "FDN", ICC_EF_FDN, "1111");
  run_next_test();

Comment 19

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #18)
> Comment on attachment 720542 [details] [diff] [review]
> Part 4: xpcshell tests for updateFdn and updateICCContact. v3
> 
> Review of attachment 720542 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great! Those overridden functions make me feel that we do have to complete
> emulator ICC supports to test these features. :(

Will try to implement this in Bug 848662.

Comment 20

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

Attachment: Part 4: xpcshell tests for updateFdn and updateICCContact. v4

Addressed comments, 
add r=vicamo

Comment 21

[Mozilla RelEng Bot]

Try run for ab08bcb9da1f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ab08bcb9da1f
Results (out of 3 total builds):
    success: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ab08bcb9da1f

Comment 22

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d432ae31092
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7d36e47a15c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b76860154d10
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e1f112be1e1

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4d432ae31092
https://hg.mozilla.org/mozilla-central/rev/f7d36e47a15c
https://hg.mozilla.org/mozilla-central/rev/b76860154d10
https://hg.mozilla.org/mozilla-central/rev/3e1f112be1e1

Comment 24

[Daniel Coloma:dcoloma]

I think PIN2 and FDN are not V1.0.1. Please re-nominate if I am wrong.