B2G RIL: Move mozContact.getSimContacts to IccManager

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: allstars.chh, Assigned: allstars.chh)

Tracking

({dev-doc-needed})

Trunk
mozilla23
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-birch])

Attachments

(5 attachments, 6 obsolete attachments)

1.26 KB, patch
allstars.chh
: review+
allstars.chh
: superreview+
Details | Diff | Splinter Review
2.39 KB, patch
allstars.chh
: review+
allstars.chh
: superreview+
Details | Diff | Splinter Review
1.05 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
12.34 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
25.49 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
Per discussion from Bug 838092, we'd move getSimContacts from ContactManager to IccManager.
Yoshi, would this work:
.getContacts()
.getFNDContacts()
OS: Gonk (Firefox OS) → All
Hardware: ARM → All
Version: unspecified → Trunk
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"?
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
(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.
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?
(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
Currently I am blocked by Bug 842981,
although I could run marionette tests first.

Try if Bug 842981 can be fixed first.
Depends on: 842981
Attachment #731040 - Attachment description: Part 2: Add readContact to IccManager. → Part 2: (IDL)Add readContact to IccManager.
Posted patch Part 5: readContacts impl. v2 (obsolete) — Splinter Review
rebase
Attachment #731043 - Attachment is obsolete: true
Attachment #731039 - Flags: review?(anygregor)
Attachment #731042 - Flags: review?(anygregor)
Attachment #731041 - Flags: review?(bugs) → review+
Attachment #731039 - Flags: review?(anygregor) → review+
Attachment #731042 - Flags: review?(anygregor) → review+
Attachment #731040 - Flags: review?(vyang) → review+
Attachment #731058 - Flags: review?(vyang) → review+
Attachment #731039 - Flags: superreview?(jonas)
Attachment #731040 - Flags: superreview?(jonas)
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
Attachment #731058 - Flags: feedback?(anygregor)
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?
Flags: needinfo?(allstars.chh)
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.
Flags: needinfo?(allstars.chh)
Are privileged applications able to use the Contacts API? If they are, fixing Gaia will unfortunately not be enough.
Flags: needinfo?(allstars.chh)
(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
Then I'll keep the older one.
Flags: needinfo?(allstars.chh)
Attachment #731058 - Flags: feedback?(anygregor) → feedback+
(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 on attachment 731039 [details] [diff] [review]
Part 1: Remove getSimContacts from ContactManager.

Cancelling sr? as Mounir has some concerns about it.
Attachment #731039 - Flags: superreview?(jonas)
Comment on attachment 731040 [details] [diff] [review]
Part 2: (IDL)Add readContact to IccManager.

Cancelling sr? with same reason above.
Attachment #731040 - Flags: superreview?(jonas)
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.
Flags: needinfo?(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.
Flags: needinfo?(mounir)
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.
Attachment #731039 - Flags: superreview?(jonas)
Attachment #731040 - Flags: superreview?(jonas)
Attachment #731039 - Flags: superreview?(jonas) → superreview?(mounir)
Attachment #731040 - Flags: superreview?(jonas) → superreview?(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.
Attachment #731039 - Flags: superreview?(mounir) → superreview+
Attachment #731040 - Flags: superreview?(mounir) → superreview+
(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"?
Flags: needinfo?(clouserw)
Flags: needinfo?(clouserw)
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.
(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?
Thanks for Gregor for helping changing sr?.
I would try to see if I can get any information from marketplace.

thanks
(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?
Flags: needinfo?(clouserw)
Nope.  It's grepping the FS, one way or the other.
Flags: needinfo?(clouserw)
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
ni? to Mounir per my comment in Comment 33.
Also I've sent an inquiry to dev-markerplace, but now seems only Andrew replied.
Flags: needinfo?(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.
Flags: needinfo?(mounir)
Keywords: dev-doc-needed
Hi, Mounir
Got you.
I'll try to update the Wiki for Contact API as well.

Thank you.
Rebase, and add sr=mounir.
This patch has been r+ by gregor and sr+ by mounir.
Attachment #731039 - Attachment is obsolete: true
Attachment #744390 - Flags: superreview+
Attachment #744390 - Flags: review+
Attachment #744390 - Attachment description: Part 1: Remove getSimContacts from ContactManager. v2 → Part 1: (IDL) Remove getSimContacts from ContactManager. v2
Rebase and add sr=mounir.

This patch has been r+ by vicamo and sr+ by mounir.
Attachment #731040 - Attachment is obsolete: true
Attachment #744391 - Flags: superreview+
Attachment #744391 - Flags: review+
rebase.
This patch has been r+ by smaug.
Attachment #731041 - Attachment is obsolete: true
Attachment #744393 - Flags: review+
rebase.

This patch has been r+ by gregor.
Attachment #731042 - Attachment is obsolete: true
Attachment #744394 - Flags: review+
rebase.
This patch has been r+ by vicamo.
Attachment #731058 - Attachment is obsolete: true
Attachment #744396 - Flags: review+
Depends on: 883162
Blocks: 1157082
You need to log in before you can comment on or make changes to this bug.