B2G RIL: support FDN for updateICCContact

RESOLVED FIXED in mozilla22

Status

()

RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: allstars.chh, Assigned: allstars.chh)

Tracking

unspecified
mozilla22
ARM
Gonk (Firefox OS)
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 7 obsolete attachments)

2.54 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
14.51 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
3.35 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
8.71 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
In Bug 809726 we support updating ADN to SIM, now we'd also support FDN as well.
Created attachment 718310 [details] [diff] [review]
Part 1: Add pin2 for updateICCContact.
Assignee: nobody → allstars.chh
Status: NEW → ASSIGNED
Created attachment 718311 [details] [diff] [review]
Part 2: implementations for updating FDN.
Created attachment 718312 [details] [diff] [review]
Part 3: xpcshell tests for updateFdn and updateICCContact.
Created attachment 718332 [details] [diff] [review]
Part 2: implementations for updating FDN. v2
Attachment #718311 - Attachment is obsolete: true
Created attachment 718333 [details] [diff] [review]
Part 3: xpcshell tests for updateFdn and updateICCContact. v2
Attachment #718312 - Attachment is obsolete: true
Attachment #718310 - Flags: review?(htsai)
Attachment #718332 - Flags: review?(htsai)
Attachment #718333 - Flags: review?(htsai)
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.
Attachment #718332 - Flags: review?(htsai)
Comment on attachment 718333 [details] [diff] [review]
Part 3: xpcshell tests for updateFdn and updateICCContact. v2

ditto
Attachment #718333 - Flags: review?(htsai)
Created attachment 720541 [details] [diff] [review]
Part 3: ICCRecordHelper.getFreeRecordId.
Attachment #720541 - Flags: review?(htsai)
Created attachment 720542 [details] [diff] [review]
Part 4: xpcshell tests for updateFdn and updateICCContact. v3
Attachment #718333 - Attachment is obsolete: true
Attachment #720542 - Flags: review?(htsai)
Attachment #718332 - Flags: review?(htsai)
Comment on attachment 718310 [details] [diff] [review]
Part 1: Add pin2 for updateICCContact.

Switch r? to Vicamo as Hsinyi is busy.
Attachment #718310 - Flags: review?(htsai) → review?(vyang)
Attachment #718332 - Flags: review?(htsai) → review?(vyang)
Attachment #720541 - Flags: review?(htsai) → review?(vyang)
Attachment #720542 - Flags: review?(htsai) → review?(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.
Attachment #718310 - Flags: review?(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.
Attachment #718332 - Flags: review?(vyang) → review+
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?
Attachment #720541 - Flags: review?(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|.
Attachment #720541 - Flags: review+
Created attachment 722066 [details] [diff] [review]
Part 1: Add pin2 for updateICCContact. v2

rebase
Attachment #718310 - Attachment is obsolete: true
Attachment #722066 - Flags: review?(vyang)
Created attachment 722068 [details] [diff] [review]
Part 2: implementations for updating FDN. v3

replaced "" with null.
Attachment #718332 - Attachment is obsolete: true
Created attachment 722070 [details] [diff] [review]
Part 3: ICCRecordHelper.getFreeRecordId. v2

add r=vicamo
Attachment #720541 - Attachment is obsolete: true
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();
Attachment #720542 - Flags: review?(vyang) → review+
Attachment #722066 - Flags: review?(vyang) → review+
(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.
Created attachment 722084 [details] [diff] [review]
Part 4: xpcshell tests for updateFdn and updateICCContact. v4

Addressed comments, 
add r=vicamo
Attachment #720542 - Attachment is obsolete: true
Attachment #722084 - Flags: review+
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/yhuang@mozilla.com-ab08bcb9da1f
blocking-b2g: --- → tef?
I think PIN2 and FDN are not V1.0.1. Please re-nominate if I am wrong.
blocking-b2g: tef? → ---
Blocks: 888902
No longer blocks: 888902

Updated

4 years ago
Blocks: 1157082
You need to log in before you can comment on or make changes to this bug.