1038716 - Add a LoopContacts class to handle contact mutations

Status: RESOLVED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

The interface of this object will be exposed to content as `navigator.mozLoop.contacts`.

[Draft]

The interface will look as follows:

mozLoop.contacts
  .add()
  .remove()
  .get()
  .getAll()
  .update()
  .block()
  .unblock()
  .startImport()
  .startSync() [strawman]

With the following characteristics:

 - Each method will be async. Wherever possible, Promises will be used to deal with handling deferred return values. I'm not sure if in-content DOM Promises can be transferred between content and chrome privileged content.
 - Each update to the data store (IndexedDB storage layer) will trigger an update event. `navigator.mozLoop` consumers may subscribe to these events and react accordingly. The `mozLoop` object will thusly assume the role of a Dispatcher object.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: WIP patch: add a contacts API

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: WIP patch: add a contacts API

Adam, I think I'm well underway here, but I'd like to touch base for a sec here.

I'll upload another patch that shows this API in full effect (a React view for the contacts list, based on Darrin's work).

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: WIP patch: consumer of this API, showing full contacts list

Comment 4

[Adam Roach [:abr]]

Comment on attachment 8456996 [details] [diff] [review]
WIP patch: add a contacts API

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

The overall shape of this looks really good. Comments inline.

::: browser/components/loop/LoopContacts.jsm
@@ +69,5 @@
> +      return;
> +
> +    // Create the 'contacts' store if it doesn't exist yet.
> +    let store = db.createObjectStore(kObjectStoreName, {
> +      keyPath: "cid",

The contact objects have an "id" field that is intended to be unique; and, for imported sources, can be used to correlate our copy of the contact with the remote version during a reimportation (or, in the future, sync) operation. I think we really want to use this as our key rather than adding our own unique key.

@@ +73,5 @@
> +      keyPath: "cid",
> +      autoIncrement: true
> +    });
> +
> +    store.createIndex("email", "email", {unique: false});

I really haven't worked with IndexedDB that much, but I think we're likely to run into some heartburn here. Email isn't a single string, but an array of strings. Unless the IndexedDB indexing does some really fancy object inspection and figures out that we want to index on all the elements present in an array (which seems pretty unlikely), I suspect this index won't actually be all that usable.

@@ +74,5 @@
> +      autoIncrement: true
> +    });
> +
> +    store.createIndex("email", "email", {unique: false});
> +    store.createIndex("name", "name", {unique: false});

This has the same issue as "email."

I also think that "name" is probably not the right thing to use here, since the UX describes features as being matched on individual field prefixes. For example, name searches are intended to be prefix matches based on individual fields. So, for example, a contact record representing you would contain name fields like:

{
  ...
  "name": ["Mike de Boer"],
  "givenName": ["Mike"],
  "familyName": ["de Boer"],
  ...
}

So, if I type "mi" in the search box, it should match you (as well as, say, "Mitchell Baker").
If I type "de" in the search box, it should match your entry (as well as, say, "Dean Willis").
If I type "bo" in the search box, it should match "Bob Smith", but *not* you, since no structured name field starts with "bo".

There's no way to implement this logic with the "name" field -- only with the individual structured fields (although I note that we're probably going to have to mock something in the case that an entry has a "name" but none of the structured fields).

On the topic -- I don't see any way to use *part* of an index field to perform searches on an IndexedDB; but, again, I'm not an expert here. If there's no way to do this, then using normal IndexedDB indices for user-searchable fields is probably not going to work out at all.

In any case, I suspect that the way we want to structure user searches is to have the UI use the contacts structure that it's maintaining for searches, rather than going back to the DB (which is *suspect* is your plan, as there is no search functionality on the API); so we can probably remove these indices altogether.

@@ +133,5 @@
> +      }
> +
> +      let contact = extend({}, details);
> +      let now = Date.now();
> +      contact.published = contact.updated = now;

Assuming this is the same interface that the importer uses, we'll want to preferentially take contact.updated from the provided details (using "now" only if the field is absent), so as to facilitate synchronization in the future.

@@ +312,5 @@
> +    callback(new Error("Not implemented yet!"));
> +  }
> +};
> +Object.freeze(LoopContactsInternal);
> +

I know the documentation isn't in here yet, but I think you definitely want to include a note at the top of this class indicating that the final parameter of each method is required to be a callback.

