972079 - Client access contact information from Google

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P3)

Comment 1

[Mark Banner (:standard8)]

We can get an account set

Comment 2

[Adam Roach [:abr]]

cf. https://developers.google.com/google-apps/contacts/v3/

Comment 3

[:shell escalante]

apparently not as straight forward as seemed from talkilla experience.  needs more investigation - but target is 33

Comment 4

[:shell escalante]

target to implement 33 - correcting to UI needed end of 32 at latest.

Comment 5

[Romain Gauthier [:tOkeshu]]

(In reply to sescalante from comment #3)
> apparently not as straight forward as seemed from talkilla experience. 
> needs more investigation - but target is 33

Can you elaborate?

In talkilla we were importing from the client only. We did not have any server side component dealing with that (So no synchronization between multiple devices).

The main problem was the UI itself. Once imported, if you had too many contacts you could not use talkilla properly anymore because everyone in the list was displayed.

Do we agree that, if this bug is just about importing contacts and nothing more, then this is more or less a port of the talkilla code to loop?

Comment 6

[Romain Testard [:RT]]

The intention here is to store contacts locally, the way it was done on Talkilla.
Beyond MVP/Phase 1 we very likely will want to use server components to store contacts but this is not in scope for now.

So this bug indeed is only about importing Google contacts and it is very similar to what we did on Talkilla.
1000107 is about re-syncing contacts already imported from Google (one way sync from Google to update the locally stored contacts only).
972015 is about displaying these contacts - the UX will define the contacts get displayed.

Comment 7

[Mark Banner (:standard8)]

On Talkilla we had problems with:

- Bad/incomplete documentation
- CORS issue for getting photos for contacts from the API, it wasn't possible to do this from a client
- Lack of support from Google (afaik they haven't responded to our issues raised from around the start of the year).
- Lack of login prompt integration (had to be a separate dialog)
- No persistent token for information, it would last ~30mins, then have to go back to the server and log in again.

We can still move forward with this if we want to, we had something on Talkilla that worked without contact images.

If we want to save data back to Google, that will probably be more complicated, we never tried that with Talkilla.

Comment 8

[Romain Testard [:RT]]

To be clear, we DO NOT want to save data back to Google.
1000107 is one way sync only (update local contacts according to new copy of Google contacts, not the other way)

Comment 9

[Alexis Metaireau (:alexis)]

Something else to note: when we managed to go trough the CORS limitation, we finally were stuck because they are throttling clients, so it's not possible to get too many avatar pictures at the same time.

Comment 10

[Adam Roach [:abr]]

(In reply to Mark Banner (:standard8) from comment #7)

> On Talkilla we had problems with...

Thanks for the summary -- good to know where the landmines lie.

If the limitations of Google's home-rolled protocol are insurmountable, we have a couple of other options to explore here; specifically:

 - Server-to-server, OAUTH-based CardDAV: https://developers.google.com/google-apps/carddav/
 - Client-to-server, password-based CardDAV: http://gmailblog.blogspot.com/2012/09/a-new-way-to-sync-google-contacts.html

Comment 11

[Romain Testard [:RT]]

Borja mentioned to me that we may want to re-use some of the FFOS code given they have already integrated with the Google contacts API to get the same data as what we want to do.

Borja can you point us to where that code is?

Comment 12

[Borja Salguero [:borjasalguero]]

Hi Romain,
In Firefox OS currently we are importing all contacts from Google & FB to our Contact list [1]. Actually we are parsing from these sources to our shared format based on mozContact [2], which is compatible with vCard as well. IMHO we should try to share the same format for the contacts (or at least one compatible), due to in a near future the sync between Loop desktop & mobile could be a great feature. I hope it helps!

[1] https://github.com/mozilla-b2g/gaia/tree/master/shared/js/contacts/import
[2] https://wiki.mozilla.org/WebAPI/ContactsAPI

Comment 13

[Romain Testard [:RT]]

The OAUTH flow should not open a pop-up.
Ideally we want the OAUTH flow in the panel bug given constraints on resizing the panel we can have it in a new tab.

Comment 14

[Romain Testard [:RT]]

We need some UX to inform the user that the download of contacts is in progress (download of contacts can potentially take several minutes given the Google rate limit). Darrin can you please update the current UX?

Comment 15

[Adam Roach [:abr]]

(In reply to Borja Salguero (this week part-time in Gaia) [:borjasalguero] from comment #12)
> Hi Romain,
> In Firefox OS currently we are importing all contacts from Google & FB to
> our Contact list [1]. Actually we are parsing from these sources to our
> shared format based on mozContact [2], which is compatible with vCard as
> well. IMHO we should try to share the same format for the contacts (or at
> least one compatible), due to in a near future the sync between Loop desktop
> & mobile could be a great feature. I hope it helps!

Thanks! I had independently arrived at the conclusion that we want to use mozContact as the basis for our contacts, so that we could eventually transition to using the Contacts API in the future. The prospect of syncing with Mobile is another good reason to go down that path.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

We actually came to that conclusion at one point in Talkilla, but my recollection is that when we talked to folks working directly on the MozContact stuff, they convinced us that it was not a good path.  I don't recall the details (I believe it had to do with maintainability & future direction of that code), but I'd suggest checking with them before committing.  Perhaps sicking or gwagner either have thoughts here, or can aim us at the current maintainers of that code...

Comment 17

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

The contacts API doesn't actually provide any functionality for importing data from Google, facebook or other data sources. All of that is currently done in application logic which then writes to the contacts API.

The contacts API is basically just a "dumb" database with some builtin features for doing search. It's unlikely to get standardized in its current form since we didn't have time to do any discussions with other browser vendors during its development. Additionally it depends on security model work which so far has been very hard to get to go anywhere with any partner.

The only thing that contacts supplies over simply using indexedDB is:
* It is shared across the whole device. I.e. all apps/pages that use it see the same data. IndexedDB is
  sandboxed per-origin.
* It has built-in features for fuzzy phone number matching and searching by name.

The former doesn't seem like something that we need for Loop. Until we get other pieces of code using the API, Loop would share the data only with itself.

The latter might be something that Loop is interested in? Though I would recommend simply reusing the libraries that we use for fuzzy phone number matching.

Another thing to keep in mind is that the API isn't entirely stable. We're likely going to do some work relatively soon to transition to using DataStore instead and make the Contacts API instead be a wrapper around DataStore.

Comment 18

[Darrin Henein [:darrin]]

(In reply to Romain Testard [:RT] from comment #14)
> We need some UX to inform the user that the download of contacts is in
> progress (download of contacts can potentially take several minutes given
> the Google rate limit). Darrin can you please update the current UX?

Will we have indications of progress (i.e. 10%, 50% done) or will it be binary (in-progress, finished)?

Comment 19

[Romain Testard [:RT]]

My understanding is that this process can be fairly slow if the user has a lot of contacts (rate limitation from Google) therefore I think we need an indication of progress.

Comment 20

[Darrin Henein [:darrin]]

Sorry, maybe my question wasn't clear. Dan can you maybe take a stab at it? Just trying to design the correct progress indicator type (determinate/indeterminate)

> Will we have indications of progress (i.e. 10%, 50% done) or will it be binary (in-progress, finished)?

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

So the question, as I understand it, is whether the API we'll be using to consume Google Contacts will tell us how many contacts there are when we start a sync, so that we'll be able compute percent-complete as we sync and display that to the user.

If we use CardDAV, it can almost certainly do that.  If we use Google Contacts 3, a quick skim of their docs didn't make it obvious how one would do that, but the docs are big, so I could have easily missed that.

IIRC, one of the problems we ran into Talkilla was that the Google Contacts 3 API was mostly intended to be server-to-server, which made it an ugly fit for trying to use client-side only.  CardDAV has its own issues, but it might still be a better path.

So, uh, "maybe".  :-)

Perhaps a better place to start from is to find out what API tOkeshu (Romain Gauthier) has been working with and how that's going.

Comment 22

[Adam Roach [:abr]]

(In reply to Dan Mosedale (:dmose - needinfo? me for responses) from comment #16)
> We actually came to that conclusion at one point in Talkilla, but my
> recollection is that when we talked to folks working directly on the
> MozContact stuff, they convinced us that it was not a good path.  I don't
> recall the details (I believe it had to do with maintainability & future
> direction of that code), but I'd suggest checking with them before
> committing.  Perhaps sicking or gwagner either have thoughts here, or can
> aim us at the current maintainers of that code...

Let's make a really clear distinction here between:

 * mozContact (https://developer.mozilla.org/en-US/docs/Web/API/mozContact) and
 * The Contacts API (https://developer.mozilla.org/en-US/docs/Web/API/Contacts_API)

Your comment appears to be conflating them (or maybe I'm just reading it as doing so in the context of Jonas' response).

What I'm trying to say above is: for the current MVP, we want to use mozContact. We do not want to use Contacts API. We probably want to use Contacts API in the future, though, and using mozContact lets us do so with minimal friction.

Jonas: The API instability you talk about -- does that apply to the mozContact objects, the Contacts API, or both?

Comment 23

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

My comment applies to them both.

To use your vocabulary: A mozContact object currently only does something useful when combined with the Contacts API. And the Contacts API always interacts with a consumer using mozContact objects. This is why we generally always talk about them together. (usually we do that using the term "contacts API", but I'll avoid doing so here).

A mozContact object is just a bundle of properties. It doesn't have any functionality that's not available in a plain JS object like |x = {}|. The way it's generally used is that you create one and then persist it using the Contacts API.

In theory you could extract just a mozContact implementation and take advantage of the predefined property names. And then build your own persistence code on top of indexedDB. I'm not sure that that is worth the effort compared to simply using plain JS objects that happens to have the same property names as mozContact does. That way you don't have to worry about any future changes to mozContact or Contacts API.

Comment 24

[Adam Roach [:abr]]

(In reply to Jonas Sicking (:sicking) from comment #23)
> My comment applies to them both.
> 
> To use your vocabulary: A mozContact object currently only does something
> useful when combined with the Contacts API. And the Contacts API always
> interacts with a consumer using mozContact objects. This is why we generally
> always talk about them together. (usually we do that using the term
> "contacts API", but I'll avoid doing so here).
> 
> A mozContact object is just a bundle of properties. It doesn't have any
> functionality that's not available in a plain JS object like |x = {}|. The
> way it's generally used is that you create one and then persist it using the
> Contacts API.
> 
> In theory you could extract just a mozContact implementation and take
> advantage of the predefined property names. And then build your own
> persistence code on top of indexedDB. I'm not sure that that is worth the
> effort compared to simply using plain JS objects that happens to have the
> same property names as mozContact does. That way you don't have to worry
> about any future changes to mozContact or Contacts API.

Thanks. I think we'll probably end up taking that approach, then.

Although we do need to worry about changes to the API if and when we decide to move our data into the Contacts store and/or sync it with mobile devices. But that's much further down the road. :)

Comment 25

[Adam Roach [:abr]]

(In reply to Darrin Henein [:darrin] from comment #20)
> Sorry, maybe my question wasn't clear. Dan can you maybe take a stab at it?
> Just trying to design the correct progress indicator type
> (determinate/indeterminate)
> 
> > Will we have indications of progress (i.e. 10%, 50% done) or will it be binary (in-progress, finished)?

In looking at the code that the mobile client uses, I see evidence that they use a progress indicator. The general flow (I'm reading code here, not using the client) appears to be: 

 1. Retrieve a list of all contacts (basically, get the list of all contacts
    in the  group called "Contacts")

 2. Ask the user which contacts to import (with an easy means to say "all")

 3. Iterate over the list of contacts, retrieving their data from the remote
    server one-by-one.

After step 2 (which we might skip for the desktop client; that is, we might assume "all contacts"), we know how many total contacts we'll be importing, and we can use the number of already-imported contacts to update a progress widget.

I'm clearing the needinfo to tOkeshu here, as I think he's been working entirely on data store rather than import.

Comment 26

[Romain Testard [:RT]]

> After step 2 (which we might skip for the desktop client; that is, we might
> assume "all contacts"), we know how many total contacts we'll be importing,
> and we can use the number of already-imported contacts to update a progress
> widget.

Yes, selection of specific contacts is not part of the requirements for MVP. We assume import of all contacts.

Comment 27

[Maire Reavy [:mreavy]]

Please see https://wiki.mozilla.org/Loop/Architecture/Address_Book for the design write up.

Comment 28

[Adam Roach [:abr]]

Attachment: WIP: Simple CardDAV Importer

Comment 29

[Adam Roach [:abr]]

Attachment: WIP: Simple CardDAV Importer

Comment 30

[Adam Roach [:abr]]

Attachment: CardDAV Address Importer for Loop

Comment 31

[Adam Roach [:abr]]

Comment on attachment 8476871 [details] [diff] [review]
CardDAV Address Importer for Loop

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

I don't have tests for this yet (I'm working on them next), but I can manually put it through its paces and verify that it's working. I'd like to go ahead and get the review started on the core functionality ahead of finishing the tests, though.

Note that this patch relies on the functionality added by Bug 1056918.

Comment 32

[Adam Roach [:abr]]

Attachment: Tests for CardDav importer

Comment 33

[Adam Roach [:abr]]

Attachment: CardDAV Address Importer for Loop

Comment 34

[Adam Roach [:abr]]

Comment on attachment 8477672 [details] [diff] [review]
CardDAV Address Importer for Loop

Matt -- if you have time to review this, I'd appreciate it. If you're too busy or think that someone else needs to look at it, let me know.

Comment 35

[Matthew N. [:MattN]]

Comment on attachment 8477672 [details] [diff] [review]
CardDAV Address Importer for Loop

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

This review pass is without looking into CardDav specifications. If you know someone who is already familiar with the specification then their help in reviewing would be appreciated.

::: browser/components/loop/CardDavImporter.jsm
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Ideally this module could live outside the Loop directory (e.g. in toolkit) for use by other code in the future but that can happen later. Was any of this code from the B2G implementation? I would feel more confident knowing the code was used in the wild already.

@@ +52,5 @@
> +   *                              - "user": Username to use for basic auth
> +   *                              - "pass": Password to use for basic auth
> +   * @param {Function} callback Callback function that will be invoked once the
> +   *                            import operation is complete. The first argument
> +   *                            passed to the callback will be an 'Error' object

OOC, why not just return a promise instead?

@@ +70,5 @@
> +      callback(new Error("No authentication specified"));
> +      return;
> +    }
> +
> +    if (options.auth === "basic") {

Mozilla coding style is to use == instead of === (unless it's really necessary). https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators

@@ +89,5 @@
> +      return;
> +    }
> +    let host = options.host;
> +
> +    Task.spawn(function* () {

This function could probably be split up into some helpers that can be individually tested.

@@ +90,5 @@
> +    }
> +    let host = options.host;
> +
> +    Task.spawn(function* () {
> +      console.info("Starting CardDAV import from " + host);

We generally don't leave debug messages on by default for shipping code so all of the console.info calls should be silenced by default. Log.jsm or a helper which checks a pref can help with this.

@@ +93,5 @@
> +    Task.spawn(function* () {
> +      console.info("Starting CardDAV import from " + host);
> +      let baseUrl = "https://" + host;
> +      let startUrl = baseUrl + "/.well-known/carddav";
> +      let abookUrl;

Nit: the common style is all-caps for URL in variable names

@@ +96,5 @@
> +      let startUrl = baseUrl + "/.well-known/carddav";
> +      let abookUrl;
> +
> +      // Get list of contact URLs
> +      let abook = yield this._davPromise("PROPFIND", startUrl, auth, 1,

Perhaps the magic number of 1 for the depth should be a const?

@@ +98,5 @@
> +
> +      // Get list of contact URLs
> +      let abook = yield this._davPromise("PROPFIND", startUrl, auth, 1,
> +         '<d:propfind xmlns:d="DAV:"><d:prop><d:getetag />' +
> +         '</d:prop></d:propfind>');

These lines should be aligned with the first argument so you may want this may need to move to a temporary variable to format nicely.
Common style is to use double-quotes for strings. If you want to do multi-line strings and don't want to escape the inner " you can now use ``.

@@ +102,5 @@
> +         '</d:prop></d:propfind>');
> +
> +      // Build multiget REPORT body from URLs in PROPFIND result
> +      let contactElements = abook.responseXML
> +          .getElementsByTagNameNS("DAV:", "href");

periods should be aligned or ideally for browser/ the period should be on the previous line and "get…" should be left aligned with abook

@@ +109,5 @@
> +                 'xmlns:c="urn:ietf:params:xml:ns:carddav">' +
> +                 '<d:prop><d:getetag /> <c:address-data /></d:prop>\n';
> +
> +      for (let i = 0; i < contactElements.length; i++) {
> +        let href = contactElements.item(i).textContent;

I *think* you can use a for…of loop here to make this easier to scan.

@@ +220,5 @@
> +          let value = match[2];
> +
> +          // Poor-man's unescaping
> +          value = value.replace(/\\:/g,':');
> +          value = value.replace(/\\,/g,',');

Nit: missing space after the comma separating arguments and use double quotes.

@@ +248,5 @@
> +            value = value.replace(/\\;/g,';');
> +            contact.name = [value];
> +          }
> +
> +          if (name === 'N') {

Nit: double-quotes

@@ +251,5 @@
> +
> +          if (name === 'N') {
> +            // Because we don't have lookbehinds, matching unescaped
> +            // semicolons is a pain. Luckily, we know that \r and \n
> +            // cannot appear int he strings, so we use them to swap

s/int he/in the/

@@ +422,5 @@
> +   */
> +  _davPromise: function(method, url, auth, depth, body) {
> +    return new Promise((resolve, reject) => {
> +      let req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> +        .createInstance(Ci.nsIXMLHttpRequest);

Nit: non-standard indentation

@@ +434,5 @@
> +
> +      req.open(method, url, true, user, pass);
> +
> +      req.setRequestHeader("Depth", depth);
> +      req.setRequestHeader("Content-Type","application/xml; charset=utf-8");

Nit: missing space after the comma

Comment 36

[Adam Roach [:abr]]

Attachment: Tests for CardDav importer

Comment 37

[Adam Roach [:abr]]

(In reply to Matthew N. [:MattN] from comment #35)
> Comment on attachment 8477672 [details] [diff] [review]
> CardDAV Address Importer for Loop
> 
> Review of attachment 8477672 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This review pass is without looking into CardDav specifications. If you know
> someone who is already familiar with the specification then their help in
> reviewing would be appreciated.

I don't. As a practical matter, this isn't the way I would ideally get stuff from a CardDAV server, but Google's implementation is strictly limited to the operations that Apple's address book performs, so we're kind of constrained to do exactly the same thing. In terms of CardDAV usage, I think we may need to treat this as a "proof is in the pudding" kind of situation: the fact that it works is probably sufficient (especially since we plan to swap this out with an OAuth-based importer as soon as we have the cycles to do so).


> ::: browser/components/loop/CardDavImporter.jsm
> @@ +1,1 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> 
> Ideally this module could live outside the Loop directory (e.g. in toolkit)
> for use by other code in the future but that can happen later.

Perhaps. Some of the logic in here is somewhat loop-specific (e.g., excluding contacts that don't have an email address or phone number: a general-purpose importer would include them, but it doesn't make sense for Loop to have them).

> Was any of
> this code from the B2G implementation? I would feel more confident knowing
> the code was used in the wild already.

No, I wrote this from scratch. It was originally intended to be a simple demonstration of how to use the API that I proposed for Contact importers (because I knew [well, thought -- it turns out that Google's incomplete implementation made it very frustrating] that doing a CardDAV importer would be very easy). But we're up against deadlines for 34; this is critical for the contacts work; and it works well enough to use until we have time to do UX and implementation for an OAuth approach.

> @@ +52,5 @@
> > +   *                              - "user": Username to use for basic auth
> > +   *                              - "pass": Password to use for basic auth
> > +   * @param {Function} callback Callback function that will be invoked once the
> > +   *                            import operation is complete. The first argument
> > +   *                            passed to the callback will be an 'Error' object
> 
> OOC, why not just return a promise instead?

This plugs in to the LoopContacts class, which is oriented towards callbacks rather than promises -- and other importers will need exactly the same signature. I didn't want to make the plugin interface incongruent with the class that uses it.

> @@ +89,5 @@
> > +      return;
> > +    }
> > +    let host = options.host;
> > +
> > +    Task.spawn(function* () {
> 
> This function could probably be split up into some helpers that can be
> individually tested.

Thanks for the suggestion. In the interest of time (and based on the fact that this task only really does three things), I'm going to decline at this time.

> Perhaps the magic number of 1 for the depth should be a const?

Fixed.


I've also fixed the nits you called out.

Ignore the patch I just uploaded -- I qrefed the wrong patch (put the importer fixes on the test patch) -- I'll have a new set of patches up momentarily.

Comment 38

[Adam Roach [:abr]]

Attachment: CardDAV Address Importer for Loop

Comment 39

[Adam Roach [:abr]]

Attachment: Tests for CardDav importer

Comment 40

[Matthew N. [:MattN]]

Comment on attachment 8479977 [details] [diff] [review]
Tests for CardDav importer

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

::: browser/components/loop/test/mochitest/browser_CardDavImporter.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +const {CardDavImporter} = Cu.import("resource:///modules/loop/CardDavImporter.jsm", {});

Nit: I'm not sure of the advantage of this over: |Cu.import("resource:///modules/loop/CardDavImporter.jsm");| (other than the const)?

@@ +45,5 @@
> +
> +const kAuth = {
> +  "method": "basic",
> +  "user": "username",
> +  "pass": "p455w0rd"

Nit which also applies to the implementation: "password" would be clearer

Comment 41

[Adam Roach [:abr]]

https://hg.mozilla.org/integration/fx-team/rev/a0b415b545f6
https://hg.mozilla.org/integration/fx-team/rev/282c0683db54

Tree burn fix:
https://hg.mozilla.org/integration/fx-team/rev/c77a88466fc7

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a0b415b545f6
https://hg.mozilla.org/mozilla-central/rev/282c0683db54
https://hg.mozilla.org/mozilla-central/rev/c77a88466fc7

Comment 43

[u279076]

Can someone please confirm if this is testable? I don't see the UI exposed in today's Nightly.

Comment 45

[Mike de Boer [:mikedeboer]]

Anthony, this is not testable, as it's not exposed by any ui atm. Bug 1069816 is the one where you'll be able to verify this work.

Comment 46

[u279076]

Actually, taking back the qe-verify- change. I'm going to keep this as qe-verify+ but mark it as blocked pending resolution of 1069816.

Comment 47

[u279076]

I've been able to import my contacts for a couple days now in Nightly and Aurora. Marking this verified fixed.