809725 - B2G RIL: Read Email in USIM Contact

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

From USim Contact, there are some additional information, for example, Email and ANR(Additional number).
We should read these information from USim as well.

Comment 1

[Edgar Chen [:edgar]]

Attachment: WIP, Part 1: Refactor getICCContacts, v1

WIP patch

Comment 2

[Edgar Chen [:edgar]]

Attachment: WIP, Part 2: Read EF_IAP, v1

WIP Patch

Comment 3

[Edgar Chen [:edgar]]

Attachment: WIP, Part 3: Read EF_EMAIL and EF_ANR, v1

WIP patch

Comment 4

[Edgar Chen [:edgar]]

Attachment: Part 4: Export Email and ANR to contact, v1

WIP Patch

Comment 5

[Edgar Chen [:edgar]]

Attachment: Part 1: Refactor getICCContacts, v2

rebase (based on bug 816893)

Comment 6

[Edgar Chen [:edgar]]

Attachment: Part 2: Read EF_IAP, v2

rebase (based on bug 816893)

Comment 7

[Edgar Chen [:edgar]]

Attachment: Part 3: Read EF_EMAIL and EF_ANR, v2

rebase (based on bug 816893)

Comment 8

[Edgar Chen [:edgar]]

Attachment: Github: Add EFpbr/EFiap/EFemail/EFanr in emulator

github pull request:
https://github.com/mozilla-b2g/platform_external_qemu/pull/14

Comment 9

[Edgar Chen [:edgar]]

Attachment: Github: Change SIM type to USIM in emulator

github pull request:
https://github.com/mozilla-b2g/platform_hardware_ril/pull/6

Comment 10

[Edgar Chen [:edgar]]

Attachment: Part 5: Marionette tests for reading email and anr, v1

Marionette tests

Comment 11

[Edgar Chen [:edgar]]

Marionette tests for USIM contacts will be tracking by bug 824904.
Let's focus on main function here first.

Comment 12

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

As I found Nokia and Android phones doesn't support writing ANR, so we should focus on Email first.

Comment 13

[Edgar Chen [:edgar]]

Attachment: WIP, Part 1: Read Email in RIL, v1

Comment 14

[Edgar Chen [:edgar]]

Attachment: WIP, Part 2: Export Email to contact, v2

Comment 15

[Edgar Chen [:edgar]]

Attachment: WIP, Part 1: Read Email in RIL, v2

1). Rebase and rewrite.

Comment 16

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

As I have discussed wtih Joe, this feature won't be included in v1.0.1

Comment 17

[Joe Cheng [:jcheng] (please needinfo)]

cc Kev/Kevin
this is bringing additional fields into Contact import from SIM.
v1.0.1 should have the same feature set as v1.0.0 in this regard (only import Name/Phone number). Therefore removing the email field import feature from the v1.0.1 list

Comment 18

[Edgar Chen [:edgar]]

Attachment: WIP, Part 1: Read Email in RIL, v3

Comment 19

[Edgar Chen [:edgar]]

Attachment: WIP, Part 3: xpcshell tests for read email, v1

xpcshell tests

Comment 20

[Edgar Chen [:edgar]]

Comment on attachment 707979 [details] [diff] [review]
WIP, Part 1: Read Email in RIL, v3

These patches are WIP patch without cleanup.
Needs your help to provide some feedback when you are available?
Thanks in advance :)

Comment 21

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

Comment on attachment 707979 [details] [diff] [review]
WIP, Part 1: Read Email in RIL, v3

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

::: dom/system/gonk/ril_worker.js
@@ +6219,5 @@
> +   *
> +   * @param numOctets   Number of octets to be read.
> +   * @param type        The type of the EF, one fo the ICC_USIM_TYPE* constants.
> +   */
> +  readEmail: function readEmail(numOctets, type) {

This function seems only used by ICC, as we're going to refactor ril_worker,
I think we don't need this util function in GsmPDUHelper.

@@ +6238,5 @@
> +
> +      // Consumes the remaining buffer
> +      Buf.seekIncoming(2 * PDU_HEX_OCTET_SIZE); // For ADN SFI and Record Identifier
> +    }
> +

Also seems the code is short, you could move them into your readEMAIL.

