Closed Bug 943234 Opened 11 years ago Closed 10 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)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

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

RESOLVED FIXED
1.3 Sprint 6 - 12/6
blocking-b2g koi+
Tracking Status
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)

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.
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+
Attachment #8339869 - Flags: review?(vyang) → review+
Blocks: 922988
Blocks: 944575
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?
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.
Flags: needinfo?(allstars.chh)
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.
just update the author name.
Attachment #8343574 - 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)
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
Closed: 11 years ago10 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.
Status: RESOLVED → REOPENED
Flags: needinfo?(allstars.chh)
Resolution: FIXED → ---
Add more tests to test the 'null' problem.
Attachment #8343578 - Attachment is obsolete: true
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)
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
Closed: 10 years ago10 years ago
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.
Blocks: 1157082
See Also: → 1161438
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.

Attachment

General

Created:
Updated:
Size: