847820 - B2G RIL: Support exporting contacts to SIM card in IccManager

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Edgar Chen [:edgar]]

Support exporting contacts to SIM card in IccManager.
(Please see the discussion in bug 838092 for more detailed information)

Comment 1

[Edgar Chen [:edgar]]

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

Comment 2

[Edgar Chen [:edgar]]

Attachment: WIP, Part 1: IDL for exporting contact to SIM, v1

Add new API in MozIccManagaer.
- DOMRequest updateContact(DOMString contactType, nsIDOMContact contact, DOMString pin2);

Comment 3

[Edgar Chen [:edgar]]

Attachment: Part 2: Modify IccManager for exporting contact to SIM, v1

IccManager changes

Comment 4

[Edgar Chen [:edgar]]

Attachment: WIP, Part 3: RIL implementation, v1

1. Remove updateICCContact() from nsIRadioInterfaceLayer.
2. Add new IPC message, RIL:IccUpdateContact, in RadioInterfaceLayer and RILContentHelper.

Comment 5

[Edgar Chen [:edgar]]

Attachment: WIP, Part 4: Add icc-write permission check, v1

Check permission "icc-write"

Comment 6

[Edgar Chen [:edgar]]

Attachment: WIP, Part 5: Marionette tests for updating SIM contact, v1

Marionette tests

Comment 7

[Edgar Chen [:edgar]]

Attachment: WIP, Part 5: Marionette tests for updating SIM contact, v2

Oh, forgot to remove the permission, icc-write, at the end of the tests.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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 ?

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 13

[Edgar Chen [:edgar]]

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

Comment 14

[Edgar Chen [:edgar]]

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

Comment 15

[Edgar Chen [:edgar]]

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

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 17

[Edgar Chen [:edgar]]

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

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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?

Comment 19

[Edgar Chen [:edgar]]

Attachment: Part 1: IDL for exporting contact to SIM, v2

1). Use lower case for contactType. ('fdn' and 'adn').
2). Make pin2 as opional argument.

Comment 20

[Edgar Chen [:edgar]]

Attachment: Part 1: IDL for exporting contact to SIM, v3

Sorry, I uploaded a wrong version, correct it.

Comment 21

[Edgar Chen [:edgar]]

Attachment: Part 3: RIL implementation, v2

1). s/error/errorMsg.
2). Pass pin2 to RadioInterfaceLayer.

Comment 22

[Edgar Chen [:edgar]]

Comment on attachment 722010 [details] [diff] [review]
WIP, Part 4: Add icc-write permission check, v1

We will discuss permission in bug 848684.

Comment 23

[Edgar Chen [:edgar]]

Attachment: Part 4: Marionette tests for updating SIM contact, v3

1). Remove icc permission.
2). Address comment #11.

Comment 24

[Edgar Chen [:edgar]]

Attachment: Part 1: IDL for exporting contact to SIM, v4

No need to include nsIDOMContactManager.idl, just use "interface nsIDOMContact".

Comment 25

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 26

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 27

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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 28

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 29

[Edgar Chen [:edgar]]

Attachment: Part 3: RIL implementation, v3, r=allstars.chh

1). Address review comment #27.
2). Add r=allstars.chh after r+.

Comment 30

[Edgar Chen [:edgar]]

Attachment: Part 4: Marionette tests for updating SIM contact, v4, r=allstars.chh

1). Address review comment #28.
2). Add r=allstars.chh after r+.

Comment 31

[Edgar Chen [:edgar]]

Attachment: Part 2: Modify ContactManager for exporting contact to SIM, v1, r=bugs

Add r=bugs after r+.

Comment 32

[Edgar Chen [:edgar]]

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

Comment 33

[Edgar Chen [:edgar]]

Attachment: Part 4: Marionette tests for updating SIM contact, v5

Disable test first. (comment #32)

Comment 34

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 35

[Edgar Chen [:edgar]]

Attachment: Part 4: Marionette tests for updating SIM contact, v6

Remove length check in test_sim_contacts.js.

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 37

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 38

[Greg Grisco]

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

Comment 39

[Edgar Chen [:edgar]]

(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

Comment 40

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 41

[Edgar Chen [:edgar]]

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

Comment 42

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yup, that sounds ok with me.

Comment 43

[Edgar Chen [:edgar]]

Attachment: Part 1: IDL for exporting contact to SIM, v4, r=allstars.chh, sr=jonas

Add r=allstars.chh and sr=jonas after r+ and sr+

Comment 44

[Edgar Chen [:edgar]]

Attachment: Part 3: RIL implementation, v4, r=allstars.chh

Rebase

Comment 45

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f0d01f00f1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/13d27a59ee08
https://hg.mozilla.org/integration/mozilla-inbound/rev/175853edfc52
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c296b25c9af

Comment 46

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0f0d01f00f1e
https://hg.mozilla.org/mozilla-central/rev/13d27a59ee08
https://hg.mozilla.org/mozilla-central/rev/175853edfc52
https://hg.mozilla.org/mozilla-central/rev/7c296b25c9af