Closed Bug 674720 Opened 13 years ago Closed 12 years ago

WebContacts (or Contacts+)

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: cjones, Assigned: gwagner)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(9 files, 12 obsolete files)

15.50 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
21.27 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
14.72 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
22.93 KB, patch
Details | Diff | Splinter Review
4.39 KB, patch
Dolske
: review-
Details | Diff | Splinter Review
IDL
4.93 KB, patch
sicking
: review+
Details | Diff | Splinter Review
10.53 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
4.12 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
71.40 KB, patch
Details | Diff | Splinter Review
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.
OS: Linux → All
Hardware: x86_64 → All
I'm not convinced we need a dedicated API for this. I would rather use a web app with local storage and web intents.
(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.
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.
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.
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.
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.
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.
(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.
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.
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.
Assignee: nobody → gal
Andreas, can you post your WIP here?
Assignee: gal → nobody
Attached patch incomplete patch (obsolete) — Splinter Review
Assignee: nobody → anygregor
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.
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.
Attached patch WiP Part 1 (obsolete) — Splinter Review
A first version that still needs work. AndroidBridge and JNI follow in part 2.
Attachment #565121 - Attachment is obsolete: true
Attached patch WiP Part 1 (obsolete) — Splinter Review
now with missing files
Attachment #576088 - Attachment is obsolete: true
Depends on: 587797
Attached patch WiP (obsolete) — Splinter Review
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.
Attachment #576578 - Attachment is obsolete: true
You should be able to use the same content-process implementation across all platforms.  If not, something is probably not quite right.
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?
Depends on: 718132
Attached patch Interface (obsolete) — Splinter Review
IDL files
Attachment #583238 - Attachment is obsolete: true
Attachment #591188 - Flags: review?(tantek)
Attachment #591188 - Flags: review?(jonas)
Attached patch interface (obsolete) — Splinter Review
Attachment #591188 - Attachment is obsolete: true
Attachment #591188 - Flags: review?(tantek)
Attachment #591188 - Flags: review?(jonas)
Attachment #591192 - Flags: review?(tantek)
Attachment #591192 - Flags: review?(jonas)
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?
(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.
(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?
(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
Attachment #591192 - Flags: review?(tantek) → review+
(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.
Depends on: 722626
Depends on: 723206
No longer depends on: 723206
Attached patch IDL (obsolete) — Splinter Review
Replaced the callback approach with DOMRequest.
Attachment #591192 - Attachment is obsolete: true
Attachment #591192 - Flags: review?(jonas)
Attachment #593530 - Flags: review?(jonas)
Depends on: 723206
Attached patch part2 ContactManager (obsolete) — Splinter Review
Based on the DOMRequest implementation in bug 722626
Attachment #593991 - Flags: review?(fabrice)
Attached patch Fallback (b2g) contactService (obsolete) — Splinter Review
Attachment #593997 - Flags: review?(fabrice)
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.
Attachment #593991 - Flags: review?(fabrice)
Attachment #593991 - Flags: review-
Attachment #593991 - Flags: feedback+
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
Fixed review comments.
Attachment #593991 - Attachment is obsolete: true
Attachment #594227 - Flags: review?(fabrice)
Attached patch TestsSplinter Review
Mochitests
Attachment #594254 - Flags: review?(fabrice)
Attached patch Setup (obsolete) — Splinter Review
Attachment #594317 - Flags: feedback?(fabrice)
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
Attachment #594227 - Flags: review?(fabrice) → review+
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 ?
Attachment #594254 - Flags: review?(fabrice) → review+
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
Attachment #594317 - Flags: feedback?(fabrice) → feedback-
Attached patch ContactDBSplinter Review
First version of ContactDB.
More features to follow in another bug.
Attachment #595248 - Flags: review?(jonas)
Attached patch demoSplinter Review
Adapted demo from philikon
Attached patch ServiceStartupSplinter Review
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?
Attachment #594317 - Attachment is obsolete: true
Attachment #595809 - Flags: review?(dolske)
Attached patch IDLSplinter Review
added limit attribute.
Attachment #593530 - Attachment is obsolete: true
Attachment #593530 - Flags: review?(jonas)
Attachment #596088 - Flags: review?(jonas)
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?
(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?
(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.
Attached patch Permissions (obsolete) — Splinter Review
Attachment #596725 - Flags: review?(fabrice)
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)
(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?
(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.
(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 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?
Attachment #596725 - Flags: review?(fabrice) → review-
Attached patch PermissionsSplinter Review
Moved permissions for mochitest to the test itself.
Attachment #596725 - Attachment is obsolete: true
Attachment #597131 - Flags: review?(fabrice)
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 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.]
Attachment #595809 - Flags: review?(dolske) → review-
(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.
Attached patch All in one patch (obsolete) — Splinter Review
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?
Attachment #596088 - Flags: review?(jonas) → review+
Comment on attachment 597131 [details] [diff] [review]
Permissions

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

nit: remove the debug() messages
Attachment #597131 - Flags: review?(fabrice) → review+
(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!
You wanted to take a look again (Comment 31).
Attachment #593997 - Attachment is obsolete: true
Attachment #593997 - Flags: review?(fabrice)
Attachment #600343 - Flags: review?(fabrice)
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.
Attachment #595248 - Flags: review?(jonas) → review+
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()
Attachment #600343 - Flags: review?(fabrice) → review+
Attached patch All in one patchSplinter Review
All in one with all review comments.
Attachment #600335 - Attachment is obsolete: true
Depends on: 731406
Depends on: 731464
https://hg.mozilla.org/mozilla-central/rev/8ec22af2fe91
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Depends on: 731855
Depends on: 732179
need to schedule a full team review for this one
Whiteboard: [secr:curtisk]
Whiteboard: [secr:curtisk] → [sec-assigned:curtisk:749355]
Depends on: 754142
Depends on: 772127
Flags: sec-review?(curtisk)
Depends on: 792594
Flags: sec-review?(curtisk) → sec-review?(ptheriault)
Whiteboard: [sec-assigned:curtisk:749355]
Flags: sec-review?(ptheriault)
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.
Flags: needinfo?
(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
Flags: needinfo?
You need to log in before you can comment on or make changes to this bug.