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

WIP patch

WIP Patch

WIP patch

WIP Patch

rebase (based on bug 816893)

rebase (based on bug 816893)

rebase (based on bug 816893)

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

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

Marionette tests

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

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

1). Rebase and rewrite.

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

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

xpcshell tests

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

(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.

(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?

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.

Separate readEmail and readIAP into another patch.

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

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 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 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, "email@mozilla.com", 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();

Formal patch

Address comment #30

Formal patch.

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

Address review comment #36

Address review comment #37

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

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

(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 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: "email@mozilla.com$#"}, > + {type: ICC_USIM_TYPE2_TAG, expectedResult: "email@mozilla.com"}, > + ]; > + > + doNextTest(); The naming needs to be revised, otherwise it would be confusing with run_next_test.

Address review comment #46

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

(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 :-)

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

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

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

just part 3

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

(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 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

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.

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

My bad, thanks. https://hg.mozilla.org/releases/mozilla-b2g18/rev/c3e876fffc07 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/8887a20b71ae