Closed
Bug 809725
Opened 12 years ago
Closed 12 years ago
B2G RIL: Read Email in USIM Contact
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
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.
Assignee | ||
Comment 1•12 years ago
|
||
WIP patch
Assignee | ||
Comment 2•12 years ago
|
||
WIP Patch
Assignee | ||
Comment 3•12 years ago
|
||
WIP patch
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → echen
Assignee | ||
Comment 4•12 years ago
|
||
WIP Patch
Assignee | ||
Comment 5•12 years ago
|
||
rebase (based on bug 816893)
Attachment #695386 -
Attachment is obsolete: true
Assignee | ||
Comment 6•12 years ago
|
||
rebase (based on bug 816893)
Attachment #695387 -
Attachment is obsolete: true
Assignee | ||
Comment 7•12 years ago
|
||
rebase (based on bug 816893)
Attachment #695388 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #695556 -
Attachment description: WIP, Part 4: Export Email and ANR to contact, v1 → Part 4: Export Email and ANR to contact, v1
Assignee | ||
Comment 8•12 years ago
|
||
github pull request:
https://github.com/mozilla-b2g/platform_external_qemu/pull/14
Assignee | ||
Comment 9•12 years ago
|
||
github pull request:
https://github.com/mozilla-b2g/platform_hardware_ril/pull/6
Assignee | ||
Comment 10•12 years ago
|
||
Marionette tests
Assignee | ||
Updated•12 years ago
|
Attachment #695936 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #695939 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #695940 -
Attachment is obsolete: true
Assignee | ||
Comment 11•12 years ago
|
||
Marionette tests for USIM contacts will be tracking by bug 824904.
Let's focus on main function here first.
Reporter | ||
Comment 12•12 years ago
|
||
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
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #695717 -
Attachment is obsolete: true
Attachment #695718 -
Attachment is obsolete: true
Attachment #695719 -
Attachment is obsolete: true
Assignee | ||
Comment 14•12 years ago
|
||
Attachment #695556 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #700475 -
Attachment description: Part 1: Read Email in RIL, v1 → WIP, Part 1: Read Email in RIL, v1
Assignee | ||
Comment 15•12 years ago
|
||
1). Rebase and rewrite.
Attachment #700475 -
Attachment is obsolete: true
Reporter | ||
Comment 16•12 years ago
|
||
As I have discussed wtih Joe, this feature won't be included in v1.0.1
No longer blocks: 804679
Comment 17•12 years ago
|
||
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)
Assignee | ||
Comment 18•12 years ago
|
||
Attachment #705770 -
Attachment is obsolete: true
Assignee | ||
Comment 19•12 years ago
|
||
xpcshell tests
Assignee | ||
Comment 20•12 years ago
|
||
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)
Reporter | ||
Comment 21•12 years ago
|
||
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)
Assignee | ||
Comment 22•12 years ago
|
||
(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.
Reporter | ||
Comment 23•12 years ago
|
||
(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•12 years ago
|
||
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)
Assignee | ||
Comment 25•12 years ago
|
||
Separate readEmail and readIAP into another patch.
Assignee | ||
Comment 26•12 years ago
|
||
1). Separate readEmail and readIAP into another patch.
2). Address comment #21
Attachment #707979 -
Attachment is obsolete: true
Assignee | ||
Comment 27•12 years ago
|
||
Attachment #700478 -
Attachment is obsolete: true
Assignee | ||
Comment 28•12 years ago
|
||
Attachment #707980 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #708977 -
Flags: feedback?(allstars.chh)
Assignee | ||
Updated•12 years ago
|
Attachment #708978 -
Flags: feedback?(allstars.chh)
Assignee | ||
Updated•12 years ago
|
Attachment #708979 -
Flags: feedback?(allstars.chh)
Assignee | ||
Comment 29•12 years ago
|
||
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)
Reporter | ||
Comment 30•12 years ago
|
||
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+
Reporter | ||
Updated•12 years ago
|
Attachment #708977 -
Flags: feedback?(allstars.chh) → feedback+
Reporter | ||
Updated•12 years ago
|
Attachment #708979 -
Flags: feedback?(allstars.chh) → feedback+
Reporter | ||
Comment 31•12 years ago
|
||
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)
Assignee | ||
Comment 33•12 years ago
|
||
Address comment #30
Attachment #708978 -
Attachment is obsolete: true
Assignee | ||
Comment 35•12 years ago
|
||
Attachment #709355 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #709354 -
Flags: review?(allstars.chh)
Assignee | ||
Updated•12 years ago
|
Attachment #709546 -
Flags: review?(allstars.chh)
Assignee | ||
Updated•12 years ago
|
Attachment #709356 -
Flags: review?(anygregor)
Reporter | ||
Comment 36•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #709356 -
Flags: review?(anygregor) → review+
Reporter | ||
Comment 37•12 years ago
|
||
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+
Assignee | ||
Comment 38•12 years ago
|
||
Attachment #708980 -
Attachment is obsolete: true
Attachment #709588 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 39•12 years ago
|
||
Address review comment #36
Attachment #709354 -
Attachment is obsolete: true
Assignee | ||
Comment 40•12 years ago
|
||
Address review comment #37
Attachment #709546 -
Attachment is obsolete: true
Assignee | ||
Comment 41•12 years ago
|
||
Reporter | ||
Comment 42•12 years ago
|
||
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-
Reporter | ||
Comment 43•12 years ago
|
||
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.
Assignee | ||
Comment 44•12 years ago
|
||
(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 :)
Assignee | ||
Comment 45•12 years ago
|
||
Attachment #709588 -
Attachment is obsolete: true
Attachment #710009 -
Flags: review?(allstars.chh)
Reporter | ||
Comment 46•12 years ago
|
||
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+
Assignee | ||
Comment 47•12 years ago
|
||
Address review comment #46
Attachment #710009 -
Attachment is obsolete: true
Assignee | ||
Comment 48•12 years ago
|
||
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)
Reporter | ||
Updated•12 years ago
|
Attachment #710470 -
Flags: review?(allstars.chh) → review+
Comment 49•12 years ago
|
||
(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 :-)
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 50•12 years ago
|
||
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
Flags: in-testsuite+
Keywords: checkin-needed
Comment 51•12 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Comment 52•12 years ago
|
||
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?
Comment 53•12 years ago
|
||
mwu, do we need the other parts or just part 3?
Comment 54•12 years ago
|
||
just part 3
Updated•12 years ago
|
Comment 55•12 years ago
|
||
echen - can you provide a risk evaluation for uplift to v1.1?
Flags: needinfo?(echen)
Assignee | ||
Comment 56•12 years ago
|
||
(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)
Updated•12 years ago
|
blocking-b2g: --- → tef+
Comment 57•12 years ago
|
||
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?
Comment 58•12 years ago
|
||
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.
Assignee | ||
Comment 59•12 years ago
|
||
As mention in comment 52 and comment 54, we should only need to uplift part3. :mwu, am I right?
Comment 60•12 years ago
|
||
My bad, thanks.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/c3e876fffc07
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/8887a20b71ae
status-firefox20:
--- → wontfix
status-firefox21:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•