Closed
Bug 943234
Opened 11 years ago
Closed 11 years ago
B2G RIL: icc.updateContact should return a mozContact with id that indicates physical location on the SIM card.
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(blocking-b2g:koi+, firefox26 wontfix, firefox27 wontfix, firefox28 fixed, b2g-v1.2 fixed, b2g-v1.3 fixed)
People
(Reporter: allstars.chh, Assigned: allstars.chh)
References
Details
Attachments
(4 files, 8 obsolete files)
(deleted),
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
allstars.chh
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Currently when we call iccManager.updateContact, the function only returns true/false to indicate the opertion is succeed or not. We should also return the updated mozContact, so Gaia could know the mozContact.id, i.e. the identifier mapped to the physical location on the SIM which the SIM contact is stored.
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8339837 -
Flags: review?(vyang)
Assignee | ||
Updated•11 years ago
|
Attachment #8339838 -
Flags: review?(vyang)
Comment 3•11 years ago
|
||
Comment on attachment 8339837 [details] [diff] [review]
Part 1: Return mozContact in iccManager.updateContact
Review of attachment 8339837 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/system/gonk/ril_worker.js
@@ +900,5 @@
> * @param contact The contact will be updated.
> * @param pin2 PIN2 is required for updating FDN.
> * @param requestId Request id from RadioInterfaceLayer.
> */
> updateICCContact: function updateICCContact(options) {
There is a line inside this function:
let isValidRecordId = contact.recordId > 0 && contact.recordId < 0xff;
and the |contact.recordId| could be undefined.
@@ +902,5 @@
> * @param requestId Request id from RadioInterfaceLayer.
> */
> updateICCContact: function updateICCContact(options) {
> + let onsuccess = function onsuccess(contact) {
> + let pbrIndex = contact.pbrIndex || 0;
|contact.pbrIndex| must have been either defined in |ICCContactHelper::addICCContact| or a few lines later in this same function by:
if (..) {
let recordIndex = contact.contactId.substring(iccid.length);
contact.pbrIndex = Math.floor(recordIndex / ICC_MAX_LINEAR_FIXED_RECORDS);
contact.recordId = recordIndex % ICC_MAX_LINEAR_FIXED_RECORDS;
}
@@ +904,5 @@
> updateICCContact: function updateICCContact(options) {
> + let onsuccess = function onsuccess(contact) {
> + let pbrIndex = contact.pbrIndex || 0;
> + let recordIndex = pbrIndex * ICC_MAX_LINEAR_FIXED_RECORDS + contact.recordId;
> + contact.contactId = this.iccInfo.iccid + recordIndex;;
nit: redundant comma at the end of line
@@ +909,2 @@
> // Reuse 'options' to get 'requestId' and 'contactType'.
> + options.contact = contact;
I think you don't need this line. |options.contact| is always assigned and that |contact| argument here is actually |options.contact|.
I'd really like to eliminate the number of arguments in callbacks. Callback arguments should only be new data produced during the async process. For those already exist before invocation of the async process, please use |callback.bind(...)| instead.
Attachment #8339837 -
Flags: review?(vyang)
Comment 4•11 years ago
|
||
Comment on attachment 8339838 [details] [diff] [review]
Part 2: update test case.
Review of attachment 8339838 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/icc/tests/marionette/test_icc_contact.js
@@ +47,5 @@
>
> updateRequest.onsuccess = function onsuccess() {
> + let updatedContact = updateRequest.result;
> + ok(updatedContact, "updateContact should have retuend a mozContact.");
> + ok(updatedContact.id.startsWith("89014103211118510720"),
nit: please help add a new constant EMULATOR_ICCID for this string and update those in |testReadContacts| as well.
Attachment #8339838 -
Flags: review?(vyang) → review+
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #8339837 -
Attachment is obsolete: true
Attachment #8339869 -
Flags: review?(vyang)
Updated•11 years ago
|
Attachment #8339869 -
Flags: review?(vyang) → review+
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #8339838 -
Attachment is obsolete: true
Attachment #8339870 -
Flags: review+
Assignee | ||
Comment 7•11 years ago
|
||
Try
https://tbpl.mozilla.org/?tree=Try&rev=481bd15cfaa4
I'll push it when it's green.
Assignee | ||
Comment 8•11 years ago
|
||
Comment 9•11 years ago
|
||
having this bug in v1.2 will improve considerably the export sim contact functionality, making some of our partners happier with v1.2 and at the same time leveraging the UX.
blocking-b2g: --- → koi?
Comment 10•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3212475bd908
https://hg.mozilla.org/mozilla-central/rev/e607d1aa8a2a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 11•11 years ago
|
||
Since multi-SIM has been landed, it should be icc, instead of iccManager now.
Summary: B2G RIL: iccManager.updateContact should return the updated mozContact → B2G RIL: icc.updateContact should return the updated mozContact
Comment 12•11 years ago
|
||
moving to 1.3? as multi SIM is not supported in 1.2
blocking-b2g: koi? → 1.3?
Assignee | ||
Comment 13•11 years ago
|
||
(In reply to Preeti Raghunath(:Preeti) from comment #12)
> moving to 1.3? as multi SIM is not supported in 1.2
This bug is not multi-SIM.
This patch is for single-SIM.
blocking-b2g: 1.3? → koi?
Comment 14•11 years ago
|
||
Hi Yoshi,
Will you open a follow up bug to cover v1.3 scenario (multi SIM)?. Thanks!
Flags: needinfo?(allstars.chh)
Assignee | ||
Comment 15•11 years ago
|
||
Sorry I don't know what are you talking about.
This bug is to return mozContact in updateContact(), has nothing to do with multi-SIM.
Flags: needinfo?(allstars.chh)
Comment 17•11 years ago
|
||
Please attach a branch-specific patch for b2g26 uplift.
status-b2g-v1.2:
--- → affected
status-firefox26:
--- → wontfix
status-firefox27:
--- → wontfix
status-firefox28:
--- → fixed
Flags: needinfo?(allstars.chh)
Keywords: branch-patch-needed
Target Milestone: --- → 1.3 Sprint 6 - 12/6
Assignee | ||
Comment 18•11 years ago
|
||
I'll upload a patch for 1.2 soon,
but I would try to run more tests locally as mozContact isn't WebIDLized back in 1.2.
Assignee | ||
Comment 19•11 years ago
|
||
Flags: needinfo?(allstars.chh)
Assignee | ||
Comment 20•11 years ago
|
||
Assignee | ||
Comment 21•11 years ago
|
||
just update the author name.
Attachment #8343574 -
Attachment is obsolete: true
Assignee | ||
Comment 22•11 years ago
|
||
Comment 23•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/0fec983a056b
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/96b4b6557736
Keywords: branch-patch-needed
Comment 24•11 years ago
|
||
Assignee | ||
Comment 25•11 years ago
|
||
fix xpcshell test failure.
Attachment #8343572 -
Attachment is obsolete: true
Assignee | ||
Comment 26•11 years ago
|
||
Assignee | ||
Comment 27•11 years ago
|
||
Hi, Ryan
Sorry for the failure (Comment 24) before,
I was waiting for the try-result and was going to inform you when it's green.
But you pushed the code before I found out the xpcshell failure. :P
The patches are ready for v1.2 branch now,
And passed the try (Comment 26)
Can you kindly help to push them again?
Thank you.
Flags: needinfo?(ryanvm)
Comment 28•11 years ago
|
||
Doh :P
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/eac3810f119c
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/b28014c2f203
Flags: needinfo?(ryanvm)
Comment 29•11 years ago
|
||
Hi Yoshi,
It seems the patch for v1.2 is incorrect. when updating a contact
var request = icc.updateContact('adn', theContact);
request.onsuccess = function onsuccess() {
request.result.id /* it is returning the id and the 'null' string concatenated i.e. 8934071100275319295null */
}
Please could you check ?
Status: RESOLVED → REOPENED
Flags: needinfo?(allstars.chh)
Resolution: FIXED → ---
Comment 30•11 years ago
|
||
(In reply to Jose M. Cantera from comment #29)
> Hi Yoshi,
>
> It seems the patch for v1.2 is incorrect. when updating a contact
>
> var request = icc.updateContact('adn', theContact);
> request.onsuccess = function onsuccess() {
> request.result.id /* it is returning the id and the 'null' string
> concatenated i.e. 8934071100275319295null */
>
> }
>
> Please could you check ?
Yoshi - Can you back this out & reland with the fix here?
Technically this actually is still closed, as this has landed. If this ends up being backed out on 1.2, then we should set 1.2 to affected on the backout. If we want to take this instead in a followup, then we should open a new bug to resolve the regression.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Comment 31•11 years ago
|
||
General FYI - bz workflow is driven by code landings to ensure tree management understands where patches have landed. If we want to reopen a bug, then we need to backout.
Assignee | ||
Comment 32•11 years ago
|
||
Can I back out the patches by myself for v1.2 branch?
Or I need RyanVM or any sheriff to do it ?
Comment 33•11 years ago
|
||
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #32)
> Can I back out the patches by myself for v1.2 branch?
> Or I need RyanVM or any sheriff to do it ?
Yup, you can back these patches out yourself.
Assignee | ||
Comment 34•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Status: RESOLVED → REOPENED
Flags: needinfo?(allstars.chh)
Resolution: FIXED → ---
Assignee | ||
Comment 35•11 years ago
|
||
Fixed the 'null' problem.
Attachment #8344459 -
Attachment is obsolete: true
Assignee | ||
Comment 36•11 years ago
|
||
Add more tests to test the 'null' problem.
Attachment #8343578 -
Attachment is obsolete: true
Assignee | ||
Comment 39•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 40•11 years ago
|
||
Assignee | ||
Comment 41•11 years ago
|
||
Ryan, sorry for bothering you again.
Can you help to check if the backout and re-push are correct ?
If they are okay,
I leave the state 'REOPENED' and the flag 'status-b2g-v1.2' affected, and leave those to you to handle them.
Thanks
Flags: needinfo?(ryanvm)
Updated•11 years ago
|
QA Contact: isabelrios
Comment 42•11 years ago
|
||
1.) Typically the bug resolution (RESOLVED) tracks landing on *m-c* (unless a patch never landed there), so this shouldn't have been reopened in the first place unless it was backed out from there as well.
2.) Tracking flags track branch uplifts, so setting status-b2g-v1.2 back to affected after backing out is sufficient.
3.) I'll trust that you pushed what you intended to push to v1.2. I can't really judge that. Tests are passing, so that's good :). I'm wondering if instead of backing and out relanding if a small follow-up patch would have been better though?
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
status-b2g-v1.3:
--- → fixed
Flags: needinfo?(ryanvm)
Resolution: --- → FIXED
Comment 43•11 years ago
|
||
(In reply to Jose M. Cantera from comment #29)
> Please could you check ?
Seems we have a gap in test coverage here? Was a test for this regression added when re-landed?
Flags: in-testsuite?
Assignee | ||
Comment 44•11 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #42)
> I'm wondering if
> instead of backing and out relanding if a small follow-up patch would have
> been better though?
Good idea, in the future, maybe I'll try to do a follow-up patch.
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #43)
> (In reply to Jose M. Cantera from comment #29)
> > Please could you check ?
>
> Seems we have a gap in test coverage here? Was a test for this regression
> added when re-landed?
Yes, for the patch landed in v1.2(v2), it has covered what Jose M. found in the first place.
I'll file another bug for adding the test coverage on m-c, too.
Comment 45•11 years ago
|
||
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #44)
> Yes, for the patch landed in v1.2(v2), it has covered what Jose M. found in
> the first place.
It wasn't causing any test failures on TBPL, though. That's what concerns me.
Assignee | ||
Comment 46•11 years ago
|
||
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #44)
> I'll file another bug for adding the test coverage on m-c, too.
Bug 949849.
Updated•10 years ago
|
Updated•10 years ago
|
Summary: B2G RIL: icc.updateContact should return the updated mozContact → B2G RIL: icc.updateContact should return a mozContact with id that indicates physical location on the SIM card.
You need to log in
before you can comment on or make changes to this bug.
Description
•