Contacts API: Use WebIDL

RESOLVED FIXED in mozilla27

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: reuben, Assigned: reuben)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

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

Firefox Tracking Flags

(blocking-b2g:-)

Details

(Whiteboard: [systemsfe])

Attachments

(7 attachments, 17 obsolete attachments)

266 bytes, text/html
gwagner
: review+
Details
11.87 KB, patch
cpeterson
: review+
Details | Diff | Splinter Review
12.56 KB, patch
cpeterson
: review+
cpeterson
: feedback+
Details | Diff | Splinter Review
8.77 KB, patch
reuben
: review+
Details | Diff | Splinter Review
119.99 KB, patch
reuben
: review+
Details | Diff | Splinter Review
5.81 KB, patch
Details | Diff | Splinter Review
3.67 KB, patch
gwagner
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Let's do this!
Depends on: 827486
Depends on: 851639
(Assignee)

Updated

5 years ago
Depends on: 742206
(Assignee)

Updated

5 years ago
Depends on: 585381
(Assignee)

Updated

5 years ago
Depends on: 851726
(Assignee)

Updated

5 years ago
Depends on: 852219
(Assignee)

Updated

5 years ago
Depends on: 851178
(Assignee)

Updated

5 years ago
Depends on: 856841
(Assignee)

Updated

5 years ago
Depends on: 731746
(Assignee)

Updated

5 years ago
Depends on: 863898
Depends on: 865544
(Assignee)

Updated

5 years ago
Depends on: 865951
(Assignee)

Updated

5 years ago
Depends on: 870219
(Assignee)

Updated

5 years ago
No longer depends on: 851726
(Assignee)

Comment 2

5 years ago
Created attachment 760182 [details] [diff] [review]
Convert the Contacts API to WebIDL, v0

Boris, note the question in ContactManager.save. Can we do better than that?
Attachment #749533 - Attachment is obsolete: true
Attachment #760182 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

5 years ago
Created attachment 760183 [details] [diff] [review]
Fix tests

Fixes a bunch of silly things that the tests were doing.
Attachment #760183 - Flags: review?(anygregor)
Comment on attachment 760182 [details] [diff] [review]
Convert the Contacts API to WebIDL, v0

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

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

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

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

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

Same here.  And for other interfaces in this file.

>+    this.value = stringOrBust(aValue);

And here.  And likewise for other interfaces.

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

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

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

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

  contact.email[5] = 17;

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

>+  toJSON: function() {

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

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

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

>   get oncontactchange() {

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

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

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

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

Not that I can think of....

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

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

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

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

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

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

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

Similar for the other dictionaries here.

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

Document what it should really be?

>+interface mozContact {

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

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

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

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

Likewise.

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

And this?

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

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

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

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

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

Remove :)

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

Why do you remove this?

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

Why do you remove this?
(Assignee)

Comment 6

5 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.
Attachment #760183 - Flags: review?(anygregor) → review+
(Assignee)

Comment 7

5 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.
> 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.
(Assignee)

Comment 9

5 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.
Flags: needinfo?(tantek)
(Assignee)

Comment 10

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

So setting oncontactchange always adds the event listener and getting it works correctly but the event may simply not fire?
(Assignee)

Comment 12

5 years ago
Yes.
(Assignee)

Comment 13

5 years ago
Created attachment 763888 [details] [diff] [review]
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?
Attachment #760182 - Attachment is obsolete: true
Attachment #763888 - Flags: review?(bzbarsky)
Attachment #763888 - Flags: review?(anygregor)
Comment on attachment 763888 [details] [diff] [review]
Convert the Contacts API to WebIDL, v1

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

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

Same for ContactField, ContactTel, 

The second arg of validateArrayField seems to be unused.

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

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

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

This is seriously alphabet-challenged (not just your change!).  Please fix, presumably by moving ConvolverNode after Coordinates.
Attachment #763888 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 16

5 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.
> 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!
(Assignee)

Comment 18

5 years ago
Created attachment 764976 [details] [diff] [review]
Convert the Contacts API to WebIDL, v2

Let's do another risky review?
Attachment #763888 - Attachment is obsolete: true
Attachment #764008 - Attachment is obsolete: true
Attachment #763888 - Flags: review?(anygregor)
Attachment #764976 - Flags: review?(bzbarsky)
Attachment #764976 - Flags: review?(anygregor)
(Assignee)

Comment 19

5 years ago
Created attachment 764977 [details] [diff] [review]
interdiff
Comment on attachment 764976 [details] [diff] [review]
Convert the Contacts API to WebIDL, v2

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

2)We also need some performance love here.
Requesting 1000 contacts takes now 14 sec vs. 8 sec with the old implementation.
Lets create some profiles with sps and perf and try to speed it up.
Maybe we should also try to create a reduced test case.
I'm very happy to try to improve the perf here if people give me profiles or a way to reproduce (on Mac).
(Assignee)

Comment 23

5 years ago
Created attachment 766948 [details] [diff] [review]
Convert the Contacts API to WebIDL, v2 + migration
Attachment #764976 - Attachment is obsolete: true
Attachment #764976 - Flags: review?(anygregor)
Attachment #766948 - Flags: review?(anygregor)
(Assignee)

Comment 24

5 years ago
Here's an SPS profile of loading 1000 contacts: https://people.mozilla.com/~rmorais/1000-contacts-webidl-profile
That profile shows 37% of the time sleeping waiting for events, 15% of the time painting, some layout stuff, 7% scrollStart in contacts_shortcuts.js, 10% under the onsuccess callback in contacts_list.js, 11% under ContactManager.prototype._convertContact.

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

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

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

We are trying to get a better profile. We shouldn't profile the contacts app but rather a simple function that retrieves all the contacts without any painting. Our UI Test app does this. Reuben is also working on a higher sample rate so we can get 1ms samples instead of 10ms.
(Assignee)

Comment 27

5 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.
Higher sample rate is less important than focusing on the thing that got slower.
(Assignee)

Comment 29

5 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;
    });
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.
(Assignee)

