Closed
Bug 850430
Opened 11 years ago
Closed 10 years ago
Contacts API: Use WebIDL
Categories
(Core :: DOM: Device Interfaces, defect)
Core
DOM: Device Interfaces
Tracking
()
People
(Reporter: reuben, Assigned: reuben)
References
Details
(Whiteboard: [systemsfe])
Attachments
(7 files, 17 obsolete files)
266 bytes,
text/html
|
gwagner
:
review+
|
Details |
11.87 KB,
patch
|
cpeterson
:
review+
|
Details | Diff | Splinter Review |
12.56 KB,
patch
|
cpeterson
:
review+
cpeterson
:
feedback+
|
Details | Diff | Splinter Review |
8.77 KB,
patch
|
reuben
:
review+
|
Details | Diff | Splinter Review |
119.99 KB,
patch
|
reuben
:
review+
|
Details | Diff | Splinter Review |
5.81 KB,
patch
|
Details | Diff | Splinter Review | |
3.67 KB,
patch
|
gwagner
:
review+
|
Details | Diff | Splinter Review |
Let's do this!
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
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•10 years ago
|
||
Fixes a bunch of silly things that the tests were doing.
Attachment #760183 -
Flags: review?(anygregor)
![]() |
||
Comment 4•10 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?
Attachment #760182 -
Flags: review?(bzbarsky) → review+
Comment 5•10 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?
Assignee | ||
Comment 6•10 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.
Updated•10 years ago
|
Attachment #760183 -
Flags: review?(anygregor) → review+
Assignee | ||
Comment 7•10 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.
![]() |
||
Comment 8•10 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.
Assignee | ||
Comment 9•10 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•10 years ago
|
||
Also for Tantek: should I prefix the ContactAddress/ContactField/ContactTelField interfaces, given that Contact is prefixed?
![]() |
||
Comment 11•10 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?
Assignee | ||
Comment 12•10 years ago
|
||
Yes.
Assignee | ||
Comment 13•10 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?
Attachment #760182 -
Attachment is obsolete: true
Attachment #763888 -
Flags: review?(bzbarsky)
Attachment #763888 -
Flags: review?(anygregor)
Comment 14•10 years ago
|
||
Interdiff before http://pastebin.mozilla.org/2536097 goes down.
![]() |
||
Comment 15•10 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.
Attachment #763888 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 16•10 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.
![]() |
||
Comment 17•10 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!
Assignee | ||
Comment 18•10 years ago
|
||
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•10 years ago
|
||
![]() |
||
Comment 20•10 years ago
|
||
Comment on attachment 764976 [details] [diff] [review] Convert the Contacts API to WebIDL, v2 r=me
Attachment #764976 -
Flags: review?(bzbarsky) → review+
Comment 21•10 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.
![]() |
||
Comment 22•10 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).
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #764976 -
Attachment is obsolete: true
Attachment #764976 -
Flags: review?(anygregor)
Attachment #766948 -
Flags: review?(anygregor)
Assignee | ||
Comment 24•10 years ago
|
||
Here's an SPS profile of loading 1000 contacts: https://people.mozilla.com/~rmorais/1000-contacts-webidl-profile
![]() |
||
Comment 25•10 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?
Comment 26•10 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.
Assignee | ||
Comment 27•10 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.
![]() |
||
Comment 28•10 years ago
|
||
Higher sample rate is less important than focusing on the thing that got slower.
Assignee | ||
Comment 29•10 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; });
![]() |
||
Comment 30•10 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.
Assignee | ||
Comment 31•10 years ago
|
||
Attachment #780080 -
Flags: review?(anygregor)
Assignee | ||
Comment 32•10 years ago
|
||
Rebased.
Attachment #766948 -
Attachment is obsolete: true
Attachment #766948 -
Flags: review?(anygregor)
Attachment #790511 -
Flags: review?(anygregor)
Assignee | ||
Comment 33•10 years ago
|
||
Rebased.
Attachment #760183 -
Attachment is obsolete: true
Attachment #790512 -
Flags: review?(anygregor)
Assignee | ||
Comment 34•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
Attachment #764977 -
Attachment is obsolete: true
Comment 37•10 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
Attachment #791533 -
Flags: review?(anygregor) → review+
Comment 38•10 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.
Attachment #791535 -
Flags: review?(anygregor) → review+
Comment 39•10 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
Assignee | ||
Comment 40•10 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•10 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.
Assignee | ||
Comment 42•10 years ago
|
||
This PR fixes users of mozContact to use the new constructor.
Attachment #799158 -
Flags: review?(anygregor)
Updated•10 years ago
|
Attachment #791536 -
Flags: review?(anygregor) → review+
Updated•10 years ago
|
Attachment #799158 -
Flags: review?(anygregor) → review+
Assignee | ||
Comment 44•10 years ago
|
||
Attachment #801308 -
Flags: review?(anygregor)
Assignee | ||
Comment 45•10 years ago
|
||
Attachment #801309 -
Flags: review?(cpeterson)
Assignee | ||
Comment 46•10 years ago
|
||
Attachment #801310 -
Flags: review?(cpeterson)
Assignee | ||
Comment 47•10 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 48•10 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.
Attachment #801309 -
Flags: review?(cpeterson) → review+
Comment 49•10 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?
Attachment #801310 -
Flags: review?(cpeterson) → feedback+
Assignee | ||
Comment 50•10 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•10 years ago
|
Whiteboard: [systemsfe]
Comment 51•10 years ago
|
||
I think most patches don't apply on current central... :(
Assignee | ||
Comment 52•10 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",
Attachment #791533 -
Attachment is obsolete: true
Attachment #804650 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 53•10 years ago
|
||
Rebased to tip, carrying r=gwagner.
Attachment #791536 -
Attachment is obsolete: true
Attachment #804656 -
Flags: review+
Assignee | ||
Updated•10 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•10 years ago
|
Attachment #791535 -
Attachment is obsolete: true
Assignee | ||
Comment 54•10 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•10 years ago
|
||
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 56•10 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. <:)
Attachment #801310 -
Flags: review?(cpeterson) → review+
Comment 57•10 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 ?
Assignee | ||
Comment 58•10 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.
Updated•10 years ago
|
blocking-b2g: koi? → -
Comment 59•10 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? ?
Attachment #801308 -
Flags: review?(anygregor) → review+
Assignee | ||
Comment 60•10 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);
![]() |
||
Comment 61•10 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.
Assignee | ||
Comment 62•10 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
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 63•10 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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 64•10 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•10 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
Comment 66•10 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
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(tantek)
Comment 67•10 years ago
|
||
Hey Reuben, is it moving forward ? :)
Comment 69•10 years ago
|
||
Also, the patches are bitrotted :(
Assignee | ||
Comment 70•10 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.
Comment 71•10 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.
Comment 73•10 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
Assignee | ||
Comment 74•10 years ago
|
||
(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•10 years ago
|
||
Rebased to tip.
Assignee | ||
Updated•10 years ago
|
Attachment #801308 -
Attachment is obsolete: true
Comment 76•10 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?
Assignee | ||
Comment 77•10 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) {
Comment 78•10 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.
Comment 79•10 years ago
|
||
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•10 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 :)
Assignee | ||
Comment 81•10 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
Comment 82•10 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>?
Comment 83•10 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
Comment 84•10 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
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment 85•10 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•10 years ago
|
||
Apparently nsISupports doesn't work for JS-implemented WebIDL.
Attachment #819181 -
Flags: review?(anygregor)
Comment 87•10 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].
Updated•10 years ago
|
Attachment #819181 -
Flags: review?(anygregor) → review+
Assignee | ||
Comment 88•10 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/d544afff519a4930dec372c6406b365d27bb9cba
Assignee | ||
Comment 89•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a900ff4a5498
Updated•10 years ago
|
Blocks: CVE-2014-8631
Comment 91•10 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.
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•