I'd like to divide bug 864489 into several tasks. This bug to refactor Icc Contacts with IPDL.

It shall be MozIcc.webidl instead of MozIccManager.webidl to prevent confusing.

[Tracking Requested - why for this release]:

Per offlined discussion, from web API perspective, we unify the input/output of |readContact| and |updateContact| as |mozContact| instances instead of |any| [1]. For |readContact|, we already return the results formed in |mozContact| [2]. In this bug, we have to replace the type of |contact| in |updateContact| from any to |mozContact|. In additon, A new Gaia bug is also required due to this API change. [1] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/MozIcc.webidl#336 [2] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RILContentHelper.js#413

Hi Edgar, May I have your review for IccContact Change? Thanks!

Gonk Implementation.

IPDL implementation.

The "contact.id" from Icc.readContacts() already contains iccId + recordIndex. There is no need to concatenate this info again for removing it.

Remove RILContentHelper!

Comment on attachment 8627544 [details] [diff] [review] (v1) Patch Part 1: Define new IDL for IccContacts. Review of attachment 8627544 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, r=me with comment addressed. ::: dom/icc/interfaces/nsIIccService.idl @@ +69,2 @@ > * The error callback of |getCardLockEnabled|, |getCardLockRetryCount|, > * |matchMvno|, and |getServiceStateEnabled|. Add |readContacts| and |updateContact|.

Comment on attachment 8627544 [details] [diff] [review] (v1) Patch Part 1: Define new IDL for IccContacts. Review of attachment 8627544 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/interfaces/nsIIccService.idl @@ +69,2 @@ > * The error callback of |getCardLockEnabled|, |getCardLockRetryCount|, > * |matchMvno|, and |getServiceStateEnabled|. Thanks for reminding this!

Comment on attachment 8627546 [details] [diff] [review] (v1) Patch Part 3: Web API change to adopt IccService for IccContacts. Review of attachment 8627546 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, thank you.

