Last Comment Bug 750768 - Contacts API: add DB modification event
: Contacts API: add DB modification event
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Gregor Wagner [:gwagner]
:
Mentors:
Depends on:
Blocks: b2g-contact 836170
  Show dependency treegraph
 
Reported: 2012-05-01 09:34 PDT by Gregor Wagner [:gwagner]
Modified: 2013-01-29 19:48 PST (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (13.12 KB, patch)
2012-05-01 16:54 PDT, Gregor Wagner [:gwagner]
fabrice: review+
Details | Diff | Splinter Review
patch (20.01 KB, patch)
2012-05-07 17:46 PDT, Gregor Wagner [:gwagner]
fabrice: review+
Details | Diff | Splinter Review

Description Gregor Wagner [:gwagner] 2012-05-01 09:34:28 PDT
Content should get informed when something changes in the contacts DB.
Comment 1 Andrew Sutherland [:asuth] 2012-05-01 10:14:45 PDT
(In reply to Gregor Wagner [:gwagner] from comment #0)
> Content should get informed when something changes in the contacts DB.

What level of granularity and how much coordination?

1) Hey, something changed!  But no details, so the query would need to be reissued.
2) Hey, something that you have an active paged listing of changed (a la bug 746439.)  The assumption would be that the UI only shows the current page of data it is exposing.  This might entail enhancements to the paging UI to cover infinite-scrolling lists cases where you'd actually want to have 2 pages at a time subscribed to.
3) Full description of the change that is always sent because there's no memory of what the UI is showing.
4) Full description, but only for what the UI is showing.  Accomplished by modifying the API so the contacts back-end knows what data is being displayed in the UI.  For example, assume all data requested is still of interest until content explicitly indicates it is done with the data.

My interest is for the e-mail app's consumption, and also so I can attempt to make the (non-going to be a WebAPI) email API between its backend and its UI consistent where reasonable.
Comment 2 Gregor Wagner [:gwagner] 2012-05-01 10:42:23 PDT
(In reply to Andrew Sutherland (:asuth) from comment #1)
> (In reply to Gregor Wagner [:gwagner] from comment #0)
> > Content should get informed when something changes in the contacts DB.
> 
> What level of granularity and how much coordination?
> 
> 1) Hey, something changed!  But no details, so the query would need to be
> reissued.
> 2) Hey, something that you have an active paged listing of changed (a la bug
> 746439.)  The assumption would be that the UI only shows the current page of
> data it is exposing.  This might entail enhancements to the paging UI to
> cover infinite-scrolling lists cases where you'd actually want to have 2
> pages at a time subscribed to.
> 3) Full description of the change that is always sent because there's no
> memory of what the UI is showing.
> 4) Full description, but only for what the UI is showing.  Accomplished by
> modifying the API so the contacts back-end knows what data is being
> displayed in the UI.  For example, assume all data requested is still of
> interest until content explicitly indicates it is done with the data.
> 
> My interest is for the e-mail app's consumption, and also so I can attempt
> to make the (non-going to be a WebAPI) email API between its backend and its
> UI consistent where reasonable.

My initial idea is to go with option 1) and maybe add the uuid of the modified contact to the event.

For the settingsDB changing event we add more information and we allow to listen to a certain setting change but I don't think it makes sense for contacts. 
2) - 4) would require some state machine and I don't think this is a good idea to add this feature before we know how our multiprocess model looks like.
Comment 3 Gregor Wagner [:gwagner] 2012-05-01 16:54:44 PDT
Created attachment 620138 [details] [diff] [review]
Patch

The additional review box disappeared...
Jonas please review the interface changes and the rest is for fabrice :) 

So this patch fires a oncontactchange event with the contactID and a reason.
A reason can be "save" or "remove". If a new contact is added, the reason is also "save".

The internal mozContacts.clear also fires an oncontactchange event with the reason "remove" but without a contactID.
Comment 4 [:fabrice] Fabrice Desré 2012-05-02 18:18:04 PDT
Comment on attachment 620138 [details] [diff] [review]
Patch

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

