879698 - [Contacts API] Allow 'higherThan' and 'lowerThan' options in nsIContactFindOptions.filterOps

Status: RESOLVED WONTFIX

(Core Graveyard :: DOM: Contacts, defect, P4)

Description

[Fernando Jiménez Moreno [:ferjm]]

After bug 864556 landed, a valid use case for this would be an application locally storing the nsIDOMContact.updated value in its cache and getting the oldest value as soon as its cache revision differs from the Contacts API DB one. In that case, the app could request the list of contacts updated after that date and refresh its cache according to the query result. This way we would avoid forcing the app to get the whole list of contacts.

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Blocks bug 862385 which is leo+, so leo?

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Attachment: wip

I'd like to write a few more tests before asking for review, but I could use some feedback in the meantime :)

Comment 3

[Reuben Morais [:reuben]]

Comment on attachment 758615 [details] [diff] [review]
wip

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +362,5 @@
> +        objectStore.createIndex("birthday", "properties.bday", {
> +          multiEntry: true
> +        });
> +        objectStore.createIndex("anniversary", "properties.anniversary", {
> +          multiEntry: true

Bday and anniversary are not arrays, why do you need multiEntry here?

@@ +368,4 @@
>        }
>  
>        // Increment the DB revision on future schema changes as well
> +      if (currVersion > 11) {

You shouldn't change this one, it's supposed to increment the revision for future schema changes, starting from version 10.

@@ +826,5 @@
>      if (DEBUG) debug("ContactDB:find val:" + aOptions.filterValue + " by: " + aOptions.filterBy + " op: " + aOptions.filterOp);
>      let self = this;
>      this.newTxn("readonly", STORE_NAME, function (txn, store) {
> +      let filterOps = ["equals", "contains", "match", "startsWith", "upper",
> +                       "lower"];

Maybe we should go with lessThan and largerThan? Upper and lower makes me think of letter cases.

::: dom/contacts/tests/test_contacts_basics.html
@@ +730,5 @@
> +    req = mozContacts.find(options);
> +    req.onsuccess = function () {
> +      is(req.result.length, 1, "Found exactly 1 contact.");
> +      findResult1 = req.result[0];
> +      ok(findResult1.id == sample_id1, "Same ID");

nit: I know it's not your fault, you just copied the existing tests, but please replace these ok() calls with is() :)

Comment 4

[Gregor Wagner [:gwagner]]

Comment on attachment 758615 [details] [diff] [review]
wip

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

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

You have to check if you already have an objectstore here.

@@ +368,4 @@
>        }
>  
>        // Increment the DB revision on future schema changes as well
> +      if (currVersion > 11) {

We should have a better comment here saying "Don't change"

@@ +888,5 @@
>          let index = store.index("telMatch");
>          let normalized = PhoneNumberUtils.normalize(options.filterValue,
>                                                      /*numbersOnly*/ true);
>          request = index.mozGetAll(normalized, limit);
> +      } else if (options.filterOp ==="upper" || options.filterOp === "lower") {

nit: space after ===

Comment 5

[Andrew Overholt [:overholt]]

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo instead of CC, please) from comment #1)
> Blocks bug 862385 which is leo+, so leo?

leo+

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Actually, this doesn't block bug 862385 anymore. Sorry for the change of plans.

I'll still work on this though.

Comment 7

[Alex Keybl [:akeybl]]

What user scenario would we consider blocking this for at this point? Or is this just leo-

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

I would say that this is a leo-. Sorry for throwing it again for triage, but I have not the permissions to set the leo- flag myself.

Comment 9

[Wayne Chang [:wchang]]

Thanks Fernando, leo-ing per comment 8

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Comment 11

[Gregor Wagner [:gwagner]]

Comment on attachment 762656 [details] [diff] [review]
v1

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

Looks good! Thanks!

::: dom/contacts/fallback/ContactDB.jsm
@@ +363,5 @@
> +        }
> +
> +        objectStore.createIndex("published", "published");
> +        objectStore.createIndex("updated", "updated");
> +        objectStore.createIndex("birthday", "properties.bday");

I think we should stick with our field names: bday

::: dom/contacts/tests/test_contacts_basics.html
@@ +1621,5 @@
> +      ok(true, "Expected error");
> +      next();
> +    };
> +  },
> +  function () {

Please also add a test where we filter by "tel" and another field so we end up in the onerror callback.

Comment 12

[Mike Lee [:mlee]]

Is this still being worked on?

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

I am not currently working on this, so feel free to take it.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

DOM: Contacts isn't used anymore. 
Closing all remaining bugs.