Currently we only inform the app that modifies or deletes a contact about the change. That's pretty useless. I think this regressed when we moved away from the message manager broadcasting approach.

Comment on attachment 708332 [details] [diff] [review] patch Review of attachment 708332 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/contacts/ContactManager.js @@ +332,5 @@ > set oncontactchange(aCallback) { > if (DEBUG) debug("set oncontactchange"); > let allowCallback = function() { > + if (!this._oncontactchange) { > + cpmm.sendAsyncMessage("Contacts:RegisterForMessages"); This doesn't handle the case where oncontactchange is set to null. @@ +437,1 @@ > default: Nit: Since you're here kill that extra whitespace @@ +551,5 @@ > > remove: function removeContact(aRecord) { > let request; > request = this.createRequest(); > + let options = { id: aRecord.id, reason: "remove" }; I don't see why you need to send the reason here. Can't you just know the reason in the parent based on the message title? (e.g. "Contacts:Remove"). Then you wouldn't have to send this only to send it right back. Here and in the other places. ::: dom/contacts/fallback/ContactService.jsm @@ +79,5 @@ > return true; > }, > > + broadcastMessage: function broadcastMessage(aMsgName, aContent) { > + debug("Broadast!!!"); Nit: better message, and hide behind DEBUG check @@ +81,5 @@ > > + broadcastMessage: function broadcastMessage(aMsgName, aContent) { > + debug("Broadast!!!"); > + this.children.forEach(function(msgMgr) { > + dump("SENDDDD\n"); Nit: remove @@ +229,5 @@ > + break; > + case "child-process-shutdown": > + if (DEBUG) debug("Unregister"); > + let index; > + if ((index = this.children.indexOf(mm)) != -1) { Nit: let index = this.children.indexOf(mm); if (index != -1) {

(In reply to ben turner [:bent] from comment #2) > Comment on attachment 708332 [details] [diff] [review] > patch > > Review of attachment 708332 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/contacts/ContactManager.js > @@ +332,5 @@ > > set oncontactchange(aCallback) { > > if (DEBUG) debug("set oncontactchange"); > > let allowCallback = function() { > > + if (!this._oncontactchange) { > > + cpmm.sendAsyncMessage("Contacts:RegisterForMessages"); > > This doesn't handle the case where oncontactchange is set to null. > We remove it in the parent once the child process shuts down. We can add an unregister function if the user sets the callback to null but I can't think of any real-world use-case where this would matter.

(In reply to Gregor Wagner [:gwagner] from comment #3) > We remove it in the parent once the child process shuts down. We can add an > unregister function if the user sets the callback to null but I can't think > of any real-world use-case where this would matter. Well, sure, but if the user does this: oncontactchange = function() { }; oncontactchange = null; oncontactchange = function() { }; You'll register twice.

(In reply to ben turner [:bent] from comment #4) > (In reply to Gregor Wagner [:gwagner] from comment #3) > > We remove it in the parent once the child process shuts down. We can add an > > unregister function if the user sets the callback to null but I can't think > > of any real-world use-case where this would matter. > > Well, sure, but if the user does this: > > oncontactchange = function() { }; > oncontactchange = null; > oncontactchange = function() { }; > > You'll register twice. I call register twice, but in the parent I only add it once.

(In reply to Gregor Wagner [:gwagner] from comment #5) > I call register twice, but in the parent I only add it once. Oh, hm. I see. Wouldn't it be simpler to keep some other state variable around to ensure you only send the register message once?

(In reply to ben turner [:bent] from comment #6) > (In reply to Gregor Wagner [:gwagner] from comment #5) > > I call register twice, but in the parent I only add it once. > > Oh, hm. I see. Wouldn't it be simpler to keep some other state variable > around to ensure you only send the register message once? No, I don't think so. We already use the same approach for settings. Why add another one?

(In reply to Gregor Wagner [:gwagner] from comment #7) > No, I don't think so. We already use the same approach for settings. Why add > another one? Well, it's unnecessary IPC traffic. We should fix both :) But we can do that later if you want.

I didn't change the register code but I fixed the reason handling.

Comment on attachment 709136 [details] [diff] [review] patch Review of attachment 709136 [details] [diff] [review]: ----------------------------------------------------------------- Ok, looks good with the underscore problem we talked about on irc fixed.

Blocking a blocker (bug 836170)

https://hg.mozilla.org/releases/mozilla-b2g18/rev/510d148bee28 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/4c5d9b5cf80a