847741 - B2G RIL: Move mozContact.getSimContacts to IccManager

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Per discussion from Bug 838092, we'd move getSimContacts from ContactManager to IccManager.

Comment 1

[Mounir Lamouri (:mounir)]

Yoshi, would this work:
.getContacts()
.getFNDContacts()

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

For reading, reading these two types of SIM contact are entirely same.
But for writing, FDN needs another parameter 'pin2'

Currently I prefer to add a 'contactType' parameter, as in SIM contact, ADN and FDN are entirely same (same PDU, same coding scheme) except FDN is used for another feature called "Fixed Dialling".

So my idea is to do as follows:

nsIDOMIccManager
{
  DOMRequest readContacts(DOMString contactType);
  DOMRequest updateContact(DOMString contactType, nsIDOMContact contact, [optional]DOMString pin2);
}

Do you prefer creating read/write functions for each one of "ADN" and "FDN"?

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Hi, Gregor and Mounir

Any idea how do I return nsIDOMContact in RILContentHelper.js?
I don't know how to access nsIDOMContact (or how to 'new Contact()') from RILContentHelper.

Could you share some idea here?

thanks

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #3)
> Hi, Gregor and Mounir
> 
> Any idea how do I return nsIDOMContact in RILContentHelper.js?
> I don't know how to access nsIDOMContact (or how to 'new Contact()') from
> RILContentHelper.
> 
> Could you share some idea here?
> 
> thanks

I just tried
let contact = Cc["@mozilla.org/contact;1"].createInstance(Ci.nsIDOMContact);

seems okay now.

Comment 5

[Mounir Lamouri (:mounir)]

Do we really want to be able to write contacts in the SIM card? I feel like this is a feature that smart phones nowadays do not have. Is that a requirement?

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

