900850 - B2G CDMA: Support accessing contacts of CDMA RUIM

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Edgar Chen [:edgar]]

We support access contacts of SIM and USIM. We should support access contacts in RUIM for CDMA.

Comment 1

[Edgar Chen [:edgar]]

Please see 3GPP2 C.S0023-D session 2.7

Comment 2

[Edgar Chen [:edgar]]

* FDN: Same as 3GPP TS 51.011 session 10.5.2
* ADN: Optionally provide enhanced phonebook in DF_PHONEBOOK.
  - If support enhanced phonebook: Same as 3GPP TS 31.105 session 4.4.2
  - If not: Same as 3GPP TS 51.011 session 10.5.1

Comment 3

[Edgar Chen [:edgar]]

Attachment: Part 1: Support accessing contacts of CDMA RUIM, v1

Comment 4

[Edgar Chen [:edgar]]

Attachment: Part 1: Support accessing contacts of CDMA RUIM, v2

Comment 5

[Edgar Chen [:edgar]]

Comment on attachment 786168 [details] [diff] [review]
Part 1: Support accessing contacts of CDMA RUIM, v2

(In reply to Edgar Chen [:edgar][:echen] from comment #2)
> * FDN: Same as 3GPP TS 51.011 session 10.5.2
> * ADN: Optionally provide enhanced phonebook in DF_PHONEBOOK.
>   - If support enhanced phonebook: Same as 3GPP TS 31.105 session 4.4.2
>   - If not: Same as 3GPP TS 51.011 session 10.5.1

Depended on Service n6 in EF_CST. (Enhanced Phone Book)
Please see C.S.0023-D session 3.4.18

If service is allocated and actived, same as 3GPP TS 31.105 session 4.4.2. (USIM)
Otherwise, same as 3GPP TS 51.011 session 10.5.1. (SIM)

I have tested this patch with n970 + 亞太 RUIM, it works well.
I am trying to write a xpcshell tests now.

Thanks

Comment 6

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

Comment on attachment 786168 [details] [diff] [review]
Part 1: Support accessing contacts of CDMA RUIM, v2

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

This patch is okay,
but I'd like to make it simpler.

::: dom/system/gonk/ril_consts.js
@@ +1164,5 @@
>      OPL: 46,
>      SPDI: 51
>    },
>    ruim: {
> +    ENHANCED_PHONE_BOOK: 6,

Is it a seperated word? 
Phonebook or Phone book?

::: dom/system/gonk/ril_worker.js
@@ +12192,5 @@
> +      case CARD_APPTYPE_RUIM:
> +        if (ICCUtilsHelper.isICCServiceAvailable("ENHANCED_PHONE_BOOK")) {
> +          return true;
> +        }
> +        return false;

return ICCUtilsHelper.isICCServiceAvailable("ENHANCED_PHONE_BOOK");

@@ +12194,5 @@
> +          return true;
> +        }
> +        return false;
> +      default:
> +        throw new Error("Unknown icc type");

This function should simply return true or false,
why do you trying to make it more complicated?