@@ +9050,5 @@
>                                     onerror: onerror});
>    },
>  
>    /**
> +   * Read ICC EF_IAP. (Index Administration Phone book)

Phonebook.

@@ +9092,5 @@
> +   * @param recordNumber The number of the record shall be loaded.
> +   * @param onsuccess    Callback to be called when success.
> +   * @param onerror      Callback to be called when error.
> +   */
> +  readEMAIL: function readEMAIL(fileId, fileType, recordNumber, onsuccess, onerror) {

EMAIL is not an abbreviation, so maybe Email should just fine.

@@ +9892,5 @@
> +   * @param contacts      The contacts need to get email.
> +   * @param onsuccess     Callback to be called when success.
> +   * @param onerror       Callback to be called when error.
> +   */
> +  readUSimAllContactsEmail: function readUSimAllContactsEmail(contacts, onsuccess, onerror) {

Seems readUSimEmails is enough.
Or if you prefer readUSimContactEmail, at least I think the plural 's' should added to Email, i.e. readUSimContactEmails.

@@ +9895,5 @@
> +   */
> +  readUSimAllContactsEmail: function readUSimAllContactsEmail(contacts, onsuccess, onerror) {
> +    let index = 0;
> +    let gotEmailCb = function gotEmailCb(email) {
> +      contacts[index].email = email;

We should add check for email here,
if (email) {
  contacts[index].email = email;
}

otherwise we will add an email property for every contact.

@@ +9916,5 @@
> +
> +      return;
> +    }
> +
> +    this.readUSimContactEmail(contacts[index], gotEmailCb, onerror);

It took me several minutes to figure out you're doing a while-loop here.
Can you make it simpler?

@@ +9926,5 @@
> +   * @param contact       The contact needs to get email.
> +   * @param onsuccess     Callback to be called when success.
> +   * @param onerror       Callback to be called when error.
> +   */
> +  readUSimContactEmail: function readUSimContactEmail(contact, onsuccess, onerror) {

I think you should create a function called getEmailRecordId to seperate the complication.

So in this function you could simply 
1. Get correct record ID.
2. readEmail
3. callback.

@@ +9961,5 @@
> +        ICCRecordHelper.readEMAIL(emailFileId, emailFileType, contact.recordId, onsuccess, onerror);
> +      }
> +    }.bind(this);
> +
> +    ICCRecordHelper.readPBR(gotPbrCb, onerror);

I don't understand why readPBR again.

@@ +10017,5 @@
> +   * @param indexInIAP   The order in IAP.
> +   * @param onsuccess    Callback to be called when success.
> +   * @param onerror      Callback to be called when error.
> +   */
> +  getRecordIdFromIAP: function getRecordIdFromIAP(fileId, recordNumber, indexInIAP, onsuccess, onerror) {

This function needs to more generalized,
it should get recordId from corresponding ADN recordId if it's FILE_TYPE_1
otherwise get the recordId from IAP.

Comment 22

[Edgar Chen [:edgar]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #21)
> Comment on attachment 707979 [details] [diff] [review]
> WIP, Part 1: Read Email in RIL, v3
> 
> Review of attachment 707979 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +9961,5 @@
> > +        ICCRecordHelper.readEMAIL(emailFileId, emailFileType, contact.recordId, onsuccess, onerror);
> > +      }
> > +    }.bind(this);
> > +
> > +    ICCRecordHelper.readPBR(gotPbrCb, onerror);
> 
> I don't understand why readPBR again.

We need to call readPBR() again to get the fileId/fileType/indexInIAP of EFemail and EFiap.

Comment 23

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

(In reply to Edgar Chen [:edgar][:echen] from comment #22) 
> We need to call readPBR() again to get the fileId/fileType/indexInIAP of
> EFemail and EFiap.

In your patch you already got pbr in line 9868,
why don't you use that?

Comment 24

[Kev Needham [:kev]]

Concur with Joe in comment #17. This sounds like a 1.1 change, as it's not in the list of changes for 1.0.1, and I'd prefer we keep any functional changes to a bare minimum.

Comment 25

[Edgar Chen [:edgar]]

Attachment: WIP, Part 1: Add readEmail and readIAP in ICCRecordHelper, v1

Separate readEmail and readIAP into another patch.

Comment 26

[Edgar Chen [:edgar]]

Attachment: WIP, Part 2: Read contact email, v4

1). Separate readEmail and readIAP into another patch.
2). Address comment #21

Comment 27

[Edgar Chen [:edgar]]

Attachment: WIP, Part 3: Export Email to contact, v2

Comment 28

[Edgar Chen [:edgar]]

Attachment: WIP, Part 4: xpcshell tests for readEmail, v2

Comment 29

[Edgar Chen [:edgar]]

Comment on attachment 708980 [details] [diff] [review]
WIP, Part 4: xpcshell tests for readEmail, v2

Hi Yoshi,
Need your help to provide some feedback when you are available.
Thanks in advance :)

Comment 30

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

Comment on attachment 708978 [details] [diff] [review]
WIP, Part 2: Read contact email, v4

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

