488783 - Tags list no longer sorted (alphabetized) in latest Shiretoko nightly

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Dave Garrett]

The list under organize bookmarks -> tags is no longer alphabetized for me in the latest nightly. (I also happen to have this list shown in my bookmarks menu itself, which is why I noticed) It works fine in a 3.5b4pre nightly from the 15th but shows jumbled on the 16th. Going back to the 15th again shows it correctly.

Sorry, but I haven't been able to reproduce this in a new profile to come up with a testcase. (yes, it does it in safe mode too)

There were a bunch of sort related bugs pushed out in this nightly. Anyone know which one might've done this?

Comment 1

[Marco Bonardo [:mak]]

yeah, looks like a regression from recent sorting changes, still have to find which.

Comment 2

[Marco Bonardo [:mak]]

it's due to bug 473157

Comment 3

[Marco Bonardo [:mak]]

taking this blocker.

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v1.0

I think that we have too many special cases for sorting, and i suggest we file a follow-up to try clean some of the sorting paths in future, the test is a clear example of what an implementer is going to hit.
Luckily that applies only to grouped queries.
The test is dedicated to grouped queries, and tests the current sorting paths they can touch (only relevant sortingModes). It is big due to special casing for different resultTypes and to avoid regressing bug 473157 (we probably need to better look at Seamonkey needs and find a cleaner way to do that for 3.6, since having 2 different ways to sort based on which sortingMode you change causes headaches).

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 373782 [details] [diff] [review]
patch v1.0

>@@ -4163,13 +4182,13 @@ nsNavHistory::GetQueryResults(nsNavHisto
>                                      paramsPresent, addParams);
>   NS_ENSURE_SUCCESS(rv,rv);
> 
>-#ifdef DEBUG_FRECENCY
>-  printf("Constructed the query: %s\n", PromiseFlatCString(queryString).get());
>-#endif
>-
>   // create statement
>   nsCOMPtr<mozIStorageStatement> statement;
>   rv = mDBConn->CreateStatement(queryString, getter_AddRefs(statement));
>+#ifdef DEBUG
>+  if (NS_FAILED(rv))
>+    printf("Places failed to create a statement from this query:\n%s\n", PromiseFlatCString(queryString).get());
>+#endif

nit: would be nice to also print the sql error itself here

>diff --git a/toolkit/components/places/src/nsNavHistoryResult.cpp b/toolkit/components/places/src/nsNavHistoryResult.cpp
>--- a/toolkit/components/places/src/nsNavHistoryResult.cpp
>+++ b/toolkit/components/places/src/nsNavHistoryResult.cpp
>@@ -2386,7 +2386,19 @@ nsNavHistoryQueryResultNode::FillChildre
>     if (comparator) {
>       nsCAutoString sortingAnnotation;
>       GetSortingAnnotation(sortingAnnotation);
>-      RecursiveSort(sortingAnnotation.get(), comparator);
>+      // Usually copntainers queries results comes already sorted from the

sp: containers

>+      // database, but some locale could have special rules to sort by title.

nit: locales

>+      // RecursiveSort won't apply these rules to containers in containers
>+      // queries because when setting sortingMode on the result we want to sort
>+      // contained items (bug 473157).
>+      // In such a case we apply the base class sorting and not our special one.
>+      if (IsContainersQuery() &&
>+          sortType == mOptions->SortingMode() &&
>+          (sortType == nsINavHistoryQueryOptions::SORT_BY_TITLE_ASCENDING ||
>+           sortType == nsINavHistoryQueryOptions::SORT_BY_TITLE_DESCENDING))
>+        nsNavHistoryContainerResultNode::RecursiveSort(sortingAnnotation.get(), comparator);
>+      else
>+        RecursiveSort(sortingAnnotation.get(), comparator);
>     }
>   }
> 

ugh. please add a more detailed comment that clearly explains the difference between container and query sorting, and when one is used over the other.

>+      break;
>+    default:
>+      do_throw("Unknown sorting type required " + aExpectedSortingMode);

nit: s/required//

>+  }
>+
>+  // Make an indipendent copy of the results array and sort it.

nit: independent

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v1.1

addressed comments.

Comment 7

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/d299a5eb0f76

Comment 8

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8db008513da0

Comment 9

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b5pre) Gecko/20090505 Shiretoko/3.5b5pre