fix for bug #367386: complex queries only return visited places.

r=mano, dietrich, sspitzer

patch by=thunder

browser/components/places/tests/unit/test_history.js

@@ -22,6 +22,7 @@
  * Contributor(s):
  *  Darin Fisher <darin@meer.net>
  *  Dietrich Ayala <dietrich@mozilla.com>
+ *  Dan Mills <thunder@mozilla.com>
  *
  * 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
@@ -44,24 +45,27 @@ try {
   do_throw("Could not get history service\n");
 } 
 
-// main
-function run_test() {
-  // test URI
-  var testURI = uri("http://mozilla.com/");
-
-  // add a visit
-  var placeID = histsvc.addVisit(testURI,
+// adds a test URI visit to the database, and checks for a valid place ID
+function add_visit(aURI) {
+  var placeID = histsvc.addVisit(aURI,
                                  Date.now(),
                                  0, // no referrer
                                  histsvc.TRANSITION_TYPED, // user typed in URL bar
                                  false, // not redirect
                                  0);
-  // test for valid place ID
   do_check_true(placeID > 0);
+  return placeID;
+}
 
-  // query for the visit
+// main
+function run_test() {
+  // add a visit
+  var testURI = uri("http://mozilla.com");
+  add_visit(testURI);
+
+  // now query for the visit, setting sorting and limit such that
+  // we should retrieve only the visit we just added
   var options = histsvc.getNewQueryOptions();
-  // set sorting and limit such that we should retrieve only the visit we just added
   options.sortingMode = options.SORT_BY_DATE_DESCENDING;
   options.maxResults = 1;
   // TODO: using full visit crashes in xpcshell test
@@ -82,6 +86,55 @@ function run_test() {
   }
   root.containerOpen = false;
 
+  // add another visit for the same URI, and a third visit for a different URI
+  var testURI2 = uri("http://google.com/");
+  add_visit(testURI);
+  add_visit(testURI2);
+
+  options.maxResults = 5;
+  options.resultType = options.RESULTS_AS_URI;
+
+  // test minVisits
+  query.minVisits = 0;
+  result = histsvc.executeQuery(query, options);
+  result.root.containerOpen = true;
+  do_check_eq(result.root.childCount, 2);
+  query.minVisits = 1;
+  result = histsvc.executeQuery(query, options);
+  result.root.containerOpen = true;
+  do_check_eq(result.root.childCount, 2);
+  query.minVisits = 2;
+  result = histsvc.executeQuery(query, options);
+  result.root.containerOpen = true;
+  do_check_eq(result.root.childCount, 1);
+  query.minVisits = 3;
+  result = histsvc.executeQuery(query, options);
+  result.root.containerOpen = true;
+  do_check_eq(result.root.childCount, 0);
+
+  // test maxVisits
+  query.minVisits = -1;
+  query.maxVisits = -1;
+  result = histsvc.executeQuery(query, options);
+  result.root.containerOpen = true;
+  do_check_eq(result.root.childCount, 2);
+  query.maxVisits = 0;
+  result = histsvc.executeQuery(query, options);
+  result.root.containerOpen = true;
+  do_check_eq(result.root.childCount, 0);
+  query.maxVisits = 1;
+  result = histsvc.executeQuery(query, options);
+  result.root.containerOpen = true;
+  do_check_eq(result.root.childCount, 1);
+  query.maxVisits = 2;
+  result = histsvc.executeQuery(query, options);
+  result.root.containerOpen = true;
+  do_check_eq(result.root.childCount, 2);
+  query.maxVisits = 3;
+  result = histsvc.executeQuery(query, options);
+  result.root.containerOpen = true;
+  do_check_eq(result.root.childCount, 2);
+
   // by default, browser.history_expire_days is 9
   do_check_true(!histsvc.historyDisabled);
 }

toolkit/components/places/public/nsINavHistoryService.idl

@@ -807,7 +807,7 @@ interface nsINavHistoryObserver : nsISupports
  * can be non-negligible.
  */
 
-[scriptable, uuid(884819a6-1860-4a71-8fd7-89d962c1d984)]
+[scriptable, uuid(53B51AFE-9DE8-40AD-9C81-F2CC1701F1FF)]
 interface nsINavHistoryQuery : nsISupports
 {
   /**
@@ -854,6 +854,14 @@ interface nsINavHistoryQuery : nsISupports
   attribute AString searchTerms;
   readonly attribute boolean hasSearchTerms;
 
+  /**
+   * Set lower or upper limits for how many times an item has been
+   * visited.  The default is -1, and in that case all items are
+   * matched regardless of their visit count.
+   */
+  attribute PRInt32 minVisits;
+  attribute PRInt32 maxVisits;
+
   /**
    * When set, returns only bookmarked items, when unset, returns anything. Setting this
    * is equivalent to listing all bookmark folders in the 'folders' parameter.

toolkit/components/places/src/nsNavHistory.cpp

@@ -1871,10 +1871,11 @@ nsNavHistory::GetQueryResults(const nsCOMArray<nsNavHistoryQuery>& aQueries,
     (aOptions->ResultType() == nsINavHistoryQueryOptions::RESULTS_AS_VISIT ||
      aOptions->ResultType() == nsINavHistoryQueryOptions::RESULTS_AS_FULL_VISIT);
 
-  nsCAutoString commonConditions("visit_count > 0 ");
+  nsCAutoString commonConditions;
+
   if (! aOptions->IncludeHidden()) {
     // The hiding code here must match the notification behavior in AddVisit
-    commonConditions.AppendLiteral("AND hidden <> 1 ");
+    commonConditions.AssignLiteral("hidden <> 1 ");
 
     // Some items are unhidden but are subframe navigations that we shouldn't
     // show. This happens especially on imported profiles because the previous
@@ -1998,7 +1999,9 @@ nsNavHistory::GetQueryResults(const nsCOMArray<nsNavHistoryQuery>& aQueries,
     queryString.AppendLiteral(" ");
   }
 
-  //printf("Constructed the query: %s\n", PromiseFlatCString(queryString).get());
+#ifdef DEBUG_thunder
+  printf("Constructed the query: %s\n", PromiseFlatCString(queryString).get());
+#endif
 
   // Put this in a transaction. Even though we are only reading, this will
   // speed up the grouped queries to the annotation service for titles and
@@ -3038,8 +3041,6 @@ nsNavHistory::QueryToSelectClause(nsNavHistoryQuery* aQuery, // const
   *aParamCount = 0;
   nsCAutoString paramString;
 
-  // note common condition visit_count > 0 is set under the annotation section
-
   // begin time
   if (NS_SUCCEEDED(aQuery->GetHasBeginTime(&hasIt)) && hasIt) {
     parameterString(aStartParameter + *aParamCount, paramString);
@@ -3058,6 +3059,23 @@ nsNavHistory::QueryToSelectClause(nsNavHistoryQuery* aQuery, // const
 
   // search terms FIXME
 
+  // min and max visit count
+  if (aQuery->MinVisits() >= 0) {
+    if (! aClause->IsEmpty())
+      *aClause += NS_LITERAL_CSTRING(" AND ");
+    parameterString(aStartParameter + *aParamCount, paramString);
+    *aClause += NS_LITERAL_CSTRING("h.visit_count >= ") + paramString;
+    (*aParamCount) ++;
+  }
+
+  if (aQuery->MaxVisits() >= 0) {
+    if (! aClause->IsEmpty())
+      *aClause += NS_LITERAL_CSTRING(" AND ");
+    parameterString(aStartParameter + *aParamCount, paramString);
+    *aClause += NS_LITERAL_CSTRING("h.visit_count <= ") + paramString;
+    (*aParamCount) ++;
+  }
+
   // only bookmarked
   if (aQuery->OnlyBookmarked()) {
     if (! aClause->IsEmpty())
@@ -3119,9 +3137,10 @@ nsNavHistory::QueryToSelectClause(nsNavHistoryQuery* aQuery, // const
 
   // annotation
   aQuery->GetHasAnnotation(&hasIt);
+  if (hasIt) {
     if (! aClause->IsEmpty())
       *aClause += NS_LITERAL_CSTRING(" AND ");
-  if (hasIt) {
+
     nsCAutoString paramString;
     parameterString(aStartParameter + *aParamCount, paramString);
     (*aParamCount) ++;
@@ -3134,7 +3153,8 @@ nsNavHistory::QueryToSelectClause(nsNavHistoryQuery* aQuery, // const
     // annotation-based queries don't get the common conditions, so you get
     // all URLs with that annotation
   } else {
-    // all non-annotation queries return only visited items
+    if (!(aClause->IsEmpty() || aCommonConditions.IsEmpty()))
+      *aClause += NS_LITERAL_CSTRING(" AND ");
     aClause->Append(aCommonConditions);
   }
 
@@ -3177,6 +3197,21 @@ nsNavHistory::BindQueryClauseParameters(mozIStorageStatement* statement,
 
   // search terms FIXME
 
+  // min and max visit count
+  PRInt32 visits = aQuery->MinVisits();
+  if (visits >= 0) {
+    rv = statement->BindInt32Parameter(aStartParameter + *aParamCount, visits);
+    NS_ENSURE_SUCCESS(rv, rv);
+    (*aParamCount) ++;
+  }
+
+  visits = aQuery->MaxVisits();
+  if (visits >= 0) {
+    rv = statement->BindInt32Parameter(aStartParameter + *aParamCount, visits);
+    NS_ENSURE_SUCCESS(rv, rv);
+    (*aParamCount) ++;
+  }
+
   // onlyBookmarked: nothing to bind
 
   // domain (see GetReversedHostname for more info on reversed host names)

toolkit/components/places/src/nsNavHistoryQuery.cpp

@@ -138,6 +138,8 @@ static void SetOptionsKeyUint32(const nsCString& aValue,
 #define QUERYKEY_END_TIME "endTime"
 #define QUERYKEY_END_TIME_REFERENCE "endTimeRef"
 #define QUERYKEY_SEARCH_TERMS "terms"
+#define QUERYKEY_MIN_VISITS "minVisits"
+#define QUERYKEY_MAX_VISITS "maxVisits"
 #define QUERYKEY_ONLY_BOOKMARKED "onlyBookmarked"
 #define QUERYKEY_DOMAIN_IS_HOST "domainIsHost"
 #define QUERYKEY_DOMAIN "domain"
@@ -308,6 +310,21 @@ nsNavHistory::QueriesToQueryString(nsINavHistoryQuery **aQueries,
       aQueryString += escapedTerms;
     }
 
+    // min and max visits
+    PRInt32 minVisits;
+    if (NS_SUCCEEDED(query->GetMinVisits(&minVisits)) && minVisits >= 0) {
+      AppendAmpersandIfNonempty(aQueryString);
+      aQueryString.Append(NS_LITERAL_CSTRING(QUERYKEY_MIN_VISITS "="));
+      AppendInt32(aQueryString, minVisits);
+    }
+
+    PRInt32 maxVisits;
+    if (NS_SUCCEEDED(query->GetMaxVisits(&maxVisits)) && maxVisits >= 0) {
+      AppendAmpersandIfNonempty(aQueryString);
+      aQueryString.Append(NS_LITERAL_CSTRING(QUERYKEY_MAX_VISITS "="));
+      AppendInt32(aQueryString, maxVisits);
+    }
+
     // only bookmarked
     AppendBoolKeyValueIfTrue(aQueryString,
                              NS_LITERAL_CSTRING(QUERYKEY_ONLY_BOOKMARKED),
@@ -544,6 +561,22 @@ nsNavHistory::TokensToQueries(const nsTArray<QueryKeyValuePair>& aTokens,
       rv = query->SetSearchTerms(NS_ConvertUTF8toUTF16(unescapedTerms));
       NS_ENSURE_SUCCESS(rv, rv);
 
+    // min visits
+    } else if (kvp.key.EqualsLiteral(QUERYKEY_MIN_VISITS)) {
+      PRInt32 visits = kvp.value.ToInteger((PRInt32*)&rv);
+      if (NS_SUCCEEDED(rv))
+        query->SetMinVisits(visits);
+      else
+        NS_WARNING("Bad number for minVisits in query");
+
+    // max visits
+    } else if (kvp.key.EqualsLiteral(QUERYKEY_MAX_VISITS)) {
+      PRInt32 visits = kvp.value.ToInteger((PRInt32*)&rv);
+      if (NS_SUCCEEDED(rv))
+        query->SetMaxVisits(visits);
+      else
+        NS_WARNING("Bad number for maxVisits in query");
+
     // onlyBookmarked flag
     } else if (kvp.key.EqualsLiteral(QUERYKEY_ONLY_BOOKMARKED)) {
       SetQueryKeyBool(kvp.value, query, &nsINavHistoryQuery::SetOnlyBookmarked);
@@ -715,10 +748,12 @@ NS_IMPL_ISUPPORTS2(nsNavHistoryQuery, nsNavHistoryQuery, nsINavHistoryQuery)
 //    just set the things it's interested in.
 
 nsNavHistoryQuery::nsNavHistoryQuery()
-  : mBeginTime(0), mBeginTimeReference(TIME_RELATIVE_EPOCH),
+  : mMinVisits(-1), mMaxVisits(-1), mBeginTime(0),
+    mBeginTimeReference(TIME_RELATIVE_EPOCH),
     mEndTime(0), mEndTimeReference(TIME_RELATIVE_EPOCH),
-    mOnlyBookmarked(PR_FALSE), mDomainIsHost(PR_FALSE),
-    mUriIsPrefix(PR_FALSE), mAnnotationIsNot(PR_FALSE)
+    mOnlyBookmarked(PR_FALSE),
+    mDomainIsHost(PR_FALSE), mUriIsPrefix(PR_FALSE),
+    mAnnotationIsNot(PR_FALSE)
 {
   // differentiate not set (IsVoid) from empty string (local files)
   mDomain.SetIsVoid(PR_TRUE);
@@ -821,6 +856,32 @@ NS_IMETHODIMP nsNavHistoryQuery::GetHasSearchTerms(PRBool* _retval)
   return NS_OK;
 }
 
+/* attribute PRInt32 minVisits; */
+NS_IMETHODIMP nsNavHistoryQuery::GetMinVisits(PRInt32* _retval)
+{
+  NS_ENSURE_ARG_POINTER(_retval);
+  *_retval = mMinVisits;
+  return NS_OK;
+}
+NS_IMETHODIMP nsNavHistoryQuery::SetMinVisits(PRInt32 aVisits)
+{
+  mMinVisits = aVisits;
+  return NS_OK;
+}
+
+/* attribute PRint32 maxVisits; */
+NS_IMETHODIMP nsNavHistoryQuery::GetMaxVisits(PRInt32* _retval)
+{
+  NS_ENSURE_ARG_POINTER(_retval);
+  *_retval = mMaxVisits;
+  return NS_OK;
+}
+NS_IMETHODIMP nsNavHistoryQuery::SetMaxVisits(PRInt32 aVisits)
+{
+  mMaxVisits = aVisits;
+  return NS_OK;
+}
+
 /* attribute boolean onlyBookmarked; */
 NS_IMETHODIMP nsNavHistoryQuery::GetOnlyBookmarked(PRBool *aOnlyBookmarked)
 {

toolkit/components/places/src/nsNavHistoryQuery.h

@@ -67,6 +67,8 @@ public:
   NS_DECL_ISUPPORTS
   NS_DECL_NSINAVHISTORYQUERY
 
+  PRInt32 MinVisits() { return mMinVisits; }
+  PRInt32 MaxVisits() { return mMaxVisits; }
   PRTime BeginTime() { return mBeginTime; }
   PRUint32 BeginTimeReference() { return mBeginTimeReference; }
   PRTime EndTime() { return mEndTime; }
@@ -86,6 +88,8 @@ private:
 
 protected:
 
+  PRInt32 mMinVisits;
+  PRInt32 mMaxVisits;
   PRTime mBeginTime;
   PRUint32 mBeginTimeReference;
   PRTime mEndTime;