Closed Bug 847820 Opened 7 years ago Closed 7 years ago

B2G RIL: Support exporting contacts to SIM card in IccManager

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: edgar, Assigned: edgar)

References

Details

Attachments

(4 files, 14 obsolete files)

1.15 KB, patch
edgar
: review+
Details | Diff | Splinter Review
3.98 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
4.41 KB, patch
edgar
: review+
Details | Diff | Splinter Review
11.75 KB, patch
edgar
: review+
Details | Diff | Splinter Review
Support exporting contacts to SIM card in IccManager.
(Please see the discussion in bug 838092 for more detailed information)
Blocks: 831663
Depends on: 809726
What I am going to do as below:

1. Add new API, updateContact(), in MozIccManagaer.
    - DOMRequest updateContact(DOMString contactType, nsIDOMContact contact, DOMString pin2);
2. Check permission "icc-write" in updateContact().
3. Remove updateICCContact() from nsIRadioInterfaceLayer.
4. Add new IPC message, RIL:IccUpdateContact, in RadioInterfaceLayer and RILContentHelper.

Thanks
Add new API in MozIccManagaer.
- DOMRequest updateContact(DOMString contactType, nsIDOMContact contact, DOMString pin2);
IccManager changes
1. Remove updateICCContact() from nsIRadioInterfaceLayer.
2. Add new IPC message, RIL:IccUpdateContact, in RadioInterfaceLayer and RILContentHelper.
Check permission "icc-write"
Marionette tests
Oh, forgot to remove the permission, icc-write, at the end of the tests.
Attachment #722011 - Attachment is obsolete: true
Duplicate of this bug: 834137
Depends on: 833174, 836644
Attachment #721990 - Flags: feedback?(allstars.chh)
Attachment #721997 - Flags: feedback?(allstars.chh)
Attachment #722008 - Flags: feedback?(allstars.chh)
Attachment #722010 - Flags: feedback?(allstars.chh)
Attachment #722014 - Flags: feedback?(allstars.chh)
Comment on attachment 721990 [details] [diff] [review]
WIP, Part 1: IDL for exporting contact to SIM, v1

Review of attachment 721990 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/interfaces/nsIDOMIccManager.idl
@@ +8,4 @@
>  
>  interface nsIDOMEventListener;
>  interface nsIDOMDOMRequest;
> +interface nsIDOMContact;

What are nsIDOMContactManager and nsIDOMContact for?

@@ +281,5 @@
> +   *
> +   * @param contactType
> +   *        One of type as below,
> +   *        - 'ADN': Abbreviated Dialling Number
> +   *        - 'FDN': Fixed Dialling Number

Oh, I plan to use lower case, 'adn' 'fdn'
as I found out Web uses lower cases mostly.
Attachment #721990 - Flags: feedback?(allstars.chh) → feedback+
Attachment #721997 - Flags: feedback?(allstars.chh) → feedback+
Comment on attachment 722008 [details] [diff] [review]
WIP, Part 3: RIL implementation, v1

Review of attachment 722008 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/RILContentHelper.js
@@ +1176,5 @@
>      }
>    },
>  
> +  handleIccUpdateContact: function handleIccUpdateContact(message) {
> +    if (message.error) {

error? I thought it's errorMsg ?
Attachment #722008 - Flags: feedback?(allstars.chh) → feedback+
Comment on attachment 722014 [details] [diff] [review]
WIP, Part 5: Marionette tests for updating SIM contact, v2

Review of attachment 722014 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/tests/marionette/test_icc_contact.js
@@ +25,5 @@
> +    name: "add",
> +    tel: [{value: "0912345678"}]
> +  });
> +
> +  let updateRequest = icc.updateContact("ADN", contact, "");

Don't have to add "" for pin2.
let it be undefined.
Attachment #722014 - Flags: feedback?(allstars.chh) → feedback+
Comment on attachment 722010 [details] [diff] [review]
WIP, Part 4: Add icc-write permission check, v1

