Last Comment Bug 674720 - WebContacts (or Contacts+)
: WebContacts (or Contacts+)
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla13
Assigned To: Gregor Wagner [:gwagner]
:
Mentors:
https://wiki.mozilla.org/WebAPI/Conta...
Depends on: 587797 718132 722626 723206 731406 731464 731855 732179 749355 754142 772127 792594
Blocks: webapi b2g-demo-phone 731950 754390
  Show dependency treegraph
 
Reported: 2011-07-27 14:54 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2014-10-13 06:30 PDT (History)
47 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
incomplete patch (34.66 KB, patch)
2011-10-05 20:45 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
WiP Part 1 (92.20 KB, patch)
2011-11-21 22:14 PST, Gregor Wagner [:gwagner]
no flags Details | Diff | Splinter Review
WiP Part 1 (133.40 KB, patch)
2011-11-23 11:44 PST, Gregor Wagner [:gwagner]
no flags Details | Diff | Splinter Review
WiP (71.66 KB, patch)
2011-12-20 12:01 PST, Gregor Wagner [:gwagner]
no flags Details | Diff | Splinter Review
Interface (20.94 KB, patch)
2012-01-24 11:15 PST, Gregor Wagner [:gwagner]
no flags Details | Diff | Splinter Review
interface (10.75 KB, patch)
2012-01-24 11:23 PST, Gregor Wagner [:gwagner]
tantek: review+
Details | Diff | Splinter Review
IDL (9.71 KB, patch)
2012-02-01 11:14 PST, Gregor Wagner [:gwagner]
no flags Details | Diff | Splinter Review
part2 ContactManager (16.75 KB, patch)
2012-02-02 15:41 PST, Gregor Wagner [:gwagner]
fabrice: review-
fabrice: feedback+
Details | Diff | Splinter Review
Fallback (b2g) contactService (5.60 KB, patch)
2012-02-02 15:56 PST, Gregor Wagner [:gwagner]
no flags Details | Diff | Splinter Review
part2 ContactManager (15.50 KB, patch)
2012-02-03 10:04 PST, Gregor Wagner [:gwagner]
fabrice: review+
Details | Diff | Splinter Review
Tests (21.27 KB, patch)
2012-02-03 11:25 PST, Gregor Wagner [:gwagner]
fabrice: review+
Details | Diff | Splinter Review
Setup (10.16 KB, patch)
2012-02-03 14:56 PST, Gregor Wagner [:gwagner]
fabrice: feedback-
Details | Diff | Splinter Review
ContactDB (14.72 KB, patch)
2012-02-07 16:33 PST, Gregor Wagner [:gwagner]
bent.mozilla: review+
Details | Diff | Splinter Review
demo (22.93 KB, patch)
2012-02-07 17:57 PST, Gregor Wagner [:gwagner]
no flags Details | Diff | Splinter Review
ServiceStartup (4.39 KB, patch)
2012-02-09 10:35 PST, Gregor Wagner [:gwagner]
dolske: review-
Details | Diff | Splinter Review
IDL (4.93 KB, patch)
2012-02-10 10:22 PST, Gregor Wagner [:gwagner]
jonas: review+
Details | Diff | Splinter Review
Permissions (9.54 KB, patch)
2012-02-13 11:07 PST, Gregor Wagner [:gwagner]
fabrice: review-
Details | Diff | Splinter Review
Permissions (10.53 KB, patch)
2012-02-14 11:54 PST, Gregor Wagner [:gwagner]
fabrice: review+
Details | Diff | Splinter Review
All in one patch (72.99 KB, patch)
2012-02-24 02:00 PST, Gregor Wagner [:gwagner]
no flags Details | Diff | Splinter Review
Fallback ContactService (4.12 KB, patch)
2012-02-24 02:54 PST, Gregor Wagner [:gwagner]
fabrice: review+
Details | Diff | Splinter Review
All in one patch (71.40 KB, patch)
2012-02-24 05:38 PST, Gregor Wagner [:gwagner]
no flags Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-07-27 14:54:14 PDT
Goals
 - allow content to query contacts
 - allow content to update contacts (add/remove/edit)

