Last Comment Bug 734198 - Contacts API: Add Sorting
: Contacts API: Add Sorting
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: Gregor Wagner [:gwagner]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-08 11:33 PST by Gregor Wagner [:gwagner]
Modified: 2012-04-03 02:08 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (9.10 KB, patch)
2012-03-30 17:41 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Splinter Review
patch (12.76 KB, patch)
2012-04-02 12:51 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Splinter Review
patch (14.89 KB, patch)
2012-04-02 15:35 PDT, Gregor Wagner [:gwagner]
bent.mozilla: review+
Details | Diff | Splinter Review

Description Gregor Wagner [:gwagner] 2012-03-08 11:33:21 PST
We should support sorting by field for find results. Like { sortBy: familyName, sortOrder: descending}.
Comment 1 Gregor Wagner [:gwagner] 2012-03-30 17:41:42 PDT
Created attachment 611097 [details] [diff] [review]
patch
Comment 2 Gregor Wagner [:gwagner] 2012-04-02 12:51:25 PDT
Created attachment 611559 [details] [diff] [review]
patch
Comment 3 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-04-02 13:14:01 PDT
Comment on attachment 611559 [details] [diff] [review]
patch

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +377,5 @@
>        }
>      }
>  
> +    // Sorting functions takes care of limit if set.
> +    let limit = options.sortBy !== 'undefined' ? null : options.filterLimit;

Hm, should that be testing against 'undefined'? Looks like you always define it to '' if it wasn't passed in...

Also, any time you are going to do something based on both branches of an if statement (in this case the ? operator) I think it's nicer to test against a non-negated condition (e.g. |options.sortBy === 'undefined' ? options.filterLimit : null|).

@@ +413,5 @@
>      debug("ContactDB:_findAll:  " + JSON.stringify(options));
>      if (!txn.result)
>        txn.result = {};
> +    // Sorting functions takes care of limit if set.
> +    let limit = options.sortBy !== 'undefined' ? null : options.filterLimit;

here too.
Comment 4 Gregor Wagner [:gwagner] 2012-04-02 15:32:41 PDT
(In reply to ben turner [:bent] from comment #3)
> Comment on attachment 611559 [details] [diff] [review]
> patch
> 
> Review of attachment 611559 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/contacts/fallback/ContactDB.jsm
> @@ +377,5 @@
> >        }
> >      }
> >  
> > +    // Sorting functions takes care of limit if set.
> > +    let limit = options.sortBy !== 'undefined' ? null : options.filterLimit;
> 
> Hm, should that be testing against 'undefined'? Looks like you always define
> it to '' if it wasn't passed in...
> 
> Also, any time you are going to do something based on both branches of an if
> statement (in this case the ? operator) I think it's nicer to test against a
> non-negated condition (e.g. |options.sortBy === 'undefined' ?
> options.filterLimit : null|).

Actually the options object isn't created by the constructor. So the values are undefined.
Comment 5 Gregor Wagner [:gwagner] 2012-04-02 15:35:50 PDT
Created attachment 611622 [details] [diff] [review]
patch
Comment 6 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-04-02 15:58:31 PDT
Comment on attachment 611622 [details] [diff] [review]
patch

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

::: dom/contacts/ContactManager.js
@@ +18,5 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/DOMRequestHelper.jsm");
>  
> +XPCOMUtils.defineLazyGetter(Services, "DOMRequest", function() {

This change doesn't seem strictly related, but ok.
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2012-04-03 02:08:42 PDT
https://hg.mozilla.org/mozilla-central/rev/3ad6cc527d5c

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