Closed Bug 836423 Opened 8 years ago Closed 8 years ago

Contacts API: Fix oncontactchange function

Categories

(Core :: DOM: Device Interfaces, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
blocking-b2g tef+
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- fixed
b2g18-v1.0.1 --- fixed

People

(Reporter: gwagner, Assigned: gwagner)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 836170
Attached patch patch (obsolete) — Splinter Review
Attachment #708332 - Flags: review?(bent.mozilla)
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.
Attached patch patchSplinter Review
I didn't change the register code but I fixed the reason handling.
Attachment #708332 - Attachment is obsolete: true
Attachment #708332 - Flags: review?(bent.mozilla)
Attachment #709136 - Flags: review?(bent.mozilla)
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.
Attachment #709136 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/4a93c9d94a3b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Blocking a blocker (bug 836170)
blocking-b2g: --- → tef?
blocking-b2g: tef? → tef+
Keywords: checkin-needed
No longer depends on: 945948
You need to log in before you can comment on or make changes to this bug.