The W3C work (http://www.w3.org/TR/contacts-api/) is relevant but read only.  We need read/write.
Comment 1 [:fabrice] Fabrice Desré 2011-07-27 19:44:42 PDT
I'm not convinced we need a dedicated API for this. I would rather use a web app with local storage and web intents.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-07-27 20:19:43 PDT
(1) We need to integrate well with existing OSes

(2) Having a contacts provider for Home that saved my contacts with Sync, e.g., is extremely attractive to me.  Sure we could have a hand-rolled unversioned non-standard impl soup built on intents, but given (1) I would rather have a standard API even in the B2G/Home world.
Comment 3 JL 2011-07-29 04:48:20 PDT
Primarily intended for the Mozilla OS project (B2G; may also be relevant to other OSes):

Personally I'd like to see an API that not is specific to a certain type of data.
Instead of contacts API, calendar API, etc, we have a Personal Information API.
Then there would be apps using that to store data in such a way that other apps can access it.
Those apps (like the default contacts/calendar apps) would also be the ones to provide the contacts API(s) and the calendar API(s). To explain, when a calendar app is installed then other apps don't need to implement the entire PI-API and the calendar API, it just has to communicate with the calendar app. Other calendar apps would hopefully use the same API and data structures to store data such that you can have 5 calendar apps sharing data (all storing/accessing it it with the PI-API), one being the default that other apps interact with.
Apps can then ask for personal information of a certain type and see which apps provides that. The OS could manage priviligies for accessing that data. Custom data types could easily be added (assuming we can make a sane API for it).

I'm aware that this could cause some serious complexity, but it should be considered, IMHO. I'm absolutely not demanding anything, but some feedback would however be nice.
Comment 4 Michael Hanson 2011-08-18 12:02:03 PDT
I'd +1 Fabrice's idea that we should see whether we can fit the Contacts API into something more like a web intent/web activity.

Unlike a pure device API (say, camera), contacts could come from a web service, a web application using local storage, or a purely native application (possibly talking to a SIM card).  That suggests that we need something like a registry of providers, and a way for the web runtime to act as a transport for IPC.

"Activities" is our current proposal for this capability in web apps, documented at https://github.com/mozilla/openwebapps/blob/master/docs/ACTIVITIES.md - the Chromium team has a similar idea at dev.chromium.org/developers/design-documents/webintentsapi.  I hear that we'll be taking it up on a W3C list shortly (as soon as the initial proposed is baked).

It might make sense to drive the W3C API through an Activities/Intent-derived channel - i.e. keep the W3C semantics but bind them to a user-selected provider.
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-08-18 13:24:39 PDT
I don't think leaving contacts up to a meta-API is a very good idea.  It's come out that getting the format of contacts correct wrt i18n is a very hard problem.  Expecting individual sites and contacts providers to negotiate their own de facto interfaces through a meta-API sounds like a recipe for widespread breakage, with no one to blame.  Instead, I'm in favor of having an intermediary with a well-specified API (the user agent) between clients and providers.  With a strong intermediary, it becomes possible to blame either the client for misusing the API, or the provider for botching the implementation.

Having a first-class contacts API doesn't preclude custom or multiple providers, and indeed that's something I'm very interested in.
Comment 6 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-18 13:33:57 PDT
Indeed. I think what we should have here is a "real" API facing the web developer. We can make the firefox implementation of that API both use the on-device contact list *and* web intents/activities to build the response to that API.
Comment 7 Michael Hanson 2011-08-18 13:38:49 PDT
Yes, I can see how that would work.

The experimental Contacts addon that Labs did last summer (http://hg.mozilla.org/labs/people) had a pluggable backend that normalized various sources (web or local) into a Portable Contacts frontend.  Adding a new backend required dropping some new code into the addon, but the pure-Portable-Contacts backend could have been dynamically bound to a provider at runtime.  Something like that could work well.
Comment 8 JL 2011-08-19 04:07:48 PDT
(Meh, looks like this weren't sent before when I thought I had sent it...)

Chris Jones: My suggested solution would be to implement a default contacts API using this "meta API", and doing it with a contacts app that does all the basics. Then there already is a default contacts API available from the beginning.
To prevent API fragmentation you should be *very* clear that the default API *always* is the preferred one unless those who want to use an alternate contacts API can give a *very* good reason for why to use another one.

I'm not sure on if that actually would work or not. I think it would work if the default contacts API is well made and fills the needs for most people, but I do understand if you don't think it would work.
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-08-19 10:10:15 PDT
That's a possible implementation route.  I suspect it'll be easier to do a first implementation that simply wraps platform contacts APIs, but we'll see.
Comment 10 Andreas Gal :gal 2011-08-24 19:05:10 PDT
The W3C Contacts API is clearly a good starting point. We will have to do some work on top of that for add/edit. I will grab this bug and do a prototype, but I am _VERY_ interested in someone taking this from me.
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-05 18:46:18 PDT
Andreas, can you post your WIP here?
Comment 12 Andreas Gal :gal 2011-10-05 20:45:10 PDT
Created attachment 565121 [details] [diff] [review]
incomplete patch
Comment 13 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-31 18:50:09 PDT
Gregor, Philipp volunteered to write the b2g-specific backend for contacts.  We'll probably want an XPCOM service or something with multiple impls: one for android that uses the android contacts API, and then one for b2g that saves contacts into its own IndexedDB.  (For starters.)  You guys should figure out what API that service should have.
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-08 23:16:15 PST
Gregor, one more thing: you mentioned in the b2g meeting adding a contacts interface to hal::.  I'm not sure that's going to make the code simpler; the setup we'll have is

 nsIContactsWhatever
   -> android backend that talks to AndroidBridge
   -> "native" backend that's implemented in JS, Philipp's part.  Can use that on desktop too, for now.
   -> IPC backend that proxies from content processes.

There's not much place for hal:: there, because the native backend forces us to go through XPCOM always for the ffi. I.e. the code never gets "lower level".

Mounir's SMS backend is a good example of what's probably simplest here: https://bugzilla.mozilla.org/attachment.cgi?id=572881&action=diff , except for two things: (i) note the review comments, don't want IPC backend in android; (ii) "fallback" impl can be native-JS impl for Contacts.
Comment 15 Gregor Wagner [:gwagner] 2011-11-21 22:14:14 PST
Created attachment 576088 [details] [diff] [review]
WiP Part 1

A first version that still needs work. AndroidBridge and JNI follow in part 2.
Comment 16 Gregor Wagner [:gwagner] 2011-11-23 11:44:40 PST
Created attachment 576578 [details] [diff] [review]
WiP Part 1

now with missing files
Comment 17 Gregor Wagner [:gwagner] 2011-12-20 12:01:12 PST
Created attachment 583238 [details] [diff] [review]
WiP

update with philikons indexedDB backend. No more c++ IPC, everything in JS. Fabrice suggested to also use this message style communication with the android backend.
Comment 18 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-20 18:24:33 PST
You should be able to use the same content-process implementation across all platforms.  If not, something is probably not quite right.
Comment 19 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-12-21 03:19:11 PST
Sounds cool.

Can you add the dom_contacts.xpt to the b2g/installer/package-manifest.in too? mobile/ should be interested in too.

About the API you have decided to use it does not fit well with the rest of the WebAPI I've played with to build webapps in Gaia.
Could you reply to the 'WebContacts API proposal' thread in dev-webapi@lists.mozilla.org?
Comment 20 Gregor Wagner [:gwagner] 2012-01-24 11:15:11 PST
Created attachment 591188 [details] [diff] [review]
Interface

IDL files
Comment 21 Gregor Wagner [:gwagner] 2012-01-24 11:23:41 PST
Created attachment 591192 [details] [diff] [review]
interface
Comment 22 Mounir Lamouri (:mounir) 2012-01-24 11:29:15 PST
Comment on attachment 591192 [details] [diff] [review]
interface

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

I think it might be interesting to use Request objects instead of callbacks to be consistent with other WebAPIs.

::: dom/interfaces/contacts/nsIDOMContactProperties.idl
@@ +52,5 @@
> +  attribute DOMString countryName;
> +};
> +
> +[scriptable, uuid(d730b0d0-cec0-11e0-9572-0800200c9a66)]
> +interface nsIDOMContactError : nsISupports

Please use DOMError, see DOM 4.

@@ +105,5 @@
> +  attribute jsval         email;              // DOMString[]
> +  attribute jsval         photo;              // DOMString[]
> +  attribute jsval         url;                // DOMString[]
> +  attribute jsval         category;           // DOMString[]
> +  attribute jsval         adr;                // ContactAddress[]

Why do you use an abbreviation?

@@ +107,5 @@
> +  attribute jsval         url;                // DOMString[]
> +  attribute jsval         category;           // DOMString[]
> +  attribute jsval         adr;                // ContactAddress[]
> +  attribute jsval         tel;                // DOMString[]
> +  attribute jsval         org;                // DOMString[]

same here

@@ +108,5 @@
> +  attribute jsval         category;           // DOMString[]
> +  attribute jsval         adr;                // ContactAddress[]
> +  attribute jsval         tel;                // DOMString[]
> +  attribute jsval         org;                // DOMString[]
> +  attribute jsval         bday;               // Date

and here

@@ +110,5 @@
> +  attribute jsval         tel;                // DOMString[]
> +  attribute jsval         org;                // DOMString[]
> +  attribute jsval         bday;               // Date
> +  attribute jsval         note;               // DOMString[]
> +  attribute jsval         impp;               // DOMString[]

What is impp?
Comment 23 Gregor Wagner [:gwagner] 2012-01-24 11:35:59 PST
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #22)
> Comment on attachment 591192 [details] [diff] [review]
> interface
> 
> Review of attachment 591192 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think it might be interesting to use Request objects instead of callbacks
> to be consistent with other WebAPIs.
> 
> ::: dom/interfaces/contacts/nsIDOMContactProperties.idl
> @@ +52,5 @@
> > +  attribute DOMString countryName;
> > +};
> > +
> > +[scriptable, uuid(d730b0d0-cec0-11e0-9572-0800200c9a66)]
> > +interface nsIDOMContactError : nsISupports
> 
> Please use DOMError, see DOM 4.

Ok can change it.

> 
> @@ +105,5 @@
> > +  attribute jsval         email;              // DOMString[]
> > +  attribute jsval         photo;              // DOMString[]
> > +  attribute jsval         url;                // DOMString[]
> > +  attribute jsval         category;           // DOMString[]
> > +  attribute jsval         adr;                // ContactAddress[]
> 
> Why do you use an abbreviation?
> 

That's Tanteks influence :) It's closer to hcard/vcard.
Comment 24 Mounir Lamouri (:mounir) 2012-01-24 11:40:25 PST
(In reply to Gregor Wagner [:gwagner] from comment #23)
> That's Tanteks influence :) It's closer to hcard/vcard.

AFAICT, abbreviations do not make sense in the web platform. What is the win of having the same property names than the hcard/vcard spec?
Comment 25 Tantek Çelik 2012-01-24 13:49:18 PST
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #22)
> Comment on attachment 591192 [details] [diff] [review]
> interface
> 
> Review of attachment 591192 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +108,5 @@
> > +  attribute jsval         category;           // DOMString[]
> > +  attribute jsval         adr;                // ContactAddress[]
> > +  attribute jsval         tel;                // DOMString[]
> > +  attribute jsval         org;                // DOMString[]
> > +  attribute jsval         bday;               // Date
> 
> Why do you use an abbreviation?

Since I expect these are questions that other webdevs may have, I've written them up as FAQs on the ContactsAPI spec.


See https://wiki.mozilla.org/WebAPI/ContactsAPI#Why_are_abbreviations_used_for_adr_bday_org_tel


> @@ +110,5 @@
> > +  attribute jsval         tel;                // DOMString[]
> > +  attribute jsval         org;                // DOMString[]
> > +  attribute jsval         bday;               // Date
> > +  attribute jsval         note;               // DOMString[]
> > +  attribute jsval         impp;               // DOMString[]
> 
> What is impp?

See https://wiki.mozilla.org/WebAPI/ContactsAPI#What_is_impp


(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #24)
> What is the win
> of having the same property names than the hcard/vcard spec?

See https://wiki.mozilla.org/WebAPI/ContactsAPI#Why_re-use_property_names_from_vCard_and_hCard
Comment 26 Gregor Wagner [:gwagner] 2012-01-25 09:55:00 PST
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #22)
> Comment on attachment 591192 [details] [diff] [review]
> 
> ::: dom/interfaces/contacts/nsIDOMContactProperties.idl
> @@ +52,5 @@
> > +  attribute DOMString countryName;
> > +};
> > +
> > +[scriptable, uuid(d730b0d0-cec0-11e0-9572-0800200c9a66)]
> > +interface nsIDOMContactError : nsISupports
> 
> Please use DOMError, see DOM 4.
> 

Well I will change it as soon as we have it in JS.
Comment 27 Gregor Wagner [:gwagner] 2012-02-01 11:14:31 PST
Created attachment 593530 [details] [diff] [review]
IDL

Replaced the callback approach with DOMRequest.
Comment 28 Gregor Wagner [:gwagner] 2012-02-02 15:41:51 PST
Created attachment 593991 [details] [diff] [review]
part2 ContactManager

Based on the DOMRequest implementation in bug 722626
Comment 29 Gregor Wagner [:gwagner] 2012-02-02 15:56:57 PST
Created attachment 593997 [details] [diff] [review]
Fallback (b2g) contactService
Comment 30 [:fabrice] Fabrice Desré 2012-02-02 17:32:24 PST
Comment on attachment 593991 [details] [diff] [review]
part2 ContactManager

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

That looks mostly good!

::: dom/contacts/ContactManager.js
@@ +34,5 @@
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.+
> + *
> + * ***** END LICENSE BLOCK ***** */
> +

use the new MPL2 license header (http://www.mozilla.org/MPL/headers/)

@@ +40,5 @@
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;
> +const Cr = Components.results;

Cr is not used.

@@ +49,5 @@
> +const CONTACTPROPERTIES_CONTRACTID        = "@mozilla.org/contactProperties;1";
> +const CONTACTPROPERTIES_CID               = Components.ID("{53ed7c20-ceda-11e0-9572-0800200c9a66}");
> +const nsIDOMContactProperties             = Components.interfaces.nsIDOMContactProperties;
> +const nsIClassInfo                        = Components.interfaces.nsIClassInfo;
> +

Nit: no need to use these const for stuff that is used only once. Also, s/Components.interface/Ci

@@ +63,5 @@
> +                                     flags: nsIClassInfo.DOM_OBJECT}),
> +
> +  QueryInterface : XPCOMUtils.generateQI([nsIDOMContactProperties])
> +}
> +

