Bug 488783 - Tags list no longer sorted (alphabetized) in latest Shiretoko nightly, r=dietrich a=blocking191
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 22 Apr 2009 14:22:53 +0200
changeset 27616 d299a5eb0f76b527e617efaf6e10aa3d2dc98927
parent 27615 fba47e2a4b289ca754c8ebad07702b43e56701b4
child 27617 d0699dd384f0ed6c4577b3f7730dccc8c5c44130
push id6653
push usermak77@bonardo.net
push dateWed, 22 Apr 2009 12:30:26 +0000
treeherdermozilla-central@d299a5eb0f76 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdietrich, blocking191
bugs488783
milestone1.9.2a1pre
Bug 488783 - Tags list no longer sorted (alphabetized) in latest Shiretoko nightly, r=dietrich a=blocking191
toolkit/components/places/src/nsNavHistory.cpp
toolkit/components/places/src/nsNavHistoryResult.cpp
toolkit/components/places/tests/queries/head_queries.js
toolkit/components/places/tests/queries/test_containersQueries_sorting.js
toolkit/components/places/tests/unit/test_history_sidebar.js
--- a/toolkit/components/places/src/nsNavHistory.cpp
+++ b/toolkit/components/places/src/nsNavHistory.cpp
@@ -86,16 +86,17 @@
 #include "mozIStorageConnection.h"
 #include "mozIStorageFunction.h"
 #include "mozIStoragePendingStatement.h"
 #include "mozIStorageService.h"
 #include "mozIStorageStatement.h"
 #include "mozIStorageValueArray.h"
 #include "mozStorageCID.h"
 #include "mozStorageHelper.h"
+#include "mozIStorageError.h"
 #include "nsPlacesTriggers.h"
 #include "nsAppDirectoryServiceDefs.h"
 #include "nsIIdleService.h"
 #include "nsILivemarkService.h"
 
 #include "nsMathUtils.h" // for NS_ceilf()
 
 // Microsecond timeout for "recent" events such as typed and bookmark following.
@@ -3019,16 +3020,19 @@ private:
 
   nsresult Where();
   nsresult GroupBy();
   nsresult OrderBy();
   nsresult Limit();
 
   void OrderByColumnIndexAsc(PRInt32 aIndex);
   void OrderByColumnIndexDesc(PRInt32 aIndex);
+  // Use these if you want a case insensitive sorting.
+  void OrderByTextColumnIndexAsc(PRInt32 aIndex);
+  void OrderByTextColumnIndexDesc(PRInt32 aIndex);
 
   const nsCString& mConditions;
   PRBool mUseLimit;
 
   PRUint16 mResultType;
   PRUint16 mQueryType;
   PRBool mIncludeHidden;
   PRUint16 mRedirectsMode;
@@ -3709,21 +3713,21 @@ PlacesSQLQueryBuilder::SelectAsSite()
             "AND visit_count > 0 "
           "UNION ALL "
           "SELECT DISTINCT rev_host FROM moz_places "
           "WHERE id NOT IN (SELECT id FROM moz_places_temp) "
             "AND hidden <> 1 "
             "AND rev_host <> '.' "
             "AND visit_count > 0 "
         ") "
-      "ORDER BY 1 ASC)",
+      "ORDER BY 1 ASC) ",
       nsINavHistoryQueryOptions::RESULTS_AS_URI,
-      nsINavHistoryQueryOptions::SORT_BY_TITLE_ASCENDING,
+      mSortingMode,
       nsINavHistoryQueryOptions::RESULTS_AS_URI,
