B2G RIL: Support reading ANR from Phonebook

RESOLVED FIXED in mozilla22

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: allstars, Unassigned)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 6 obsolete attachments)

In EF_PBR there's also an EF called "ANR" (Additional Number),
we should also read this EF when we read Phonebook.
(Assignee)

Updated

5 years ago
Assignee: nobody → allstars.chh
Created attachment 717039 [details] [diff] [review]
Part 1: rename anr[] to anr0 and anr1.
Created attachment 717040 [details] [diff] [review]
Part 2: Read ANR in RIL.
Created attachment 717042 [details] [diff] [review]
Part 3: Update ContactManager.
Created attachment 717045 [details] [diff] [review]
Part 4: xpcshell for read Anr
Created attachment 717046 [details] [diff] [review]
Part 1: rename anr[] to anr0 and anr1. v2
Attachment #717039 - Attachment is obsolete: true
Created attachment 717051 [details] [diff] [review]
Part 4: xpcshell for read Anr v2
Attachment #717045 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #717046 - Flags: review?(htsai)
(Assignee)

Updated

5 years ago
Attachment #717040 - Flags: review?(htsai)
(Assignee)

Updated

5 years ago
Attachment #717042 - Flags: review?(htsai)
(Assignee)

Updated

5 years ago
Attachment #717042 - Flags: review?(htsai) → review?(anygregor)
(Assignee)

Updated

5 years ago
Attachment #717051 - Flags: review?(htsai)
Comment on attachment 717042 [details] [diff] [review]
Part 3: Update ContactManager.

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

::: dom/contacts/ContactManager.js
@@ +394,5 @@
> +            // ANR - Additional Number
> +            if (c.anr) {
> +              for (let i = 0; i < c.anr.length; i++) {
> +                prop.tel.push(c.anr[i]);
> +              }

You have to create a new object with the value property set.
look above:
tel: [ { value: c.number } ]}
Comment on attachment 717042 [details] [diff] [review]
Part 3: Update ContactManager.

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

Got it, 
thank you,
Attachment #717042 - Flags: review?(anygregor)
Created attachment 718273 [details] [diff] [review]
Part 2: Read ANR in RIL. v2
Attachment #717040 - Attachment is obsolete: true
Attachment #717040 - Flags: review?(htsai)
Attachment #718273 - Flags: review?(htsai)
Created attachment 718275 [details] [diff] [review]
Part 3: Update ContactManager. v2

Addressed to Gregor's comment,
but I didn't send r? to Gregor because Bug 838092 is still under discussion.
Attachment #717042 - Attachment is obsolete: true
Created attachment 718276 [details] [diff] [review]
Part 4: xpcshell for read Anr v3
Attachment #717051 - Attachment is obsolete: true
Attachment #717051 - Flags: review?(htsai)
(Assignee)

Updated

5 years ago
Attachment #718276 - Flags: review?(htsai)

Updated

5 years ago
Attachment #717046 - Flags: review?(htsai) → review+
Comment on attachment 718273 [details] [diff] [review]
Part 2: Read ANR in RIL. v2

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

::: dom/system/gonk/ril_worker.js
@@ +9169,5 @@
>  
>    /**
> +   * Read USIM Phonebook EF_ANR.
> +   *
> +   * @see TS 131.102, clause 4.4.2.13

Should be clause 4.4.2.9?

@@ +9171,5 @@
> +   * Read USIM Phonebook EF_ANR.
> +   *
> +   * @see TS 131.102, clause 4.4.2.13
> +   *
> +   * @param fileId       EF id of the EMAIL.

EF id should be ANR?

@@ +9172,5 @@
> +   *
> +   * @see TS 131.102, clause 4.4.2.13
> +   *
> +   * @param fileId       EF id of the EMAIL.
> +   * @param fileType     The type of the EMAIL, one of the ICC_USIM_TYPE* constants.

ditto.

@@ +10063,5 @@
>  
>    /**
> +   * Read Phonebook fields.
> +   *
> +   * @param pbr         PBR file

I prefer the full name to the abbreviation PBR.

@@ +10069,5 @@
> +   * @param onsuccess   Callback to be called when success.
> +   * @param onerror     Callback to be called when error.
> +   */
> +  readPhonebookFields: function readPhonebookFields(pbr, contacts, onsuccess, onerror) {
> +    const fields = ["email", "anr0"];

Please leave a comment here, saying something that if we want to read more anr in the future, we just need to modify this const.

@@ +10093,5 @@
> +   * @param field         Phonebook field to be retrieved.
> +   * @param onsuccess     Callback to be called when success.
> +   * @param onerror       Callback to be called when error.
> +   */
> +  readPhonebookField: function readPhonebookField(pbr, contacts, field, onsuccess, onerror) {

readPhonebookField and readPhonebookFields are too similar. Though I understand why you use these names, I also think it would be good to adopt more distinguishable names, or at least leave some comment.

@@ +10103,5 @@
> +    }
> +
> +    (function doReadContactField(n) {
> +      if (n >= contacts.length) {
> +        // All contact's fields are readed.

nit: past participle of 'read' is read, not readed.

@@ +10122,3 @@
>     *
>     * @param pbr           The phonebook reference file.
>     * @param contact       The contact needs to get email.

to get email? revise this comment?
Attachment #718273 - Flags: review?(htsai) → review+
Comment on attachment 718276 [details] [diff] [review]
Part 4: xpcshell for read Anr v3

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

Great, thanks!
Attachment #718276 - Flags: review?(htsai) → review+
Created attachment 718342 [details] [diff] [review]
Part 2: Read ANR in RIL. v3

Addressed Hsinyi's comments:
1. update comments (remove email-related comments, add full name for comments of 'pbr', and for current supported fields to be read)

2. rename readPhonebookFields to readSupportedPBRFields
Attachment #718273 - Attachment is obsolete: true

Comment 15

5 years ago
Try run for 28edeb408ceb is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=28edeb408ceb
Results (out of 4 total builds):
    success: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/yhuang@mozilla.com-28edeb408ceb
(Assignee)

Updated

5 years ago
Attachment #718275 - Flags: review?(anygregor)
Attachment #718275 - Flags: review?(anygregor) → review+

Updated

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