::: dom/system/gonk/ril_worker.js
@@ +9999,5 @@
> +   * @param pbr           The phonebook reference file.
> +   * @param onsuccess     Callback to be called when success.
> +   * @param onerror       Callback to be called when error.
> +   */
> +  readUSimEmails: function readUSimEmails(contacts, pbr, onsuccess, onerror) {

pbr should be first parameter.

@@ +10001,5 @@
> +   * @param onerror       Callback to be called when error.
> +   */
> +  readUSimEmails: function readUSimEmails(contacts, pbr, onsuccess, onerror) {
> +    let n = 0;
> +    let doReadContactEmail = function doReadContactEmail() {

(function doReadContactEmail(n) {
...
})(0);

@@ +10026,5 @@
> +   * @param pbr           The phonebook reference file.
> +   * @param onsuccess     Callback to be called when success.
> +   * @param onerror       Callback to be called when error.
> +   */
> +  readUSimContactEmail: function readUSimContactEmail(contact, pbr, onsuccess, onerror) {

pbr should be first parameter.

@@ +10101,5 @@
> +   * Get the recordId of Email.
> +   *
> +   * @see TS 131.102, clause 4.4.2.2
> +   *
> +   * @param contact      The contact will be updated.

update comments.

Comment 31

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

Comment on attachment 708980 [details] [diff] [review]
WIP, Part 4: xpcshell tests for readEmail, v2

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

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +1653,5 @@
> +    }
> +  };
> +
> +  function doTestReadEmail(type, expectedResult, oncomplete) {
> +    record.readEmail(0x6a75, type, 1, function (email) {

I guess 0x6a75 and 1 here are for test purpose only.
Could you add comments to indicate that some arguments are dummy?

@@ +1665,5 @@
> +    // Test ICC_USIM_TYPE2_TAG
> +    doTestReadEmail(ICC_USIM_TYPE2_TAG, "[e-mail]", function () {
> +      run_next_test();
> +    });
> +  });

I wouldn't suggest to use this nested function to run multiple tests.

Can you try to make it simpler?
like 
doTestReadEmail(testData, expectData);
doTestReadEmail(testData1, expectData1);

run_next_test();

Comment 32

[Edgar Chen [:edgar]]

Attachment: Part 1: Add readEmail and readIAP in ICCRecordHelper, v2

Formal patch

Comment 33

[Edgar Chen [:edgar]]

Attachment: Part 2: Read contact email, v5

Address comment #30

Comment 34

[Edgar Chen [:edgar]]

Attachment: Part 3: Export Email to contact, v2

Formal patch.

Comment 35

[Edgar Chen [:edgar]]

Attachment: Part 2: Read contact email, v6

Comment 36

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

Comment on attachment 709354 [details] [diff] [review]
Part 1: Add readEmail and readIAP in ICCRecordHelper, v2

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

I suggest you remove those debug message, otherwise it may pollute our ril log.

::: dom/system/gonk/ril_worker.js
@@ +9156,5 @@
> +
> +      let iap = GsmPDUHelper.readHexOctetArray(octetLen);
> +      Buf.readStringDelimiter(strLen);
> +
> +      if (DEBUG) debug("readIAP: " + JSON.stringify(iap));

nit, always prefer 
if (DEBUG) {

}

even it's just one line.

Also should we print it here? readIAP could be called for 254 times here.

@@ +9205,5 @@
> +      }
> +
> +      Buf.readStringDelimiter(strLen);
> +
> +      if (DEBUG) debug("readEmail: " + email);

ditto

Comment 37

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

Comment on attachment 709546 [details] [diff] [review]
Part 2: Read contact email, v6

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

::: dom/system/gonk/ril_worker.js
@@ +10116,5 @@
> +      let gotIapCb = function gotIapCb(iap) {
> +        let indexInIAP = pbr.email.indexInIAP;
> +        let recordId = iap[indexInIAP];
> +
> +        if (DEBUG) debug("getEmaildRecordId: " + recordId);

Remove this debug

Comment 38

[Edgar Chen [:edgar]]

Attachment: Part 4: xpcshell tests for readEmail, v3

Comment 39

[Edgar Chen [:edgar]]

Attachment: Part 1: Add readEmail and readIAP in ICCRecordHelper, v3, r=allstars.chh

Address review comment #36

Comment 40

[Edgar Chen [:edgar]]

Attachment: Part 2: Read contact email, v7, r=allstars.chh

Address review comment #37

Comment 41

[Edgar Chen [:edgar]]

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

Comment 42

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

Comment on attachment 709588 [details] [diff] [review]
Part 4: xpcshell tests for readEmail, v3

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

Please address comment 31 first.

Comment 43

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

Comment on attachment 709588 [details] [diff] [review]
Part 4: xpcshell tests for readEmail, v3

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

I didn't ask you to write a perfect patch here, but at least 'Try' to do it.

You could move those tests into seperate functions, or even create your helper function/object.
Anyway, it's just a test case, so please use your imagination here.

Just don't give it up without trying.

Comment 44

[Edgar Chen [:edgar]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #43)
> Comment on attachment 709588 [details] [diff] [review]
> Part 4: xpcshell tests for readEmail, v3
> 
> Review of attachment 709588 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I didn't ask you to write a perfect patch here, but at least 'Try' to do it.
> 
> You could move those tests into seperate functions, or even create your
> helper function/object.
> Anyway, it's just a test case, so please use your imagination here.
> 
> Just don't give it up without trying.

Sure, I will try it, thanks :)

Comment 45

[Edgar Chen [:edgar]]

Attachment: Part 4: xpcshell tests for readEmail, v4

Comment 46

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

Comment on attachment 710009 [details] [diff] [review]
Part 4: xpcshell tests for readEmail, v4

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

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +1672,5 @@
> +      return;
> +    }
> +
> +    doTestReadEmail(test.type, test.expectedResult, doNextTest);
> +  };

(function doNextTest() {
...
})();

@@ +1674,5 @@
> +
> +    doTestReadEmail(test.type, test.expectedResult, doNextTest);
> +  };
> +
> +  let tests = [

Move tests into function.

@@ +1679,5 @@
> +    {type: ICC_USIM_TYPE1_TAG, expectedResult: "[e-mail]$#"},
> +    {type: ICC_USIM_TYPE2_TAG, expectedResult: "[e-mail]"},
> +  ];
> +
> +  doNextTest();