None of the properties are initialized. If it's guaranteed to not be an issue, ok, but add a comment.

@@ +157,5 @@
> +    },
> +    set: function(aPublished) {
> +      this._published = aPublished;
> +    }});
> +

or you can use :
get published() { } and 
set published() { } in the prototype

@@ +193,5 @@
> +    this.category =        _create(aProp.category) || null;
> +
> +    if (aProp.adr) {
> +      this.adr = new Array();
> +      for (let i = 0; i < aProp.adr.length; i++)

what if aProp.adr is not an array?

@@ +325,5 @@
> +    let contacts = msg.contacts;
> +
> +    switch (aMessage.name) {
> +      case "Contacts:Find:Return:OK":
> +        let req = this.getRequest(msg.requestID);

what happens when req is null? If two pages are using the contacts API, you will get messages back from both, and must filter to only process requests you fired.

@@ +382,5 @@
> +    }).bind(this));
> +
> +    this._rs = Cc["@mozilla.org/dom/dom-request-service;1"].getService(Ci.nsIDOMRequestService);
> +    this._requests = [];
> +    Services.obs.addObserver(this.cleanup.bind(this), "profile-before-change", false);

You should rather listen for inner-window-destroyed and match the innerWindowID (see http://mxr.mozilla.org/mozilla-central/source/dom/base/Webapps.js#252)

@@ +411,5 @@
> +let DEBUG = 0;
> +if (DEBUG)
> +  debug = function (s) { dump("-*- ContactManager: " + s + "\n"); }
> +else
> +  debug = function (s) {}

please move this to the top of the file.
Comment 31 [:fabrice] Fabrice Desré 2012-02-02 17:37:49 PST
Comment on attachment 593997 [details] [diff] [review]
Fallback (b2g) contactService

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

Sounds fine, but I want to see ContactDB.jsm before r+

::: dom/contacts/fallback/ContactService.jsm
@@ +46,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/ContactDB.jsm");
> +

I could not find ContactDB.jsm. Is it part of another bug?

@@ +122,5 @@
> +let DEBUG = 0;
> +if (DEBUG)
> +  debug = function (s) { dump("-*- Fallback ContactService component: " + s + "\n"); }
> +else
> +  debug = function (s) {}

Nit: move to the top of the file
Comment 32 Gregor Wagner [:gwagner] 2012-02-03 10:04:51 PST
Created attachment 594227 [details] [diff] [review]
part2 ContactManager

Fixed review comments.
Comment 33 Gregor Wagner [:gwagner] 2012-02-03 11:25:29 PST
Created attachment 594254 [details] [diff] [review]
Tests

Mochitests
Comment 34 Gregor Wagner [:gwagner] 2012-02-03 14:56:53 PST
Created attachment 594317 [details] [diff] [review]
Setup
Comment 35 [:fabrice] Fabrice Desré 2012-02-06 12:25:17 PST
Comment on attachment 594227 [details] [diff] [review]
part2 ContactManager

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

Code sounds fine to me, except that there is no permission verification mechanism. Is it on purpose? r=me if it is.

::: dom/contacts/ContactManager.js
@@ +49,5 @@
> +  this.postalCode = aPostalCode || null;
> +  this.countryName = aCountryName || null;
> +};
> +
> +function ContactProperties(aProp) { dump("!!!!!!ContactProperties Constructor\n"); }