(In reply to Mounir Lamouri (:mounir) from comment #5)
> Do we really want to be able to write contacts in the SIM card? I feel like
> this is a feature that smart phones nowadays do not have. Is that a
> requirement?

Yes

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Currently I am blocked by Bug 842981,
although I could run marionette tests first.

Try if Bug 842981 can be fixed first.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 1: Remove getSimContacts from ContactManager.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 2: (IDL)Add readContact to IccManager.

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 3: (DOM) Add readContacts in IccManager.

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 4: Remove getSimContacts impl.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 5: readContacts impl.

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 5: readContacts impl. v2

rebase

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Comment on attachment 731058 [details] [diff] [review]
Part 5: readContacts impl. v2

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

::: dom/system/gonk/RILContentHelper.js
@@ +1232,5 @@
> +    let window = this._windowsMap[message.requestId];
> +    delete this._windowsMap[message.requestId];
> +    let contacts = message.contacts;
> +    let result = contacts.map(function(c) {
> +      let contact = Cc["@mozilla.org/contact;1"].createInstance(Ci.nsIDOMContact);

Hi, Gregor
Can you give me some feeback about this line?

Now I'd like to return nsIDOMContact in icc.readContacts,
but ContactManager.js is not a JSM so I can only use Cc[contractId].createInstance to get mozContact here.

Do you have a better way here?

Thanks

Comment 15

[Mounir Lamouri (:mounir)]

According to our documentation, the API is available to privileged applications. How are we expecting them to not break? Should we keep the other method for a release or two and make it do nothing/throw/warn as deprecated? or do we expect to have barely no usage of that method until we ship that fix?

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Hi, Mounir
My original plan is land this bug and Bug 848233(Gaia part) together.
But if you suggest we should keep the old interface (Part 1 patch) for some period, it's okay to me.

Comment 17

[Mounir Lamouri (:mounir)]

Are privileged applications able to use the Contacts API? If they are, fixing Gaia will unfortunately not be enough.

Comment 18

[[:fabrice] Fabrice Desré]

(In reply to Mounir Lamouri (:mounir) from comment #17)
> Are privileged applications able to use the Contacts API? If they are,
> fixing Gaia will unfortunately not be enough.

Yes they are: https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsTable.jsm#63

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Then I'll keep the older one.

Comment 20

[Gregor Wagner [:gwagner]]

(In reply to Mounir Lamouri (:mounir) from comment #17)
> Are privileged applications able to use the Contacts API? If they are,
> fixing Gaia will unfortunately not be enough.

Should we really care at this point? We haven't released yet and I don't think any privileged app is using it.

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Comment on attachment 731039 [details] [diff] [review]
Part 1: Remove getSimContacts from ContactManager.

Cancelling sr? as Mounir has some concerns about it.

Comment 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Comment on attachment 731040 [details] [diff] [review]
Part 2: (IDL)Add readContact to IccManager.

Cancelling sr? with same reason above.

Comment 23

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Hi, Mounir
Can you help to answer Gregor's comments in Comment 20?
Also in Comment 15, you suggested I should Throw/Warn when using the older API,
Can you provide more detail on this?
Or do you have an example for this? 
I'd like to make those behavior more consistent.
If you have some suggestions/examples at hand there will be great.

Comment 24

[Mounir Lamouri (:mounir)]

I agree that it is early enough that we could probably change the API. Hopefully, even if a couple of application happen to be using this method, we could contact them personally to ask them to fix this.

Comment 25

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Comment on attachment 731039 [details] [diff] [review]
Part 1: Remove getSimContacts from ContactManager.

Re-send sr? to Jonas since Mounir agrees we could remove this API.

Comment 26

[Mounir Lamouri (:mounir)]

Comment on attachment 731039 [details] [diff] [review]
Part 1: Remove getSimContacts from ContactManager.

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

Please, could you try to see with the marketplace if they can check if an application is currently using this method. In that case, we should contact them to update their code.

Comment 27

[Gregor Wagner [:gwagner]]

(In reply to Mounir Lamouri (:mounir) from comment #26)
> Comment on attachment 731039 [details] [diff] [review]
> Part 1: Remove getSimContacts from ContactManager.
> 
> Review of attachment 731039 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please, could you try to see with the marketplace if they can check if an
> application is currently using this method. In that case, we should contact
> them to update their code.

I don't know the right person in the marketplace team. Phonebook shows Wil is working on marketplace :) Wil, could you cc someone that can check if any app is using "getSimContacts"?

Comment 28

[Andrew Williamson [:eviljeff]]

I'm afraid we don't have a way of checking (en masse) what permissions apps use at the moment.  Bug 842831 would let us narrow down the list to privileged apps at least, when implemented.

Comment 29

[Gregor Wagner [:gwagner]]

(In reply to Andrew Williamson [:eviljeff] from comment #28)
> I'm afraid we don't have a way of checking (en masse) what permissions apps
> use at the moment.  Bug 842831 would let us narrow down the list to
> privileged apps at least, when implemented.

Thanks for checking! Do we have a way to check the source code of apps if "getSimContacts" is used?

Comment 30

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Thanks for Gregor for helping changing sr?.
I would try to see if I can get any information from marketplace.

thanks

Comment 31

[Andrew Williamson [:eviljeff]]

(In reply to Gregor Wagner [:gwagner] from comment #29)
> (In reply to Andrew Williamson [:eviljeff] from comment #28)
> > I'm afraid we don't have a way of checking (en masse) what permissions apps
> > use at the moment.  Bug 842831 would let us narrow down the list to
> > privileged apps at least, when implemented.
> 
> Thanks for checking! Do we have a way to check the source code of apps if
> "getSimContacts" is used?

We currently have no tools that would let us search for code patterns over the packaged apps.  I've made some initial enquiries with IT about creating something similar to the addons mxr (https://mxr.mozilla.org/addons (LDAP login)) but its a non-trivial task on an old and unmaintained service.  I will file a bug and link here when I've investigated further.

Wil - any hacky workarounds you can think of?

Comment 32

[Wil Clouser [:clouserw]]

Nope.  It's grepping the FS, one way or the other.

Comment 33

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Hi, Mounir
It seems Bug 842831 and related functionalities cannot be landed in a short time.

Do we need to wait their result first?
Or I should deprecate the older API first (Throw/Warning, ..etc)

Thanks

Comment 34

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

ni? to Mounir per my comment in Comment 33.
Also I've sent an inquiry to dev-markerplace, but now seems only Andrew replied.

Comment 35

[Mounir Lamouri (:mounir)]

Yoshi, I think you can just move forward and land. However, if this is only going to land in 1.2, we might want to warn in 1.1 and 1.0.1. In any case, we should have documentation for this.

Comment 36

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Hi, Mounir
Got you.
I'll try to update the Wiki for Contact API as well.

Thank you.

Comment 37

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 1: (IDL) Remove getSimContacts from ContactManager. v2

Rebase, and add sr=mounir.
This patch has been r+ by gregor and sr+ by mounir.

Comment 38

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 2: (IDL)Add readContact to IccManager. v2

Rebase and add sr=mounir.

This patch has been r+ by vicamo and sr+ by mounir.

Comment 39

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 3: (DOM) Add readContacts in IccManager. v2

rebase.
This patch has been r+ by smaug.

Comment 40

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 4: Remove getSimContacts impl. v2

rebase.

This patch has been r+ by gregor.

Comment 41

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 5: readContacts impl. v3

rebase.
This patch has been r+ by vicamo.

Comment 42

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

http://hg.mozilla.org/projects/birch/rev/2131ced28811
http://hg.mozilla.org/projects/birch/rev/273e98426db0
http://hg.mozilla.org/projects/birch/rev/71f84b64af4f
http://hg.mozilla.org/projects/birch/rev/e4ce63318b84
http://hg.mozilla.org/projects/birch/rev/8f4cc68ae3c2

Comment 43

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2131ced28811
https://hg.mozilla.org/mozilla-central/rev/273e98426db0
https://hg.mozilla.org/mozilla-central/rev/71f84b64af4f
https://hg.mozilla.org/mozilla-central/rev/e4ce63318b84
https://hg.mozilla.org/mozilla-central/rev/8f4cc68ae3c2