I don't know permission well, maybe Gene can help.
Attachment #722010 - Flags: feedback?(allstars.chh) → feedback?(gene.lian)
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #10)
> Comment on attachment 722008 [details] [diff] [review]
> WIP, Part 3: RIL implementation, v1
> 
> Review of attachment 722008 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +1176,5 @@
> >      }
> >    },
> >  
> > +  handleIccUpdateContact: function handleIccUpdateContact(message) {
> > +    if (message.error) {
> 
> error? I thought it's errorMsg ?

Oh, I will check this again. Thanks.
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #9)
> Comment on attachment 721990 [details] [diff] [review]
> WIP, Part 1: IDL for exporting contact to SIM, v1
> 
> Review of attachment 721990 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/icc/interfaces/nsIDOMIccManager.idl
> @@ +8,4 @@
> >  
> >  interface nsIDOMEventListener;
> >  interface nsIDOMDOMRequest;
> > +interface nsIDOMContact;
> 
> What are nsIDOMContactManager and nsIDOMContact for?

Oh, |inteface nsIDOMContact;| is redundant. (Having |#include "nsIDOMContactManager.idl"| seems enough)
The nsIDOMContact is defined in nsIDOMContactManger.idl, so we need to include the this idl. 

> 
> @@ +281,5 @@
> > +   *
> > +   * @param contactType
> > +   *        One of type as below,
> > +   *        - 'ADN': Abbreviated Dialling Number
> > +   *        - 'FDN': Fixed Dialling Number
> 
> Oh, I plan to use lower case, 'adn' 'fdn'
> as I found out Web uses lower cases mostly.

Ok, I will change to lower case.
(In reply to Edgar Chen [:edgar][:echen] from comment #13)
> (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #10)
> > Comment on attachment 722008 [details] [diff] [review]
> > WIP, Part 3: RIL implementation, v1
> > 
> > Review of attachment 722008 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/RILContentHelper.js
> > @@ +1176,5 @@
> > >      }
> > >    },
> > >  
> > > +  handleIccUpdateContact: function handleIccUpdateContact(message) {
> > > +    if (message.error) {
> > 
> > error? I thought it's errorMsg ?
> 
> Oh, I will check this again. Thanks.

Yes, you are right, it should be *errorMsg*.

And I found some error messages are using "error" and some are using "errorMsg".
Maybe we should have a consistent naming. I will file another bug for this.
Thanks.
Comment on attachment 722010 [details] [diff] [review]
WIP, Part 4: Add icc-write permission check, v1

We should file another bug for permission and discuss this with Jonas, Mounir, ...etc.
Attachment #722010 - Flags: feedback?(gene.lian)
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #11)
> Comment on attachment 722014 [details] [diff] [review]
> WIP, Part 5: Marionette tests for updating SIM contact, v2
> 
> Review of attachment 722014 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/icc/tests/marionette/test_icc_contact.js
> @@ +25,5 @@
> > +    name: "add",
> > +    tel: [{value: "0912345678"}]
> > +  });
> > +
> > +  let updateRequest = icc.updateContact("ADN", contact, "");
> 
> Don't have to add "" for pin2.
> let it be undefined.

I just tried. We can not remove pin2, otherwise we will get "JavascriptException: NS_ERROR_XPC_NOT_ENOUGH_ARGS" exception. :(
Comment on attachment 721990 [details] [diff] [review]
WIP, Part 1: IDL for exporting contact to SIM, v1

Review of attachment 721990 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/interfaces/nsIDOMIccManager.idl
@@ +289,5 @@
> +   *        PIN2 is only required for 'FDN'.
> +   */
> +  nsIDOMDOMRequest updateContact(in DOMString contactType,
> +                                 in nsIDOMContact contact,
> +                                 in DOMString pin2);

Can we make pin2 optional?
1). Use lower case for contactType. ('fdn' and 'adn').
2). Make pin2 as opional argument.
Attachment #721990 - Attachment is obsolete: true
Sorry, I uploaded a wrong version, correct it.
Attachment #722111 - Attachment is obsolete: true
Attachment #721997 - Attachment description: WIP, Part 2: Modify IccManager for exporting contact to SIM, v1 → Part 2: Modify IccManager for exporting contact to SIM, v1
Attached patch Part 3: RIL implementation, v2 (obsolete) — Splinter Review
1). s/error/errorMsg.
2). Pass pin2 to RadioInterfaceLayer.
Attachment #722008 - Attachment is obsolete: true
Comment on attachment 722010 [details] [diff] [review]
WIP, Part 4: Add icc-write permission check, v1

