850430 - Contacts API: Use WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Reuben Morais [:reuben]]

Let's do this!

Comment 1

[Reuben Morais [:reuben]]

Attachment: WIP

Comment 2

[Reuben Morais [:reuben]]

Attachment: Convert the Contacts API to WebIDL, v0

Boris, note the question in ContactManager.save. Can we do better than that?

Comment 3

[Reuben Morais [:reuben]]

Attachment: Fix tests

Fixes a bunch of silly things that the tests were doing.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?

Comment 5

[Gregor Wagner [:gwagner]]

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: "[phone]"}],
> +      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?

Comment 6

[Reuben Morais [:reuben]]

(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.

Comment 7

[Reuben Morais [:reuben]]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 9

[Reuben Morais [:reuben]]

(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.

Comment 10

[Reuben Morais [:reuben]]

Also for Tantek: should I prefix the ContactAddress/ContactField/ContactTelField interfaces, given that Contact is prefixed?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 12

[Reuben Morais [:reuben]]

Yes.

Comment 13

[Reuben Morais [:reuben]]

Attachment: Convert the Contacts API to WebIDL, v1

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?

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Interdiff

Interdiff before http://pastebin.mozilla.org/2536097 goes down.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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.

Comment 16

[Reuben Morais [:reuben]]

(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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> 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!

Comment 18

[Reuben Morais [:reuben]]

Attachment: Convert the Contacts API to WebIDL, v2

Let's do another risky review?

Comment 19

[Reuben Morais [:reuben]]

Attachment: interdiff

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 764976 [details] [diff] [review]
Convert the Contacts API to WebIDL, v2

r=me

Comment 21

[Gregor Wagner [:gwagner]]

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.

Comment 22

[Boris Zbarsky [:bzbarsky]]

I'm very happy to try to improve the perf here if people give me profiles or a way to reproduce (on Mac).

Comment 23

[Reuben Morais [:reuben]]

Attachment: Convert the Contacts API to WebIDL, v2 + migration

Comment 24

[Reuben Morais [:reuben]]

Here's an SPS profile of loading 1000 contacts: https://people.mozilla.com/~rmorais/1000-contacts-webidl-profile

Comment 25

[Boris Zbarsky [:bzbarsky]]

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?

Comment 26

[Gregor Wagner [:gwagner]]

(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.

Comment 27

[Reuben Morais [:reuben]]

(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.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Higher sample rate is less important than focusing on the thing that got slower.

Comment 29

[Reuben Morais [:reuben]]

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;
    });

Comment 30

[Boris Zbarsky [:bzbarsky]]

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.

Comment 31

[Reuben Morais [:reuben]]

Attachment: Optimize mozContact creation and validation

Comment 32

[Reuben Morais [:reuben]]

Attachment: Convert the Contacts API to WebIDL, v2 + migration

Rebased.

Comment 33

[Reuben Morais [:reuben]]

Attachment: Fix tests

Rebased.

Comment 34

[Reuben Morais [:reuben]]

Attachment: 1 - Convert the Contacts API to WebIDL, v3

The Android backend broke some things.

Comment 35

[Reuben Morais [:reuben]]

Attachment: 2 - Fix tests, v2

Fix some new code in the tests.

Comment 36

[Reuben Morais [:reuben]]

Attachment: 3 - Optimize mozContact creation and validation

And this is just a rebase to tip.

Comment 37

[Gregor Wagner [:gwagner]]

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

Comment 38

[Gregor Wagner [:gwagner]]

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.

Comment 39

[Gregor Wagner [:gwagner]]

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

Comment 40

[Reuben Morais [:reuben]]

(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);

Comment 41

[Reuben Morais [:reuben]]

(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.

Comment 42

[Reuben Morais [:reuben]]

Attachment: Gaia PR

This PR fixes users of mozContact to use the new constructor.

Comment 43

[Julien Wajsberg [:julienw]]

blocks koi+ bug 898337 -> koi?

Comment 44

[Reuben Morais [:reuben]]

Attachment: 4 - Handle Date properties consistently across the Contacts code.

Comment 45

[Reuben Morais [:reuben]]

Attachment: 5 - Handle 'pref' property correctly in the Android contacts backend

Comment 46

[Reuben Morais [:reuben]]

Attachment: 6 - Handle new defaults correctly in the Android contacts backend

Comment 47

[Reuben Morais [:reuben]]

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 48

[Chris Peterson [:cpeterson]]

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.

Comment 49

[Chris Peterson [:cpeterson]]

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?

Comment 50

[Reuben Morais [:reuben]]

(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.

Comment 51

[Julien Wajsberg [:julienw]]

I think most patches don't apply on current central... :(

Comment 52

[Reuben Morais [:reuben]]

Attachment: 1 - Convert the Contacts API to WebIDL, v4

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",

Comment 53

[Reuben Morais [:reuben]]

Attachment: 2 - Optimize mozContact creation and validation

Rebased to tip, carrying r=gwagner.

Comment 54

[Reuben Morais [:reuben]]

Comment on attachment 801310 [details] [diff] [review]
6 - Handle new defaults correctly in the Android contacts backend

See comment 50.

Comment 55

[Reuben Morais [:reuben]]

Attachment: 1 - Convert the Contacts API to WebIDL, v5

As discussed on IRC, instead of adding the interfaces to test_interfaces.html I made them ChromeOnly. Carrying r=bz, r=gwagner.

Comment 56

[Chris Peterson [:cpeterson]]

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. <:)

Comment 57

[Julien Wajsberg [:julienw]]

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 ?

Comment 58

[Reuben Morais [:reuben]]

(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.

Comment 59

[Gregor Wagner [:gwagner]]

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? ?

Comment 60

[Reuben Morais [:reuben]]

(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);

Comment 61

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 62

[Reuben Morais [:reuben]]

Let's see if it sticks! :)

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/50e468564056
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fa6402167f7e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8d834b090d00
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1f2d4802682f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7b668cc9f9e1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ac541c7e88b7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9f86de07a061

https://github.com/mozilla-b2g/gaia/compare/fb758390a5e9...3286df632d9a

Comment 63

[Jason Smith [:jsmith]]

(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.

Comment 64

[Reuben Morais [:reuben]]

(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.

Comment 65

[Reuben Morais [:reuben]]

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

Comment 66

[Anthony Ricaud (:rik)]

I also had to revert the Gaia patch has it was breaking two Gaia UI Tests.
https://travis-ci.org/mozilla-b2g/gaia/jobs/11945544

https://github.com/mozilla-b2g/gaia/commit/c3fe1fbc70aecd7e9591fcb90e9fa130aeaf44ce

Comment 67

[Julien Wajsberg [:julienw]]

Hey Reuben, is it moving forward ? :)

Comment 68

[Julien Wajsberg [:julienw]]

blocks bug 898337

Comment 69

[Julien Wajsberg [:julienw]]

Also, the patches are bitrotted :(

Comment 70

[Reuben Morais [:reuben]]

(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.

Comment 71

[Jason Smith [:jsmith]]

(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.

Comment 72

[Gregor Wagner [:gwagner]]

That has to bake on master.

Comment 73

[Jason Smith [:jsmith]]

(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

Comment 74

[Reuben Morais [:reuben]]

Attachment: 1 - Convert the Contacts API to WebIDL, v6

(In reply to Julien Wajsberg [:julienw] from comment #69)
> Also, the patches are bitrotted :(

Rebased to tip.

Comment 75

[Reuben Morais [:reuben]]

Attachment: 4 - Handle Date properties across the Contacts code. v2

Rebased to tip.

Comment 76

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 77

[Reuben Morais [:reuben]]

(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) {

Comment 78

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 79

[Julien Wajsberg [:julienw]]

I agree with Ben here.

Also, if the reference workload have this, is possible that real user data has this as well ?

Comment 80

[Reuben Morais [:reuben]]

(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 :)

Comment 81

[Reuben Morais [:reuben]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d88c084c93e
https://hg.mozilla.org/integration/mozilla-inbound/rev/86b4b7e02167
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a55fecc7454
https://hg.mozilla.org/integration/mozilla-inbound/rev/caa15ceeba4b
https://hg.mozilla.org/integration/mozilla-inbound/rev/dad5d17328b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/533d30be7e6e

Comment 82

[(no longer active)]

Do we need to repeat [ChromeOnly] on both methods and interfaces like this: <https://hg.mozilla.org/integration/mozilla-inbound/rev/533d30be7e6e#l20.21>?

Comment 83

[Masatoshi Kimura [:emk]]

(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

Comment 84

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/533d30be7e6e
https://hg.mozilla.org/mozilla-central/rev/dad5d17328b2
https://hg.mozilla.org/mozilla-central/rev/caa15ceeba4b
https://hg.mozilla.org/mozilla-central/rev/3a55fecc7454
https://hg.mozilla.org/mozilla-central/rev/86b4b7e02167
https://hg.mozilla.org/mozilla-central/rev/0d88c084c93e

Comment 85

[(no longer active)]

(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?

Comment 86

[Reuben Morais [:reuben]]

Attachment: Fix ICC

Apparently nsISupports doesn't work for JS-implemented WebIDL.

Comment 87

[Masatoshi Kimura [:emk]]

(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].

Comment 88

[Reuben Morais [:reuben]]

https://github.com/mozilla-b2g/gaia/commit/d544afff519a4930dec372c6406b365d27bb9cba

Comment 89

[Reuben Morais [:reuben]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a900ff4a5498

Comment 90

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/a900ff4a5498

Comment 91

[Jason Smith [:jsmith]]

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.