r=me with nits addressed.

::: dom/contacts/ContactManager.js
@@ +190,5 @@
> +  set oncontactchange(aCallback) {
> +    if (this.hasPrivileges)
> +      this._oncontactchange = aCallback;
> +    else
> +      throw Components.results.NS_ERROR_NOT_IMPLEMENTED;

Nit: maybe we should throw NS_ERROR_FAILURE ?

@@ +306,5 @@
> +          } else {
> +            debug("reason remove" + aMessage.name);
> +            reason = "remove";
> +          }
> +

let reason = aMessage.name == "Contact:Save:Return:OK" ? "save" : "remove";

::: dom/interfaces/contacts/nsIDOMContactManager.idl
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "domstubs.idl"
>  #include "nsIDOMContactProperties.idl"
> +#include "nsIDOMEventTarget.idl"

Ununsed
Comment 5 Mike Conley (:mconley) - (Needinfo me!) 2012-05-03 09:36:34 PDT
Am I interpreting this correctly in that only a single oncontactchange function can be set?

I can easily imagine several applications simultaneously wanting change event notifications.

Perhaps it'd be better to be able to register / unregister functions, like we do with addEventListener.

Or am I misunderstanding the patch here?
Comment 6 Etienne Segonzac (:etienne) 2012-05-03 09:45:53 PDT
(In reply to Gregor Wagner [:gwagner] from comment #3)
> So this patch fires a oncontactchange event with the contactID and a reason.
> A reason can be "save" or "remove". If a new contact is added, the reason is
> also "save".
> 
> The internal mozContacts.clear also fires an oncontactchange event with the
> reason "remove" but without a contactID.

FWIW I think this will cover all the use cases we have now.
Comment 7 Gregor Wagner [:gwagner] 2012-05-03 10:15:32 PDT
(In reply to Mike Conley (:mconley) from comment #5)
> Am I interpreting this correctly in that only a single oncontactchange
> function can be set?
> 
> I can easily imagine several applications simultaneously wanting change
> event notifications.
> 
> Perhaps it'd be better to be able to register / unregister functions, like
> we do with addEventListener.
> 
> Or am I misunderstanding the patch here?


Should work fine because the ContactManager is per window.navigator.
Comment 8 Mike Conley (:mconley) - (Needinfo me!) 2012-05-03 10:16:44 PDT
(In reply to Gregor Wagner [:gwagner] from comment #7)
> Should work fine because the ContactManager is per window.navigator.

Ah, k, thanks for clearing that up.
Comment 9 Jonas Sicking (:sicking) PTO Until July 5th 2012-05-03 17:55:23 PDT
Rather than "save" and "remove", can we have "create", "update" and "remove"?
Comment 10 Gregor Wagner [:gwagner] 2012-05-03 18:11:54 PDT
Yeah I can see what I can do. The difference between create and update is the tricky part.
Comment 11 Gregor Wagner [:gwagner] 2012-05-03 19:41:15 PDT
The other question is if it really makes sense to differentiate. An App would have to reload all contacts anyways if something changes. Or is there a realistic use-case where a differentiation would help.

Lets say an app has an unknown number. It always has to perform a new search if a contact gets created or modified.
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2012-05-03 23:46:54 PDT
The case I was thinking of is if an app has contact information for a number of people in-memory, or displayed on screen or something similar. If a contact is updated they'll need to update that information. If a contact is created they don't.
Comment 13 Gregor Wagner [:gwagner] 2012-05-07 17:46:33 PDT
Created attachment 621810 [details] [diff] [review]
patch

The reason is now stored in the request. So we don't have to pass it to the DB code.
Comment 14 Gregor Wagner [:gwagner] 2012-05-07 17:46:56 PDT
And I changed it to "create", "update" and "remove".
Comment 16 Gregor Wagner [:gwagner] 2012-05-08 14:13:26 PDT
fix red: https://hg.mozilla.org/integration/mozilla-inbound/rev/632bf4981115

Note You need to log in before you can comment on or make changes to this bug.