836423 - Contacts API: Fix oncontactchange function

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gregor Wagner [:gwagner]]

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 1

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 3

[Gregor Wagner [:gwagner]]

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

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 5

[Gregor Wagner [:gwagner]]

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

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 7

[Gregor Wagner [:gwagner]]

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

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 9

[Gregor Wagner [:gwagner]]

Attachment: patch

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

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 11

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a93c9d94a3b

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4a93c9d94a3b

Comment 13

[Gregor Wagner [:gwagner]]

Blocking a blocker (bug 836170)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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