Comment 31

5 years ago
Created attachment 780080 [details] [diff] [review]
Optimize mozContact creation and validation
Attachment #780080 - Flags: review?(anygregor)
(Assignee)

Updated

5 years ago
Blocks: 888591
(Assignee)

Comment 32

5 years ago
Created attachment 790511 [details] [diff] [review]
Convert the Contacts API to WebIDL, v2 + migration

Rebased.
Attachment #766948 - Attachment is obsolete: true
Attachment #766948 - Flags: review?(anygregor)
Attachment #790511 - Flags: review?(anygregor)
(Assignee)

Comment 33

5 years ago
Created attachment 790512 [details] [diff] [review]
Fix tests

Rebased.
Attachment #760183 - Attachment is obsolete: true
Attachment #790512 - Flags: review?(anygregor)
(Assignee)

Comment 34

5 years ago
Created attachment 791533 [details] [diff] [review]
1 - Convert the Contacts API to WebIDL, v3

The Android backend broke some things.
Attachment #790511 - Attachment is obsolete: true
Attachment #790511 - Flags: review?(anygregor)
Attachment #791533 - Flags: review?(anygregor)
(Assignee)

Comment 35

5 years ago
Created attachment 791535 [details] [diff] [review]
2 - Fix tests, v2

Fix some new code in the tests.
Attachment #790512 - Attachment is obsolete: true
Attachment #790512 - Flags: review?(anygregor)
Attachment #791535 - Flags: review?(anygregor)
(Assignee)

Comment 36

5 years ago
Created attachment 791536 [details] [diff] [review]
3 - Optimize mozContact creation and validation

And this is just a rebase to tip.
Attachment #780080 - Attachment is obsolete: true
Attachment #780080 - Flags: review?(anygregor)
Attachment #791536 - Flags: review?(anygregor)
(Assignee)

Updated

5 years ago
Attachment #764977 - Attachment is obsolete: true
Comment on attachment 791533 [details] [diff] [review]
1 - Convert the Contacts API to WebIDL, v3

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

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

Where is my next() function? :)

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

nit: Whitespace

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

Do we need this in the interface?

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

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

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

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

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

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

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

Uh aRecord can be null here!

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

Where is my next() function? :)

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

nit: Whitespace

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

Do we need this in the interface?

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

Remove
(Assignee)

Comment 40

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

Comment 41

5 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.
Blocks: 898337
(Assignee)

Comment 42

5 years ago
Created attachment 799158 [details]
Gaia PR

This PR fixes users of mozContact to use the new constructor.
Attachment #799158 - Flags: review?(anygregor)
blocks koi+ bug 898337 -> koi?
blocking-b2g: --- → koi?
Attachment #791536 - Flags: review?(anygregor) → review+
Attachment #799158 - Flags: review?(anygregor) → review+
(Assignee)

Comment 44

5 years ago
Created attachment 801308 [details] [diff] [review]
4 - Handle Date properties consistently across the Contacts code.
Attachment #801308 - Flags: review?(anygregor)
(Assignee)

Comment 45

5 years ago
Created attachment 801309 [details] [diff] [review]
5 - Handle 'pref' property correctly in the Android contacts backend
Attachment #801309 - Flags: review?(cpeterson)
(Assignee)

Comment 46

5 years ago
Created attachment 801310 [details] [diff] [review]
6 - Handle new defaults correctly in the Android contacts backend
Attachment #801310 - Flags: review?(cpeterson)
(Assignee)

Comment 47

5 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.
Comment on attachment 801309 [details] [diff] [review]
5 - Handle 'pref' property correctly in the Android contacts backend

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

LGTM

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

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

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

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

Do you want to enter this conditional branch if filterValue or filterOp are null (i.e. they were not defined in findOptions)? If findOptions must contain filterValue and filterOp, then why change the getString() calls to optString() above?
Attachment #801310 - Flags: review?(cpeterson) → feedback+
(Assignee)

Comment 50

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

Updated

5 years ago
Whiteboard: [systemsfe]
I think most patches don't apply on current central... :(
(Assignee)

