Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: reuben, Assigned: reuben)

Tracking

Trunk
mozilla27
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:-)

Details

(Whiteboard: [systemsfe])

Attachments

(7 attachments, 17 obsolete attachments)

266 bytes, text/html
gwagner
: review+
Details
11.87 KB, patch
cpeterson
: review+
Details | Diff | Splinter Review
12.56 KB, patch
cpeterson
: review+
cpeterson
: feedback+
Details | Diff | Splinter Review
8.77 KB, patch
reuben
: review+
Details | Diff | Splinter Review
119.99 KB, patch
reuben
: review+
Details | Diff | Splinter Review
5.81 KB, patch
Details | Diff | Splinter Review
3.67 KB, patch
gwagner
: review+
Details | Diff | Splinter Review
Let's do this!
Depends on: 827486
Depends on: 851639
Depends on: 742206
Depends on: 585381
Depends on: 851726
Depends on: 852219
Depends on: 851178
Depends on: 856841
Depends on: 731746
Depends on: 863898
Depends on: 865544
Depends on: 865951
Depends on: 870219
Posted patch WIP (obsolete) — Splinter Review
No longer depends on: 851726
Boris, note the question in ContactManager.save. Can we do better than that?
Attachment #749533 - Attachment is obsolete: true
Attachment #760182 - Flags: review?(bzbarsky)
Posted patch Fix tests (obsolete) — Splinter Review
Fixes a bunch of silly things that the tests were doing.
Attachment #760183 - Flags: review?(anygregor)
Comment on attachment 760182 [details] [diff] [review]
Convert the Contacts API to WebIDL, v0

