Reuben Morais [:reuben] Assignee
	Description • 11 years ago

Let's do this!

	Reuben Morais [:reuben] Assignee
	Comment 2 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 3 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 11 years ago

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?

	Gregor Wagner [:gwagner]
	Comment 5 • 11 years ago

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?

	Reuben Morais [:reuben] Assignee
	Comment 6 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 7 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 9 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 10 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 12 • 11 years ago

Yes.

	Reuben Morais [:reuben] Assignee
	Comment 13 • 11 years ago

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?

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 14 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 11 years ago

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.

	Reuben Morais [:reuben] Assignee
	Comment 16 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 18 • 11 years ago

Let's do another risky review?

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 11 years ago

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

r=me

	Gregor Wagner [:gwagner]
	Comment 21 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 24 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 11 years ago

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?

	Gregor Wagner [:gwagner]
	Comment 26 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 27 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 28 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 29 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 11 years ago

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.

	Reuben Morais [:reuben] Assignee
	Comment 32 • 11 years ago

Rebased.

	Reuben Morais [:reuben] Assignee
	Comment 33 • 11 years ago

Rebased.

	Reuben Morais [:reuben] Assignee
	Comment 34 • 11 years ago

The Android backend broke some things.

	Reuben Morais [:reuben] Assignee
	Comment 35 • 11 years ago

Fix some new code in the tests.

	Reuben Morais [:reuben] Assignee
	Comment 36 • 11 years ago

And this is just a rebase to tip.

	Gregor Wagner [:gwagner]
	Comment 37 • 11 years ago

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

	Gregor Wagner [:gwagner]
	Comment 38 • 11 years ago

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.

	Gregor Wagner [:gwagner]
	Comment 39 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 40 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 41 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 42 • 11 years ago

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

	Julien Wajsberg [:julienw]
	Comment 43 • 11 years ago

blocks koi+ bug 898337 -> koi?

	Reuben Morais [:reuben] Assignee
	Comment 47 • 11 years ago

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.

	Chris Peterson [:cpeterson]
	Comment 48 • 11 years ago

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.

	Chris Peterson [:cpeterson]
	Comment 49 • 11 years ago

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?

	Reuben Morais [:reuben] Assignee
	Comment 50 • 11 years ago

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

	Julien Wajsberg [:julienw]
	Comment 51 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 52 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 53 • 11 years ago

Rebased to tip, carrying r=gwagner.

	Reuben Morais [:reuben] Assignee
	Comment 54 • 11 years ago

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

See comment 50.

	Reuben Morais [:reuben] Assignee
	Comment 55 • 11 years ago

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

	Chris Peterson [:cpeterson]
	Comment 56 • 11 years ago

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

	Julien Wajsberg [:julienw]
	Comment 57 • 11 years ago

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 ?

	Reuben Morais [:reuben] Assignee
	Comment 58 • 11 years ago

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

	Gregor Wagner [:gwagner]
	Comment 59 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 60 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 61 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 62 • 11 years ago

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

	Jason Smith [:jsmith]
	Comment 63 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 64 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 65 • 11 years ago

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

	Anthony Ricaud (:rik)
	Comment 66 • 11 years ago

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

	Julien Wajsberg [:julienw]
	Comment 67 • 11 years ago

Hey Reuben, is it moving forward ? :)

	Julien Wajsberg [:julienw]
	Comment 68 • 11 years ago

blocks bug 898337

	Julien Wajsberg [:julienw]
	Comment 69 • 11 years ago

Also, the patches are bitrotted :(

	Reuben Morais [:reuben] Assignee
	Comment 70 • 11 years ago

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

	Jason Smith [:jsmith]
	Comment 71 • 11 years ago

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

	Gregor Wagner [:gwagner]
	Comment 72 • 11 years ago

That has to bake on master.

	Jason Smith [:jsmith]
	Comment 73 • 11 years ago

(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

	Reuben Morais [:reuben] Assignee
	Comment 74 • 11 years ago

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

Rebased to tip.

	Reuben Morais [:reuben] Assignee
	Comment 75 • 11 years ago

Rebased to tip.

	Ben Kelly [:bkelly, not reviewing]
	Comment 76 • 11 years ago

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?

	Reuben Morais [:reuben] Assignee
	Comment 77 • 11 years ago

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

	Ben Kelly [:bkelly, not reviewing]
	Comment 78 • 11 years ago

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.

	Julien Wajsberg [:julienw]
	Comment 79 • 11 years ago

I agree with Ben here.

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

	Reuben Morais [:reuben] Assignee
	Comment 80 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 81 • 11 years ago

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

	(no longer active)
	Comment 82 • 11 years ago

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

	Masatoshi Kimura [:emk]
	Comment 83 • 11 years ago

(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

	Ryan VanderMeulen [:RyanVM]
	Comment 84 • 11 years ago

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

	(no longer active)
	Comment 85 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 86 • 11 years ago

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

	Masatoshi Kimura [:emk]
	Comment 87 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 88 • 11 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 89 • 11 years ago

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

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 90 • 11 years ago

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

	Jason Smith [:jsmith]
	Comment 91 • 11 years ago

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.