Contacts API: Expose database revision in API

RESOLVED FIXED in Firefox 23, Firefox OS v1.1hd

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: reuben, Assigned: reuben)

Tracking

(Blocks: 1 bug)

Trunk
mozilla23
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:leo+, firefox22 wontfix, firefox23 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

(Whiteboard: [fixed-in-birch])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
The Dialer needs this for performance, and it's relatively easy to expose something like that.

navigator.mozContacts.getRevision().onsuccess = function(e) {
    console.log("rev: " + e.target.result);
}
(Assignee)

Comment 1

5 years ago
Created attachment 740547 [details] [diff] [review]
Expose DB revision in the API
Attachment #740547 - Flags: review?(anygregor)
Blocks: 847406
Thanks Reuben and Gregor!

Requesting leo+ since this blocks a blocker.
blocking-b2g: --- → leo?
Comment on attachment 740547 [details] [diff] [review]
Expose DB revision in the API

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

Adding Jonas for the API change review.

::: dom/contacts/ContactManager.js
@@ +544,5 @@
> +        if (DEBUG) debug("new revision: " + msg.revision);
> +        req = this.getRequest(msg.requestID);
> +        if (req) {
> +          Services.DOMRequest.fireSuccess(req, msg.revision);
> +        }

break;

::: dom/contacts/fallback/ContactDB.jsm
@@ +318,5 @@
>          db.createObjectStore(SAVED_GETALL_STORE_NAME);
> +      } else if (currVersion == 8) {
> +        if (DEBUG) debug("Adding object store for database revision");
> +        db.createObjectStore(REVISION_KEY).put(0, REVISION_KEY);
> +      }

We should also add a general way to increase the revision number after every upgrade.

