Closed Bug 900850 Opened 8 years ago Closed 8 years ago

B2G CDMA: Support accessing contacts of CDMA RUIM

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26
blocking-b2g koi+
Tracking Status
firefox26 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: edgar, Assigned: edgar)

References

Details

Attachments

(2 files, 4 obsolete files)

We support access contacts of SIM and USIM. We should support access contacts in RUIM for CDMA.
Please see 3GPP2 C.S0023-D session 2.7
* 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
Attachment #784924 - Attachment is obsolete: true
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
Attachment #786168 - Flags: review?(allstars.chh)
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.
Attachment #786168 - Flags: review?(allstars.chh)
(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
Attachment #786168 - Attachment is obsolete: true
Comment on attachment 786709 [details] [diff] [review]
Part 1: Support accessing contacts of CDMA RUIM, v3

Address review comment #6
Attachment #786709 - Attachment description: bug_900850_part1_support_accessing_contacts_of_cdam_ruim_v3.patch → Part 1: Support accessing contacts of CDMA RUIM, v3
Attachment #786709 - Flags: review?(allstars.chh)
Attachment #786772 - Flags: review?(allstars.chh)
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 ...
Attachment #786709 - Flags: review?(allstars.chh) → review+
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
Attachment #786772 - Flags: review?(allstars.chh) → review+
Address review comment #11.
Attachment #786709 - Attachment is obsolete: true
Attachment #786858 - Flags: review+
blocking-b2g: --- → koi?
Blocks: 902754
https://hg.mozilla.org/mozilla-central/rev/bee83df3c47d
https://hg.mozilla.org/mozilla-central/rev/733697c789e8
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
blocking-b2g: koi? → koi+
Blocks: 1157082
You need to log in before you can comment on or make changes to this bug.