-      nsINavHistoryQueryOptions::SORT_BY_TITLE_ASCENDING);
+      mSortingMode);
   // Now we need to use the filters - we need them all
   } else {
 
     mQueryString = nsPrintfCString(4096,
       "SELECT DISTINCT null, "
              "'place:type=%ld&sort=%ld&domain=&domainIsHost=true"
                "&beginTime='||:begin_time||'&endTime='||:end_time, "
              ":localhost, :localhost, null, null, null, null, null "
@@ -3797,21 +3801,21 @@ PlacesSQLQueryBuilder::SelectAsSite()
         "SELECT DISTINCT get_unreversed_host(rev_host) AS host "
         "FROM moz_places_temp h "
         "JOIN moz_historyvisits_temp v ON v.place_id = h.id "        
         "WHERE h.hidden <> 1 AND h.rev_host <> '.' "
           "AND h.visit_count > 0 "
           "{QUERY_OPTIONS} "
           "{ADDITIONAL_CONDITIONS} "        
         "ORDER BY 1 ASC "
-      ")",
+      ") ",
       nsINavHistoryQueryOptions::RESULTS_AS_URI,
-      nsINavHistoryQueryOptions::SORT_BY_TITLE_ASCENDING,
+      mSortingMode,
       nsINavHistoryQueryOptions::RESULTS_AS_URI,
-      nsINavHistoryQueryOptions::SORT_BY_TITLE_ASCENDING);
+      mSortingMode);
   }
 
   return NS_OK;
 }
 
 nsresult
 PlacesSQLQueryBuilder::SelectAsTag()
 {
@@ -3900,24 +3904,28 @@ PlacesSQLQueryBuilder::OrderBy()
   // our later sort will be basically free. The DB can sort these for free
   // most of the time anyway, because it has indices over these items.
   switch(mSortingMode)
   {
     case nsINavHistoryQueryOptions::SORT_BY_NONE:
       break;
     case nsINavHistoryQueryOptions::SORT_BY_TITLE_ASCENDING:
     case nsINavHistoryQueryOptions::SORT_BY_TITLE_DESCENDING:
-      // the DB doesn't have indices on titles, and we need to do special
-      // sorting for locales. This type of sorting is done only at the end.
-      //
       // If the user wants few results, we limit them by date, necessitating
-      // a sort by date here (see the IDL definition for maxResults). We'll
-      // still do the official sort by title later.
+      // a sort by date here (see the IDL definition for maxResults).
+      // Otherwise we will do actual sorting by title, but since we could need
+      // to special sort for some locale we will repeat a second sorting at the
+      // end in nsNavHistoryResult, that should be faster since the list will be
+      // almost ordered.
       if (mMaxResults > 0)
         OrderByColumnIndexDesc(nsNavHistory::kGetInfoIndex_VisitDate);
+      else if (mSortingMode == nsINavHistoryQueryOptions::SORT_BY_TITLE_ASCENDING)
+        OrderByTextColumnIndexAsc(nsNavHistory::kGetInfoIndex_Title);
+      else
+        OrderByTextColumnIndexDesc(nsNavHistory::kGetInfoIndex_Title);
       break;
     case nsINavHistoryQueryOptions::SORT_BY_DATE_ASCENDING:
       OrderByColumnIndexAsc(nsNavHistory::kGetInfoIndex_VisitDate);
       break;
     case nsINavHistoryQueryOptions::SORT_BY_DATE_DESCENDING:
       OrderByColumnIndexDesc(nsNavHistory::kGetInfoIndex_VisitDate);
       break;
     case nsINavHistoryQueryOptions::SORT_BY_URI_ASCENDING:
@@ -3964,16 +3972,28 @@ void PlacesSQLQueryBuilder::OrderByColum
   mQueryString += nsPrintfCString(128, " ORDER BY %d ASC", aIndex+1);
 }
 
 void PlacesSQLQueryBuilder::OrderByColumnIndexDesc(PRInt32 aIndex)
 {
   mQueryString += nsPrintfCString(128, " ORDER BY %d DESC", aIndex+1);
 }
 
+void PlacesSQLQueryBuilder::OrderByTextColumnIndexAsc(PRInt32 aIndex)
+{
+  mQueryString += nsPrintfCString(128, " ORDER BY %d COLLATE NOCASE ASC",
+                                  aIndex+1);
+}
+
+void PlacesSQLQueryBuilder::OrderByTextColumnIndexDesc(PRInt32 aIndex)
+{
+  mQueryString += nsPrintfCString(128, " ORDER BY %d COLLATE NOCASE DESC",
+                                  aIndex+1);
+}
+
 nsresult
 PlacesSQLQueryBuilder::Limit()
 {
   if (mUseLimit && mMaxResults > 0) {
     mQueryString += NS_LITERAL_CSTRING(" LIMIT ");
     mQueryString.AppendInt(mMaxResults);
     mQueryString.AppendLiteral(" ");
   }
@@ -4158,23 +4178,31 @@ nsNavHistory::GetQueryResults(nsNavHisto
   nsCString queryString;
   PRBool paramsPresent = PR_FALSE;
   nsNavHistory::StringHash addParams;
   addParams.Init(1);
   nsresult rv = ConstructQueryString(aQueries, aOptions, queryString, 
                                      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)) {
+    nsCAutoString lastErrorString;
+    (void)mDBConn->GetLastErrorString(lastErrorString);
+    PRInt32 lastError = 0;
+    (void)mDBConn->GetLastError(&lastError);
+    printf("Places failed to create a statement from this query:\n%s\nStorage error (%ld): %s\n",
+           PromiseFlatCString(queryString).get(),
+           lastError,
+           PromiseFlatCString(lastErrorString).get());
+  }
+#endif
   NS_ENSURE_SUCCESS(rv, rv);
 
   if (paramsPresent) {
     // bind parameters
     PRInt32 i;
     for (i = 0; i < aQueries.Count(); i++) {
       rv = BindQueryClauseParameters(statement, i, aQueries[i], aOptions);
       NS_ENSURE_SUCCESS(rv, rv);
--- a/toolkit/components/places/src/nsNavHistoryResult.cpp
+++ b/toolkit/components/places/src/nsNavHistoryResult.cpp
@@ -2381,17 +2381,34 @@ nsNavHistoryQueryResultNode::FillChildre
   if (mOptions->QueryType() != nsINavHistoryQueryOptions::QUERY_TYPE_HISTORY ||
       sortType != nsINavHistoryQueryOptions::SORT_BY_NONE) {
     // once we've computed all tree stats, we can sort, because containers will
     // then have proper visit counts and dates
     SortComparator comparator = GetSortingComparator(GetSortType());
     if (comparator) {
       nsCAutoString sortingAnnotation;
       GetSortingAnnotation(sortingAnnotation);
-      RecursiveSort(sortingAnnotation.get(), comparator);
+      // Usually containers queries results comes already sorted from the
+      // database, but some locales could have special rules to sort by title.
+      // 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).
+      // Base container RecursiveSort will sort both our children and all
+      // descendants, and is used in this case because we have to do manual
+      // title sorting.
+      // Query RecursiveSort will instead only sort descendants if we are a
+      // constinaersQuery, e.g. a grouped query that will return other queries.
+      // For other type of queries it will act as the base 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);
     }
   }
 
   // if we are limiting our results remove items from the end of the
   // mChildren array after sorting. This is done for root node only.
   // note, if count < max results, we won't do anything.
   if (!mParent && mOptions->MaxResults()) {
     while ((PRUint32)mChildren.Count() > mOptions->MaxResults())
@@ -2565,19 +2582,21 @@ nsNavHistoryQueryResultNode::GetSortingA
 void
 nsNavHistoryQueryResultNode::RecursiveSort(
     const char* aData, SortComparator aComparator)
 {
   void* data = const_cast<void*>(static_cast<const void*>(aData));
 
   if (!IsContainersQuery())
     mChildren.Sort(aComparator, data);
-  else
-    for (PRInt32 i = 0; i < mChildren.Count(); i ++)
+
+  for (PRInt32 i = 0; i < mChildren.Count(); i++) {
+    if (mChildren[i]->IsContainer())
       mChildren[i]->GetAsContainer()->RecursiveSort(aData, aComparator);
+  }
 }
 
 
 // nsNavHistoryResultNode::OnBeginUpdateBatch
 
 NS_IMETHODIMP
 nsNavHistoryQueryResultNode::OnBeginUpdateBatch()
 {
--- a/toolkit/components/places/tests/queries/head_queries.js
+++ b/toolkit/components/places/tests/queries/head_queries.js
@@ -241,20 +241,16 @@ function populateDB(aArray) {
 
       if (qdata.isItemBinaryAnnotation) {
         annosvc.setItemAnnotationBinary(qdata.itemId, qdata.annoName,
                                         qdata.binaryData, qdata.binaryDataLength,
                                         qdata.annoMimeType, qdata.annoFlags,
                                         qdata.annoExpiration);
       }
 
-      if (qdata.isTag) {
-        tagssvc.tagURI(uri(qdata.uri), qdata.tagArray);
-      }
-
       if (qdata.isFavicon) {
         // Not planning on doing deep testing of favIcon service so these two
         // calls should be sufficient to get favicons into the database
         try {
           faviconsvc.setFaviconData(uri(qdata.faviconURI), qdata.favicon,
                                     qdata.faviconLen, qdata.faviconMimeType,
                                     qdata.faviconExpiration);
         } catch (ex) {}
@@ -278,16 +274,20 @@ function populateDB(aArray) {
         if (qdata.dateAdded)
           bmsvc.setItemDateAdded(itemId, qdata.dateAdded);
         if (qdata.lastModified)
           bmsvc.setItemLastModified(itemId, qdata.lastModified);
 
         LOG("added bookmark");
       }
 
+      if (qdata.isTag) {
+        tagssvc.tagURI(uri(qdata.uri), qdata.tagArray);
+      }
+
       if (qdata.isDynContainer) {
         bmsvc.createDynamicContainer(qdata.parentFolder, qdata.title,
                                        qdata.contractId, qdata.index);
       }
     } catch (ex) {
       // use the data object here in case instantiation of qdata failed
       LOG("Problem with this URI: " + data.uri);
       do_throw("Error creating database: " + ex + "\n");
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/queries/test_containersQueries_sorting.js
@@ -0,0 +1,447 @@
+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
+ *
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is mozilla.org code.
+ *
+ * The Initial Developer of the Original Code is
+ * Mozilla Corporation.
+ * Portions created by the Initial Developer are Copyright (C) 2009
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *   Marco Bonardo <mak77@bonardo.net> (Original Author)
+ *
+ * Alternatively, the contents of this file may be used under the terms of
+ * either the GNU General Public License Version 2 or later (the "GPL"), or
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+/**
+ * Testing behavior of bug 473157
+ * "Want to sort history in container view without sorting the containers"
+ * and regression bug 488783
+ * Tags list no longer sorted (alphabetized).
+ * This test is for global testing sorting containers queries.
+ */
+
+////////////////////////////////////////////////////////////////////////////////
+//// Globals and Constants
+
+var hs = Cc["@mozilla.org/browser/nav-history-service;1"].
+         getService(Ci.nsINavHistoryService);
+var bh = hs.QueryInterface(Ci.nsIBrowserHistory);
+var tagging = Cc["@mozilla.org/browser/tagging-service;1"].
+              getService(Ci.nsITaggingService);
+
+var resultTypes = [
+  {value: Ci.nsINavHistoryQueryOptions.RESULTS_AS_DATE_QUERY, name: "RESULTS_AS_DATE_QUERY"},
+  {value: Ci.nsINavHistoryQueryOptions.RESULTS_AS_SITE_QUERY, name: "RESULTS_AS_SITE_QUERY"},
+  {value: Ci.nsINavHistoryQueryOptions.RESULTS_AS_DATE_SITE_QUERY, name: "RESULTS_AS_DATE_SITE_QUERY"},
+  {value: Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_QUERY, name: "RESULTS_AS_TAG_QUERY"},
+];
+
+var sortingModes = [
+  {value: Ci.nsINavHistoryQueryOptions.SORT_BY_TITLE_ASCENDING, name: "SORT_BY_TITLE_ASCENDING"},
+  {value: Ci.nsINavHistoryQueryOptions.SORT_BY_TITLE_DESCENDING, name: "SORT_BY_TITLE_DESCENDING"},
+  {value: Ci.nsINavHistoryQueryOptions.SORT_BY_DATE_ASCENDING, name: "SORT_BY_DATE_ASCENDING"},
+  {value: Ci.nsINavHistoryQueryOptions.SORT_BY_DATE_DESCENDING, name: "SORT_BY_DATE_DESCENDING"},
+  {value: Ci.nsINavHistoryQueryOptions.SORT_BY_DATEADDED_ASCENDING, name: "SORT_BY_DATEADDED_ASCENDING"},
+  {value: Ci.nsINavHistoryQueryOptions.SORT_BY_DATEADDED_DESCENDING, name: "SORT_BY_DATEADDED_DESCENDING"},
+];
+
+// These pages will be added from newest to oldest and from less visited to most
+// visited.
+var pages = [
+  "http://www.mozilla.org/c/",
+  "http://www.mozilla.org/a/",
+  "http://www.mozilla.org/b/",
+  "http://www.mozilla.com/c/",
+  "http://www.mozilla.com/a/",
+  "http://www.mozilla.com/b/",
+];
+
+var tags = [
+  "mozilla",
+  "Development",
+  "test",
+];
+
+////////////////////////////////////////////////////////////////////////////////
+//// Test Runner
+
+/**
+ * Enumerates all the sequences of the cartesian product of the arrays contained
+ * in aSequences.  Examples:
+ *
+ *   cartProd([[1, 2, 3], ["a", "b"]], callback);
+ *   // callback is called 3 * 2 = 6 times with the following arrays:
+ *   // [1, "a"], [1, "b"], [2, "a"], [2, "b"], [3, "a"], [3, "b"]
+ *
+ *   cartProd([["a"], [1, 2, 3], ["X", "Y"]], callback);
+ *   // callback is called 1 * 3 * 2 = 6 times with the following arrays:
+ *   // ["a", 1, "X"], ["a", 1, "Y"], ["a", 2, "X"], ["a", 2, "Y"],
+ *   // ["a", 3, "X"], ["a", 3, "Y"]
+ *
+ *   cartProd([[1], [2], [3], [4]], callback);
+ *   // callback is called 1 * 1 * 1 * 1 = 1 time with the following array:
+ *   // [1, 2, 3, 4]
+ *
+ *   cartProd([], callback);
+ *   // callback is 0 times
+ *
+ *   cartProd([[1, 2, 3, 4]], callback);
+ *   // callback is called 4 times with the following arrays:
+ *   // [1], [2], [3], [4]
+ *
+ * @param  aSequences
+ *         an array that contains an arbitrary number of arrays
+ * @param  aCallback
+ *         a function that is passed each sequence of the product as it's
+ *         computed
+ * @return the total number of sequences in the product
+ */
+function cartProd(aSequences, aCallback)
+{
+  if (aSequences.length === 0)
+    return 0;
+
+  // For each sequence in aSequences, we maintain a pointer (an array index,
+  // really) to the element we're currently enumerating in that sequence
+  var seqEltPtrs = aSequences.map(function (i) 0);
+
+  var numProds = 0;
+  var done = false;
+  while (!done) {
+    numProds++;
+
+    // prod = sequence in product we're currently enumerating
+    var prod = [];
+    for (var i = 0; i < aSequences.length; i++) {
+      prod.push(aSequences[i][seqEltPtrs[i]]);
+    }
+    aCallback(prod);
+
+    // The next sequence in the product differs from the current one by just a
+    // single element.  Determine which element that is.  We advance the
+    // "rightmost" element pointer to the "right" by one.  If we move past the
+    // end of that pointer's sequence, reset the pointer to the first element
+    // in its sequence and then try the sequence to the "left", and so on.
+
+    // seqPtr = index of rightmost input sequence whose element pointer is not
+    // past the end of the sequence
+    var seqPtr = aSequences.length - 1;
+    while (!done) {
+      // Advance the rightmost element pointer.
+      seqEltPtrs[seqPtr]++;
+
+      // The rightmost element pointer is past the end of its sequence.
+      if (seqEltPtrs[seqPtr] >= aSequences[seqPtr].length) {
+        seqEltPtrs[seqPtr] = 0;
+        seqPtr--;
+
+        // All element pointers are past the ends of their sequences.
+        if (seqPtr < 0)
+          done = true;
+      }
+      else break;
+    }
+  }
+  return numProds;
+}
+
+/**
+ * Test a query based on passed-in options.
+ *
+ * @param aSequence
+ *        array of options we will use to query.
+ */
+function test_query_callback(aSequence) {
+  do_check_eq(aSequence.length, 2);
+  var resultType = aSequence[0];
+  var sortingMode = aSequence[1];
+  print("\n\n*** Testing default sorting for resultType (" + resultType.name + ") and sortingMode (" + sortingMode.name + ")");
+
+  // Skip invalid combinations sorting queries by none.
+  if (resultType.value == Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_QUERY &&
+      (sortingMode.value == Ci.nsINavHistoryQueryOptions.SORT_BY_DATE_ASCENDING ||
+       sortingMode.value == Ci.nsINavHistoryQueryOptions.SORT_BY_DATE_DESCENDING)) {
+    // This is a bookmark query, we can't sort by visit date.
+    sortingMode.value = Ci.nsINavHistoryQueryOptions.SORT_BY_NONE;
+  }
+  if (resultType.value == Ci.nsINavHistoryQueryOptions.RESULTS_AS_DATE_QUERY ||
+      resultType.value == Ci.nsINavHistoryQueryOptions.RESULTS_AS_SITE_QUERY ||
+      resultType.value == Ci.nsINavHistoryQueryOptions.RESULTS_AS_DATE_SITE_QUERY) {
+    // This is an history query, we can't sort by date added.
+    if (sortingMode.value == Ci.nsINavHistoryQueryOptions.SORT_BY_DATEADDED_ASCENDING ||
+       sortingMode.value == Ci.nsINavHistoryQueryOptions.SORT_BY_DATEADDED_DESCENDING)
+    sortingMode.value = Ci.nsINavHistoryQueryOptions.SORT_BY_NONE;
+  }
+
+  // Create a new query with required options.
+  var query = histsvc.getNewQuery();
+  var options = histsvc.getNewQueryOptions();
+  options.resultType = resultType.value;
+  options.sortingMode = sortingMode.value;
+
+  // Compare resultset with expectedData.
+  var result = histsvc.executeQuery(query, options);
+  var root = result.root;
+  root.containerOpen = true;
+
+  if (resultType.value == Ci.nsINavHistoryQueryOptions.RESULTS_AS_DATE_QUERY ||
+      resultType.value == Ci.nsINavHistoryQueryOptions.RESULTS_AS_DATE_SITE_QUERY) {
+    // Date containers are always sorted by date descending.
+    check_children_sorting(root,
+                           Ci.nsINavHistoryQueryOptions.SORT_BY_DATE_DESCENDING);
+  }
+  else
+    check_children_sorting(root, sortingMode.value);
+
+  // Now Check sorting of the first child container.
+  var container = root.getChild(0)
+                      .QueryInterface(Ci.nsINavHistoryContainerResultNode);
+  container.containerOpen = true;
+
+  if (resultType.value == Ci.nsINavHistoryQueryOptions.RESULTS_AS_DATE_SITE_QUERY) {
+    // Has more than one level of containers, first we check the sorting of
+    // the first level (site containers), those won't inherit sorting...
+    check_children_sorting(container,
+                           Ci.nsINavHistoryQueryOptions.SORT_BY_TITLE_ASCENDING);
+    // ...then we check sorting of the contained urls, we can't inherit sorting
+    // since the above level does not inherit it, so they will be sorted by
+    // title ascending.
+    innerContainer = container.getChild(0)
+                              .QueryInterface(Ci.nsINavHistoryContainerResultNode);
+    innerContainer.containerOpen = true;
+    check_children_sorting(innerContainer,
+                           Ci.nsINavHistoryQueryOptions.SORT_BY_TITLE_ASCENDING);
+    innerContainer.containerOpen = false;
+  }
+  else if (resultType.value == Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_QUERY) {
+    // Sorting mode for tag contents is hardcoded for now, to allow for faster
+    // duplicates filtering.
+    check_children_sorting(container,
+                           Ci.nsINavHistoryQueryOptions.SORT_BY_NONE);
+  }
+  else
+    check_children_sorting(container, sortingMode.value);
+
+  container.containerOpen = false;
+  root.containerOpen = false;
+
+  test_result_sortingMode_change(result, resultType, sortingMode);
+}
+
+/**
+ * Sets sortingMode on aResult and checks for correct sorting of children.
+ * Containers should not change their sorting, while contained uri nodes should.
+ *
+ * @param aResult
+ *        nsINavHistoryResult generated by our query.
+ * @param aResultType
+ *        required result type.
+ * @param aOriginalSortingMode
+ *        the sorting mode from query's options.
+ */
+function test_result_sortingMode_change(aResult, aResultType, aOriginalSortingMode) {
+  var root = aResult.root;
+  // Now we set sortingMode on the result and check that containers are not
+  // sorted while children are.
+  sortingModes.forEach(function sortingModeChecker(aForcedSortingMode) {
+    print("\n* Test setting sortingMode (" + aForcedSortingMode.name + ") " +
+          "on result with resultType (" + aResultType.name + ") " +
+          "currently sorted as (" + aOriginalSortingMode.name + ")");
+
+    aResult.sortingMode = aForcedSortingMode.value;
+    root.containerOpen = true;
+
+    if (aResultType.value == Ci.nsINavHistoryQueryOptions.RESULTS_AS_DATE_QUERY ||
+        aResultType.value == Ci.nsINavHistoryQueryOptions.RESULTS_AS_DATE_SITE_QUERY) {
+      // Date containers are always sorted by date descending.
+      check_children_sorting(root,
+                             Ci.nsINavHistoryQueryOptions.SORT_BY_DATE_DESCENDING);
+    }
+    else if (aResultType.value == Ci.nsINavHistoryQueryOptions.RESULTS_AS_SITE_QUERY &&
+             (aOriginalSortingMode.value == Ci.nsINavHistoryQueryOptions.SORT_BY_DATE_ASCENDING ||
+              aOriginalSortingMode.value == Ci.nsINavHistoryQueryOptions.SORT_BY_DATE_DESCENDING)) {
+      // Site containers don't have a good time property to sort by.
+      check_children_sorting(root,
+                             Ci.nsINavHistoryQueryOptions.SORT_BY_NONE);
+    }
+    else
+      check_children_sorting(root, aOriginalSortingMode.value);
+
+    // Now Check sorting of the first child container.
+    var container = root.getChild(0)
+                        .QueryInterface(Ci.nsINavHistoryContainerResultNode);
+    container.containerOpen = true;
+
+    if (aResultType.value == Ci.nsINavHistoryQueryOptions.RESULTS_AS_DATE_SITE_QUERY) {
+      // Has more than one level of containers, first we check the sorting of
+      // the first level (site containers), those won't be sorted...
+      check_children_sorting(container,
+                             Ci.nsINavHistoryQueryOptions.SORT_BY_TITLE_ASCENDING);
+      // ...then we check sorting of the second level of containers, result
+      // will sort them through recursiveSort.
+      innerContainer = container.getChild(0)
+                                .QueryInterface(Ci.nsINavHistoryContainerResultNode);
+      innerContainer.containerOpen = true;
+      check_children_sorting(innerContainer, aForcedSortingMode.value);
+      innerContainer.containerOpen = false;
+    }
+    else {
+      if (aResultType.value == Ci.nsINavHistoryQueryOptions.RESULTS_AS_DATE_QUERY ||
+          aResultType.value == Ci.nsINavHistoryQueryOptions.RESULTS_AS_DATE_SITE_QUERY ||
+          aResultType.value == Ci.nsINavHistoryQueryOptions.RESULTS_AS_SITE_QUERY) {
+        // Date containers are always sorted by date descending.
+        check_children_sorting(root, Ci.nsINavHistoryQueryOptions.SORT_BY_NONE);
+      }
+      else if (aResultType.value == Ci.nsINavHistoryQueryOptions.RESULTS_AS_SITE_QUERY &&
+             (aOriginalSortingMode.value == Ci.nsINavHistoryQueryOptions.SORT_BY_DATE_ASCENDING ||
+              aOriginalSortingMode.value == Ci.nsINavHistoryQueryOptions.SORT_BY_DATE_DESCENDING)) {
+        // Site containers don't have a good time property to sort by.
+        check_children_sorting(root, Ci.nsINavHistoryQueryOptions.SORT_BY_NONE);
+      }
+      else
+        check_children_sorting(root, aOriginalSortingMode.value);
+
+      // Children should always be sorted.
+      check_children_sorting(container, aForcedSortingMode.value);
+    }
+
+    container.containerOpen = false;
+    root.containerOpen = false;
+  });
+}
+
+/**
+ * Test if children of aRootNode are correctly sorted.
+ * @param aRootNode
+ *        already opened root node from our query's result.
+ * @param aExpectedSortingMode
+ *        The sortingMode we expect results to be.
+ */
+function check_children_sorting(aRootNode, aExpectedSortingMode) {
+  var results = [];
+  print("Found children:");
+  for (var i = 0; i < aRootNode.childCount; i++) {
+    results[i] = aRootNode.getChild(i);
+    print(i + " " + results[i].title);
+  }
+
+  // Helper for case insensitive string comparison.
+  function caseInsensitiveStringComparator(a, b) {
+    var aLC = a.toLowerCase();
+    var bLC = b.toLowerCase();
+    if (aLC < bLC)
+      return -1;
+    if (aLC > bLC)
+      return 1;
+    return 0;
+  }
+
+  // Get a comparator based on expected sortingMode.
+  var comparator;
+  switch(aExpectedSortingMode) {
+    case Ci.nsINavHistoryQueryOptions.SORT_BY_NONE:
+      comparator = function (a, b) {
+        return 0;
+      }
+      break;
+    case Ci.nsINavHistoryQueryOptions.SORT_BY_TITLE_ASCENDING:
+      comparator = function (a, b) {
+        return caseInsensitiveStringComparator(a.title, b.title);
+      }
+      break;
+    case Ci.nsINavHistoryQueryOptions.SORT_BY_TITLE_DESCENDING:
+      comparator = function (a, b) {
+        return -caseInsensitiveStringComparator(a.title, b.title);
+      }
+      break;
+    case Ci.nsINavHistoryQueryOptions.SORT_BY_DATE_ASCENDING:
+      comparator = function (a, b) {
+        return a.time - b.time;
+      }
+      break;
+    case Ci.nsINavHistoryQueryOptions.SORT_BY_DATE_DESCENDING:
+      comparator = function (a, b) {
+        return b.time - a.time;
+      }
+    case Ci.nsINavHistoryQueryOptions.SORT_BY_DATEADDED_ASCENDING:
+      comparator = function (a, b) {
+        return a.dateAdded - b.dateAdded;
+      }
+      break;
+    case Ci.nsINavHistoryQueryOptions.SORT_BY_DATEADDED_DESCENDING:
+      comparator = function (a, b) {
+        return b.dateAdded - a.dateAdded;
+      }
+      break;
+    default:
+      do_throw("Unknown sorting type: " + aExpectedSortingMode);
+  }
+
+  // Make an independent copy of the results array and sort it.
+  var sortedResults = results.slice();
+  sortedResults.sort(comparator);
+  // Actually compare returned children with our sorted array.
+  for (var i = 0; i < sortedResults.length; i++) {
+    if (sortedResults[i].title != results[i].title)
+      print(i + " index wrong, expected " + sortedResults[i].title +
+            " found " + results[i].title);
+    do_check_eq(sortedResults[i].title, results[i].title);
+  }
+}
+
+////////////////////////////////////////////////////////////////////////////////
+//// Main
+
+function run_test()
+{
+  // Add visits, bookmarks and tags to our database.
+  var timeInMilliseconds = Date.now();
+  var visitCount = 0;
+  var dayOffset = 0;
+  var visits = [];
+  pages.forEach(function (aPageUrl) visits.push(
+    { isVisit: true,
+      isBookmark: true,
+      transType: Ci.nsINavHistoryService.TRANSITION_TYPED,
+      uri: aPageUrl,
+      title: aPageUrl,
+      // subtract 5 hours per iteration, to expose more than one day container.
+      lastVisit: (timeInMilliseconds - (18000 * 1000 * dayOffset++)) * 1000,
+      visitCount: visitCount++,
+      isTag: true,
+      tagArray: tags,
+      isInQuery: true }));
+  populateDB(visits);
+
+  cartProd([resultTypes, sortingModes], test_query_callback);
+
+  // Cleanup.
+  pages.forEach(function(aPageUrl) tagging.untagURI(uri(aPageUrl), tags));
+  remove_all_bookmarks();
+  bh.removeAllPages();
+}
--- a/toolkit/components/places/tests/unit/test_history_sidebar.js
+++ b/toolkit/components/places/tests/unit/test_history_sidebar.js
@@ -306,16 +306,17 @@ function test_RESULTS_AS_DATE_QUERY() {
 function test_RESULTS_AS_SITE_QUERY() {
   print("\n\n*** TEST RESULTS_AS_SITE_QUERY\n");
   // add a bookmark with a domain not in the set of visits in the db
   var itemId = bs.insertBookmark(bs.toolbarFolder, uri("http://foobar"),
                                  bs.DEFAULT_INDEX, "");
 
   var options = hs.getNewQueryOptions();
   options.resultType = options.RESULTS_AS_SITE_QUERY;
+  options.sortingMode = options.SORT_BY_TITLE_ASCENDING;
   var query = hs.getNewQuery();
   var result = hs.executeQuery(query, options);
   var root = result.root;
   root.containerOpen = true;
   do_check_eq(root.childCount, containers.length * 2);
 
 /* Expected results:
     "mirror0.google.com",