::: dom/contacts/tests/test_contacts_basics.html
@@ +283,5 @@
>      req = navigator.mozContacts.save(createResult1);
>      req.onsuccess = function () {
>        ok(createResult1.id, "The contact now has an ID.");
>        sample_id1 = createResult1.id;
> +      checkRevision(2, "Revision was incremented on save", next);

please check the revision increment for deletion as well.
Attachment #740547 - Flags: superreview?(jonas)
If I understand correctly, by "database version" you mean the "schema version", right? I do not understand how this could increase performances. What kind of assumptions can be made to improve performances based on that?

Also, we should keep in mind that this API is exposed to third-party applications.
(Assignee)

Comment 5

5 years ago
(In reply to Mounir Lamouri (:mounir) from comment #4)
> If I understand correctly, by "database version" you mean the "schema
> version", right?

No, the database revision is a positive integer that is incremented whenever a contact is removed, changed or added.
(Assignee)

Comment 6

5 years ago
Fernando, can you clarify (or link to the Gaia bug) the Dialer use case for the API? Bug 847406 doesn't have a lot of explanation.
Flags: needinfo?(ferjmoreno)
(In reply to Mounir Lamouri (:mounir) from comment #4)
> If I understand correctly, by "database version" you mean the "schema
> version", right? I do not understand how this could increase performances.
> What kind of assumptions can be made to improve performances based on that?
> 

The dialer caches the names of contacts for the call log. When the application starts, it checks if the contacts DB has changed. The performance improvement comes from not having to check the contacts DB for every single call log entry.
Can't we have revision as a sync property or would that cost a lot to keep that attribute in sync with every ContactManager instance?
Gregor already explained the call log use case.

Another use case might be the contacts app locally storing the HTML generated after processing the contacts API data and reusing it in consecutive executions if no change is detected in the contacts API database.

A sync property would be great, if possible.
Flags: needinfo?(ferjmoreno)
(In reply to Fernando Jiménez Moreno [:ferjm] (use needinfo instead of CC, please) from comment #9)
> Gregor already explained the call log use case.
> 
> Another use case might be the contacts app locally storing the HTML
> generated after processing the contacts API data and reusing it in
> consecutive executions if no change is detected in the contacts API database.
> 
> A sync property would be great, if possible.

I am not a big fan of the sync property idea. It might be used during app startup and we have seen that sync calls to the parent can take >1sec. The current use-case is in the dialer where we already have troubles with startup time.
We could cache it in the child for consecutive calls but why would an app ask for it more than 1 time? We have change events for this.
Blocks: 746443
(Assignee)

Comment 11

5 years ago
Created attachment 741927 [details] [diff] [review]
Expose DB revision in the API

Updated to address initial comments.
Attachment #740547 - Attachment is obsolete: true
Attachment #740547 - Flags: superreview?(jonas)
Attachment #740547 - Flags: review?(anygregor)
Attachment #741927 - Flags: superreview?(jonas)
Attachment #741927 - Flags: review?(anygregor)
(Assignee)

Updated

5 years ago
Attachment #741927 - Flags: superreview?(jonas) → superreview?(mounir)

Updated

5 years ago
blocking-b2g: leo? → leo+
Blocking a blocker hence leo+
(In reply to Gregor Wagner [:gwagner] from comment #10)
> We could cache it in the child for consecutive calls but why would an app
> ask for it more than 1 time? We have change events for this.

They will. I believe that making simple actions simple to use is always better.

I guess it's not very important to fight on this given that we will anyway deprecate this API sooner or later.
Attachment #741927 - Flags: superreview?(mounir) → superreview+
(Assignee)

Comment 14

5 years ago
Created attachment 743288 [details] [diff] [review]
Expose DB revision in the API

Carrying sr=mounir
Attachment #741927 - Attachment is obsolete: true
Attachment #741927 - Flags: review?(anygregor)
Attachment #743288 - Flags: superreview+
Attachment #743288 - Flags: review?(anygregor)
Comment on attachment 743288 [details] [diff] [review]
Expose DB revision in the API

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

::: dom/contacts/ContactManager.js
@@ +772,5 @@
> +    }.bind(this);
> +
> +    let cancelCallback = function() {
> +      Services.DOMRequest.fireError(request);
> +    };

no bind needed here?

::: dom/contacts/fallback/ContactDB.jsm
@@ +23,5 @@
>  const STORE_NAME = "contacts";
>  const SAVED_GETALL_STORE_NAME = "getallcache";
>  const CHUNK_SIZE = 20;
>  const CHUNK_INTERVAL = 500;
> +const REVISION_KEY = "revision";

You should reflect it in the name if you are using it as key and store.
Attachment #743288 - Flags: review?(anygregor) → review+
(Assignee)

Comment 16

5 years ago
(In reply to Gregor Wagner [:gwagner] from comment #15)
> ::: dom/contacts/ContactManager.js
> @@ +772,5 @@
> > +    }.bind(this);
> > +
> > +    let cancelCallback = function() {
> > +      Services.DOMRequest.fireError(request);
> > +    };
> 
> no bind needed here?

No this, no bind(this).

> ::: dom/contacts/fallback/ContactDB.jsm
> @@ +23,5 @@
> >  const STORE_NAME = "contacts";
> >  const SAVED_GETALL_STORE_NAME = "getallcache";
> >  const CHUNK_SIZE = 20;
> >  const CHUNK_INTERVAL = 500;
> > +const REVISION_KEY = "revision";
> 
> You should reflect it in the name if you are using it as key and store.

I split it in two.

Thanks!

https://hg.mozilla.org/projects/birch/rev/15c489d2e9d2
Whiteboard: [fixed-in-birch]
https://hg.mozilla.org/mozilla-central/rev/15c489d2e9d2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
This will need a branch-specific patch. Thanks!
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/releases/mozilla-b2g18/rev/03f5da3acb5b
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix

Updated

5 years ago
Flags: in-moztrap-
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/44de5fb4e8d0
status-b2g-v1.1hd: --- → fixed
status-firefox22: --- → wontfix
status-firefox23: --- → fixed
You need to log in before you can comment on or make changes to this bug.