Comment on attachment 8627545 [details] [diff] [review] (v1) Patch Part 2: IDL implementation in Gonk. Review of attachment 8627545 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/IccCallback.cpp @@ +75,5 @@ > + > + ErrorResult er; > + nsRefPtr<mozContact> contact > + = mozContact::Constructor(aGlobal, aCx, properties, er); > + NS_ENSURE_SUCCESS(er.StealNSResult(), NS_ERROR_FAILURE); NS_ENSURE_FALSE(er.Failed(), er.StealNSResult()) maybe? Here and elsewhere. ::: dom/icc/IccContact.cpp @@ +87,5 @@ > + aId = mId; > + return NS_OK; > +} > + > +NS_IMETHODIMP IccContact::GetNames(uint32_t* aCount, char16_t*** aNames) Add NS_ENSURE_ARG_POINTER checks for aCount and aNames and same in other getters. @@ +106,5 @@ > + (*aNames)[i] = ToNewUnicode(mNames[i]); > + if (!(*aNames)[i]) { > + NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(i, *aNames); > + *aCount = 0; > + *aNames = nullptr; I prefer having a local variable to cache the returned array, and assign it to |*aNames| only when it can be allocated successfully. And what about handling |*Count| in a similar way (assign length to |*Count| only when the returned array is allocated successfully)?

Comment on attachment 8627545 [details] [diff] [review] (v1) Patch Part 2: IDL implementation in Gonk. Review of attachment 8627545 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/IccCallback.cpp @@ +75,5 @@ > + > + ErrorResult er; > + nsRefPtr<mozContact> contact > + = mozContact::Constructor(aGlobal, aCx, properties, er); > + NS_ENSURE_SUCCESS(er.StealNSResult(), NS_ERROR_FAILURE); Will do! ::: dom/icc/IccContact.cpp @@ +87,5 @@ > + aId = mId; > + return NS_OK; > +} > + > +NS_IMETHODIMP IccContact::GetNames(uint32_t* aCount, char16_t*** aNames) Will do! @@ +106,5 @@ > + (*aNames)[i] = ToNewUnicode(mNames[i]); > + if (!(*aNames)[i]) { > + NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(i, *aNames); > + *aCount = 0; > + *aNames = nullptr; I see your point. I'll update the patch to address this. :)

Comment on attachment 8627546 [details] [diff] [review] (v1) Patch Part 3: Web API change to adopt IccService for IccContacts. Review of attachment 8627546 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thank you.

Comment on attachment 8627547 [details] [diff] [review] (v1) Patch Part 4: IPDL Implementation. Review of attachment 8627547 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/ipc/IccChild.cpp @@ +382,5 @@ > + > + // Id > + IccContactData contactData; > + nsresult rv = aContact->GetId(contactData.id()); > + NS_ENSURE_SUCCESS(rv, rv); Could we reuse the GetIccContactDataFromIccContact? @@ +482,5 @@ > + > + nsresult rv; > + nsCOMPtr<nsIIccContact> contact; > + nsIIccContact** contacts > + = (nsIIccContact**) moz_xmalloc(sizeof(nsIIccContact*) * count); What about using |nsTArray<nsCOMPtr<nsIIccContact>>|, let nsCOMPtr handle the memory for you?

Comment on attachment 8627549 [details] [diff] [review] (v1) Patch Part 5: Fix Test Case to Remove Contact with Correct Contact Id. Review of attachment 8627549 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/tests/marionette/test_icc_contact_add.js @@ +71,5 @@ > log("removeContact: contactId=" + aContactId + > ", type=" + aType + ", pin2=" + aPin2); > > let contact = new mozContact({}); > + contact.id = aContactId; Nice catch!!!

Comment on attachment 8627547 [details] [diff] [review] (v1) Patch Part 4: IPDL Implementation. Review of attachment 8627547 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/ipc/IccChild.cpp @@ +382,5 @@ > + > + // Id > + IccContactData contactData; > + nsresult rv = aContact->GetId(contactData.id()); > + NS_ENSURE_SUCCESS(rv, rv); Sure! I'll define a utility class for this. :) @@ +482,5 @@ > + > + nsresult rv; > + nsCOMPtr<nsIIccContact> contact; > + nsIIccContact** contacts > + = (nsIIccContact**) moz_xmalloc(sizeof(nsIIccContact*) * count); Sound better. I'll address this in next update.

Comment on attachment 8627551 [details] [diff] [review] (v1) Patch Part 6: Deprecate RILContentHelper. Review of attachment 8627551 [details] [diff] [review]: ----------------------------------------------------------------- Good to see RILContentHelper is totally removed. \o/ BTW, could you also remove nsRadioInterfaceLayer.h [1] and put NS_RADIOINTERFACELAYER_{CID|CONTRACTID} into nsIRadioInterfaceLayer.idl [2]? Thank you. [1] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsRadioInterfaceLayer.h [2] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsIRadioInterfaceLayer.idl

update patch (v2) to address comment 10.

update patch (v2) to address comment 13: 1. NS_ENSURE_FALSE(er.Failed(), er.StealNSResult()) 2. NS_ENSURE_ARG_POINTER 3. Have local variable to build the array to be returned.

rebased for v2.

Address comment 16: 1. Define a Utility class for the data conversion between IPC data and nsIIcc related objects. 2. Adopt nsCOMArray for better memory management.

rebase for v2.

address comment 16 to deprecate nsRadioInterfaceLayer.h.

Update try server result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b71d51a27cea

Comment on attachment 8634518 [details] [diff] [review] (v2) Patch Part 4: IPDL Implementation. Review of attachment 8634518 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/ipc/IccChild.cpp @@ +439,5 @@ > + > + uint32_t count = data.Length(); > + if (count == 0) { > + return NS_SUCCEEDED( > + mRequestReply->NotifyRetrievedIccContacts(nullptr, 0)); I just found there is an API behaviour change: Previously API notify empty array as result [1] if there is no contact stored in sim card, but in this patch we changes to notify `nullptr` as result. This might break gaia app and since this bug is to refactor only, we should keep the API behaviour unchanged. Thank you. [1] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RILContentHelper.js?from=RILContentHelper.js#291

Comment on attachment 8634518 [details] [diff] [review] (v2) Patch Part 4: IPDL Implementation. Review of attachment 8634518 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/ipc/IccChild.cpp @@ +439,5 @@ > + > + uint32_t count = data.Length(); > + if (count == 0) { > + return NS_SUCCEEDED( > + mRequestReply->NotifyRetrievedIccContacts(nullptr, 0)); Thanks for identifying this. I'll update the patch again to address this issue.

rebased for v3.

update patch v3 to address comment 27.

rebased for v3.

update patch v3 to address comment 27.

rebased for v3.

rebased for v3.

update tryserver result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fc3d37bf3cd Ready to say goodbye to RILContentHelper. \o/

seems part6 has problems to apply: renamed 1114937 -> 1114937_part6.patch applying 1114937_part6.patch patching file dom/system/gonk/RadioInterfaceLayer.js Hunk #1 FAILED at 62 1 out of 5 hunks FAILED -- saving rejects to file dom/system/gonk/RadioInterfaceLayer.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and refresh 1114937_part6.patch could you take a look, thanks!

(In reply to Carsten Book [:Tomcat] from comment #37) > seems part6 has problems to apply: renamed 1114937 -> 1114937_part6.patch > applying 1114937_part6.patch > patching file dom/system/gonk/RadioInterfaceLayer.js > Hunk #1 FAILED at 62 > 1 out of 5 hunks FAILED -- saving rejects to file > dom/system/gonk/RadioInterfaceLayer.js.rej > patch failed, unable to continue (try -v) > patch failed, rejects left in working directory > errors during apply, please fix and refresh 1114937_part6.patch > > could you take a look, thanks! I'll look into this. Thanks!

rebase to resolve conflict in comment 37.

update try server result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=194cee9aacb8

https://hg.mozilla.org/integration/b2g-inbound/rev/44935ff56b63 https://hg.mozilla.org/integration/b2g-inbound/rev/0c3aa0444339 https://hg.mozilla.org/integration/b2g-inbound/rev/244594f4817c https://hg.mozilla.org/integration/b2g-inbound/rev/95df4f99f500 https://hg.mozilla.org/integration/b2g-inbound/rev/d25074fc4e1f https://hg.mozilla.org/integration/b2g-inbound/rev/69d0fee7d878

https://hg.mozilla.org/mozilla-central/rev/44935ff56b63 https://hg.mozilla.org/mozilla-central/rev/0c3aa0444339 https://hg.mozilla.org/mozilla-central/rev/244594f4817c https://hg.mozilla.org/mozilla-central/rev/95df4f99f500 https://hg.mozilla.org/mozilla-central/rev/d25074fc4e1f https://hg.mozilla.org/mozilla-central/rev/69d0fee7d878