Comment on attachment 801307 [details] [diff] [review] 2- Refactor contacts search result sorting code on Android Review of attachment 801307 [details] [diff] [review]: ----------------------------------------------------------------- LGTM! ::: mobile/android/base/ContactService.java @@ +829,5 @@ > mSortBy = sortBy.toLowerCase(); > mSortOrder = sortOrder.toLowerCase(); > } > > + private String flattenJSONArray(JSONArray array) { The flattenJSONArray() method can be static because it is a stateless helper function. @@ +830,5 @@ > mSortOrder = sortOrder.toLowerCase(); > } > > + private String flattenJSONArray(JSONArray array) { > + StringBuilder result = new StringBuilder(); Micro-optimization time! :) StringBuilder's default capacity is 16 characters. If the JSONArrays of family and given names we flatten are typically larger than 16 characters, we can give a better capacity hint. Maybe something like `array.length() * 20` with a comment?

Comment on attachment 801306 [details] [diff] [review] 1- Refactor contacts search results sorting code Review of attachment 801306 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/contacts/fallback/ContactDB.jsm @@ +934,5 @@ > + > + // Accumulate a single string from the contacts' sort properties, eg.: > + // {givenName: ["Mary", "Ann"], familyName: ["Smith"]} is "maryannsmith" > + // when sorting by "givenName", and "smithmaryann" when sorting by > + // "familyName". I don't think that works. How about given: ["Test1", "Test2"], family: ["Test3"] and given: ["Test1"], family: ["Test2", "Test3"] The sort string would always look like "Test1Test2Test3" We used this approach in the beginning but it didn't support all use-cases.

(In reply to Gregor Wagner [:gwagner] from comment #4) > Comment on attachment 801306 [details] [diff] [review] > 1- Refactor contacts search results sorting code > > Review of attachment 801306 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/contacts/fallback/ContactDB.jsm > @@ +934,5 @@ > > + > > + // Accumulate a single string from the contacts' sort properties, eg.: > > + // {givenName: ["Mary", "Ann"], familyName: ["Smith"]} is "maryannsmith" > > + // when sorting by "givenName", and "smithmaryann" when sorting by > > + // "familyName". > > I don't think that works. > How about > given: ["Test1", "Test2"], family: ["Test3"] > and given: ["Test1"], family: ["Test2", "Test3"] I can just add a space between given and family name. Also, we should have tests for this.

(In reply to Chris Peterson (:cpeterson) from comment #3) > Comment on attachment 801307 [details] [diff] [review] > 2- Refactor contacts search result sorting code on Android > > Review of attachment 801307 [details] [diff] [review]: > ----------------------------------------------------------------- > > LGTM! Thanks for the review! > ::: mobile/android/base/ContactService.java > @@ +829,5 @@ > > mSortBy = sortBy.toLowerCase(); > > mSortOrder = sortOrder.toLowerCase(); > > } > > > > + private String flattenJSONArray(JSONArray array) { > > The flattenJSONArray() method can be static because it is a stateless helper > function. Inner classes cannot define static methods or attributes. > @@ +830,5 @@ > > mSortOrder = sortOrder.toLowerCase(); > > } > > > > + private String flattenJSONArray(JSONArray array) { > > + StringBuilder result = new StringBuilder(); > > Micro-optimization time! :) StringBuilder's default capacity is 16 > characters. If the JSONArrays of family and given names we flatten are > typically larger than 16 characters, we can give a better capacity hint. > Maybe something like `array.length() * 20` with a comment? Will do.

(In reply to Reuben Morais [:reuben] from comment #5) > (In reply to Gregor Wagner [:gwagner] from comment #4) > > Comment on attachment 801306 [details] [diff] [review] > > 1- Refactor contacts search results sorting code > > > > Review of attachment 801306 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: dom/contacts/fallback/ContactDB.jsm > > @@ +934,5 @@ > > > + > > > + // Accumulate a single string from the contacts' sort properties, eg.: > > > + // {givenName: ["Mary", "Ann"], familyName: ["Smith"]} is "maryannsmith" > > > + // when sorting by "givenName", and "smithmaryann" when sorting by > > > + // "familyName". > > > > I don't think that works. > > How about > > given: ["Test1", "Test2"], family: ["Test3"] > > and given: ["Test1"], family: ["Test2", "Test3"] > > I can just add a space between given and family name. Also, we should have > tests for this. Yes we should have :)

Updated to work in the case you mentioned, plus tests.

Comment on attachment 812124 [details] [diff] [review] 1- Refactor contacts search results sorting code, v2 We found that we have to make some changes here.

We'll have to do something about this eventually, specially because the Android sorting code doesn't match ContactDB exactly. For now let's just land the tests.

Comment on attachment 8364655 [details] [diff] [review] Just the tests Review of attachment 8364655 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/contacts/tests/test_contacts_basics2.html @@ -438,5 @@ > - next(); > - }; > - req.onerror = onFailure; > - }, > - function () { Well you are removing the test-case that deals with contacts with identical fields and you replace it with tests that do the sorting based on the names. I don't think we want this.

(In reply to Gregor Wagner [:gwagner] from comment #11) > Well you are removing the test-case that deals with contacts with identical > fields and you replace it with tests that do the sorting based on the names. > I don't think we want this. What do you mean? The removed test made sure tests with the same sort property (in this case, givenName), were sorted by the published field. The first test I added does the same.

(In reply to Reuben Morais [:reuben] from comment #12) > (In reply to Gregor Wagner [:gwagner] from comment #11) > > Well you are removing the test-case that deals with contacts with identical > > fields and you replace it with tests that do the sorting based on the names. > > I don't think we want this. > > What do you mean? The removed test made sure tests with the same sort > property (in this case, givenName), were sorted by the published field. The > first test I added does the same. It does something similar but not the same. The original test does sorting with identical contacts. The new test does it with empty sort string fields.

OOOOOH, I see what you mean. This was definitely not intended :) Interdiff: diff --git a/dom/contacts/tests/test_contacts_basics2.html b/dom/contacts/tests/test_contacts_basics2.html --- a/dom/contacts/tests/test_contacts_basics2.html +++ b/dom/contacts/tests/test_contacts_basics2.html @@ -435,17 +435,17 @@ var steps = [ { name: ["8"], givenName: ["a"] }, ]; saveContacts(contacts, function() { - var options = {sortBy: "familyName", + var options = {sortBy: "givenName", sortOrder: "ascending"}; req = navigator.mozContacts.find(options); req.onsuccess = function () { is(req.result.length, 8, "8 results");

Comment on attachment 8364857 [details] [diff] [review] Just the tests, v2 Review of attachment 8364857 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/contacts/tests/test_contacts_basics2.html @@ +435,5 @@ > + // props, but they're equal. > + { name: ["5"], givenName: ["a"] }, > + { name: ["6"], givenName: ["a"] }, > + { name: ["7"], givenName: ["a"] }, > + { name: ["8"], givenName: ["a"] }, Now there is just one more difference :) We iterate over all properties with the old version. We take given and family name afaik. With the new version we just compare one single property.

Oh wow, I completely forgot about this patch :(

This code no longer exists.