::: browser/components/loop/MozLoopAPI.jsm
@@ +98,5 @@
> +    contacts: {
> +      enumerable: true,
> +      get: function() {
> +        if (contactsAPI)
> +          return contactsAPI;

Please brace controlled statements (cf.https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures)

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: add a contacts API

Dan, I heard your review queue is rather long, so please feel free to defer review to someone else.

Comment 6

[Mike de Boer [:mikedeboer]]

Marco, can you add this to the current iteration?

Comment 7

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: tests

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.1: tests

Now with tests for the event emitter API.

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.1: tests

forgot to remove something...

Comment 10

[Marco Mucci [:MarcoM]]

Added to Iteration 33.3

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Since we're in the midst of the last-minute land-before-uplift push, I'm not going to be able to get to this before Monday.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Adam Roach [:abr] (Traveling through July 28th) from comment #4)
> Comment on attachment 8456996 [details] [diff] [review]
> WIP patch: add a contacts API
> 
> Review of attachment 8456996 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The overall shape of this looks really good. Comments inline.
> 
> ::: browser/components/loop/LoopContacts.jsm
> @@ +69,5 @@
> > +      return;
> > +
> > +    // Create the 'contacts' store if it doesn't exist yet.
> > +    let store = db.createObjectStore(kObjectStoreName, {
> > +      keyPath: "cid",
> 
> The contact objects have an "id" field that is intended to be unique; and,
> for imported sources, can be used to correlate our copy of the contact with
> the remote version during a reimportation (or, in the future, sync)
> operation. I think we really want to use this as our key rather than adding
> our own unique key.

In the past, I tried implementing a CalDAV client that worked this way.  I ran into cases where I could end up with varying copies of the same event when events got passed around and showed up from different sources (eg Google Calendar and iCloud...).  Based on that learning, I'd recommend against using anything external as a primary key, because when you run into something that violates the hoped-for-but-not-actually-guaranteed invariant, things explode in ugly ways.  I'd expect contacts with the same ID to be floating around similarly, due to broad use of imports, exports, and syncing across web and devices.

Comment 13

[Mike de Boer [:mikedeboer]]

(In reply to Dan Mosedale (:dmose - needinfo? me for responses) from comment #12)
> In the past, I tried implementing a CalDAV client that worked this way.  I
> ran into cases where I could end up with varying copies of the same event
> when events got passed around and showed up from different sources (eg
> Google Calendar and iCloud...).  Based on that learning, I'd recommend
> against using anything external as a primary key, because when you run into
> something that violates the hoped-for-but-not-actually-guaranteed invariant,
> things explode in ugly ways.  I'd expect contacts with the same ID to be
> floating around similarly, due to broad use of imports, exports, and syncing
> across web and devices.

This actually neatly summarizes the intent of my initial implementation!

Comment 14

[Adam Roach [:abr]]

(In reply to Mike de Boer [:mikedeboer] from comment #13)
> This actually neatly summarizes the intent of my initial implementation!

Okay, let's tease apart two different things, then: what we *call* our unique key, and what *value* we use for that unique key.

We're trying to match the format of mozContact here, which uses the name "id" as its unique key. So, the important thing, from my perspective, is that our unique key use that name.

The somewhat secondary issue is whether we're using remote unique keys or our own when we're grabbing data from a remote source. In this case, I do think we're better off retaining the remote value. I'll note that our first proposed data source -- Google Contacts -- uses an ID that is pretty tightly scoped to itself (e.g., http://www.google.com/m8/feeds/contacts/[e-mail]/base/bunch-of-random-stuff).

I would think that, as long as we ensure that all of our importers retain this basic property -- that the ID contains information that clearly scopes it to the source from which it came -- then we can avoid the issue that you're highlighting.

Comment 15

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.1: add a contacts API

Alright, I see both your points.

So what I did was prefix all the properties we use internally with an `_`, so we know what to work with ourselves.

I called the public `id` field a service ID, so that fetching a contact with that property will have to be done explicitly with the API call `getByServiceId("[e-mail]")`.
This is mainly useful for the (future) import feature.

So, IMO, this implementation looks quite safe to use to me.

Curious to hear what you think!

Comment 16

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.2: tests

Comment 17

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.2: add a contacts API

Made some improvements (getMany() method now works, ahum.)

Comment 18

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.3: tests

Comment 20

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.2: add a contacts API

Mark is back, I hope he has time to review this one!

Added more documentation and the storage layer shuts down properly now upon application-quit.

Comment 21

[Mike de Boer [:mikedeboer]]

Comment on attachment 8463315 [details] [diff] [review]
Patch 1.2: add a contacts API

Switched back to Adam for review, as discussed on IRC.

Comment 22

[Adam Roach [:abr]]

Comment on attachment 8463315 [details] [diff] [review]
Patch 1.2: add a contacts API

Adding Mark back on as reviewer -- I was unable to get to these reviews today. If Mark can get to them before I get a change, awesome. Otherwise, I will try to find time tomorrow (although Tuesdays are quite meeting-heavy for me in general, so this may not happen).

Also adding Justin Dolske to review, as we need browser peer sign-off to land changes to the MozLoopAPI.

Mark: if you r+ this, you can take me off. I'll do the same for you.

Comment 23

[Adam Roach [:abr]]

Comment on attachment 8463315 [details] [diff] [review]
Patch 1.2: add a contacts API

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

Functionally, this looks correct. I have a handful of nits of varying size. I'm not a javascript expert, and definitely not an IndexedDB expert, so I'm going to rely on Justin to sanity check those aspects of the patch.

::: browser/components/loop/LoopContacts.jsm
@@ +56,5 @@
> +      next();
> +    });
> +  }, err => {
> +    if (err) {
> +      callback(err);

It seems we probably want to make this "callback(err, processed)" so that the caller can differentiate between partial failures and complete failures (and, for partial failures, know which data were successfully processed).

@@ +83,5 @@
> + * The Contacts class.
> + *
> + * Each method that is a member of this class requires the last argument to be a
> + * callback Function. You'll notice this as well in the documentation for each
> + * method.

Perhaps something a bit stronger indicating that this isn't merely a convention: MozLoopAPI will cause things to break if this invariant is violated. What I want to avoid is someone deciding to add a method that doesn't do this, and then getting bitten by the handling in injectObjectAPI.

@@ +111,5 @@
> +      }
> +
> +      let contact = extend({}, details);
> +      let now = (new Date()).toISOString();
> +      if (!("published" in contact))

Please brace controlled statements. In this case, if you're trying to keep things compact, I would imagine that you can do "contact.published = contact.published || now;"

@@ +113,5 @@
> +      let contact = extend({}, details);
> +      let now = (new Date()).toISOString();
> +      if (!("published" in contact))
> +        contact.published = now;
> +      if (!("updated" in contact))

Same here.

@@ +378,5 @@
> +    });
> +  },
> +
> +  /**
> +   * Update a specific contact in the data store.

The documentation should be clearer here: the object is modified by replacing the fields passed in in "details;" and any fields not passed in are left unchanged. I think that's okay, as behavior goes, but it's not necessarily what users of this API will assume.

::: browser/components/loop/LoopStorage.jsm
@@ +65,5 @@
> +
> +  let openRequest = indexedDB.open(kDatabaseName, kDatabaseVersion);
> +
> +  openRequest.onblocked = function(event) {
> +    invokeCallbacks(new Error("Database access blocked: " + event.target.error));

Is "event.target.error" correct? This isn't really my area, but it's not clear to me why we would use "error" here and "errorCode" below...

@@ +77,5 @@
> +  };
> +
> +  openRequest.onupgradeneeded = function(event) {
> +    let db = event.target.result;
> +    eventEmitter.emit("upgrade", db);

Aren't we discarding oldVerion/newVersion information here? I suspect we'll need this in the case that we actually do an actual upgrade in the future (rather than simply using this to trigger initial table setup).

@@ +91,5 @@
> +
> +/**
> + * Start a transaction on the loop database and return it.
> + * If only one argument is passed, the default mode will be assumed and the first
> + * argument is assumed to be a callback.

I think this comment needs to be updated, as we don't do this here (although we *do* do it below, if you replace "one argument" with "two arguments" and so on).

@@ +125,5 @@
> +
> +/**
> + * Start a transaction on the loop database and return the contacts store.
> + * If only one argument is passed, the default mode will be assumed and the first
> + * argument is assumed to be a callback.

Same comment as above.

@@ +165,5 @@
> +  getInstance: function(callback) {
> +    openDatabase(callback);
> +  },
> +
> +  getTransaction: function(store, mode = "readonly", callback) {

I would add documentation for this method.

@@ +173,5 @@
> +    }
> +    getTransaction(store, mode, callback);
> +  },
> +
> +  getStore: function(store, mode = "readonly", callback) {

Same here.

@@ +211,5 @@
> +      }
> +
> +      i++;
> +      if (i < len) {
> +        onItem(list[i], handler, i);

My reading of Bug 723959 is that we don't have TCO implemented yet -- so, feeding a long list into this function can cause us to choke on an outsized call stack unless we ensure that the stack unwinds between calling onItem and its calling the handler function (which appears to be true for all our current uses).

After thinking about this a bit, I believe we most easily address this by adding a warning to the documentation block that warns against calling the handler directly from the onItem function for a successful operation. In other words, we need to warn that this can be used *only* for operations that are guaranteed to be async in the success case.

::: browser/components/loop/MozLoopAPI.jsm
@@ +18,5 @@
>  
>  this.EXPORTED_SYMBOLS = ["injectLoopAPI"];
>  
>  /**
> + * Trying to clone an Error object into a different container will yield an error.

Wouldn't we expect this to work? I can't find a bug open on it.

@@ +51,5 @@
> +        let callback = params.pop();
> +        api[func](...params, function(...results) {
> +          results = results.map(result => {
> +            if (result && typeof result == "object") {
> +              // XXXmikedeboer: for the life of me, `result instanceof Error` doesn't work here!

This seems like a bug. It would be good to open an issue on it and cite it here, as this is kind of a landmine -- I'd hate for someone to return on object that happens to have an attribute called "stack" and have it do unexpected things.

As a suggestion -- have you tried "Error.protoype.isPrototypeOf(result)"?

Comment 24

[Adam Roach [:abr]]

Mike -- I've run out of time today, but hope to get to the test patch tomorrow AM (CDT).

Comment 25

[Mike de Boer [:mikedeboer]]

(In reply to Adam Roach [:abr] from comment #23)
> ::: browser/components/loop/MozLoopAPI.jsm
> @@ +18,5 @@
> >  
> >  this.EXPORTED_SYMBOLS = ["injectLoopAPI"];
> >  
> >  /**
> > + * Trying to clone an Error object into a different container will yield an error.
> 
> Wouldn't we expect this to work? I can't find a bug open on it.

https://developer.mozilla.org/en-US/docs/Web/Guide/API/DOM/The_structured_clone_algorithm

> @@ +51,5 @@
> > +        let callback = params.pop();
> > +        api[func](...params, function(...results) {
> > +          results = results.map(result => {
> > +            if (result && typeof result == "object") {
> > +              // XXXmikedeboer: for the life of me, `result instanceof Error` doesn't work here!
> 
> This seems like a bug. It would be good to open an issue on it and cite it
> here, as this is kind of a landmine -- I'd hate for someone to return on
> object that happens to have an attribute called "stack" and have it do
> unexpected things.
> 
> As a suggestion -- have you tried "Error.protoype.isPrototypeOf(result)"?

This didn't work either :( I ended up with a workaround using `Object.prototype.toString.call(result) == "[object Error]"`. I'll ask around to see if someone can tell me what on earth is going on here.

Comment 26

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.3: add a contacts API

Addressed review comments, carrying over r=abr.

Comment 27

[Adam Roach [:abr]]

Comment on attachment 8461528 [details] [diff] [review]
Patch 2.3: tests

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

Yep, I like it.

There are a couple of places you're using scalars where we should be using arrays (name and category) -- I note them once, but they apply to all records.

The one additional test that I'd like to see explicitly run is a removeAll() followed by a getAll() and a check that there are zero results.

r+ with those changes.

Please consider making the contact comparison more robust as I suggest below, but the r+ stands without that specific change.

::: browser/components/loop/test/mochitest/browser_LoopContacts.js
@@ +4,5 @@
> +const {LoopContacts} = Cu.import("resource:///modules/loop/LoopContacts.jsm", {});
> +
> +const kContacts = [{
> +  id: 1,
> +  name: "Ally Avocado",

name: ["Ally Avocado"],

@@ +10,5 @@
> +    "pref": true,
> +    "type": ["work"],
> +    "value": "[e-mail]"
> +  }],
> +  category: "google"

category: ["google"]

@@ +89,5 @@
> +               "Value of first email address should be the same");
> +  Assert.equal(contact1.category, contact2.category,
> +               "Categories should be the same");
> +  Assert.equal(contact1.blocked, contact2.blocked,
> +               "Blocked statuses should be the same");

This seems a little cumbersome and error-prone. Wouldn't it make sense to recursively walk through the attributes of the first contact and verify that they have the same value in the second contact?

const compare = function(o1, o2, path) {
  path = path || "";
  if (o1 instanceof Object) {
    for (let p in o1) {
      if (!p.startsWith("_") && o1.hasOwnProperty(p)) {
        compare(o1[p], o2[p], path + "." + p);
      }
    }
  } else {
    Assert.strictEqual(o1, o2, path + " should match: " + JSON.stringify(o1) + " vs " + JSON.stringify(o2));
  }
}

(Or something like that -- you get the idea)

Comment 28

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.4: add a contacts API

Fixed strict mode warnings. Carrying over r=abr

Comment 29

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.4: tests

Addressed review comments. Also found a neat solution for `compareContacts()`. Carrying over r=abr

Comment 30

[Justin Dolske [:Dolske]]

Comment on attachment 8465285 [details] [diff] [review]
Patch 1.4: add a contacts API

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

::: browser/components/loop/MozLoopAPI.jsm
@@ +33,5 @@
> +  return obj;
> +};
> +
> +/**
> + * Inject any API containing _only_ function properties into the given window.

This API is only being injected into Loop-specific windows, and not arbitrary web pages, right? Is this intended to eventually go into a standards track, or is it a product-specific API that will never be standardized more broadly?

Has a DOM peer looked signed off on any of this new (or existing code)? I'm not sure if it matter as a Loop-content-only API, but the potentially subtle security aspects always terrify me. (Even Loop-content-only having potential chrome access would be a problem.)

Comment 31

[Justin Dolske [:Dolske]]

(Preemptive khuey CC for comment 30)

Comment 32

[Justin Dolske [:Dolske]]

Comment on attachment 8465285 [details] [diff] [review]
Patch 1.4: add a contacts API

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

Some quick comments from a skim. I'm not super familiar with indexdb, so I'll have to come back to that again or defer, if you want a review specifically on that angle.

::: browser/components/loop/LoopContacts.jsm
@@ +352,5 @@
> +
> +  /**
> +   * Retrieve an arbitrary amount of contacts from the data store.
> +   * CAUTION: If the amount of contacts is very large (say > 1000), this method
> +   *          may slow down your application!

That seems bad. Is this going to be used in production, or is it a quick hack for tests? I'd sorta expect that real users (especially those with lots of contacts via GMail or other sources) could easily get 1000+ contacts?

@@ +483,5 @@
> +   *                            `Error` object or `null`. The second argument will
> +   *                            be the result of the operation, if successfull.
> +   */
> +  startImport: function(options, callback) {
> +    //TODO

Put a bug # here?

@@ +497,5 @@
> +   *                            `Error` object or `null`. The second argument will
> +   *                            be an `Array` of contact objects, if successfull.
> +   */
> +  search: function(query, callback) {
> +    //TODO

Put a bug # here?

::: browser/components/loop/LoopStorage.jsm
@@ +10,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +const {EventEmitter} = Cu.import("resource://gre/modules/devtools/event-emitter.js", {});
> +const eventEmitter = new EventEmitter();
> +
> +this.EXPORTED_SYMBOLS = ["LoopContacts"];

Hmm. Shouldn't this be LoopStorage? And how is this currently working? :)

@@ +21,5 @@
> +
> +/**
> + * Properly shut the database instance down. This is done on application shutdown.
> + */
> +const closeDatabase = function() {

Why not |function closeDatabase()|?

@@ +78,5 @@
> +  };
> +
> +  openRequest.onupgradeneeded = function(event) {
> +    let db = event.target.result;
> +    eventEmitter.emit("upgrade", db, event.oldVersion, kDatabaseVersion);

Is it safe to pass chrome objects (like |db|) to content via eventEmitter?

From a quick skim, does any of this really need to be running in chrome? If Loop just wants to store things in IndexDB, couldn't this whole thing just live in content?

::: browser/components/loop/MozLoopAPI.jsm
@@ +33,5 @@
> +  return obj;
> +};
> +
> +/**
> + * Inject any API containing _only_ function properties into the given window.

Also... Does this work with E10S?

Comment 33

[Mike de Boer [:mikedeboer]]

(In reply to Justin Dolske [:Dolske] from comment #32)
> Some quick comments from a skim. I'm not super familiar with indexdb, so
> I'll have to come back to that again or defer, if you want a review
> specifically on that angle.

Adam (abr) already reviewed those bits, so I think we're fine there.

> 
> ::: browser/components/loop/LoopContacts.jsm
> @@ +352,5 @@
> > +
> > +  /**
> > +   * Retrieve an arbitrary amount of contacts from the data store.
> > +   * CAUTION: If the amount of contacts is very large (say > 1000), this method
> > +   *          may slow down your application!
> 
> That seems bad. Is this going to be used in production, or is it a quick
> hack for tests? I'd sorta expect that real users (especially those with lots
> of contacts via GMail or other sources) could easily get 1000+ contacts?

I think I should up the number here. It's just a word of caution that bulky queries may slow down an app, but that will only really happen with > 10.000 or 100.000 contacts.

> Hmm. Shouldn't this be LoopStorage? And how is this currently working? :)

Wowzers! No clue as to why/ how this worked!

> 
> @@ +21,5 @@
> > +
> > +/**
> > + * Properly shut the database instance down. This is done on application shutdown.
> > + */
> > +const closeDatabase = function() {
> 
> Why not |function closeDatabase()|?

Matter of style preference. Making it a const renders it immutable, but we're in a module compartment already, so that doesn't yield us many benefits, if at all. I'm quite indifferent as to which notation to use.

> 
> @@ +78,5 @@
> > +  };
> > +
> > +  openRequest.onupgradeneeded = function(event) {
> > +    let db = event.target.result;
> > +    eventEmitter.emit("upgrade", db, event.oldVersion, kDatabaseVersion);
> 
> Is it safe to pass chrome objects (like |db|) to content via eventEmitter?

No. That's why only the LoopStorage object emits this event, which is not exposed to content. Only LoopContacts is, which only consumes this event, it doesn't forward it to content.

> 
> From a quick skim, does any of this really need to be running in chrome? If
> Loop just wants to store things in IndexDB, couldn't this whole thing just
> live in content?

It could, but I talked this over with :abr and figured that
a) importing contacts needs to happen in chrome, because x-domain XHRs.
b) connecting to Fx Sync is easier to hook up in chrome.

> 
> ::: browser/components/loop/MozLoopAPI.jsm
> @@ +33,5 @@
> > +  return obj;
> > +};
> > +
> > +/**
> > + * Inject any API containing _only_ function properties into the given window.
> 
> Also... Does this work with E10S?

I hope so, otherwise we'd need to revisit SocialAPI, because it uses this construct as well!

Comment 34

[:Gavin Sharp [email: [e-mail]]]

I would not count on anything the social API does being e10s-compatible given bug 915546. It looks like we've already dug a bit of an e10s hole with MozLoopAPI, but we should at the very least stop digging where we can.

Comment 35

[Mike de Boer [:mikedeboer]]

It looks to me like this should mostly be ok, because CPOWs solve most of the problems. The only thing that might not work is `getChromeWindow()` which is used in the `startRinging` API method.

I started Fx with `browser.tabs.remote.autostart` set to `true` and did not get any errors or warnings in the terminal output. Not even for `getChromeWindow()`!

Is this the proper way to turn on e10s for all the things, not just browser tabs?

Comment 36

[Adam Roach [:abr]]

(In reply to Mike de Boer [:mikedeboer] from comment #33)
> (In reply to Justin Dolske [:Dolske] from comment #32)
> > Some quick comments from a skim. I'm not super familiar with indexdb, so
> > I'll have to come back to that again or defer, if you want a review
> > specifically on that angle.
> 
> Adam (abr) already reviewed those bits, so I think we're fine there.

Well, to be clear, I caveated my review with "I'm not a javascript expert, and definitely not an IndexedDB expert, so I'm going to rely on Justin to sanity check those aspects of the patch." We may want to track down an expert to give it a quick once-over.

Comment 37

[Mike de Boer [:mikedeboer]]

Thus, the conclusion I have to draw at this point is that this API injection technique bypasses e10s entirely, because the content is loaded in an iframe element and not in a <tab> and/ or <browser> element.

Comment 38

[Mike de Boer [:mikedeboer]]

(In reply to Adam Roach [:abr] from comment #36)
> Well, to be clear, I caveated my review with "I'm not a javascript expert,
> and definitely not an IndexedDB expert, so I'm going to rely on Justin to
> sanity check those aspects of the patch." We may want to track down an
> expert to give it a quick once-over.

Alright, thanks for mentioning that. I'll ask Marco (mak) for review once we're getting close.

Comment 39

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mike de Boer [:mikedeboer] from comment #35)
> Is this the proper way to turn on e10s for all the things, not just browser
> tabs?

That pref makes browser tabs launch with remote=true, it won't affect other content frames (e.g. social or loop panels).

It sounds like we need (at least) a bug on making this work properly longer-term when we'll want to make all content out-of-process.

Comment 40

[Justin Dolske [:Dolske]]

Comment on attachment 8465285 [details] [diff] [review]
Patch 1.4: add a contacts API

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

r+, with:

  1) Date format change
  2) Someone looking at general IndexedDB usage
  3) Someone (khuey/bholley/mrbkap) more familiar with content/chrome security bits looking at the safety of the MozLoopAPI.jsm changes.

::: browser/components/loop/LoopContacts.jsm
@@ +111,5 @@
> +        return;
> +      }
> +
> +      let contact = extend({}, details);
> +      let now = (new Date()).toISOString();

Storing dates in string formats like this all too often ends up being a minefield, and requires inefficient conversions when you actually want to use them (eg to show something in the user's local TZ or do do comparisons/math on them). I dinged Paolo for this in bug 853549 too. :)

Please store these as simple Unix-epoch offsets -- Date.now().

If this is really difficult due to interactions with the Loop architecture linked above we can reconsider, but I'd strongly prefer not to.

::: browser/components/loop/LoopStorage.jsm
@@ +191,5 @@
> +  getTransaction: function(store, mode = "readonly", callback) {
> +    if (arguments.length === 2) {
> +      callback = mode;
> +      mode = "readonly"
> +    }

Is there a reason for supporting this? I think it would be strongly preferable to always have |callback| be the (required) 2nd arg, with mode being an optional 3rd arg... As is, it gets really confusing to use if you end up adding another arg.

(Alternatively, using a object helps avoid the ordering issue -- getTransaction({store: foo, mode: blah, callback: cb}). I vaguely recall there are some new ES6 gizmos to sweeten the syntax/usage.)

@@ +215,5 @@
> +  getStore: function(store, mode = "readonly", callback) {
> +    if (arguments.length === 2) {
> +      callback = mode;
> +      mode = "readonly"
> +    }

Ditto.

Comment 41

[Adam Roach [:abr]]

(In reply to Justin Dolske [:Dolske] from comment #40)
> Storing dates in string formats like this all too often ends up being a
> minefield, and requires inefficient conversions when you actually want to
> use them (eg to show something in the user's local TZ or do do
> comparisons/math on them). I dinged Paolo for this in bug 853549 too. :)
> 
> Please store these as simple Unix-epoch offsets -- Date.now().

Mea culpa; this is what I show in my example. In practice, this just needs to be something convertible to a Date. So using seconds since the epoch is perfectly fine (and preferable for the reasons you cite).

Comment 42

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.5: add a contacts API

Bobby, could you please check the safety implications of the changes I made in MozLoopAPI.jsm?

Marco, could you please review the IndexedDB code?

Comment 43

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.4: tests

Update to use Unix-epoch offset timestamps. Carrying over r=abr.

Comment 44

[Bobby Holley (:bholley)]

This isn't part of the code being reviewed, but:

>  targetWindow.navigator.wrappedJSObject.__defineGetter__("mozLoop", function() {
>    // We do this in a getter, so that we create these objects
>    // only on demand (this is a potential concern, since
>    // otherwise we might add one per iframe, and keep them
>    // alive for as long as the window is alive).
>    delete targetWindow.navigator.wrappedJSObject.mozLoop;
>    return targetWindow.navigator.wrappedJSObject.mozLoop = contentObj;
>  });

I'm not convinced this is doing what it thinks it's doing. With this setup, the iframe holds alive the lazy getter closure, which itself holds alive |contentObj|. Is there something I'm missing, or do we need to get a bug on file to fix this?

Comment 45

[Adam Roach [:abr]]

(In reply to Bobby Holley (:bholley) from comment #44)
> This isn't part of the code being reviewed, but:
> 
> >  targetWindow.navigator.wrappedJSObject.__defineGetter__("mozLoop", function() {
> >    // We do this in a getter, so that we create these objects
> >    // only on demand (this is a potential concern, since
> >    // otherwise we might add one per iframe, and keep them
> >    // alive for as long as the window is alive).
> >    delete targetWindow.navigator.wrappedJSObject.mozLoop;
> >    return targetWindow.navigator.wrappedJSObject.mozLoop = contentObj;
> >  });
> 
> I'm not convinced this is doing what it thinks it's doing. With this setup,
> the iframe holds alive the lazy getter closure, which itself holds alive
> |contentObj|. Is there something I'm missing, or do we need to get a bug on
> file to fix this?

It's not necessarily an assertion that it's *correct*, but I will point out that this is code that Loop inherited from Social: http://dxr.mozilla.org/mozilla-central/source/toolkit/components/social/MozSocialAPI.jsm#210

If HG is to be trusted, this is originally Gavin's code; I'm moving the getinfo over to him.

Comment 46

[Bobby Holley (:bholley)]

Comment on attachment 8468310 [details] [diff] [review]
Patch 1.5: add a contacts API

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

::: browser/components/loop/MozLoopAPI.jsm
@@ +29,5 @@
> +  let obj = {};
> +  for (let prop of Object.getOwnPropertyNames(error)) {
> +    obj[prop] = String(error[prop]);
> +  }
> +  return obj;

How about doing |new targetWindow.Error(error.message);| and then copying over the rest of the error properties? Or alternatively, just returning simple Object instances to begin with? This seems like the worst of both worlds.

I've been considering adding an option to Cu.cloneInto to pass a JS-implemented callback to handle things that the structured clone algorithm can't handle, which would make things prettier here.

I'd also take a patch to just add Error support to Cu.cloneInto.

@@ +48,5 @@
> +      enumerable: true,
> +      writable: true,
> +      value: function(...params) {
> +        let callback = params.pop();
> +        api[func](...params, function(...results) {

Hm, what are |params| expected to be? Are they primitives, simple objects, or something else? How smart is the API code about dealing with tricky inputs? Note that our new JS Xray wrappers make this a lot safer than it used to be, but it's still worth being clear on the potential threats.

@@ +65,5 @@
> +      }
> +    };
> +  });
> +
> +  let contentObj = Cu.createObjectIn(targetWindow);

you don't need createObjectIn anymore, unless you want the { defineAs: ... } stuff - you can just do |new targetWindow.Object()|. But this isn't actually needed at all - see below.

@@ +68,5 @@
> +
> +  let contentObj = Cu.createObjectIn(targetWindow);
> +  Object.defineProperties(contentObj, propsMap);
> +  Object.seal(contentObj);
> +  Cu.makeObjectPropsNormal(contentObj);

I think this should all look something like this:

let injectedAPI = {};
for (key in api) {
  injectedAPI[key] = function(...params) {...};
}
let contentObj = Cu.cloneInto(api, {cloneFunctions: true});
Object.seal(contentObj);
return contentObj;

Comment 47

[Mike de Boer [:mikedeboer]]

(In reply to Bobby Holley (:bholley) from comment #46)
> Hm, what are |params| expected to be? Are they primitives, simple objects,
> or something else? How smart is the API code about dealing with tricky
> inputs? Note that our new JS Xray wrappers make this a lot safer than it
> used to be, but it's still worth being clear on the potential threats.

They are primitives and simple objects, nothing else. The API doesn't deal with inputs like validating the fields in the simple object case, but I could implement that to make it safer.

I looked at adding support for Error objects in ExportHelpers.cpp, but I'm afraid it's a little too much out of my comfort zone atm. I *think* I'd need a bit of your help for that :)

I'm happy I asked you for a review, because this is awesome feedback! Thanks!

Comment 48

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.6: add a contacts API

Bobby, does this look better? Note that I had to remove the `Object.seal` call, because that threw an error after `Cu.cloneInto`.

I also added object validation in the LoopContacts class that will yield errors if erroneous objects are passed, so no garbage will ever be stored in the database.

Comment 49

[Bobby Holley (:bholley)]

Comment on attachment 8469210 [details] [diff] [review]
Patch 1.6: add a contacts API

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

Per discussion elsewhere, I think you should clone the content object into the chrome's global before doing any input validation.

::: browser/components/loop/LoopContacts.jsm
@@ +213,5 @@
> + * @oaram {Object} def The definition of properties to validate against. Defaults
> + *                     to `kContactFields`
> + */
> +const validateContact = function(obj, def = kContactFields) {
> +  for (let propName in obj) {

This is going to miss non-enumerable properties. You should use Object.getOwnPropertyNames.

::: browser/components/loop/MozLoopAPI.jsm
@@ +33,5 @@
> +      api[func](...params, function(...results) {
> +        results = results.map(result => {
> +          if (result && typeof result == "object") {
> +            // Inspect for an error this way, because the Error object is special.
> +            if (Object.prototype.toString.call(result) == "[object Error]") {

I'd just do if (result.constructor.name == "Error")

@@ +34,5 @@
> +        results = results.map(result => {
> +          if (result && typeof result == "object") {
> +            // Inspect for an error this way, because the Error object is special.
> +            if (Object.prototype.toString.call(result) == "[object Error]") {
> +              return new targetWindow.Error(result.message)

You also need to copy over the other fields - stack, lineNumber, columnNumber, and fileName. Otherwise they'll end up referring to the code right here.

@@ +45,5 @@
> +      });
> +    };
> +  });
> +
> +  let contentObj = Cu.cloneInto(injectedAPI, targetWindow, {cloneFunctions: true});

We deny preventExtensions on XrayWrappers because Xray semantics make it difficult to act like an object has actually been frozen.

You can still do the seal call - just do try { Object.seal(Cu.waiveXrays(contentObj)) } catch (e) {}. This is safe because you're not getting properties, so the worst thing that content could do would be to _maybe_ cause an exception to be thrown.

Comment 50

[:Gavin Sharp [email: [e-mail]]]

(In reply to Adam Roach [:abr] from comment #45)
> If HG is to be trusted, this is originally Gavin's code; I'm moving the
> getinfo over to him.

I've got nothing - it does look broken, getting a bug on file sounds good.

Comment 51

[Adam Roach [:abr]]

Bobby -- I'm not sure what information you're seeking here. Can you clarify?

Comment 52

[Bobby Holley (:bholley)]

(In reply to Adam Roach [:abr] from comment #51)
> Bobby -- I'm not sure what information you're seeking here. Can you clarify?

I am asking you to file a bug on the issue in comment 44, because you bounced the needinfo to gavin in comment 45, and gavin indicated in comment 50 that the code is probably wrong.

Comment 53

[Bobby Holley (:bholley)]

Oh, and please add [Memshrink] in the whiteboard so that we can get this on their radar. :-)

Comment 54

[Marco Bonardo [:mak]]

Comment on attachment 8469210 [details] [diff] [review]
Patch 1.6: add a contacts API

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

it looks mostly good, I have some questions and suggestions, sorry if it's a bit long.

::: browser/components/loop/LoopContacts.jsm
@@ +8,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "console",
> +                                  "resource://gre/modules/devtools/Console.jsm");
> +const {LoopStorage} = Cu.import("resource:///modules/loop/LoopStorage.jsm", {});
> +const {EventEmitter} = Cu.import("resource://gre/modules/devtools/event-emitter.js", {});

might you make these lazy?
What I'd like to avoid is Firefox, or an add-on, including modules for Loop but actually not needing some parts of them until used.

@@ +9,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "console",
> +                                  "resource://gre/modules/devtools/Console.jsm");
> +const {LoopStorage} = Cu.import("resource:///modules/loop/LoopStorage.jsm", {});
> +const {EventEmitter} = Cu.import("resource://gre/modules/devtools/event-emitter.js", {});
> +const eventEmitter = new EventEmitter();

as well as making this lazy... probably you could make eventEmitter lazy and import the module only inside the getter.

@@ +21,5 @@
> +const kFieldTypeString = "string";
> +const kFieldTypeNumber = "number";
> +const kFieldTypeNumberOrString = "number|string";
> +const kFieldTypeArray = "array";
> +const kFieldTypeBool = "boolean";

honestly I don't see usefulness of these const, they just seem to rename something that would already be clear and short by itself.

@@ +25,5 @@
> +const kFieldTypeBool = "boolean";
> +const kContactFields = {
> +  "id": {
> +    type: kFieldTypeNumberOrString
> +  },

might you please add a small comment above each field explaining what they are used for. while id is clear, for example I wonder why we need both published and updated and what other fields are for. who reads this code should know what each field is for and how it's intended to be used.

@@ +30,5 @@
> +  "published": {
> +    type: kFieldTypeNumberOrString
> +  },
> +  "updated": {
> +    type: kFieldTypeNumberOrString

apart these dates (is published really needed?), we will need to setup a frecency algorithm, for that we need a frecency field, the last time a contact has been "used" and how many times it has been used

@@ +33,5 @@
> +  "updated": {
> +    type: kFieldTypeNumberOrString
> +  },
> +  "bday": {
> +    type: kFieldTypeNumberOrString

why are all of these number or string, rather than a well specified type? updated and bday look like dates, considered the precision you need for them, you might store seconds from the epoch.

@@ +38,5 @@
> +  },
> +  "blocked": {
> +    type: kFieldTypeBool
> +  },
> +  "adr": {

location?

@@ +41,5 @@
> +  },
> +  "adr": {
> +    type: kFieldTypeArray,
> +    contains: {
> +      "countryName": {

why not just country?

@@ +44,5 @@
> +    contains: {
> +      "countryName": {
> +        type: kFieldTypeString
> +      },
> +      "locality": {

and city. I think it's more important for these to be easily readable by developers than exact for any possible contents.

@@ +50,5 @@
> +      },
> +      "postalCode": {
> +        type: kFieldTypeString
> +      },
> +      "pref": {

pref looks confusing in our code, maybe fav, or default

@@ +56,5 @@
> +      },
> +      "region": {
> +        type: kFieldTypeString
> +      },
> +      "streetAddress": {

just street?

@@ +80,5 @@
> +        type: kFieldTypeString
> +      }
> +    }
> +  },
> +  "tel": {

phone?

@@ +91,5 @@
> +        type: kFieldTypeArray,
> +        contains: kFieldTypeString
> +      },
> +      "value": {
> +        type: kFieldTypeString

I wonder if you want to keep the phone country apart (for the +NN prefix)

@@ +107,5 @@
> +  "givenName": {
> +    type: kFieldTypeArray,
> +    contains: kFieldTypeString
> +  },
> +  "additionalName": {

is this for middle name?

@@ +119,5 @@
> +  "honorificSuffix": {
> +    type: kFieldTypeArray,
> +    contains: kFieldTypeString
> +  },
> +  "category": {

group?

@@ +127,5 @@
> +  "org": {
> +    type: kFieldTypeArray,
> +    contains: kFieldTypeString
> +  },
> +  "jobTitle": {

I wonder if instead of org and job title wouldn't be simpler to have a single freeform job field

@@ +144,5 @@
> +  }
> +
> +  // Create the 'contacts' store as it doesn't exist yet.
> +  let store = db.createObjectStore(kObjectStoreName, {
> +    keyPath: "_id",

so, if _id is the local primary key, what is the "id" you have in the schema? is it a global guid in the whole loop system? maybe it should be named like that if so.

@@ +147,5 @@
> +  let store = db.createObjectStore(kObjectStoreName, {
> +    keyPath: "_id",
> +    autoIncrement: true
> +  });
> +  store.createIndex(kServiceIdIndex, kServiceIdIndex, {unique: false});

why not unique? is not this guaranteed unique?

I think we need some more indices here, from the beginning.
lastUsed, email, familyName and name look like the minimum set based on mockups.
I think email should be a unique index.

@@ +191,5 @@
> + * @param {Object} target The target object to receive properties defined in `source`
> + * @param {Object} source The source object to copy properties from
> + */
> +const extend = function(target, source) {
> +  for (let key in source) {

Object.getOwnPropertyNames

@@ +306,5 @@
> +
> +      let contact = extend({}, details);
> +      let now = Date.now();
> +      contact.published = contact.published ? new Date(contact.published).getTime() : now;
> +      contact.updated = contact.updated ? new Date(contact.updated).getTime() : now;

is milliseconds precision really useful? I think seconds is more than enough and can be a perf win

@@ +308,5 @@
> +      let now = Date.now();
> +      contact.published = contact.published ? new Date(contact.published).getTime() : now;
> +      contact.updated = contact.updated ? new Date(contact.updated).getTime() : now;
> +      // Internal properties to keep metadata around for future reference.
> +      contact._date_add = contact._date_lch = now;

how do these differ from published and updated? are we storing them in the store?

@@ +513,5 @@
> +   *                            finished. The first argument passed will be an
> +   *                            `Error` object or `null`. The second argument will
> +   *                            be an `Array` of contact objects, if successfull.
> +   */
> +  getAll: function(callback) {

there's no way to get them sorted, I think you might want an optional index so get results sorted by one of the predefined indices.

I think it would be quite handy (and more performant) also to be able to limit the number or results without having to get them all and then crop.

@@ +695,5 @@
> +   *                            finished. The first argument passed will be an
> +   *                            `Error` object or `null`. The second argument will
> +   *                            be an `Array` of contact objects, if successfull.
> +   */
> +  search: function(query, callback) {

would also be handy to be able to provide an index and limit to search

I wonder if for APIs returning multiple items would not be nicer to have an onResult callback that can cancel the iteration, rather than a single callback getting all results at once. Might allow to build a more responsive UI.

::: browser/components/loop/LoopStorage.jsm
@@ +7,5 @@
> +
> +Cu.importGlobalProperties(["indexedDB"]);
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +const {EventEmitter} = Cu.import("resource://gre/modules/devtools/event-emitter.js", {});

is it possible to make this lazy?

@@ +42,5 @@
> + *                          established. It takes an Error object as first argument
> + *                          and the database connection object as second argument,
> + *                          if successful.
> + */
> +const openDatabase = function(onOpen) {

nit: considering the way this works, naming it ensureDatabaseOpen would be more consistent

I'd probably add protection to avoid allowing to open a database after closeDatabase has already been invoked, it should return Error, null

@@ +66,5 @@
> +
> +  let openRequest = indexedDB.open(kDatabaseName, kDatabaseVersion);
> +
> +  openRequest.onblocked = function(event) {
> +    invokeCallbacks(new Error("Database access blocked: " + event.target.error));

I'd say "Database cannot be upgraded cause in use: "

@@ +92,5 @@
> +
> +/**
> + * Start a transaction on the loop database and return it.
> + *
> + * Note: we can't use a Promise here, as they are resolved after a spin of the

these might be moved to a @note

@@ +167,5 @@
> +   *                            is established. It takes an Error object as first
> +   *                            argument and the database connection object as
> +   *                            second argument, if successful.
> +   */
> +  getInstance: function(callback) {

well, maybe getSingleton :)

@@ +193,5 @@
> +  },
> +
> +  /**
> +   * Start a transaction on the loop database and return the store with the name
> +   * `store`.

"return the requested store." imo it's confusing " name 'store' "

@@ +215,5 @@
> +
> +  /**
> +   * Perform an async function in serial on each of the list items.
> +   * IMPORTANT: only use this iteration method if you are sure that the operations
> +   * performed in `onItem` are guaranteed to be async in the success case.

I think this is worth a better comment, why async?
But looking at it sounds like it's async in the sense the consumer must invoke a callback once done... This comment should clarify that.

Even if you can't use promises in the common case, I think it might be possible to pass a deferred to the consumer callback, instead of passing an handler, that could be a cleaner api for the consumer. Though I didn't check callpoints so I let you evaluate whether it's worth it or not.

@@ +233,5 @@
> +  asyncForEach: function(list, onItem, onDone) {
> +    let i = 0;
> +    let len = list.length;
> +
> +    if (!len) {

I think this should be considered an argument error and the method should throw... why invoke this with an empty list?

@@ +256,5 @@
> +
> +  /**
> +   * Perform an async function in parallel on each of the list items.
> +   * IMPORTANT: only use this iteration method if you are sure that the operations
> +   * performed in `onItem` are guaranteed to be async in the success case.

ditto, see asyncForEach

in this case Promise.all, would also greatly simplify this code if the consumer gets a deferred.

@@ +276,5 @@
> +    let done = 0;
> +    let callbackCalled = false;
> +    let len = list.length;
> +
> +    if (!len) {

ditto for argument error

@@ +305,5 @@
> +  on: (...params) => eventEmitter.on(...params),
> +
> +  off: (...params) => eventEmitter.off(...params)
> +};
> +Object.freeze(this.LoopStorage);

nit: you might just
this.LoopStorage = Object.freeze({
...
});

Comment 55

[Adam Roach [:abr]]

(In reply to Marco Bonardo [:mak] (Away 15-31 Aug) from comment #54)
> Comment on attachment 8469210 [details] [diff] [review]
> Patch 1.6: add a contacts API
> 
> Review of attachment 8469210 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> it looks mostly good, I have some questions and suggestions, sorry if it's a
> bit long.

One quick reply on the topic of field naming and formats: to allow for future integration with the Contacts API and/or potential integration with contact synchronization across devices (including Firefox OS devices), we are using objects with properties having the same names and structure as those used by mozContact. In theory, to store these in the Contacts API database, all that would be required would be passing in the object as the argument to the mozContact constructor, and then storing it via navigator.mozContacts.save().

It would probably be worth mentioning this in a comment, and including a link to https://developer.mozilla.org/en-US/docs/Web/API/mozContact for a description of each field's purpose.

Comment 56

[Adam Roach [:abr]]

Filed Bug 1051883 (Loop API) and 1051885 (Social API) regarding issue raised in Comment 44.

Comment 57

[Marco Bonardo [:mak]]

(In reply to Adam Roach [:abr] from comment #55)
> One quick reply on the topic of field naming and formats: to allow for
> future integration with the Contacts API and/or potential integration with
> contact synchronization across devices (including Firefox OS devices), we
> are using objects with properties having the same names and structure as
> those used by mozContact.
> 
> It would probably be worth mentioning this in a comment, and including a
> link to https://developer.mozilla.org/en-US/docs/Web/API/mozContact for a
> description of each field's purpose.

Oh yes, that comment would have helped, especially if the format must be kept in sync across products.

Comment 58

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.7: add a contacts API

Thanks Bobby and Marco, for your continued efforts! And thanks Adam for replying to comments ;)

I tried to address all review comments in this patch. I hoped that worked out well.

Comment 59

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.5: tests

Carrying over r=abr.

Comment 60

[Marco Bonardo [:mak]]

Comment on attachment 8471486 [details] [diff] [review]
Patch 1.7: add a contacts API

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

I would have appreciated some more answers to comments, but most were not blocking things, so let's move on.

The main concern I have about this is that someone (like me) who didn't directly code on this will be very confused by the many different "id"s used here. There's id, guid, serviceId and no comment explaining what they are. Having some more comments, and naming variables in a coherent way (so that guid is always guid, serviceId is always serviceId), would help.

And I'd like to have a few other things explained here and where applicable code comments for future reference.

that said, I don't see anything really blocking this, so r=me, with these comments answered.

::: browser/components/loop/LoopContacts.jsm
@@ +17,5 @@
> +
> +this.EXPORTED_SYMBOLS = ["LoopContacts"];
> +
> +const kObjectStoreName = "contacts";
> +const kServiceIdIndex = "id";

could you please add a comment explaining what a service-id is, how it differs from id, how it is used? I think it would help clarifying from the beginning the difference and usage of service-id VS id.

@@ +37,5 @@
> +const kFieldTypeArray = "array";
> +const kFieldTypeBool = "boolean";
> +const kContactFields = {
> +  "id": {
> +    type: kFieldTypeNumberOrString

So, we figured where the names come from, but I still don't know why some fields are "number or string", instead of being well typed. Same reason? Please explain or even better, comment :)

@@ +231,5 @@
> + *                             an `Error` object or `null`. The second argument
> + *                             will be the `data` Array, if all operations finished
> + *                             successfully.
> + */
> +const batch = function(operation, data, callback) {

the javadoc contains a "mode" param that doesn't exist

would be nice to check operation for validity and throw more properly than with "LoopContactsInternal[operation] is not a function"

@@ +264,5 @@
> +  }
> +  return target;
> +};
> +
> +LoopStorage.on("upgrade", function(e, db) {

would it be possible to do this lazily in the LoopStorage getter, so that importing the module won't initialize the store yet?

@@ +274,5 @@
> +  let store = db.createObjectStore(kObjectStoreName, {
> +    keyPath: "_guid",
> +    autoIncrement: true
> +  });
> +  store.createIndex(kServiceIdIndex, kServiceIdIndex, {unique: false});

So, what about the other indices I suggested? Are we going to add them in a next iteration, or do we expect the number of contacts to be so small that a scan won't be that expensive?

Why is this service id index not unique? (I still have issues figuring what serviceId is going to contain, the source service?)

@@ +319,5 @@
> +      let now = Date.now();
> +      contact.published = contact.published ? new Date(contact.published).getTime() : now;
> +      contact.updated = contact.updated ? new Date(contact.updated).getTime() : now;
> +      // Internal properties to keep metadata around for future reference.
> +      contact._date_add = contact._date_lch = now;

these still need some comment explaining what "future reference" is. why can't we just use .published and .updated rather than these internals? The comment should tell which problem we are trying to solve

@@ +524,5 @@
> +   *                            finished. The first argument passed will be an
> +   *                            `Error` object or `null`. The second argument will
> +   *                            be an `Array` of contact objects, if successfull.
> +   */
> +  getAll: function(callback) {

so, what's the plan to add sorting and limit to .getAll and .search? Basically my question i similar to the indices case, whether we expect so few contacts that looping likely won't matter.

@@ +595,5 @@
> +   *                            be the contact object, if successfull.
> +   */
> +  update: function(details, callback) {
> +    if (!("_guid" in details)) {
> +      callback(new Error("No 'id' field present"));

unclear _guid VS id clash

@@ +605,5 @@
> +      callback(ex);
> +      return;
> +    }
> +
> +    let id = details._guid;

ditto... would be nice if the whole code used consistent naming for different entities, so when something says id, guid or serviceid, one can know what it refers to

@@ +623,5 @@
> +        let previous = extend({}, contact);
> +        // Update the contact with properties provided by `details`.
> +        extend(contact, details);
> +
> +        details._date_lch = Date.now();

why don't we update .updated instead? Are we supposed to trust the passed-in value? I'd not expect the caller to care about keeping .updated in sync, I would expect this code to do that...

Comment 61

[Maire Reavy [:mreavy]]

Bobby -- Thanks for fixing Bug 1050340!  Do you think you can re-review this today?  (I'd love to land this today or tomorrow.)

Comment 62

[Bobby Holley (:bholley)]

Comment on attachment 8471486 [details] [diff] [review]
Patch 1.7: add a contacts API

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

r=bholley on the security bits. Sorry for the delay.

Comment 63

[Adam Roach [:abr]]

I'm landing this for Michael while he's on PTO, and addressing your review comments. Where I haven't made the requested changes, I've replied inline.

(In reply to Marco Bonardo [:mak] (Away 15-31 Aug) from comment #60)


> would it be possible to do this lazily in the LoopStorage getter, so that
> importing the module won't initialize the store yet?

That won't work. From https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API/Using_IndexedDB : "onupgradeneeded is the only place where you can alter the structure of the database."
 
> @@ +274,5 @@
> > +  let store = db.createObjectStore(kObjectStoreName, {
> > +    keyPath: "_guid",
> > +    autoIncrement: true
> > +  });
> > +  store.createIndex(kServiceIdIndex, kServiceIdIndex, {unique: false});
> 
> So, what about the other indices I suggested? Are we going to add them in a
> next iteration, or do we expect the number of contacts to be so small that a
> scan won't be that expensive?

The way this ends up being used, the UI needs to load the entire list of entries and manipulate it locally. Unfortunately, the kinds of searches we need to do aren't possible with IndexedDB, and would require a shift to use SQL directly. It's far easier to have the UI iterate over the O(100 to 1000) entries and apply the search criteria that are appropriate for its current operation.


> @@ +524,5 @@
> > +   *                            finished. The first argument passed will be an
> > +   *                            `Error` object or `null`. The second argument will
> > +   *                            be an `Array` of contact objects, if successfull.
> > +   */
> > +  getAll: function(callback) {
> 
> so, what's the plan to add sorting and limit to .getAll and .search?
> Basically my question i similar to the indices case, whether we expect so
> few contacts that looping likely won't matter.

That's the plan, yes.

Comment 64

[Adam Roach [:abr]]

Attachment: add unit tests for LoopContacts crud operations.

Comment 65

[Adam Roach [:abr]]

Attachment: [Loop] add a contacts API.

Comment 66

[Adam Roach [:abr]]

Comment on attachment 8476341 [details] [diff] [review]
add unit tests for LoopContacts crud operations.

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

Update to match requested changes in accompanying patch. Carrying forward r=dmose

Comment 67

[Adam Roach [:abr]]

Comment on attachment 8476343 [details] [diff] [review]
[Loop] add a contacts API.

Addressed review comments. Carrying forward r=abr,dolske,bholley

Comment 68

[Adam Roach [:abr]]

https://hg.mozilla.org/integration/fx-team/rev/ea93d83f49b9
https://hg.mozilla.org/integration/fx-team/rev/b91d006131d3

Comment 69

[Adam Roach [:abr]]

Attachment: WIP patch: consumer of this API, showing full contacts list

Comment 70

[Adam Roach [:abr]]

Attachment: WIP patch: consumer of this API, showing full contacts list

Comment 71

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/mozilla-central/rev/ea93d83f49b9
https://hg.mozilla.org/mozilla-central/rev/b91d006131d3

Comment 72

[Adam Roach [:abr]]

NiKo`: When working on my contact imports patch, I noticed that the WIP patch attached to this bug bogs down in the react code so badly that it triggers the "this script is taking too long, do you want to kill it?" dialog.

I hope that this is just some minor oversight in the design of the demo patch, but I don't want it to be a surprise that keeps us from landing at the last minute -- if you could take a look at this and see if you can figure out what's going on, that would be awesome.

For reference, I was seeing the script timeout warnings after loading in approximately 850 contacts. Let me know if there's anything else you need to know...

Comment 73

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8476360 [details] [diff] [review]
WIP patch: consumer of this API, showing full contacts list

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

Here are a few hints for trying to improve performance a little. Basically I can see room for improvement in the way we handle state for each list entry — as they're rendered frequently, the less logic they contain, the faster they get rendered to the user.

Ideally, you would construct a ProfileDetail instance only using props; internal state would be used just to keep track of the dropdown menu state, for instance.

I'd totally see rebuilding this UI from scratch, iteratively, so we could check for performance degradation over time. Right now the code is hard to track. Another approach would be to disable features sequentially to isolate the source of the problem more precisely.

I could try to do this but I'm a bit busy on other fronts atm. But depending on the priority for this to land, I can totally switch to working on this if required. Your call.

::: browser/components/loop/content/js/contacts.jsx
@@ +58,5 @@
> +    toggleDropdown: function(){
> +      if (this.state.isDropdownVisible) {
> +        $(this.refs.callDropdown.getDOMNode()).hide();
> +      } else {
> +        $(this.refs.callDropdown.getDOMNode()).show();

Showing/hiding should be handled by state; one should always avoid touching the DOM by hand as it's 1. slow 2. hardly maintainable. jQuery is really not needed here.

Have a look at the dropdown menu component we used for the panel.

@@ +68,5 @@
> +    toggleBlocked: function(){
> +      this.props.user.blocked = !this.state.blocked;
> +      this.setState({
> +        blocked: !this.state.blocked
> +      });

This profile detail component does too much, and shouldn't imho rely on its own state changes. Rather, we should use props (read static state) so we could render a ProfileDetail component instance without bothering too much (which is faster btw).

Any action performed from the profile entry should rather trigger a function call attached as a prop by the parent, so the whole list can be rerendered using VDom diff, which is faster.

@@ +122,5 @@
> +        this.props.delegate.panelOpened();
> +      }
> +      $(this.refs.listSlider.getDOMNode()).velocity({
> +        translateX: "-50%"
> +      }, Defaults.animation);

We should try to disable any computed animation to see if it improves the performance situation. Btw in the future, we'll probably want to rely on pure css animations instead.

@@ +126,5 @@
> +      }, Defaults.animation);
> +    },
> +    closePanel: function(){
> +      if (this.props.delegate.hasOwnProperty("panelClosed")) {
> +        this.props.delegate.panelClosed();

I don't get where does this delegate prop comes from…

Comment 74

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Clearing needinfo.

Comment 75

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Side note, if we want virtual views to deal with very big lists at some point, this jsfiddle shows a really nice implementation http://jsfiddle.net/vjeux/KbWJ2/9/