Closed Bug 750768 Opened 9 years ago Closed 9 years ago

Contacts API: add DB modification event

Categories

(Core :: DOM: Device Interfaces, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: gwagner, Assigned: gwagner)

References

Details

Attachments

(1 file, 1 obsolete file)

Content should get informed when something changes in the contacts DB.
Blocks: b2g-contact
(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.
(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.
Assignee: nobody → anygregor
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #620138 - Flags: superreview?(jonas)
Attachment #620138 - Flags: review?(fabrice)
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
Attachment #620138 - Flags: review?(fabrice) → review+
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?
(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.
(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.
(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.
Rather than "save" and "remove", can we have "create", "update" and "remove"?
Yeah I can see what I can do. The difference between create and update is the tricky part.
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.
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.
Attached patch patchSplinter Review
The reason is now stored in the request. So we don't have to pass it to the DB code.
Attachment #620138 - Attachment is obsolete: true
Attachment #620138 - Flags: superreview?(jonas)
Attachment #621810 - Flags: review?(fabrice)
And I changed it to "create", "update" and "remove".
Attachment #621810 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/2e7efc12fd0b
https://hg.mozilla.org/mozilla-central/rev/632bf4981115
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.