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

RESOLVED FIXED in Firefox 42

Status

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bevis, Assigned: bevis)

Tracking

unspecified
FxOS-S4 (07Aug)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(tracking-b2g:backlog, firefox42 fixed)

Details

Attachments

(6 attachments, 13 obsolete attachments)

8.08 KB, patch
bevis
: review+
Details | Diff | Splinter Review
16.83 KB, patch
bevis
: review+
Details | Diff | Splinter Review
9.56 KB, patch
bevis
: review+
Details | Diff | Splinter Review
19.31 KB, patch
edgar
: review+
Details | Diff | Splinter Review
1016 bytes, patch
bevis
: review+
Details | Diff | Splinter Review
33.78 KB, patch
bevis
: review+
Details | Diff | Splinter Review
I'd like to divide bug 864489 into several tasks.

This bug to refactor Icc Contacts with IPDL.
(Assignee)

Comment 1

4 years ago
It shall be MozIcc.webidl instead of MozIccManager.webidl to prevent confusing.
Summary: [B2G][ICC] Refactor Icc Contacts in MozIccManager with IPDL. → [B2G][ICC] Refactor Icc Contacts in MozIcc.webidl with IPDL.

Updated

4 years ago
Blocks: 1157082
[Tracking Requested - why for this release]:
tracking-b2g: --- → backlog
(Assignee)

Comment 3

4 years ago
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
(Assignee)

Updated

3 years ago
Depends on: 1167507
(Assignee)

Updated

3 years ago
Blocks: 935398
(Assignee)

Comment 4

3 years ago
Created attachment 8627544 [details] [diff] [review]
(v1) Patch Part 1: Define new IDL for IccContacts.

Hi Edgar,

May I have your review for IccContact Change?

Thanks!
Attachment #8627544 - Flags: review?(echen)
(Assignee)

Comment 5

3 years ago
Created attachment 8627545 [details] [diff] [review]
(v1) Patch Part 2: IDL implementation in Gonk.

Gonk Implementation.
Attachment #8627545 - Flags: review?(echen)
(Assignee)

Comment 6

3 years ago
Created attachment 8627546 [details] [diff] [review]
(v1) Patch Part 3: Web API change to adopt IccService for IccContacts.
Attachment #8627546 - Flags: review?(htsai)
Attachment #8627546 - Flags: review?(echen)
(Assignee)

Comment 7

3 years ago
Created attachment 8627547 [details] [diff] [review]
(v1) Patch Part 4: IPDL Implementation.

IPDL implementation.
Attachment #8627547 - Flags: review?(echen)
(Assignee)

Comment 8

3 years ago
Created attachment 8627549 [details] [diff] [review]
(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.
Attachment #8627549 - Flags: review?(echen)
(Assignee)

Comment 9

3 years ago
Created attachment 8627551 [details] [diff] [review]
(v1) Patch Part 6: Deprecate RILContentHelper.

Remove RILContentHelper!
Attachment #8627551 - Flags: review?(echen)

Comment 10

3 years ago
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|.
Attachment #8627544 - Flags: review?(echen) → review+
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.
Attachment #8627546 - Flags: review?(htsai) → review+

Comment 13

3 years ago
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)?
Attachment #8627545 - Flags: review?(echen)
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

3 years ago
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.
Attachment #8627546 - Flags: review?(echen) → review+

Comment 16

3 years ago
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?
Attachment #8627547 - Flags: review?(echen)

Comment 17

3 years ago
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!!!
Attachment #8627549 - Flags: review?(echen) → review+
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

3 years ago
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
Attachment #8627551 - Flags: review?(echen) → review+
Created attachment 8634513 [details] [diff] [review]
(v2) Patch Part 1: Define new IDL for IccContacts.r=echen

