All users were logged out of Bugzilla on October 13th, 2018

B2G RIL: icc.updateContact should return a mozContact with id that indicates physical location on the SIM card.

RESOLVED FIXED in Firefox 28, Firefox OS v1.2

Status

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: allstars.chh, Unassigned)

Tracking

unspecified
1.3 Sprint 6 - 12/6
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking-b2g:koi+, firefox26 wontfix, firefox27 wontfix, firefox28 fixed, b2g-v1.2 fixed, b2g-v1.3 fixed)

Details

Attachments

(4 attachments, 8 obsolete attachments)

5.38 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
2.64 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
5.04 KB, patch
Details | Diff | Splinter Review
2.74 KB, 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.
Created attachment 8339837 [details] [diff] [review]
Part 1: Return mozContact in iccManager.updateContact
Created attachment 8339838 [details] [diff] [review]
Part 2: update test case.
(Assignee)

Updated

5 years ago
Attachment #8339837 - Flags: review?(vyang)
(Assignee)

Updated

5 years ago
Attachment #8339838 - Flags: review?(vyang)
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 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+
Created attachment 8339869 [details] [diff] [review]
Part 1: Return mozContact in iccManager.updateContact v2.
Attachment #8339837 - Attachment is obsolete: true
Attachment #8339869 - Flags: review?(vyang)
Attachment #8339869 - Flags: review?(vyang) → review+
Created attachment 8339870 [details] [diff] [review]
Part 2: update test case.
Attachment #8339838 - Attachment is obsolete: true
Attachment #8339870 - Flags: review+

Updated

5 years ago
Blocks: 922988

Updated

5 years ago
Blocks: 944575

Comment 9

5 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?
https://hg.mozilla.org/mozilla-central/rev/3212475bd908
https://hg.mozilla.org/mozilla-central/rev/e607d1aa8a2a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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
moving to 1.3? as multi SIM is not supported in 1.2
blocking-b2g: koi? → 1.3?
(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?
Hi Yoshi,

Will you open a follow up bug to cover v1.3 scenario (multi SIM)?. Thanks!
Flags: needinfo?(allstars.chh)
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)
blocking+ for bad bug in new 1.2 feature
blocking-b2g: koi? → koi+
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
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.
Created attachment 8343572 [details] [diff] [review]
(V1.2 Branch) Part 1: Return mozContact in iccManager.updateContact.
Flags: needinfo?(allstars.chh)
Created attachment 8343574 [details] [diff] [review]
(V1.2 Branch) Part 2: update test case.
Created attachment 8343578 [details] [diff] [review]
(V1.2 Branch) Part 2: update test case.

just update the author name.
Attachment #8343574 - Attachment is obsolete: true
Created attachment 8344459 [details] [diff] [review]
(V1.2 Branch) Part 1: Return mozContact in iccManager.updateContact. v2.

fix xpcshell test failure.
Attachment #8343572 - Attachment is obsolete: true
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 29

5 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 → ---
(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
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
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.
Can I back out the patches by myself for v1.2 branch?
Or I need RyanVM or any sheriff to do it ?
(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)

Updated

5 years ago
Status: RESOLVED → REOPENED
Flags: needinfo?(allstars.chh)
Resolution: FIXED → ---
Created attachment 8346409 [details] [diff] [review]
(V1.2 Branch) Part 1: Return mozContact in iccManager.updateContact. v3.

Fixed the 'null' problem.
Attachment #8344459 - Attachment is obsolete: true
Created attachment 8346410 [details] [diff] [review]
(V1.2 Branch) Part 2: update test case. v2.

Add more tests to test the 'null' problem.
Attachment #8343578 - Attachment is obsolete: true
Created attachment 8346411 [details] [diff] [review]
(V1.2 Branch) Part 1: Return mozContact in iccManager.updateContact. v3.

Add a=koi+
Attachment #8346409 - Attachment is obsolete: true
Created attachment 8346412 [details] [diff] [review]
(V1.2 Branch) Part 2: update test case. v2.

add a=koi+
Attachment #8346410 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
status-b2g-v1.2: fixed → affected
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

5 years ago
QA Contact: isabelrios
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
Last Resolved: 5 years ago5 years ago
status-b2g-v1.2: affected → fixed
status-b2g-v1.3: --- → fixed
Flags: needinfo?(ryanvm)
Resolution: --- → FIXED
(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?
(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.
(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.
(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

4 years ago
Duplicate of this bug: 964586

Updated

4 years ago
Blocks: 1157082

Updated

4 years ago
See Also: → bug 1161438

Updated

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