We will discuss permission in bug 848684.
Attachment #722010 - Attachment is obsolete: true
1). Remove icc permission.
2). Address comment #11.
Attachment #722014 - Attachment is obsolete: true
Attachment #722119 - Flags: superreview?(jonas)
Attachment #722119 - Flags: review?(allstars.chh)
Attachment #721997 - Flags: review?(bugs)
Attachment #722122 - Flags: review?(allstars.chh)
Attachment #722124 - Flags: review?(allstars.chh)
No need to include nsIDOMContactManager.idl, just use "interface nsIDOMContact".
Attachment #722119 - Attachment is obsolete: true
Attachment #722119 - Flags: superreview?(jonas)
Attachment #722119 - Flags: review?(allstars.chh)
Attachment #722147 - Flags: superreview?(jonas)
Attachment #722147 - Flags: review?(allstars.chh)
Comment on attachment 722147 [details] [diff] [review]
Part 1: IDL for exporting contact to SIM, v4

Review of attachment 722147 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/interfaces/nsIDOMIccManager.idl
@@ +6,5 @@
>  #include "SimToolKit.idl"
>  
>  interface nsIDOMEventListener;
>  interface nsIDOMDOMRequest;
> +interface nsIDOMContact;

Can you help to list it in alphabetical order?

::: dom/icc/interfaces/nsIIccProvider.idl
@@ +5,5 @@
>  #include "nsISupports.idl"
>  
>  interface nsIDOMWindow;
>  interface nsIDOMDOMRequest;
> +interface nsIDOMContact;

ditto
Attachment #722147 - Flags: review?(allstars.chh) → review+
Comment on attachment 722122 [details] [diff] [review]
Part 3: RIL implementation, v2

Review of attachment 722122 [details] [diff] [review]:
-----------------------------------------------------------------

Move those functions into proper position.

::: dom/system/gonk/RILContentHelper.js
@@ +78,5 @@
>    "RIL:SetCallForwardingOption",
>    "RIL:GetCallForwardingOption",
>    "RIL:CellBroadcastReceived",
>    "RIL:CfStateChanged",
> +  "RIL:IccUpdateContact",

insert this message last

@@ +1080,5 @@
>          this._deliverEvent("_iccListeners", "notifyStkSessionEnd", null);
>          break;
> +      case "RIL:IccUpdateContact":
> +        this.handleIccUpdateContact(msg.json);
> +        break;

move to after IccExchangeAPDU