nit: use debug() here

@@ +231,5 @@
> +      impp:            [],
> +      anniversary:     null,
> +      sex:             null,
> +      genderIdentity:  null
> +    };

nit: add a blank line
Comment 36 [:fabrice] Fabrice Desré 2012-02-06 12:53:27 PST
Comment on attachment 594254 [details] [diff] [review]
Tests

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

Looks good!

you could also save window.navigator.mozContacts in a variable to help readability

::: dom/contacts/tests/Makefile.in
@@ +26,5 @@
> +libs:: $(_TEST_FILES)
> +	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/tests/$(relativesrcdir)
> +
> +#libs:: $(_CHROME_TEST_FILES)
> +#	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/chrome/$(relativesrcdir)

nit: please remove.

::: dom/contacts/tests/test_contacts_basics.html
@@ +88,5 @@
> +}
> +
> +function onSuccess() {
> +  ok(false, "onSuccess: shouldn't get here");
> +}

Maybe rename this function so it's clearer that it's some "unwanted success"

@@ +138,5 @@
> +  function () {
> +    ok(true, "Deleting database1");
> +    req = window.navigator.mozContacts.clear();
> +    req.onsuccess = function () {
> +      ok(true, "Deleted the database2");

nit: why database1 and database2 ?
Comment 37 [:fabrice] Fabrice Desré 2012-02-06 13:02:49 PST
Comment on attachment 594317 [details] [diff] [review]
Setup

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

I'm putting feedback- mostly because you must find someone else to review the browser/* part. I only made comments based on my experience with the webapps support for desktop. Ping gavin, dolske or dao on this.

::: b2g/installer/package-manifest.in
@@ +159,5 @@
>  @BINPATH@/components/dom_geolocation.xpt
>  @BINPATH@/components/dom_network.xpt
>  @BINPATH@/components/dom_notification.xpt
>  @BINPATH@/components/dom_html.xpt
>  @BINPATH@/components/dom_indexeddb.xpt

don't you need to also add :
@BINPATH@/components/ContactManager.js
@BINPATH@/components/ContactManager.manifest

::: browser/base/content/contacts.js
@@ +2,5 @@
> +
> +var contactsService = {
> +  init: function() {}
> +}
> +

for such a small file, you can move it to browser.js

::: browser/base/content/global-scripts.inc
@@ +42,5 @@
>  <script type="application/javascript" src="chrome://browser/content/places/browserPlacesViews.js"/>
>  <script type="application/javascript" src="chrome://browser/content/browser.js"/>
>  <script type="application/javascript" src="chrome://global/content/inlineSpellCheckUI.js"/>
>  <script type="application/javascript" src="chrome://global/content/viewSourceUtils.js"/>
> +<script type="application/javascript" src="chrome://browser/content/contacts.js"/>

probably not needed

::: browser/base/jar.mn
@@ +29,5 @@
>  *       content/browser/browser.js                    (content/browser.js)
>  *       content/browser/browser.xul                   (content/browser.xul)
>  *       content/browser/browser-tabPreviews.xml       (content/browser-tabPreviews.xml)
>  *       content/browser/content.js                    (content/content.js)
> +        content/browser/contacts.js                   (content/contacts.js)

not needed
Comment 38 Gregor Wagner [:gwagner] 2012-02-07 16:33:26 PST
Created attachment 595248 [details] [diff] [review]
ContactDB

First version of ContactDB.
More features to follow in another bug.
Comment 39 Gregor Wagner [:gwagner] 2012-02-07 17:57:35 PST
Created attachment 595271 [details] [diff] [review]
demo

Adapted demo from philikon
Comment 40 Gregor Wagner [:gwagner] 2012-02-09 10:35:41 PST
Created attachment 595809 [details] [diff] [review]
ServiceStartup

I moved the service startup to the delayedStartup function.
I also looked at defineLazyServiceGetter but I only want to communicate with messages (nsIFrameMessageManager). Is there a way to use it anyway?
Comment 41 Gregor Wagner [:gwagner] 2012-02-10 10:22:35 PST
Created attachment 596088 [details] [diff] [review]
IDL

added limit attribute.
Comment 42 Justin Dolske [:Dolske] 2012-02-10 16:31:59 PST
Comment on attachment 595809 [details] [diff] [review]
ServiceStartup


>+++ b/browser/base/content/browser.js	Thu Feb 09 10:31:21 2012 -0800
...
>+  Components.utils.import("resource://gre/modules/ContactService.jsm");

Hmm. A few general questions here...

1) Is this API even relevant on desktop? 

2) Why isn't it being added for mobile (Fennec)?

