Closed Bug 809725 Opened 8 years ago Closed 8 years ago

B2G RIL: Read Email in USIM Contact

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
blocking-b2g tef+
Tracking Status
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: allstars.chh, Assigned: edgar)

References

Details

Attachments

(4 files, 25 obsolete files)

1.29 KB, patch
gwagner
: review+
Details | Diff | Splinter Review
3.58 KB, patch
Details | Diff | Splinter Review
5.24 KB, patch
Details | Diff | Splinter Review
1.91 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
From USim Contact, there are some additional information, for example, Email and ANR(Additional number).
We should read these information from USim as well.
Depends on: 821584
WIP patch
Attached patch WIP, Part 2: Read EF_IAP, v1 (obsolete) — Splinter Review
WIP Patch
WIP patch
Assignee: nobody → echen
WIP Patch
Depends on: 816893
rebase (based on bug 816893)
Attachment #695386 - Attachment is obsolete: true
Attached patch Part 2: Read EF_IAP, v2 (obsolete) — Splinter Review
rebase (based on bug 816893)
Attachment #695387 - Attachment is obsolete: true
rebase (based on bug 816893)
Attachment #695388 - Attachment is obsolete: true
Attachment #695556 - Attachment description: WIP, Part 4: Export Email and ANR to contact, v1 → Part 4: Export Email and ANR to contact, v1
Marionette tests
Blocks: 824904
Attachment #695936 - Attachment is obsolete: true
Attachment #695939 - Attachment is obsolete: true
Attachment #695940 - Attachment is obsolete: true
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.
Summary: B2G RIL: Read Email and ANR in USIM Contact → B2G RIL: Read Email in USIM Contact
Attachment #695717 - Attachment is obsolete: true
Attachment #695718 - Attachment is obsolete: true
Attachment #695719 - Attachment is obsolete: true
Attachment #695556 - Attachment is obsolete: true
Attachment #700475 - Attachment description: Part 1: Read Email in RIL, v1 → WIP, Part 1: Read Email in RIL, v1
1). Rebase and rewrite.
Attachment #700475 - Attachment is obsolete: true
As I have discussed wtih Joe, this feature won't be included in v1.0.1
No longer blocks: 804679
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
Flags: needinfo?(kev)
Attachment #705770 - Attachment is obsolete: true
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 :)
Attachment #707979 - Flags: feedback?(allstars.chh)
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.
Attachment #707979 - Flags: feedback?(allstars.chh)
(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.
Flags: needinfo?(kev)
Separate readEmail and readIAP into another patch.
1). Separate readEmail and readIAP into another patch.
2). Address comment #21
Attachment #707979 - Attachment is obsolete: true
Attachment #700478 - Attachment is obsolete: true
Attachment #707980 - Attachment is obsolete: true
Attachment #708977 - Flags: feedback?(allstars.chh)
Attachment #708978 - Flags: feedback?(allstars.chh)
Attachment #708979 - Flags: feedback?(allstars.chh)
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 :)
Attachment #708980 - Flags: feedback?(allstars.chh)
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.
Attachment #708978 - Flags: feedback?(allstars.chh) → feedback+
Attachment #708977 - Flags: feedback?(allstars.chh) → feedback+
Attachment #708979 - Flags: feedback?(allstars.chh) → feedback+
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();
Attachment #708980 - Flags: feedback?(allstars.chh)
Formal patch
Attachment #708977 - Attachment is obsolete: true
Attached patch Part 2: Read contact email, v5 (obsolete) — Splinter Review
Address comment #30
Attachment #708978 - Attachment is obsolete: true
Formal patch.
Attachment #708979 - Attachment is obsolete: true
Attached patch Part 2: Read contact email, v6 (obsolete) — Splinter Review
Attachment #709355 - Attachment is obsolete: true
Attachment #709354 - Flags: review?(allstars.chh)
Attachment #709546 - Flags: review?(allstars.chh)
Attachment #709356 - Flags: review?(anygregor)
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
Attachment #709354 - Flags: review?(allstars.chh) → review+
Attachment #709356 - Flags: review?(anygregor) → review+
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
Attachment #709546 - Flags: review?(allstars.chh) → review+
Attachment #708980 - Attachment is obsolete: true
Attachment #709588 - Flags: review?(allstars.chh)
Address review comment #37
Attachment #709546 - Attachment is obsolete: true
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.
Attachment #709588 - Flags: review?(allstars.chh) → review-
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 :)
Attachment #709588 - Attachment is obsolete: true
Attachment #710009 - Flags: review?(allstars.chh)
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.
Attachment #710009 - Flags: review?(allstars.chh) → review+
Address review comment #46
Attachment #710009 - Attachment is obsolete: true
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 :)
Attachment #710052 - Attachment is obsolete: true
Attachment #710470 - Flags: review?(allstars.chh)
Attachment #710470 - Flags: review?(allstars.chh) → review+
(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 :-)
Keywords: checkin-needed
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.
Attachment #709356 - Flags: approval-mozilla-b2g18?
mwu, do we need the other parts or just part 3?
echen - can you provide a risk evaluation for uplift to v1.1?
Flags: needinfo?(echen)
(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.
Flags: needinfo?(echen)
blocking-b2g: --- → tef+
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
Attachment #709356 - Flags: approval-mozilla-b2g18?
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?
Blocks: 1157082
You need to log in before you can comment on or make changes to this bug.