Updated

5 years ago
Blocks: 916267
(Assignee)

Comment 52

5 years ago
Created attachment 804650 [details] [diff] [review]
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",
Attachment #791533 - Attachment is obsolete: true
Attachment #804650 - Flags: review?(bzbarsky)
(Assignee)

Comment 53

5 years ago
Created attachment 804656 [details] [diff] [review]
2 - Optimize mozContact creation and validation

Rebased to tip, carrying r=gwagner.
Attachment #791536 - Attachment is obsolete: true
Attachment #804656 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #801308 - Attachment description: 4 - Handle Date properties across the Contacts code. → 4 - Handle Date properties consistently across the Contacts code.
(Assignee)

Updated

5 years ago
Attachment #791535 - Attachment is obsolete: true
(Assignee)

Comment 54

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

See comment 50.
Attachment #801310 - Flags: review?(cpeterson)
(Assignee)

Comment 55

5 years ago
Created attachment 804692 [details] [diff] [review]
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.
Attachment #804650 - Attachment is obsolete: true
Attachment #804650 - Flags: review?(bzbarsky)
Attachment #804692 - Flags: review+
Comment on attachment 801310 [details] [diff] [review]
6 - Handle new defaults correctly in the Android contacts backend

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

LGTM!

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

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

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

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

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

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

I thought it was useful to have defaults here. Or does WebIDL provide this us for free ?
(Assignee)

Comment 58

5 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.
blocking-b2g: koi? → -
Comment on attachment 801308 [details] [diff] [review]
4 - Handle Date properties consistently across the Contacts code.

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

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

What's the difference between optional and Date? ?
Attachment #801308 - Flags: review?(anygregor) → review+
(Assignee)

Comment 60

5 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);
> foo(undefined);

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

> while |foo(Date?)| allows

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

Not to be a process nut, but tree management needs to keep the bug open until the patch lands on central. Reopening on that basis until the patch cleanly hits central.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 64

5 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.
(Assignee)

Comment 65

5 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
(Assignee)

Updated

5 years ago
Flags: needinfo?(tantek)
Hey Reuben, is it moving forward ? :)
blocks bug 898337
blocking-b2g: - → koi?
Blocks: 904623
Also, the patches are bitrotted :(
(Assignee)

Comment 70

5 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.
(Assignee)

Updated

5 years ago
Depends on: 927868
(In reply to Julien Wajsberg [:julienw] from comment #68)
> blocks bug 898337

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

meant to say - without relying on the changes here
(Assignee)

Comment 74

5 years ago
Created attachment 818520 [details] [diff] [review]
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.
Attachment #804692 - Attachment is obsolete: true
Attachment #818520 - Flags: review+
(Assignee)

Comment 75

5 years ago
Created attachment 818557 [details] [diff] [review]
4 - Handle Date properties across the Contacts code. v2

Rebased to tip.
(Assignee)

Updated

5 years ago
Attachment #801308 - Attachment is obsolete: true
I tried running this using the reference-workload-heavy data set from the gaia repo.  I got the following error:

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

Is there an issue with some fields being stored as strings before and now they must be Date objects?
(Assignee)

Comment 77

5 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) {
Shouldn't any change like that be part of a database upgrade process?  I believe the reference workloads are designed to load on v1.1 with the expectation they will update correctly to future versions of mozContacts.
I agree with Ben here.

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

Comment 80

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

Comment 82

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

5 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?
(Assignee)

Comment 86

5 years ago
Created attachment 819181 [details] [diff] [review]
Fix ICC

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

[ChromeOnly] on the interface will just hide the interface object. If content obtain the prototype object somehow (e.g. via navigator.mozFoobar), all methods and properties will be visible unless they are also marked with [ChromeOnly].
Attachment #819181 - Flags: review?(anygregor) → review+
Depends on: 928875
Depends on: 928635
Depends on: 928890
Blocks: 821573

Updated

5 years ago
Depends on: 928915
Depends on: 928954
Depends on: 928922
Depends on: 928937
No longer depends on: 929043
To bring the IRC discussion into the bug - QA is going to wait for a day to see if the fixes coming in are enough to get daily tests & gaia ui tests back online. If we go past a day, then we might need to consider backing this out.

Updated

5 years ago
Depends on: 929043

Updated

5 years ago
No longer blocks: 929228

Updated

5 years ago
No longer blocks: 929240
Depends on: 929652
Depends on: 931164

Updated

5 years ago
Blocks: 929435

Updated

5 years ago
No longer blocks: 929435
Depends on: 929435
Depends on: 931711
Depends on: 932763
Depends on: 932765
Depends on: 933200

Updated

5 years ago
Depends on: 938219
Depends on: 951766
Depends on: 952533

Updated

5 years ago
Depends on: 966566
(Assignee)

Updated

5 years ago
No longer depends on: 966566

Updated

5 years ago
Depends on: 966566

Updated

5 years ago
No longer depends on: 966566
Blocks: 850266
You need to log in before you can comment on or make changes to this bug.