3) What's the security model? From a quick skim I'm not seeing anything... What prevents sites from adding spam entries or pulling out all contacts? Or, yikes, just calling .clear()? Is there a permission/prompt somewhere? I'm a bit confused that there seems to be no UI at all?
Comment 43 Gregor Wagner [:gwagner] 2012-02-12 21:47:31 PST
(In reply to Justin Dolske [:Dolske] from comment #42)
> Comment on attachment 595809 [details] [diff] [review]
> ServiceStartup
> 
> 
> >+++ b/browser/base/content/browser.js	Thu Feb 09 10:31:21 2012 -0800
> ...
> >+  Components.utils.import("resource://gre/modules/ContactService.jsm");
> 
> Hmm. A few general questions here...
> 
> 1) Is this API even relevant on desktop? 
>

Jonas, any comments on this?
 
> 2) Why isn't it being added for mobile (Fennec)?

For fennec we need a different backend that talks to android. This is not implemented right now.

> 
> 3) What's the security model? From a quick skim I'm not seeing anything...
> What prevents sites from adding spam entries or pulling out all contacts?
> Or, yikes, just calling .clear()? Is there a permission/prompt somewhere?
> I'm a bit confused that there seems to be no UI at all?

We don't have a global security solution so far. Jonas is working on it. I will add a whitelist approach as a temporary workaround.

