1114937 - [B2G][ICC] Refactor Icc Contacts in MozIcc.webidl with IPDL.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

I'd like to divide bug 864489 into several tasks.

This bug to refactor Icc Contacts with IPDL.

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 2

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

[Tracking Requested - why for this release]:

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Patch Part 1: Define new IDL for IccContacts.

Hi Edgar,

May I have your review for IccContact Change?

Thanks!

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Patch Part 2: IDL implementation in Gonk.

Gonk Implementation.

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Patch Part 3: Web API change to adopt IccService for IccContacts.

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Patch Part 4: IPDL Implementation.

IPDL implementation.

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Patch Part 5: Fix Test Case to Remove Contact with Correct Contact Id.

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

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Patch Part 6: Deprecate RILContentHelper.

Remove RILContentHelper!

Comment 10

[Edgar Chen [:edgar]]

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 11

[Bevis Tseng[:bevis][:btseng](Exited)]

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 12

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

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 13

[Edgar Chen [:edgar]]

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 14

[Bevis Tseng[:bevis][:btseng](Exited)]

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 15

[Edgar Chen [:edgar]]

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 16

[Edgar Chen [:edgar]]

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 17

[Edgar Chen [:edgar]]

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 18

[Bevis Tseng[:bevis][:btseng](Exited)]

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 19

[Edgar Chen [:edgar]]

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

Comment 20

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Patch Part 1: Define new IDL for IccContacts.r=echen

update patch (v2) to address comment 10.

Comment 21

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Patch Part 2: IDL implementation in Gonk.

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.

Comment 22

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Patch Part 3: Web API change to adopt IccService for IccContacts. r=echen,hsinyi

rebased for v2.

Comment 23

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Patch Part 4: IPDL Implementation.

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.

Comment 24

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Patch Part 5: Fix Test Case to Remove Contact with Correct Contact Id. r=echen

rebase for v2.

Comment 25

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Patch Part 6: Deprecate RILContentHelper. r=echen

address comment 16 to deprecate nsRadioInterfaceLayer.h.

Comment 26

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 27

[Edgar Chen [:edgar]]

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 28

[Bevis Tseng[:bevis][:btseng](Exited)]

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.

Comment 29

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v3) Patch Part 1: Define new IDL for IccContacts.r=echen

rebased for v3.

Comment 30

[Bevis Tseng[:bevis][:btseng](Exited)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=de9b1989b341

Comment 31

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v3) Patch Part 2: IDL implementation in Gonk. r=echen

update patch v3 to address comment 27.

Comment 32

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v3) Patch Part 3: Web API change to adopt IccService for IccContacts. r=echen,hsinyi

rebased for v3.

Comment 33

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v3) Patch Part 4: IPDL Implementation. r=echen

update patch v3 to address comment 27.

Comment 34

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v3) Patch Part 5: Fix Test Case to Remove Contact with Correct Contact Id. r=echen

rebased for v3.

Comment 35

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v3) Patch Part 6: Deprecate RILContentHelper. r=echen

rebased for v3.

Comment 36

[Bevis Tseng[:bevis][:btseng](Exited)]

update tryserver result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fc3d37bf3cd

Ready to say goodbye to RILContentHelper. \o/

Comment 37

[Carsten Book [:Tomcat]]

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!

Comment 38

[Bevis Tseng[:bevis][:btseng](Exited)]

(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!

Comment 39

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v4) Patch Part 6: Deprecate RILContentHelper. r=echen

rebase to resolve conflict in comment 37.

Comment 40

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 41

[Pulsebot]

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

Comment 42

[Ryan VanderMeulen [:RyanVM]]

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