The naming needs to be revised, otherwise it would be confusing with run_next_test.

Comment 47

[Edgar Chen [:edgar]]

Attachment: Part 4: xpcshell tests for readEmail, v5, r=allstars.chh

Address review comment #46

Comment 48

[Edgar Chen [:edgar]]

Attachment: Part 4: xpcshell tests for readEmail, v6

Oh, I just realize that we fake the loadLinearFixedEF(), so the process of readEmail() is not asynchronous any more. We can run tests one by one instead of using callback. Hi Yoshi, can you help to review again. Thanks :)

Comment 49

[Ed Morley [:emorley]]

(In reply to Edgar Chen [:edgar][:echen] from comment #41)
> try server: https://tbpl.mozilla.org/?tree=Try&rev=2ec52dab8866

Please don't use -p all for B2G only pushes! This used ~140 hours of compute time since it tested on every platform.

(You are #3 on top users for the last week, see bug 829932 / http://people.mozilla.org/~catlee/highscores/highscores.html)

Thank you :-)

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc166e26efa4
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d2238d5ff59
https://hg.mozilla.org/integration/mozilla-inbound/rev/726591db8896
https://hg.mozilla.org/integration/mozilla-inbound/rev/8feaf3eba8ab

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bc166e26efa4
https://hg.mozilla.org/mozilla-central/rev/2d2238d5ff59
https://hg.mozilla.org/mozilla-central/rev/726591db8896
https://hg.mozilla.org/mozilla-central/rev/8feaf3eba8ab

Comment 52

[Michael Wu [:mwu]]

Comment on attachment 709356 [details] [diff] [review]
Part 3: Export Email to contact, v2

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
This patch is in the codeaurora patch queue. If the risk is low, we should land it since downstream is using it regardless.

Comment 53

[Andrew Overholt [:overholt]]

mwu, do we need the other parts or just part 3?

Comment 54

[Michael Vines [:m1] [:evilmachines]]

just part 3

Comment 55

[Alex Keybl [:akeybl]]

echen - can you provide a risk evaluation for uplift to v1.1?

Comment 56

[Edgar Chen [:edgar]]

(In reply to Alex Keybl [:akeybl] from comment #55)
> echen - can you provide a risk evaluation for uplift to v1.1?

The risk is low for uplift only part 3. If RIL doesn't support reading email from SIM card, it will not export email property to contact either.

Comment 57

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 709356 [details] [diff] [review]
Part 3: Export Email to contact, v2

Go ahead with uplift to v1-train and v1.0.1 before 2/28

Comment 58

[Ryan VanderMeulen [:RyanVM]]

This doesn't apply cleanly to b2g18. Please post branch-specific patches for uplift or get uplift approval for any other bugs this depends on.

Comment 59

[Edgar Chen [:edgar]]

As mention in comment 52 and comment 54, we should only need to uplift part3. :mwu, am I right?

Comment 60

[Ryan VanderMeulen [:RyanVM]]

My bad, thanks.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/c3e876fffc07
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/8887a20b71ae