842460 - B2G RIL: Support reading ANR from Phonebook

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

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

Comment 1

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

Attachment: Part 1: rename anr[] to anr0 and anr1.

Comment 2

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

Attachment: Part 2: Read ANR in RIL.

Comment 3

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

Attachment: Part 3: Update ContactManager.

Comment 4

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

Attachment: Part 4: xpcshell for read Anr

Comment 5

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

Attachment: Part 1: rename anr[] to anr0 and anr1. v2

Comment 6

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

Attachment: Part 4: xpcshell for read Anr v2

Comment 7

[Gregor Wagner [:gwagner]]

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 8

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

Comment on attachment 717042 [details] [diff] [review]
Part 3: Update ContactManager.

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

Got it, 
thank you,

Comment 9

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

Attachment: Part 2: Read ANR in RIL. v2

Comment 10

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

Attachment: 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.

Comment 11

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

Attachment: Part 4: xpcshell for read Anr v3

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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?

Comment 13

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 718276 [details] [diff] [review]
Part 4: xpcshell for read Anr v3

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

Great, thanks!

Comment 14

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

Attachment: 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

Comment 15

[Mozilla RelEng Bot]

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/[e-mail]-28edeb408ceb

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b1aad4d18a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/37185db4b9fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a37e14650b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/d811830b3c44

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3b1aad4d18a9
https://hg.mozilla.org/mozilla-central/rev/37185db4b9fe
https://hg.mozilla.org/mozilla-central/rev/5a37e14650b5
https://hg.mozilla.org/mozilla-central/rev/d811830b3c44