update patch (v2) to address comment 10.
Attachment #8627544 - Attachment is obsolete: true
Attachment #8634513 - Flags: review+
Created attachment 8634515 [details] [diff] [review]
(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.
Attachment #8627545 - Attachment is obsolete: true
Attachment #8634515 - Flags: review?(echen)
Created attachment 8634516 [details] [diff] [review]
(v2) Patch Part 3: Web API change to adopt IccService for IccContacts. r=echen,hsinyi

rebased for v2.
Attachment #8627546 - Attachment is obsolete: true
Attachment #8634516 - Flags: review+
Created attachment 8634518 [details] [diff] [review]
(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.
Attachment #8627547 - Attachment is obsolete: true
Attachment #8634518 - Flags: review?(echen)
Created attachment 8634519 [details] [diff] [review]
(v2) Patch Part 5: Fix Test Case to Remove Contact with Correct Contact Id. r=echen

rebase for v2.
Attachment #8627549 - Attachment is obsolete: true
Attachment #8634519 - Flags: review+
Created attachment 8634520 [details] [diff] [review]
(v2) Patch Part 6: Deprecate RILContentHelper. r=echen

address comment 16 to deprecate nsRadioInterfaceLayer.h.
Attachment #8627551 - Attachment is obsolete: true
Attachment #8634520 - Flags: review+

Updated

3 years ago
Attachment #8634515 - Flags: review?(echen) → review+

Comment 27

3 years ago
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
Attachment #8634518 - Flags: review?(echen)
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.
Created attachment 8637748 [details] [diff] [review]
(v3) Patch Part 1: Define new IDL for IccContacts.r=echen

rebased for v3.
Attachment #8634513 - Attachment is obsolete: true
Attachment #8637748 - Flags: review+
Created attachment 8637749 [details] [diff] [review]
(v3) Patch Part 2: IDL implementation in Gonk. r=echen

update patch v3 to address comment 27.
Attachment #8634515 - Attachment is obsolete: true
Attachment #8637749 - Flags: review+
Created attachment 8637750 [details] [diff] [review]
(v3) Patch Part 3: Web API change to adopt IccService for IccContacts. r=echen,hsinyi

rebased for v3.
Attachment #8634516 - Attachment is obsolete: true
Attachment #8637750 - Flags: review+
Created attachment 8637751 [details] [diff] [review]
(v3) Patch Part 4: IPDL Implementation. r=echen

update patch v3 to address comment 27.
Attachment #8634518 - Attachment is obsolete: true
Attachment #8637751 - Flags: review?(echen)
Created attachment 8637752 [details] [diff] [review]
(v3) Patch Part 5: Fix Test Case to Remove Contact with Correct Contact Id. r=echen

rebased for v3.
Attachment #8634519 - Attachment is obsolete: true
Attachment #8637752 - Flags: review+
Created attachment 8637753 [details] [diff] [review]
(v3) Patch Part 6: Deprecate RILContentHelper. r=echen

rebased for v3.
Attachment #8634520 - Attachment is obsolete: true
Attachment #8637753 - Flags: review+

Updated

3 years ago
Attachment #8637751 - Flags: review?(echen) → review+
update tryserver result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fc3d37bf3cd

Ready to say goodbye to RILContentHelper. \o/
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Attachment #8637751 - Attachment description: (v3) Patch Part 4: IPDL Implementation. → (v3) Patch Part 4: IPDL Implementation. r=echen
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!
Flags: needinfo?(btseng)
Keywords: checkin-needed
(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!
Flags: needinfo?(btseng)
Created attachment 8639104 [details] [diff] [review]
(v4) Patch Part 6: Deprecate RILContentHelper. r=echen

rebase to resolve conflict in comment 37.
Attachment #8637753 - Attachment is obsolete: true
Attachment #8639104 - Flags: review+
update try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=194cee9aacb8
Keywords: checkin-needed

Updated

3 years ago
Depends on: 1189884

Updated

3 years ago
Depends on: 1195273
Depends on: 1218721
You need to log in before you can comment on or make changes to this bug.