@@ +12209,5 @@
>     */
>    readICCContacts: function readICCContacts(appType, contactType, onsuccess, onerror) {
>      switch (contactType) {
>        case "adn":
> +        try {

If we remove the throwing above, then we could remove this try-catch block as well to make it simpler.

@@ +12224,5 @@
>          break;
>        case "fdn":
>          ICCRecordHelper.readADNLike(ICC_EF_FDN, onsuccess, onerror);
>          break;
> +      default:

How about 
let error = onerorr || debug;
error(...)
with more detail error message.

Same as below.

Comment 7

[Edgar Chen [:edgar]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #6)
> Comment on attachment 786168 [details] [diff] [review]
> Part 1: Support accessing contacts of CDMA RUIM, v2
> 
> Review of attachment 786168 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch is okay,
> but I'd like to make it simpler.
> 
> ::: dom/system/gonk/ril_consts.js
> @@ +1164,5 @@
> >      OPL: 46,
> >      SPDI: 51
> >    },
> >    ruim: {
> > +    ENHANCED_PHONE_BOOK: 6,
> 
> Is it a seperated word? 
> Phonebook or Phone book?
It should be Phonebook, I will correct this.

> 
> ::: dom/system/gonk/ril_worker.js
> @@ +12192,5 @@
> > +      case CARD_APPTYPE_RUIM:
> > +        if (ICCUtilsHelper.isICCServiceAvailable("ENHANCED_PHONE_BOOK")) {
> > +          return true;
> > +        }
> > +        return false;
> 
> return ICCUtilsHelper.isICCServiceAvailable("ENHANCED_PHONE_BOOK");
> 
> @@ +12194,5 @@
> > +          return true;
> > +        }
> > +        return false;
> > +      default:
> > +        throw new Error("Unknown icc type");
> 
> This function should simply return true or false,
> why do you trying to make it more complicated?
It is because I am not sure what value should be returned if someone pass a unknown SIM type. Maybe we could just return false in this case, instead of throwing an error.
> 
> @@ +12209,5 @@
> >     */
> >    readICCContacts: function readICCContacts(appType, contactType, onsuccess, onerror) {
> >      switch (contactType) {
> >        case "adn":
> > +        try {
> 
> If we remove the throwing above, then we could remove this try-catch block
> as well to make it simpler.
Okay, I will remove throwing and try-catch to make it simpler.

> 
> @@ +12224,5 @@
> >          break;
> >        case "fdn":
> >          ICCRecordHelper.readADNLike(ICC_EF_FDN, onsuccess, onerror);
> >          break;
> > +      default:
> 
> How about 
> let error = onerorr || debug;
> error(...)
> with more detail error message.
> 
> Same as below.

Okay, I will use this.

Thanks

Comment 8

[Edgar Chen [:edgar]]

Attachment: Part 1: Support accessing contacts of CDMA RUIM, v3

Comment 9

[Edgar Chen [:edgar]]

Comment on attachment 786709 [details] [diff] [review]
Part 1: Support accessing contacts of CDMA RUIM, v3

Address review comment #6

Comment 10

[Edgar Chen [:edgar]]

Attachment: Part 2: Xpcshell tests for accessing contacts of CDMA RUIM, v1

Comment 11

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

Comment on attachment 786709 [details] [diff] [review]
Part 1: Support accessing contacts of CDMA RUIM, v3

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

::: dom/system/gonk/ril_worker.js
@@ +12299,5 @@
>  
>    /**
>     * Helper function to update ICC contact.
>     *
> +   * @param appType       CARD_APPTYPE_SIM or CARD_APPTYPE_USIM or CARD_APPTYPE_RUIM.

nit, One of ...

Comment 12

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

Comment on attachment 786772 [details] [diff] [review]
Part 2: Xpcshell tests for accessing contacts of CDMA RUIM, v1

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

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +2444,5 @@
> +    let onsuccess = function onsuccess(contacts) {
> +      let contact = contacts[0];
> +      for (key in contact) {
> +        do_print("check " + key);
> +        if (contact[key] instanceof Array) {

Array.isArray
See http://web.mit.edu/jwalden/www/isArray.html

Comment 13

[Edgar Chen [:edgar]]

Attachment: Part 1: Support accessing contacts of CDMA RUIM, v4, r=allstars.chh

Address review comment #11.

Comment 14

[Edgar Chen [:edgar]]

Attachment: Part 2: Xpcshell tests for accessing contacts of CDMA RUIM, v2, r=allstars.chh

Address review comment #12.

Comment 15

[Edgar Chen [:edgar]]

try server: https://tbpl.mozilla.org/?tree=Try&rev=c7286c9ccfd7

Comment 16

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/rev/bee83df3c47d
https://hg.mozilla.org/integration/b2g-inbound/rev/733697c789e8

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bee83df3c47d
https://hg.mozilla.org/mozilla-central/rev/733697c789e8