>+++ b/dom/contacts/ContactManager.js
>+ContactAddress.prototype = {
>+  initialize: function(aType, aStreetAddress, aLocality, aRegion, aPostalCode, aCountryName, aPref) {
>+    this.type = sanitizeStringArray(aType);

Please document in the IDL that once aType can become "(DOMString or sequence<DOMString>)" this sanitizeStringArray call can go away somewhat?  Also, file a bug to do that, depending on bug 767924?

>+    this.streetAddress = stringOrBust(aStreetAddress);
>+    this.locality = stringOrBust(aLocality);
>+    this.region = stringOrBust(aRegion);
>+    this.postalCode = stringOrBust(aPostalCode);
>+    this.countryName = stringOrBust(aCountryName);

Why are those stringOrBust bits needed?  I guess we allow passing in undefined for all these arguments?  If so, it's worth documenting that here.

>+ContactField.prototype = {
>+  initialize: function(aType, aValue, aPref) {
>+    this.type = sanitizeStringArray(aType);

Same here.  And for other interfaces in this file.

>+    this.value = stringOrBust(aValue);

And here.  And likewise for other interfaces.

> function checkBlobArray(aBlob) {
>   if (Array.isArray(aBlob)) {
>     for (let i = 0; i < aBlob.length; i++) {
>       if (typeof aBlob != 'object') {

I know this is preexisting, but doing this check on every loop iteration doesn't make any sense.

>   set email(aEmail) {
>     this._email = validateArrayField(aEmail, function(email) {

I somewhat question the usefulness of this given that the page can do:

  contact.email[5] = 17;

or whatever, so you can't rely on this._email holding ContactField objects anyway....  But I guess it was here before.

>+  toJSON: function() {

(Still for Contact.prototype here.)  This is called from content code, right?  Doesn't it need some exposedprops stuff?  Otherwise content will get an opaque object.... was this tested?

>+  set oncontactchange(aHandler) {
>+      if (!this._listeningForMessages) {
>         cpmm.sendAsyncMessage("Contacts:RegisterForMessages");

So this won't work if the page does addEventListener("contactchange", something), right?  Please file a followup on that?

>   get oncontactchange() {

So the set is async, right?  So if a web page does:

  function f() {}
  mgr.oncontactchange = f;
  alert(mgr.oncontactchange == f);

it will alert false, as far as I can tell?  That seems fairly weird to me, as APIs go....  Is this purposeful?

>+    // to deal with a mozContact object. Can we do something better than
>+    // copying over the properties by hand?

Not that I can think of....

>     let newContact = {};
>-    newContact.properties = {
>-      name:            [],
>-      honorificPrefix: [],
>-      givenName:       [],
>-      additionalName:  [],
>-      familyName:      [],
>-      honorificSuffix: [],
>-      nickname:        [],
>-      email:           [],
>-      photo:           [],
>-      url:             [],
>-      category:        [],
>-      adr:             [],
>-      tel:             [],
>-      org:             [],
>-      jobTitle:        [],
>-      bday:            null,
>-      note:            [],
>-      impp:            [],
>-      anniversary:     null,
>-      sex:             null,
>-      genderIdentity:  null
>-    };
>-    for (let field in newContact.properties) {
>-      newContact.properties[field] = aContact[field];
>-    }
>+
>+    // If we end up having to do this, these consts should be moved next to the
>+    // implementations.
>+    const PROPERTIES = [
>+      "name", "honorificPrefix", "givenName", "additionalName", "familyName",
>+      "honorificSuffix", "nickname", "photo", "category", "org", "jobTitle",
>+      "bday", "note", "anniversary", "sex", "genderIdentity"
>+    ];
>+    newContact.properties = {};
>+    for (let field of PROPERTIES) {
>+      if (aContact[field]) {
>+        newContact.properties[field] = aContact[field];
>+      }
>+    }
>+
>+    const ADDRESS_PROPERTIES = ["adr"];
>+    const FIELD_PROPERTIES = ["email", "url", "impp"];
>+    const TELFIELD_PROPERTIES = ["tel"];
>+
>+    for (let prop of ADDRESS_PROPERTIES) {
>+      if (aContact[prop]) {
>+        newContact.properties[prop] = [];
>+        for (let i of aContact[prop]) {
>+          newContact.properties[prop].push(ContactAddress.prototype.toJSON.apply(i));
>+        }
>+      }
>+    }
>+
>+    for (let prop of FIELD_PROPERTIES) {
>+      if (aContact[prop]) {
>+        newContact.properties[prop] = [];
>+        for (let i of aContact[prop]) {
>+          newContact.properties[prop].push(ContactField.prototype.toJSON.apply(i));
>+        }
>+      }
>+    }
>+
>+    for (let prop of TELFIELD_PROPERTIES) {
>+      if (aContact[prop]) {
>+        newContact.properties[prop] = [];
>+        for (let i of aContact[prop]) {
>+          newContact.properties[prop].push(ContactTelField.prototype.toJSON.apply(i));
>+        }
>+      }
>+    }
> 
>     let reason;
>     if (aContact.id == "undefined") {
>       // for example {25c00f01-90e5-c545-b4d4-21E2ddbab9e0} becomes
>       // 25c00f0190e5c545b4d421E2ddbab9e0
>-      aContact.id = this._getRandomId().replace('-', '', 'g').replace('{', '').replace('}', '');
>+      aContact.id = this._getRandomId().replace(/[{}-]/g, "");
>       reason = "create";
>     } else {
>       reason = "update";
>     }
> 
>-    this._setMetaData(newContact, aContact);
>-    if (DEBUG) debug("send: " + JSON.stringify(newContact));
>-    request = this.createRequest();
>+    newContact.id = aContact.id;
>+    newContact.published = aContact.published;
>+    newContact.updated = aContact.updated;
>+
>+    let request = this.createRequest();
>     let options = { contact: newContact, reason: reason };
>     let allowCallback = function() {
>       cpmm.sendAsyncMessage("Contact:Save", {requestID: this.getRequestId({request: request, reason: reason}), options: options});
>     }.bind(this)
>     this.askPermission(reason, request, allowCallback);
>     return request;
>   },
> 
>@@ -830,35 +739,33 @@ ContactManager.prototype = {
>         cpmm.sendAsyncMessage("Contacts:GetAll:SendNow", { cursorId: aCursorId });
>       }
>     } else {
>       if (DEBUG) debug("waiting for contact");
>       data.waitingForNext = true;
>     }
>   },
> 
>-  remove: function removeContact(aRecord) {
>-    let request;
>-    request = this.createRequest();
>+  remove: function(aRecord) {
>+    let request = this.createRequest();
>     let options = { id: aRecord.id };
>     let allowCallback = function() {
>       cpmm.sendAsyncMessage("Contact:Remove", {requestID: this.getRequestId({request: request, reason: "remove"}), options: options});
>-    }.bind(this)
>+    }.bind(this);
>     this.askPermission("remove", request, allowCallback);
>     return request;
>   },
> 
>   clear: function() {
>     if (DEBUG) debug("clear");
>-    let request;
>-    request = this.createRequest();
>+    let request = this.createRequest();
>     let options = {};
>     let allowCallback = function() {
>       cpmm.sendAsyncMessage("Contacts:Clear", {requestID: this.getRequestId({request: request, reason: "remove"}), options: options});
>-    }.bind(this)
>+    }.bind(this);
>     this.askPermission("remove", request, allowCallback);
>     return request;
>   },
> 
>   getRevision: function() {
>     let request = this.createRequest();
> 
>     let allowCallback = function() {
>@@ -898,27 +805,16 @@ ContactManager.prototype = {
>                               "Contact:Save:Return:OK", "Contact:Save:Return:KO",
>                               "Contact:Remove:Return:OK", "Contact:Remove:Return:KO",
>                               "Contact:Changed",
>                               "PermissionPromptHelper:AskPermission:OK",
>                               "Contacts:GetAll:Next", "Contacts:Revision",
>                               "Contacts:Count"]);
>   },
> 
>-  // Called from DOMRequestIpcHelper
>-  uninit: function uninit() {
>-    if (DEBUG) debug("uninit call");
>-    if (this._oncontactchange)
>-      this._oncontactchange = null;
>-  },
>-
>-  classID : CONTACTMANAGER_CID,
>-  QueryInterface : XPCOMUtils.generateQI([nsIDOMContactManager, Ci.nsIDOMGlobalPropertyInitializer]),
>-
>-  classInfo : XPCOMUtils.generateCI({classID: CONTACTMANAGER_CID,
>-                                     contractID: CONTACTMANAGER_CONTRACTID,
>-                                     classDescription: "ContactManager",
>-                                     interfaces: [nsIDOMContactManager],
>-                                     flags: nsIClassInfo.DOM_OBJECT})
>-}
>+  classID: Components.ID("{8beb3a66-d70a-4111-b216-b8e995ad3aff}"),
>+  contractID: "@mozilla.org/contactManager;1",
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsISupports,
>+                                         Ci.nsIDOMGlobalPropertyInitializer]),
>+};
> 
> this.NSGetFactory = XPCOMUtils.generateNSGetFactory(
>-                       [Contact, ContactManager, ContactProperties, ContactAddress, ContactField, ContactTelField, ContactFindSortOptions, ContactFindOptions])
>+                       [Contact, ContactManager, ContactAddress, ContactField, ContactTelField]);
>diff --git a/dom/contacts/ContactManager.manifest b/dom/contacts/ContactManager.manifest
>--- a/dom/contacts/ContactManager.manifest
>+++ b/dom/contacts/ContactManager.manifest
>@@ -1,25 +1,14 @@
>-component {6cb78b21-4218-414b-8a84-3b7bf0088b34} ContactManager.js
>-contract @mozilla.org/contactProperties;1 {6cb78b21-4218-414b-8a84-3b7bf0088b34}
>-
> component {9cbfa81c-bcab-4ca9-b0d2-f4318f295e33} ContactManager.js
> contract @mozilla.org/contactAddress;1 {9cbfa81c-bcab-4ca9-b0d2-f4318f295e33}
> 
> component {ad19a543-69e4-44f0-adfa-37c011556bc1} ContactManager.js
> contract @mozilla.org/contactField;1 {ad19a543-69e4-44f0-adfa-37c011556bc1}
> 
> component {4d42c5a9-ea5d-4102-80c3-40cc986367ca} ContactManager.js
> contract @mozilla.org/contactTelField;1 {4d42c5a9-ea5d-4102-80c3-40cc986367ca}
> 
>-component {0a5b1fab-70da-46dd-b902-619904d920c2} ContactManager.js
>-contract @mozilla.org/contactFindSortOptions;1 {0a5b1fab-70da-46dd-b902-619904d920c2}
>-
>-component {28ce07d0-45d9-4b7a-8843-521df4edd8bc} ContactManager.js
>-contract @mozilla.org/contactFindOptions;1 {28ce07d0-45d9-4b7a-8843-521df4edd8bc}
>-
> component {72a5ee28-81d8-4af8-90b3-ae935396cc66} ContactManager.js
> contract @mozilla.org/contact;1 {72a5ee28-81d8-4af8-90b3-ae935396cc66}
>-category JavaScript-global-constructor mozContact @mozilla.org/contact;1
> 
> component {8beb3a66-d70a-4111-b216-b8e995ad3aff} ContactManager.js
> contract @mozilla.org/contactManager;1 {8beb3a66-d70a-4111-b216-b8e995ad3aff}
>-category JavaScript-navigator-property mozContacts @mozilla.org/contactManager;1
>diff --git a/dom/contacts/fallback/ContactDB.jsm b/dom/contacts/fallback/ContactDB.jsm
>--- a/dom/contacts/fallback/ContactDB.jsm
>+++ b/dom/contacts/fallback/ContactDB.jsm
>@@ -513,37 +513,28 @@ ContactDB.prototype = {
>               if (typeof val == "string") {
>                 contact.search[field].push(val.toLowerCase());
>               }
>             }
>           }
>         }
>       }
>     }
>-    if (DEBUG) debug("contact:" + JSON.stringify(contact));
> 
>     contact.updated = aContact.updated;
>     contact.published = aContact.published;
>     contact.id = aContact.id;
> 
>     return contact;
>   },
> 
>   makeExport: function makeExport(aRecord) {
>-    let contact = {};
>-    contact.properties = aRecord.properties;
>-
>-    for (let field in aRecord.properties)
>-      contact.properties[field] = aRecord.properties[field];
>-
>-    contact.updated = aRecord.updated;
>-    contact.published = aRecord.published;
>-    contact.id = aRecord.id;
>-    return contact;
>-  },
>+    delete aRecord.search;
>+    return aRecord;
>+  },
> 
>   updateRecordMetadata: function updateRecordMetadata(record) {
>     if (!record.id) {
>       Cu.reportError("Contact without ID");
>     }
>     if (!record.published) {
>       record.published = new Date();
>     }
>diff --git a/dom/interfaces/contacts/moz.build b/dom/interfaces/contacts/moz.build
>--- a/dom/interfaces/contacts/moz.build
>+++ b/dom/interfaces/contacts/moz.build
>@@ -1,18 +1,16 @@
> # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> # vim: set filetype=python:
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> XPIDL_SOURCES += [
>-    'nsIContactProperties.idl',
>-    'nsIDOMContactManager.idl',
>-    'nsIDOMMozContactChangeEvent.idl',
>+    'nsIDOMMozContactChangeEvent.idl'
> ]
> 
> XPIDL_MODULE = 'dom_contacts'
> 
> XPIDL_FLAGS += [
>     '-I$(topsrcdir)/dom/interfaces/base',
>     '-I$(topsrcdir)/dom/interfaces/events',
> ]

>diff --git a/dom/webidl/Contacts.webidl b/dom/webidl/Contacts.webidl

>+interface ContactAddress {
>+  attribute any        type; // DOMString[]

If it's really an object all the time, why "any" and not "object"?

Should this interface have a toJSON on it so that JSON.stringify works?  Then you wouldn't need to special-case it for the IPC communication stuff too.

>+dictionary ContactAddressInit {
>+ sequence<DOMString>? type;
>+ DOMString?           streetAddress;
>+ DOMString?           locality;
>+ DOMString?           region;
>+ DOMString?           postalCode;
>+ DOMString?           countryName;
>+ boolean?             pref;
>+};

Why are those nullable?  What's the use case for passing { pref: null } or { pref: undefined } and having it be treated like "pref" is not set (which afaict is what your code does)?

Similar for the other dictionaries here.

>+interface ContactTelField : ContactField {
>+  void initialize(optional any type,

Document what it should really be?

>+interface mozContact {

Lots of "any" members here that should also be documented.

>+dictionary ContactFindSortOptions {
>+  DOMString sortBy;                    // "givenName" or "familyName"

If those are the only two options, that should be a WebIDL enum, I would think.

>+  DOMString sortOrder = "ascending";   // e.g. "descending"

Likewise.

>+dictionary ContactFindOptions : ContactFindSortOptions {
>+  DOMString      filterOp;     // e.g. "startsWith"

And this?

>+  any            filterBy;     // e.g. ["givenName", "nickname"]

I assume this is reasonably free-form so can't be an enum?

r=me with the above fixed.  Maybe add some tests for the EventTarget bits too?
Attachment #760182 - Flags: review?(bzbarsky) → review+
Comment on attachment 760183 [details] [diff] [review]
Fix tests

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

::: dom/contacts/tests/test_contacts_basics.html
@@ +353,3 @@
>        // Some manual testing. Testint the testfunctions
>        // tel: [{type: ["work"], value: "123456", carrier: "testCarrier"} , {type: ["home", "fax"], value: "+55 (31) 9876-3456"}],
> +      Math.sin(findResult1.tel[0]);

Remove :)

@@ -1370,5 @@
> -        nickname: [8, 9],
> -        org: [10, 11],
> -        jobTitle: [12, 13],
> -        note: 14,
> -        category: [15, 16],

Why do you remove this?

::: dom/contacts/tests/test_contacts_blobs.html
@@ -270,5 @@
> -      next();
> -    };
> -    req.onerror = onFailure;
> -  },
> -  function () {

Why do you remove this?
(In reply to Gregor Wagner [:gwagner] from comment #5)
> @@ -1370,5 @@
> > -        nickname: [8, 9],
> > -        org: [10, 11],
> > -        jobTitle: [12, 13],
> > -        note: 14,
> > -        category: [15, 16],
> 
> Why do you remove this?

Because they're just converted to strings now.

> ::: dom/contacts/tests/test_contacts_blobs.html
> @@ -270,5 @@
> > -      next();
> > -    };
> > -    req.onerror = onFailure;
> > -  },
> > -  function () {
> 
> Why do you remove this?

They all throw now, since a string is a not a Blob.
Attachment #760183 - Flags: review?(anygregor) → review+
Thanks for the review! Comments inline:

(In reply to Boris Zbarsky (:bz) from comment #4)
> Comment on attachment 760182 [details] [diff] [review]
> Convert the Contacts API to WebIDL, v0
> 
> >+++ b/dom/contacts/ContactManager.js
> >+ContactAddress.prototype = {
> >+  initialize: function(aType, aStreetAddress, aLocality, aRegion, aPostalCode, aCountryName, aPref) {
> >+    this.type = sanitizeStringArray(aType);
> 
> Please document in the IDL that once aType can become "(DOMString or
> sequence<DOMString>)" this sanitizeStringArray call can go away somewhat? 
> Also, file a bug to do that, depending on bug 767924?

This can actually go away right now, since I can just make the argument a sequence<DOMString>. (It doesn't have to be an union).

> >+    this.streetAddress = stringOrBust(aStreetAddress);
> >+    this.locality = stringOrBust(aLocality);
> >+    this.region = stringOrBust(aRegion);
> >+    this.postalCode = stringOrBust(aPostalCode);
> >+    this.countryName = stringOrBust(aCountryName);
> 
> Why are those stringOrBust bits needed?  I guess we allow passing in
> undefined for all these arguments?  If so, it's worth documenting that here.

They're not, I removed those.

> >   set email(aEmail) {
> >     this._email = validateArrayField(aEmail, function(email) {
> 
> I somewhat question the usefulness of this given that the page can do:
> 
>   contact.email[5] = 17;
> 
> or whatever, so you can't rely on this._email holding ContactField objects
> anyway....  But I guess it was here before.

Well, as long as it turns into a ContactField on the way back (from a find() call, for example), it's fine. The main problem is that if I don't create those interfaces on the way back, content sees the objects in the array as a COW, and can't access any of the properties inside it. (Maybe this is a bug?).

The secondary problem is that not doing any validation would allow users to pass in garbage, that will then be passed over to ContactDB, and that could die in the parent, breaking every other app that uses it…
I wonder if it's worth doing a stronger validation for those array properties (defining setters for the array values), since that would make us slower.

> >+  toJSON: function() {
> 
> (Still for Contact.prototype here.)  This is called from content code,
> right?  Doesn't it need some exposedprops stuff?  Otherwise content will get
> an opaque object.... was this tested?

It does need the exposedProps stuff, I was only testing from chrome code.

> >   get oncontactchange() {
> 
> So the set is async, right?  So if a web page does:
> 
>   function f() {}
>   mgr.oncontactchange = f;
>   alert(mgr.oncontactchange == f);
> 
> it will alert false, as far as I can tell?  That seems fairly weird to me,
> as APIs go....  Is this purposeful?

Yes. No, this is just a byproduct of not having a good way to do the permission check here, I suspect other JS implemented APIs with events will suffer from the same problem.
We could check the permission synchronously on the constructor, but in any case listeners added with addEventListener will just fail silently.

> If it's really an object all the time, why "any" and not "object"?

Good idea.

> Should this interface have a toJSON on it so that JSON.stringify works? 
> Then you wouldn't need to special-case it for the IPC communication stuff
> too.

For some reason, __exposedProps__ objects are also stringified when you stringify those objects from chrome.
I'll file a follow up to look into that.

> >+dictionary ContactAddressInit {
> >+ sequence<DOMString>? type;
> >+ DOMString?           streetAddress;
> >+ DOMString?           locality;
> >+ DOMString?           region;
> >+ DOMString?           postalCode;
> >+ DOMString?           countryName;
> >+ boolean?             pref;
> >+};
> 
> Why are those nullable?  What's the use case for passing { pref: null } or {
> pref: undefined } and having it be treated like "pref" is not set (which
> afaict is what your code does)?
> 
> Similar for the other dictionaries here.

Will change this after bug 882547 is fixed.

> >+dictionary ContactFindSortOptions {
> >+  DOMString sortBy;                    // "givenName" or "familyName"
> 
> If those are the only two options, that should be a WebIDL enum, I would
> think.

They're not, they could be any of the mozContact fields.

> >+  DOMString sortOrder = "ascending";   // e.g. "descending"
> 
> Likewise.
> 
> >+dictionary ContactFindOptions : ContactFindSortOptions {
> >+  DOMString      filterOp;     // e.g. "startsWith"
> 
> And this?

I created enums for those.

> >+  any            filterBy;     // e.g. ["givenName", "nickname"]
> 
> I assume this is reasonably free-form so can't be an enum?

Right.

> r=me with the above fixed.  Maybe add some tests for the EventTarget bits
> too?

Good catch, I forgot about that.
> They're not, I removed those.

How are they not, given that we pass whatever stuff the page handed to the "adr" setter directly in to here?

> The main problem is that if I don't create those
> interfaces on the way back, content sees the objects in the array as a COW

OK, then you should document this very carefully in this code, since it's highly non-obvious.

> The secondary problem is that not doing any validation would allow users to
> pass in garbage, that will then be passed over to ContactDB

My point is that the page can just directly write garbage into .email, no?  So it doesn't seem to me like we're solving the secondary problem at all, unless I'm missing something.

> No, this is just a byproduct of not having a good way to do the permission
> check here, I suspect other JS implemented APIs with events will suffer from
> the same problem.

I think you should locally store the oncontactchange value in your code while the permission check is pending, and if permission is denied just return that instead of the actual event handler....  Or something.  There's the question of whether oncontactchange should reflect the thing that was set even if the permission check fails.

I expect it's too much to hope for actual design or spec for the expected behavior here? :(

> They're not, they could be any of the mozContact fields.

The I suggest saying |e.g. "givenName" or "familyName"| to make it clear that this is not an exhaustive list.  Or maybe just saying that it's any of the mozContact fields and then giving some examples.
(In reply to Boris Zbarsky (:bz) from comment #8)
> > They're not, I removed those.
> 
> How are they not, given that we pass whatever stuff the page handed to the
> "adr" setter directly in to here?

Hm, they're not when called from the mozContact constructor, but *are* needed when called from the setters… Maybe I'll split the code between the public constructor and a [ChromeOnly] initialize that bypasses the validation.

> > The main problem is that if I don't create those
> > interfaces on the way back, content sees the objects in the array as a COW
> 
> OK, then you should document this very carefully in this code, since it's
> highly non-obvious.

Ok.

> > The secondary problem is that not doing any validation would allow users to
> > pass in garbage, that will then be passed over to ContactDB
> 
> My point is that the page can just directly write garbage into .email, no? 
> So it doesn't seem to me like we're solving the secondary problem at all,
> unless I'm missing something.

Right. I'll write code validate data written directly to the array properties too.

> > No, this is just a byproduct of not having a good way to do the permission
> > check here, I suspect other JS implemented APIs with events will suffer from
> > the same problem.
> 
> I think you should locally store the oncontactchange value in your code
> while the permission check is pending, and if permission is denied just
> return that instead of the actual event handler....  Or something.  There's
> the question of whether oncontactchange should reflect the thing that was
> set even if the permission check fails.

What I'm doing now is checking for the permission during initialization, and then if we don't have permission I never broadcast the change message.

> I expect it's too much to hope for actual design or spec for the expected
> behavior here? :(

ni? on Tantek for this question.

> > They're not, they could be any of the mozContact fields.
> 
> The I suggest saying |e.g. "givenName" or "familyName"| to make it clear
> that this is not an exhaustive list.  Or maybe just saying that it's any of
> the mozContact fields and then giving some examples.

Ok.
Flags: needinfo?(tantek)
Also for Tantek: should I prefix the ContactAddress/ContactField/ContactTelField interfaces, given that Contact is prefixed?
> What I'm doing now is checking for the permission during initialization, and then if we
> don't have permission I never broadcast the change message.

So setting oncontactchange always adds the event listener and getting it works correctly but the event may simply not fire?
Yes.
bz, there are significant changes to things like validateArrayField, can you give this patch a second look? Here's an interdiff http://pastebin.mozilla.org/2536097

gwagner, this patch changes some API behaviors (like removing init in favor of the ctor), are we OK with breaking compat?
Attachment #760182 - Attachment is obsolete: true
Attachment #763888 - Flags: review?(bzbarsky)
Attachment #763888 - Flags: review?(anygregor)
Posted patch Interdiff (obsolete) — Splinter Review
Interdiff before http://pastebin.mozilla.org/2536097 goes down.
Comment on attachment 763888 [details] [diff] [review]
Convert the Contacts API to WebIDL, v1

>+++ b/dom/contacts/ContactManager.js
>+ContactAddress.prototype = {
>+  toJSON: function(excludeExposedProps) {
>+    if (excludeExposedProps) {
>+      json.__exposedProps__ = {

Isn't that backwards?  Either in the naming or _something_.

Same for ContactField, ContactTel, 

The second arg of validateArrayField seems to be unused.

I do not understand why you can assume the right types in initialize() when your createCB arguments to validateArrayField happily pass in whatever the heck the page wants.

Your interdiff _and_ your main diff are both missing the .webidl file, so I can't tell whether any of these random changes to not go through setters are right or not; it really depends on the IDL.

>+++ b/dom/webidl/WebIDL.mk
>   ConvolverNode.webidl \
>+  Contacts.webidl \
>   Coordinates.webidl \

This is seriously alphabet-challenged (not just your change!).  Please fix, presumably by moving ConvolverNode after Coordinates.
Attachment #763888 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky (:bz) from comment #15)
> Comment on attachment 763888 [details] [diff] [review]
> Convert the Contacts API to WebIDL, v1
> 
> >+++ b/dom/contacts/ContactManager.js
> >+ContactAddress.prototype = {
> >+  toJSON: function(excludeExposedProps) {
> >+    if (excludeExposedProps) {
> >+      json.__exposedProps__ = {
> 
> Isn't that backwards?  Either in the naming or _something_.

Argh, yes, too much global search and replace causes that.

> I do not understand why you can assume the right types in initialize() when
> your createCB arguments to validateArrayField happily pass in whatever the
> heck the page wants.

initialize() is

  [ChromeOnly] 
  void initialize(optional sequence<DOMString> type,
                  optional DOMString streetAddress,
                  optional DOMString locality,
                  optional DOMString region,
                  optional DOMString postalCode,
                  optional DOMString countryName,
                  optional boolean pref);

Shouldn't bindings take care of making sure it's the right types for me?

> Your interdiff _and_ your main diff are both missing the .webidl file, so I
> can't tell whether any of these random changes to not go through setters are
> right or not; it really depends on the IDL.

Yep, I messed up.
> Shouldn't bindings take care of making sure it's the right types for me?

Yes, when called via the bindings....  Ah, I guess you're calling initialize() on an Xray around a content object, so that part is safe.  It's worth a comment on initialize() to make sure people don't misuse it.  But other than that, this is nice approach!
Let's do another risky review?
Attachment #763888 - Attachment is obsolete: true
Attachment #764008 - Attachment is obsolete: true
Attachment #763888 - Flags: review?(anygregor)
Attachment #764976 - Flags: review?(bzbarsky)
Attachment #764976 - Flags: review?(anygregor)
Posted patch interdiff (obsolete) — Splinter Review
Comment on attachment 764976 [details] [diff] [review]
Convert the Contacts API to WebIDL, v2

r=me
Attachment #764976 - Flags: review?(bzbarsky) → review+
Already talked to reuben on IRC.
1)We need some DB migration code:
E/GeckoConsole(  562): [JavaScript Error: "'type' member of ContactFieldInit can't be converted to a sequence." {file: "jar:file:///system/b2g/omni.ja!/components/ContactManager.js" line: 404}]

2)We also need some performance love here.
Requesting 1000 contacts takes now 14 sec vs. 8 sec with the old implementation.
Lets create some profiles with sps and perf and try to speed it up.
Maybe we should also try to create a reduced test case.
I'm very happy to try to improve the perf here if people give me profiles or a way to reproduce (on Mac).
Attachment #764976 - Attachment is obsolete: true
Attachment #764976 - Flags: review?(anygregor)
Attachment #766948 - Flags: review?(anygregor)
Here's an SPS profile of loading 1000 contacts: https://people.mozilla.com/~rmorais/1000-contacts-webidl-profile
That profile shows 37% of the time sleeping waiting for events, 15% of the time painting, some layout stuff, 7% scrollStart in contacts_shortcuts.js, 10% under the onsuccess callback in contacts_list.js, 11% under ContactManager.prototype._convertContact.

So at most 21% of the time here is in code affected by this patch, right?  And even then the onsuccess code seems unrelated to these changes.

The C++ symbols in that profile are also kind of wonky.  :(  For example, I'm not seeing any generated binding code in this profile so far....

In any case, for the convertContact bit we seem to be mostly spending time under validateArrayField.  Perhaps creating JS-implemented WebIDL objects?
(In reply to Boris Zbarsky (:bz) (reading mail, but on paternity leave) from comment #25)
> That profile shows 37% of the time sleeping waiting for events, 15% of the
> time painting, some layout stuff, 7% scrollStart in contacts_shortcuts.js,
> 10% under the onsuccess callback in contacts_list.js, 11% under
> ContactManager.prototype._convertContact.
> 
> So at most 21% of the time here is in code affected by this patch, right? 
> And even then the onsuccess code seems unrelated to these changes.
> 
> The C++ symbols in that profile are also kind of wonky.  :(  For example,
> I'm not seeing any generated binding code in this profile so far....
> 
> In any case, for the convertContact bit we seem to be mostly spending time
> under validateArrayField.  Perhaps creating JS-implemented WebIDL objects?

We are trying to get a better profile. We shouldn't profile the contacts app but rather a simple function that retrieves all the contacts without any painting. Our UI Test app does this. Reuben is also working on a higher sample rate so we can get 1ms samples instead of 10ms.
(In reply to Gregor Wagner [:gwagner] from comment #26)
> We are trying to get a better profile. We shouldn't profile the contacts app
> but rather a simple function that retrieves all the contacts without any
> painting. Our UI Test app does this. Reuben is also working on a higher
> sample rate so we can get 1ms samples instead of 10ms.

Btw, I don't think a higher sample rate will change a lot of things here. But I'll get a profile anyway.
Higher sample rate is less important than focusing on the thing that got slower.
This is a profile of creating a 1000 contacts in a loop on the device:
https://people.mozilla.com/~bgirard/cleopatra/#report=40e9b8e19b12435c1a5bc2e16fbd8cc933302b05

Test case here: https://people.mozilla.com/~rmorais/webidl.html

validateArrayField and its callback are responsible for most of it. Can we avoid the round-trip to C++ land to create those objects? Can we make this:

    this._adr = validateArrayField(aAdr, function(adr) {
      let obj = new this._window.ContactAddress();
      obj.initialize(adr.type, adr.streetAddress, adr.locality,
                     adr.region, adr.postalCode, adr.countryName,
                     adr.pref);
      return obj;
    }.bind(this));

be more like this:

    this._adr = validateArrayField(aAdr, function(adr) {
      let obj = new ContactAddress();
      obj.initialize(adr.type, adr.streetAddress, adr.locality,
                     adr.region, adr.postalCode, adr.countryName,
                     adr.pref);
      return obj;
    });
There are two issues with trying to make that change in a naive way:

1)  You lose the argument-sanitization initialize() ends up doing right now.

2)  You still need to create page-side ContactAddress objects at some point to hand them to the page... and nothing tells the binding code to do that, because this array is not listed as an array of ContactAddress in any way in the IDL.

Now what might be good is to have a way to create a content-side object with a given chrome-side object without having to go through all the CreateInstance machinery.  There's been talk of being able to expose chrome-only constructors, or maybe we can find something else clever like that.  Let me think about it a bit.
Attachment #780080 - Flags: review?(anygregor)
Blocks: 888591
Rebased.
Attachment #766948 - Attachment is obsolete: true
Attachment #766948 - Flags: review?(anygregor)
Attachment #790511 - Flags: review?(anygregor)
Posted patch Fix tests (obsolete) — Splinter Review
Rebased.
Attachment #760183 - Attachment is obsolete: true
Attachment #790512 - Flags: review?(anygregor)
The Android backend broke some things.
Attachment #790511 - Attachment is obsolete: true
Attachment #790511 - Flags: review?(anygregor)
Attachment #791533 - Flags: review?(anygregor)
Posted patch 2 - Fix tests, v2 (obsolete) — Splinter Review
Fix some new code in the tests.
Attachment #790512 - Attachment is obsolete: true
Attachment #790512 - Flags: review?(anygregor)
Attachment #791535 - Flags: review?(anygregor)
And this is just a rebase to tip.
Attachment #780080 - Attachment is obsolete: true
Attachment #780080 - Flags: review?(anygregor)
Attachment #791536 - Flags: review?(anygregor)
Attachment #764977 - Attachment is obsolete: true
Comment on attachment 791533 [details] [diff] [review]
1 - Convert the Contacts API to WebIDL, v3

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +574,5 @@
> +           if (changed) {
> +             cursor.update(cursor.value);
> +           }
> +           cursor.continue();
> +         }

Where is my next() function? :)

::: dom/webidl/Contacts.webidl
@@ +77,5 @@
> +  DOMString?                     sex;
> +  DOMString?                     genderIdentity;
> +
> +  sequence<Blob>?                photo;
> + 

nit: Whitespace

@@ +137,5 @@
> +           attribute object?      note;
> +           attribute object?      publicKey;
> +
> +  [ChromeOnly]
> +  void setMetadata(DOMString id, optional Date published, optional Date updated);

Do we need this in the interface?

@@ +162,5 @@
> +  DOMCursor  getAll(optional ContactFindSortOptions options);
> +  DOMRequest clear();
> +  DOMRequest save(mozContact contact);
> +  DOMRequest remove(mozContact contact);
> +  DOMRequest getSimContacts(DOMString type);

Remove
Attachment #791533 - Flags: review?(anygregor) → review+
Comment on attachment 791535 [details] [diff] [review]
2 - Fix tests, v2

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

::: dom/contacts/tests/test_contacts_getall.html
@@ +241,5 @@
>  let steps = [
> +  function start() {
> +    SpecialPowers.Cc["@mozilla.org/tools/profiler;1"].getService(SpecialPowers.Ci.nsIProfiler).AddMarker("GETALL_START");
> +    next();
> +  },

Do we still want this? Well could be useful.
Attachment #791535 - Flags: review?(anygregor) → review+
Comment on attachment 791533 [details] [diff] [review]
1 - Convert the Contacts API to WebIDL, v3

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +28,5 @@
>  const REVISION_KEY = "revision";
>  
>  function exportContact(aRecord) {
> +  delete aRecord.search;
> +  return aRecord;

Uh aRecord can be null here!

@@ +574,5 @@
> +           if (changed) {
> +             cursor.update(cursor.value);
> +           }
> +           cursor.continue();
> +         }

Where is my next() function? :)

::: dom/webidl/Contacts.webidl
@@ +77,5 @@
> +  DOMString?                     sex;
> +  DOMString?                     genderIdentity;
> +
> +  sequence<Blob>?                photo;
> + 

nit: Whitespace

@@ +137,5 @@
> +           attribute object?      note;
> +           attribute object?      publicKey;
> +
> +  [ChromeOnly]
> +  void setMetadata(DOMString id, optional Date published, optional Date updated);

Do we need this in the interface?

@@ +162,5 @@
> +  DOMCursor  getAll(optional ContactFindSortOptions options);
> +  DOMRequest clear();
> +  DOMRequest save(mozContact contact);
> +  DOMRequest remove(mozContact contact);
> +  DOMRequest getSimContacts(DOMString type);

Remove
(In reply to Gregor Wagner [:gwagner] from comment #39)
> @@ +137,5 @@
> > +           attribute object?      note;
> > +           attribute object?      publicKey;
> > +
> > +  [ChromeOnly]
> > +  void setMetadata(DOMString id, optional Date published, optional Date updated);
> 
> Do we need this in the interface?

Yep, see ContactManager._convertContact:

  let newContact = new this._window.mozContact(aContact.properties);
  newContact.setMetadata(aContact.id, aContact.published, aContact.updated);
(In reply to Gregor Wagner [:gwagner] from comment #39)
> ::: dom/contacts/fallback/ContactDB.jsm
> @@ +28,5 @@
> >  const REVISION_KEY = "revision";
> >  
> >  function exportContact(aRecord) {
> > +  delete aRecord.search;
> > +  return aRecord;
> 
> Uh aRecord can be null here!

Hmm, if aRecord is undefined here it means we have invalid entries in the getAll cache :( For now I'll check in exportContact, but this needs to be investigated more carefully.
Posted file Gaia PR
This PR fixes users of mozContact to use the new constructor.
Attachment #799158 - Flags: review?(anygregor)
blocks koi+ bug 898337 -> koi?
blocking-b2g: --- → koi?
Attachment #791536 - Flags: review?(anygregor) → review+
Attachment #799158 - Flags: review?(anygregor) → review+
While trying to land this over the weekend I ran into a bunch of problems with the Android backend relying on very specific behaviors of the front-end. Most of those problems are fixed in patches 4 - 6, some are fixed in bug 913976. The Travis failures on the Gaia PR are also fixed, so I expect to land this without any major problems as soon as these new patches are reviewed.
Comment on attachment 801309 [details] [diff] [review]
5 - Handle 'pref' property correctly in the Android contacts backend

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

LGTM

::: mobile/android/base/ContactService.java
@@ +665,5 @@
>          return contact;
>      }
>  
> +    private boolean bool(int integer) {
> +        return integer != 0 ? true : false;

You can just `return integer != 0` without the ternary operator. You can also make the bool() method static, since it's a stateless helper function.
Attachment #801309 - Flags: review?(cpeterson) → review+
Comment on attachment 801310 [details] [diff] [review]
6 - Handle new defaults correctly in the Android contacts backend

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

::: mobile/android/base/ContactService.java
@@ +247,5 @@
>              final int substringMatching = findOptions.getInt("substringMatching");
>  
>              // If filter value is undefined, avoid all the logic below and just return
>              // all available raw contact IDs
> +            if ("".equals(filterValue) || "".equals(filterOp)) {

Do you want to enter this conditional branch if filterValue or filterOp are null (i.e. they were not defined in findOptions)? If findOptions must contain filterValue and filterOp, then why change the getString() calls to optString() above?
Attachment #801310 - Flags: review?(cpeterson) → feedback+
(In reply to Chris Peterson (:cpeterson) from comment #49)
> Comment on attachment 801310 [details] [diff] [review]
> 6 - Handle new defaults correctly in the Android contacts backend
> 
> Review of attachment 801310 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/ContactService.java
> @@ +247,5 @@
> >              final int substringMatching = findOptions.getInt("substringMatching");
> >  
> >              // If filter value is undefined, avoid all the logic below and just return
> >              // all available raw contact IDs
> > +            if ("".equals(filterValue) || "".equals(filterOp)) {
> 
> Do you want to enter this conditional branch if filterValue or filterOp are
> null (i.e. they were not defined in findOptions)?

Yes.

> If findOptions must  contain filterValue and filterOp, then why change the getString()
> calls to optString() above?

filterValue and filterOp are both optional. Undefined filterOp or undefined filterValue mean the same thing: get all contacts.
Whiteboard: [systemsfe]
I think most patches don't apply on current central... :(
Blocks: 916267
Carrying r=bz, r=gwagner.

Boris, can you take a look at the additions to test_interfaces.html? They weren't included in the previous versions of the patch. Thanks!

diff --git a/dom/tests/mochitest/general/test_interfaces.html b/dom/tests/mochitest/general/test_interfaces.html
--- a/dom/tests/mochitest/general/test_interfaces.html
+++ b/dom/tests/mochitest/general/test_interfaces.html
@@ -122,8 +122,11 @@ var interfaceNamesInGlobalScope =
     "CloseEvent",
     "CommandEvent",
     "Comment",
     "CompositionEvent",
+    "ContactAddress",
+    "ContactField",
+    "ContactTelField",
     "Controllers",
     "ConvolverNode",
     {name: "CRMFObject", desktop: true},
     "Crypto",
Attachment #791533 - Attachment is obsolete: true
Attachment #804650 - Flags: review?(bzbarsky)
Rebased to tip, carrying r=gwagner.
Attachment #791536 - Attachment is obsolete: true
Attachment #804656 - Flags: review+
Attachment #801308 - Attachment description: 4 - Handle Date properties across the Contacts code. → 4 - Handle Date properties consistently across the Contacts code.
Attachment #791535 - Attachment is obsolete: true
Comment on attachment 801310 [details] [diff] [review]
6 - Handle new defaults correctly in the Android contacts backend

See comment 50.
Attachment #801310 - Flags: review?(cpeterson)
As discussed on IRC, instead of adding the interfaces to test_interfaces.html I made them ChromeOnly. Carrying r=bz, r=gwagner.
Attachment #804650 - Attachment is obsolete: true
Attachment #804650 - Flags: review?(bzbarsky)
Attachment #804692 - Flags: review+
Comment on attachment 801310 [details] [diff] [review]
6 - Handle new defaults correctly in the Android contacts backend

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

LGTM!

(In reply to Reuben Morais [:reuben] from comment #50)
> > > +            if ("".equals(filterValue) || "".equals(filterOp)) {
> > 
> > Do you want to enter this conditional branch if filterValue or filterOp are
> > null (i.e. they were not defined in findOptions)?
> 
> Yes.

Sorry, I didn't know JSONObject.optString() returns "" instead of null, so your code was correct. <:)
Attachment #801310 - Flags: review?(cpeterson) → review+
Comment on attachment 804692 [details] [diff] [review]
1 - Convert the Contacts API to WebIDL, v5

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

::: dom/contacts/ContactManager.js
@@ +343,5 @@
> +      anniversary:     this.anniversary,
> +      key:             this.key,
> +
> +      __exposedProps__: {
> +        id:              "rw",

I always wondered why "id" is "rw" and not read-only ?

::: dom/contacts/fallback/ContactDB.jsm
@@ -599,5 @@
> -      anniversary:     null,
> -      sex:             null,
> -      genderIdentity:  null,
> -      key:             [],
> -    };

I thought it was useful to have defaults here. Or does WebIDL provide this us for free ?
(In reply to Julien Wajsberg [:julienw] from comment #57)
> ::: dom/contacts/ContactManager.js
> @@ +343,5 @@
> > +      anniversary:     this.anniversary,
> > +      key:             this.key,
> > +
> > +      __exposedProps__: {
> > +        id:              "rw",
> 
> I always wondered why "id" is "rw" and not read-only ?

That's only used for JSON.stringify so it doesn't actually matter.

> ::: dom/contacts/fallback/ContactDB.jsm
> @@ -599,5 @@
> > -      anniversary:     null,
> > -      sex:             null,
> > -      genderIdentity:  null,
> > -      key:             [],
> > -    };
> 
> I thought it was useful to have defaults here. Or does WebIDL provide this
> us for free ?

No need to store all those nulls for undefined properties.
blocking-b2g: koi? → -
Comment on attachment 801308 [details] [diff] [review]
4 - Handle Date properties consistently across the Contacts code.

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

::: dom/webidl/Contacts.webidl
@@ -137,5 @@
>             attribute object?      note;
>             attribute object?      publicKey;
>  
>    [ChromeOnly]
> -  void setMetadata(DOMString id, optional Date published, optional Date updated);

What's the difference between optional and Date? ?
Attachment #801308 - Flags: review?(anygregor) → review+
(In reply to Gregor Wagner [:gwagner] from comment #59)
> > -  void setMetadata(DOMString id, optional Date published, optional Date updated);
> 
> What's the difference between optional and Date? ?

|foo(optional Date)| allows
 foo();
 foo(new Date());
 foo(undefined);

but NOT
 foo(null);

while |foo(Date?)| allows
 foo(new Date());
 foo(null);
> foo(undefined);

Actually, not yet; that's bug 882541.  But you could explicitly flag it as [TreatUndefinedAs=Missing] for now.

> while |foo(Date?)| allows

Also allows foo(undefined) and converts it to null.
(In reply to Reuben Morais [:reuben] from comment #62)
> Let's see if it sticks! :)

Not to be a process nut, but tree management needs to keep the bug open until the patch lands on central. Reopening on that basis until the patch cleanly hits central.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Jason Smith [:jsmith] from comment #63)
> (In reply to Reuben Morais [:reuben] from comment #62)
> > Let's see if it sticks! :)
> 
> Not to be a process nut, but tree management needs to keep the bug open
> until the patch lands on central. Reopening on that basis until the patch
> cleanly hits central.

My mistake, the last thing I did was pasting the Gaia commit URL, and you're supposed to mark RESOLVED for Gaia merges so I confused the two processes :) Thanks for fixing it up.
Backed out for Android mochitest failures. I had those failures fixed, but maybe I need the patches in bug 913976.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d09f86ec45cd
Flags: needinfo?(tantek)
Hey Reuben, is it moving forward ? :)
blocks bug 898337
blocking-b2g: - → koi?
Also, the patches are bitrotted :(
(In reply to Julien Wajsberg [:julienw] from comment #67)
> Hey Reuben, is it moving forward ? :)

Yes. Right now I'm looking at bug 920302, but if this is koi+ it should be easy to land, since we decided we can disable the Android backend for now.
Depends on: 927868
(In reply to Julien Wajsberg [:julienw] from comment #68)
> blocks bug 898337

We aren't going to block on this at this point - you need to figure out a way to implement that patch with relying on the changes here.
That has to bake on master.
blocking-b2g: koi? → -
(In reply to Jason Smith [:jsmith] from comment #71)
> (In reply to Julien Wajsberg [:julienw] from comment #68)
> > blocks bug 898337
> 
> We aren't going to block on this at this point - you need to figure out a
> way to implement that patch with relying on the changes here.

meant to say - without relying on the changes here
(In reply to Julien Wajsberg [:julienw] from comment #69)
> Also, the patches are bitrotted :(

Rebased to tip.
Attachment #804692 - Attachment is obsolete: true
Attachment #818520 - Flags: review+
Attachment #801308 - Attachment is obsolete: true
I tried running this using the reference-workload-heavy data set from the gaia repo.  I got the following error:

  E/GeckoConsole(13914): [JavaScript Error: "'bday' member of ContactProperties is not a date." {file: "jar:file:///system/b2g/omni.ja!/components/ContactManager.js" line: 397}]

Is there an issue with some fields being stored as strings before and now they must be Date objects?
(In reply to Ben Kelly [:bkelly] from comment #76)
> Is there an issue with some fields being stored as strings before and now
> they must be Date objects?

Yes. I thought I had checked the reference workloads but I'll check again. They may need to be regenerated. You should be able to workaround this by undoing this hunk (from patch 4):

diff --git a/dom/contacts/ContactManager.js b/dom/contacts/ContactManager.js
--- a/dom/contacts/ContactManager.js
+++ b/dom/contacts/ContactManager.js
@@ -389,22 +389,16 @@ ContactManager.prototype = {
     this.__DOM_IMPL__.setEventHandler("oncontactchange", aHandler);
   },
 
   get oncontactchange() {
     return this.__DOM_IMPL__.getEventHandler("oncontactchange");
   },
 
   _convertContact: function(aContact) {
-    if (aContact.properties.bday) {
-      aContact.properties.bday = new Date(aContact.properties.bday);
-    }
-    if (aContact.properties.anniversary) {
-      aContact.properties.anniversary = new Date(aContact.properties.anniversary);
-    }
     let newContact = new this._window.mozContact(aContact.properties);
     newContact.setMetadata(aContact.id, aContact.published, aContact.updated);
     return newContact;
   },
 
   _convertContacts: function(aContacts) {
     let contacts = [];
     for (let i in aContacts) {
Shouldn't any change like that be part of a database upgrade process?  I believe the reference workloads are designed to load on v1.1 with the expectation they will update correctly to future versions of mozContacts.
I agree with Ben here.

Also, if the reference workload have this, is possible that real user data has this as well ?
(In reply to Ben Kelly [:bkelly] from comment #78)
> Shouldn't any change like that be part of a database upgrade process?

Probably! I was expecting things to just work with the current setup in ContactDB, and they did at some point, but something regressed. I'll investigate :)
Do we need to repeat [ChromeOnly] on both methods and interfaces like this: <https://hg.mozilla.org/integration/mozilla-inbound/rev/533d30be7e6e#l20.21>?
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #82)
> Do we need to repeat [ChromeOnly] on both methods and interfaces like this:
> <https://hg.mozilla.org/integration/mozilla-inbound/rev/533d30be7e6e#l20.21>?

Is a ContactAddress instance available from content, but only Chrome can create an instance? If so, it does make sense to mark both the interface and the method with [ChromeOnly].
Indeed the ContactAddress interface object should not be visible from content per the spec.
http://www.w3.org/TR/contacts-manager-api/#contactaddress-interface
(In reply to Masatoshi Kimura [:emk] from comment #83)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #82)
> > Do we need to repeat [ChromeOnly] on both methods and interfaces like this:
> > <https://hg.mozilla.org/integration/mozilla-inbound/rev/533d30be7e6e#l20.21>?
> 
> Is a ContactAddress instance available from content, but only Chrome can
> create an instance? If so, it does make sense to mark both the interface and
> the method with [ChromeOnly].

Oh, I thought specifying [ChromeOnly] on the interface makes all of the methods and attributes [ChromeOnly] as well.  Is that not the case?
Posted patch Fix ICCSplinter Review
Apparently nsISupports doesn't work for JS-implemented WebIDL.
Attachment #819181 - Flags: review?(anygregor)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #85)
> Oh, I thought specifying [ChromeOnly] on the interface makes all of the
> methods and attributes [ChromeOnly] as well.  Is that not the case?

[ChromeOnly] on the interface will just hide the interface object. If content obtain the prototype object somehow (e.g. via navigator.mozFoobar), all methods and properties will be visible unless they are also marked with [ChromeOnly].
Attachment #819181 - Flags: review?(anygregor) → review+
Depends on: 928915
No longer depends on: 929043
To bring the IRC discussion into the bug - QA is going to wait for a day to see if the fixes coming in are enough to get daily tests & gaia ui tests back online. If we go past a day, then we might need to consider backing this out.
Depends on: 929043
No longer blocks: 929228
No longer blocks: 929240
Blocks: 929435
No longer blocks: 929435
Depends on: 929435
Depends on: 938219
Depends on: 966566
No longer depends on: 966566
Depends on: 966566
No longer depends on: 966566
Blocks: 850266
You need to log in before you can comment on or make changes to this bug.