Do you see any problems with loading this service in the delayed-startup code or do you have a better idea?
Comment 44 Philipp von Weitershausen [:philikon] 2012-02-13 10:03:44 PST
(In reply to Justin Dolske [:Dolske] from comment #42)
> 1) Is this API even relevant on desktop? 

Why not? I could see this connecting to the OS X address book, for instance.
Comment 45 Gregor Wagner [:gwagner] 2012-02-13 11:07:26 PST
Created attachment 596725 [details] [diff] [review]
Permissions
Comment 46 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-13 16:40:25 PST
Comment on attachment 593997 [details] [diff] [review]
Fallback (b2g) contactService

Just a couple of drive-by nits I noticed!

>diff -r fea2518d759b dom/contacts/fallback/ContactService.jsm

>+/* ***** BEGIN LICENSE BLOCK *****

should use the new MPL headers for new code: https://www.mozilla.org/MPL/headers/

>+    this._messages.forEach((function(msgName) {
>+      this._mm.addMessageListener(msgName, this);
>+    }).bind(this));

forEach takes an optional thisParam as the second parameter (no need to bind)
Comment 47 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-13 16:46:38 PST
(In reply to Gregor Wagner [:gwagner] from comment #43)
> Do you see any problems with loading this service in the delayed-startup
> code or do you have a better idea?

Can you use nsIMessageWakeupService?
Comment 48 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-13 17:07:54 PST
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #47)
> Can you use nsIMessageWakeupService?

Apparently not, until bug 726864 is fixed. After that it seems like it might be ideal, though.
Comment 49 Gregor Wagner [:gwagner] 2012-02-13 17:36:39 PST
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #48)
> (In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> #47)
> > Can you use nsIMessageWakeupService?
> 
> Apparently not, until bug 726864 is fixed. After that it seems like it might
> be ideal, though.

nsIMessageWakeupService looks good. Thanks!
Comment 50 [:fabrice] Fabrice Desré 2012-02-14 10:09:30 PST
Comment on attachment 596725 [details] [diff] [review]
Permissions

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

r- for the test being granted permissions here. 

Also, this patch does not give the opportunity to the font-end to grant permissions on the fly. If we are ok with this whitelist-only approach this is fine for me.

::: dom/contacts/fallback/ContactService.jsm
@@ +41,5 @@
> +    try {
> +      let hosts = Services.prefs.getCharPref("dom.mozContacts.whitelist")
> +      hosts.split(",").forEach(function(aHost) {
> +        debug("Add host: " + JSON.stringify(aHost));
> +        if (aHost != "")

testing |aHost.length > 0| may be cheaper

@@ +45,5 @@
> +        if (aHost != "")
> +          Services.perms.add(Services.io.newURI(aHost, null, null), "webcontacts-manage",
> +                             Ci.nsIPermissionManager.ALLOW_ACTION);
> +      });
> +      // Add mochitest

can we move that to the test itself?
Comment 51 Gregor Wagner [:gwagner] 2012-02-14 11:54:16 PST
Created attachment 597131 [details] [diff] [review]
Permissions

Moved permissions for mochitest to the test itself.
Comment 52 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-02-20 05:54:43 PST
The current API store a file path for pictures of contacts.
In an application point of view, this means the application will need to access the file system as well as mozContacts. Why can't we store it as a blob?
Comment 53 Justin Dolske [:Dolske] 2012-02-21 14:30:10 PST
Comment on attachment 595809 [details] [diff] [review]
ServiceStartup

(In reply to Gregor Wagner [:gwagner] from comment #43)

> > 1) Is this API even relevant on desktop? 
> >
> 
> Jonas, any comments on this?

I'm going to r- for desktop pending an outcome for this. It sounds like this is all still very much in flux (no security model, no _actual_ desktop integration). I'll generally defer to Jonas here, but if these things are still being figured out it sounds not-yet-ready.

[otoh, landing but disabled via pref might be an option for you.]
Comment 54 Gregor Wagner [:gwagner] 2012-02-22 06:13:13 PST
(In reply to Justin Dolske [:Dolske] from comment #53)
> Comment on attachment 595809 [details] [diff] [review]
> ServiceStartup
> 
> (In reply to Gregor Wagner [:gwagner] from comment #43)
> 
> > > 1) Is this API even relevant on desktop? 
> > >
> > 
> > Jonas, any comments on this?
> 
> I'm going to r- for desktop pending an outcome for this. It sounds like this
> is all still very much in flux (no security model, no _actual_ desktop
> integration). I'll generally defer to Jonas here, but if these things are
> still being figured out it sounds not-yet-ready.
> 
> [otoh, landing but disabled via pref might be an option for you.]

I understand.
The focus is on B2G for now so I won't enable it on desktop.
Comment 55 Gregor Wagner [:gwagner] 2012-02-24 02:00:31 PST
Created attachment 600335 [details] [diff] [review]
All in one patch
Comment 56 Jonas Sicking (:sicking) PTO Until July 5th 2012-02-24 02:01:13 PST
Comment on attachment 596088 [details] [diff] [review]
IDL

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

r=me with that fixed.

::: dom/interfaces/contacts/nsIDOMContactProperties.idl
@@ +17,5 @@
> +  attribute DOMString countryName;
> +};
> +
> +[scriptable, uuid(d730b0d0-cec0-11e0-9572-0800200c9a66)]
> +interface nsIDOMContactError : nsISupports

Where do we use this interface? We should be moving to using string-based error names rather than number-based error codes.

@@ +35,5 @@
> +interface nsIDOMContactFindOptions : nsISupports
> +{
> +  attribute DOMString filterValue;  // e.g. "Tom"
> +  attribute DOMString filterOp;     // e.g. "contains"
> +  attribute jsval filterBy;         // DOMString[], e.g. "givenName, nickname"

This should be "givenName", "nickname", right? I.e. we don't parse a comma separated string?
Comment 57 [:fabrice] Fabrice Desré 2012-02-24 02:07:24 PST
Comment on attachment 597131 [details] [diff] [review]
Permissions

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

nit: remove the debug() messages
Comment 58 Gregor Wagner [:gwagner] 2012-02-24 02:34:50 PST
(In reply to Jonas Sicking (:sicking) from comment #56)
> Comment on attachment 596088 [details] [diff] [review]
> IDL
> 
> Review of attachment 596088 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with that fixed.
> 
> ::: dom/interfaces/contacts/nsIDOMContactProperties.idl
> @@ +17,5 @@
> > +  attribute DOMString countryName;
> > +};
> > +
> > +[scriptable, uuid(d730b0d0-cec0-11e0-9572-0800200c9a66)]
> > +interface nsIDOMContactError : nsISupports
> 
> Where do we use this interface? We should be moving to using string-based
> error names rather than number-based error codes.

I removed it and return strings now.

> 
> @@ +35,5 @@
> > +interface nsIDOMContactFindOptions : nsISupports
> > +{
> > +  attribute DOMString filterValue;  // e.g. "Tom"
> > +  attribute DOMString filterOp;     // e.g. "contains"
> > +  attribute jsval filterBy;         // DOMString[], e.g. "givenName, nickname"
> 
> This should be "givenName", "nickname", right? I.e. we don't parse a comma
> separated string?

fixed!
Comment 59 Gregor Wagner [:gwagner] 2012-02-24 02:54:31 PST
Created attachment 600343 [details] [diff] [review]
Fallback ContactService

You wanted to take a look again (Comment 31).
Comment 60 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-02-24 03:24:37 PST
Comment on attachment 595248 [details] [diff] [review]
ContactDB

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

Looks good! r=me with these things addressed:

::: dom/contacts/fallback/ContactDB.jsm
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const EXPORTED_SYMBOLS = ['ContactDB'];
> +
> +"use strict";

Doesn't this have to appear before any other statements?

@@ +54,5 @@
> +      return;
> +    }
> +
> +    let self = this;
> +    debug("try to open database:", DB_NAME, DB_VERSION, this._indexedDB);

You want "+" here, not "," right?

@@ +87,5 @@
> +      failureCb(event.target.errorMessage);
> +    };
> +    request.onblocked = function (event) {
> +      debug("Opening database request is blocked.");
> +      //failureCb(event.target.errorMessage);

I'd remove this line.

@@ +106,5 @@
> +  createSchema: function createSchema(db) {
> +    let objectStore = db.createObjectStore(STORE_NAME, {keyPath: "id"});
> +
> +    // Metadata indexes
> +    //objectStore.createIndex("id",        "id",        { unique: true });

Remove.

@@ +107,5 @@
> +    let objectStore = db.createObjectStore(STORE_NAME, {keyPath: "id"});
> +
> +    // Metadata indexes
> +    //objectStore.createIndex("id",        "id",        { unique: true });
> +    objectStore.createIndex("published", "published", { unique: false });

To be clear, unique defaults to false, so you could leave out the options object entirely if you want.

@@ +164,5 @@
> +          case Ci.nsIIDBDatabaseException.VERSION_ERR:
> +            failureCb(event.target.errorMessage);
> +            break;
> +          case Ci.nsIIDBDatabaseException.UNKNOWN_ERR:
> +            failureCb(event.target.errorMessage);

Do you need a separate case for this?

@@ +254,5 @@
> +        } else {
> +          debug("old record!")
> +          if (new Date(typeof contact.updated === "undefined" ? 0 : contact.updated) < new Date(event.target.result.updated)) {
> +            debug("rev check fail!");
> +            txn.abort();

Might want to put a return here in case you ever add anything below.

@@ +273,5 @@
> +      store.delete(aId);
> +    }, aSuccessCb, aErrorCb);
> +  },
> +
> +  clear: function clear(aSuccessCb, aErrorCb) {

Jonas and I both think that we should remove this function... Not sure when you'd ever want to do this.

@@ +345,5 @@
> +        request = store.getAll(options.filterValue);
> +      } else {
> +        debug("Getting index: " + key);
> +        let index = store.index(key);
> +        request = index.getAll(options.filterValue, options.filterLimit != 0 ? options.filterLimit : 100000);

This is fixed now, no need for the large number.

@@ +382,5 @@
> +              value = [f for each (f in [properties[field]])].join("\n") || '';
> +              break;
> +            default:
> +              value = properties[field];
> +              dump("unknown field: " + field + "\n");

debug()

@@ +408,5 @@
> +  _findAll: function _findAll(txn, store, options) {
> +    debug("ContactDB:_findAll:  " + JSON.stringify(options));
> +    if (!txn.result)
> +      txn.result = {};
> +    // store.getAll(null, null) is not equeal to sotre.getAll() so the workaround with 100000

"sotre" -> "store"

@@ +409,5 @@
> +    debug("ContactDB:_findAll:  " + JSON.stringify(options));
> +    if (!txn.result)
> +      txn.result = {};
> +    // store.getAll(null, null) is not equeal to sotre.getAll() so the workaround with 100000
> +    store.getAll(null, options.filterLimit != 0 ? options.filterLimit : 100000).onsuccess = function (event) {

This is fixed now, no need for large number.
Comment 61 [:fabrice] Fabrice Desré 2012-02-24 04:02:43 PST
Comment on attachment 600343 [details] [diff] [review]
Fallback ContactService

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

r=me with comments addressed

::: dom/contacts/fallback/ContactService.jsm
@@ +35,5 @@
> +    var idbManager = Components.classes["@mozilla.org/dom/indexeddb/manager;1"].getService(Ci.nsIIndexedDatabaseManager);
> +    idbManager.initWindowless(myGlobal);
> +    this._db = new ContactDB(myGlobal);
> +
> +    Services.obs.addObserver(this.cleanup.bind(this), "profile-before-change", false);

either make this a weak ref, or cal .removeObserver() in cleanup()
Comment 62 Gregor Wagner [:gwagner] 2012-02-24 05:38:55 PST
Created attachment 600366 [details] [diff] [review]
All in one patch

All in one with all review comments.
Comment 64 Matt Brubeck (:mbrubeck) 2012-02-29 11:18:23 PST
https://hg.mozilla.org/mozilla-central/rev/8ec22af2fe91
Comment 65 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-03-21 14:36:04 PDT
need to schedule a full team review for this one
Comment 66 Jeremie Patonnier :Jeremie 2013-04-24 07:54:50 PDT
Hi,

There is a first draft of the documentation available here:
https://developer.mozilla.org/en-US/docs/WebAPI/Contacts

I'm quite puzzled by this API, which is very clear on the paper but kind of blurry when it comes to implementation and usage. Feedback (of any kind) will be very welcomed on what I wrote so far.
Comment 67 Reuben Morais [:reuben] 2013-04-24 09:25:13 PDT
(In reply to Jeremie Patonnier :Jeremie from comment #66)
> Hi,
> 
> There is a first draft of the documentation available here:
> https://developer.mozilla.org/en-US/docs/WebAPI/Contacts
> 
> I'm quite puzzled by this API, which is very clear on the paper but kind of
> blurry when it comes to implementation and usage. Feedback (of any kind)
> will be very welcomed on what I wrote so far.

Hi Jeremie, thanks for writing that up!

The only thing I noticed is that it doesn't document the difference between find() and getAll(). I fixed that and a few typos: https://developer.mozilla.org/en-US/docs/WebAPI/Contacts$compare?to=381647&from=381251

Note You need to log in before you can comment on or make changes to this bug.