@@ +1176,5 @@
>        this.fireRequestSuccess(message.requestId, null);
>      }
>    },
>  
> +  handleIccUpdateContact: function handleIccUpdateContact(message) {

ditto
Attachment #722122 - Flags: review?(allstars.chh) → review+
Comment on attachment 722122 [details] [diff] [review]
Part 3: RIL implementation, v2

Review of attachment 722122 [details] [diff] [review]:
-----------------------------------------------------------------

Move those functions into proper position.

::: dom/system/gonk/RILContentHelper.js
@@ +78,5 @@
>    "RIL:SetCallForwardingOption",
>    "RIL:GetCallForwardingOption",
>    "RIL:CellBroadcastReceived",
>    "RIL:CfStateChanged",
> +  "RIL:IccUpdateContact",

insert this message last

@@ +1080,5 @@
>          this._deliverEvent("_iccListeners", "notifyStkSessionEnd", null);
>          break;
> +      case "RIL:IccUpdateContact":
> +        this.handleIccUpdateContact(msg.json);
> +        break;

move to after IccExchangeAPDU

@@ +1176,5 @@
>        this.fireRequestSuccess(message.requestId, null);
>      }
>    },
>  
> +  handleIccUpdateContact: function handleIccUpdateContact(message) {

ditto

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +95,5 @@
>    "RIL:SendStkResponse",
>    "RIL:SendStkMenuSelection",
>    "RIL:SendStkTimerExpiration",
>    "RIL:SendStkEventDownload",
> +  "RIL:IccUpdateContact",

Same with RILContentHelper, and below.
Comment on attachment 722124 [details] [diff] [review]
Part 4: Marionette tests for updating SIM contact, v3

Review of attachment 722124 [details] [diff] [review]:
-----------------------------------------------------------------

Update bug title, shoud be Part 4.
Attachment #722124 - Flags: review?(allstars.chh) → review+
1). Address review comment #27.
2). Add r=allstars.chh after r+.
Attachment #722122 - Attachment is obsolete: true
Attachment #722252 - Flags: review+
1). Address review comment #28.
2). Add r=allstars.chh after r+.
Attachment #722124 - Attachment is obsolete: true
Attachment #722256 - Flags: review+
Attachment #721997 - Flags: review?(bugs) → review+
Add r=bugs after r+.
Attachment #721997 - Attachment is obsolete: true
Attachment #722609 - Flags: review+
Oh, Part 4 makes "test_sim_contacts.js" failed. :(

https://tbpl.mozilla.org/?tree=Try&rev=8c5717f89412

Maybe we should disable test_icc_contact.js first.
In bug 847741, we could move reading test to test_icc_contact.js and enable it.

Thanks
Disable test first. (comment #32)
Attachment #722256 - Attachment is obsolete: true
Attachment #722650 - Flags: review?(allstars.chh)
Comment on attachment 722650 [details] [diff] [review]
Part 4: Marionette tests for updating SIM contact, v5

Review of attachment 722650 [details] [diff] [review]:
-----------------------------------------------------------------

You could remove the length check in test_sim_contact.js
Attachment #722650 - Flags: review?(allstars.chh)
Remove length check in test_sim_contacts.js.
Attachment #722650 - Attachment is obsolete: true
Attachment #722659 - Flags: review?(allstars.chh)
Attachment #722659 - Flags: review?(allstars.chh) → review+
Comment on attachment 722147 [details] [diff] [review]
Part 1: IDL for exporting contact to SIM, v4

Review of attachment 722147 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/icc/interfaces/nsIDOMIccManager.idl
@@ +288,5 @@
> +   *        PIN2 is only required for 'fdn'.
> +   */
> +  nsIDOMDOMRequest updateContact(in DOMString contactType,
> +                                 in nsIDOMContact contact,
> +                                 [optional] in DOMString pin2);

Don't you also need to pass something that identifies the contact to be updated? I.e. some form of identity?

What is used as identifying information on the sim card? I assume there's not a uuid like we use in the Contacts API.
(In reply to Jonas Sicking (:sicking) from comment #36)
> Comment on attachment 722147 [details] [diff] [review]
> Part 1: IDL for exporting contact to SIM, v4
> 
> Review of attachment 722147 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/icc/interfaces/nsIDOMIccManager.idl
> @@ +288,5 @@
> > +   *        PIN2 is only required for 'fdn'.
> > +   */
> > +  nsIDOMDOMRequest updateContact(in DOMString contactType,
> > +                                 in nsIDOMContact contact,
> > +                                 [optional] in DOMString pin2);
> 
> Don't you also need to pass something that identifies the contact to be
> updated? I.e. some form of identity?
>
Yes, we do need an identifier.
However, just like mozContacts.save,
if 'contact' has the id, we update the corresponding contact,
otherwise we create a new one.

So in this bug, its purpose to do the latter one, 'create'.
But we'd like to name this interface 'updateContact',
because we plan to reuse this interface when we're going to support the former one, 'update'.

Currently for importing (Bug 847741)/ exporting SIM contact(this bug), we still use nsIDOMContact.

For the 'identifier', I just filed a bug for it, Bug 850098

> What is used as identifying information on the sim card? I assume there's
> not a uuid like we use in the Contacts API.
We'll move this discussion to Bug 850098.
I have several questions about this feature.  Most of which cannot be answered by looking at the patches, so my apologies if this is not the right forum to ask:

*  I'm wondering how the export feature maps phone contacts to SIM contacts.  How does updateContact know which SIM contact to update?
*  Will the Export feature clear the SIM phonebook before performing the export?
*  What are the expectations if attempting to export more contacts than the SIM is configured to handle?
*  Can this solution scale if more contacts are supported (initially 254, but maybe up to 1016 or 2032 for example)?
*  Can this solution scale if more attributes are supported (multiple ANR, SNE, email, long phone numbers)?
*  Do we warn the user that information can be lost during export?  For example, contacts might not be able to export extra numbers of no ANR supported by SIM.
*  In regards to error handling, do we fail if a Type2 fills up even if there is space for more contacts?
*  For the API, does the import code mirror the export code?  Just wondering if there is some consistency... 

Thanks,

-Greg
(In reply to Greg Grisco from comment #38)
> I have several questions about this feature.  Most of which cannot be
> answered by looking at the patches, so my apologies if this is not the right
> forum to ask:
This bug is for interface part.
For detailed RIL implementation, please see bug 809726.

> 
> *  I'm wondering how the export feature maps phone contacts to SIM contacts.
> How does updateContact know which SIM contact to update?
Currently, we only support 'create' action.
For 'udpate' we need to have an identifier to know which SIM contact should be updated.
For the 'identifier', we will discuss in bug 850098.

> *  Will the Export feature clear the SIM phonebook before performing the
> export?
No, we don't clear SIM phonebook.
We save exporting contacts into empty record.

> *  What are the expectations if attempting to export more contacts than the
> SIM is configured to handle?
> *  Can this solution scale if more contacts are supported (initially 254,
> but maybe up to 1016 or 2032 for example)?
> *  Can this solution scale if more attributes are supported (multiple ANR,
> SNE, email, long phone numbers)?
> *  Do we warn the user that information can be lost during export?  For
> example, contacts might not be able to export extra numbers of no ANR
> supported by SIM.
> *  In regards to error handling, do we fail if a Type2 fills up even if
> there is space for more contacts?
This bug is for interface part.
I think above questions are more closely related to RIL implementation.
Maybe we can move the discussion to bug 809726 or you can file a new bug.

> *  For the API, does the import code mirror the export code?  Just wondering
> if there is some consistency... 
For import API, we will handle in bug 847741

> 
> Thanks,
> 
> -Greg
(In reply to Edgar Chen [:edgar][:echen] from comment #39)
> (In reply to Greg Grisco from comment #38)
> > *  What are the expectations if attempting to export more contacts than the
> > SIM is configured to handle?

In this bug, the interface is used to export 'one' contact.

> > *  Can this solution scale if more contacts are supported (initially 254,
> > but maybe up to 1016 or 2032 for example)?

This should be related to Bug 850098 (interface) and Bug 835802 (implementations)
Hi Jonas,

As we mention in comment 37, the updateContact() only support 'create' for now.
For 'update', it needs an identifier, this will be discussed in bug 850098.

Is it ok for you?

Thanks
Attachment #722147 - Flags: superreview?(jonas) → superreview+
Add r=allstars.chh and sr=jonas after r+ and sr+
Attachment #722147 - Attachment is obsolete: true
Attachment #725982 - Flags: superreview+
Attachment #725982 - Flags: review+
Rebase
Attachment #722252 - Attachment is obsolete: true
Attachment #725984 - Flags: review+
Keywords: checkin-needed
Depends on: 853049
No longer depends on: 853049
Blocks: 1157082
You need to log in before you can comment on or make changes to this bug.