Bug 737841 - Part 1: ensure Places history properly handles redirecting and error visits.

r=dietrich

toolkit/components/places/History.cpp

@@ -97,6 +97,7 @@ struct VisitData {
   , typed(false)
   , transitionType(PR_UINT32_MAX)
   , visitTime(0)
+  , frecency(-1)
   , titleChanged(false)
   {
     guid.SetIsVoid(true);
@@ -112,6 +113,7 @@ struct VisitData {
   , typed(false)
   , transitionType(PR_UINT32_MAX)
   , visitTime(0)
+  , frecency(-1)
   , titleChanged(false)
   {
     (void)aURI->GetSpec(spec);
@@ -124,8 +126,7 @@ struct VisitData {
   }
 
   /**
-   * Sets the transition type of the visit, as well as if it was typed and
-   * should be hidden (based on the transition type specified).
+   * Sets the transition type of the visit, as well as if it was typed.
    *
    * @param aTransitionType
    *        The transition type constant to set.  Must be one of the
@@ -134,10 +135,6 @@ struct VisitData {
   void SetTransitionType(PRUint32 aTransitionType)
   {
     typed = aTransitionType == nsINavHistoryService::TRANSITION_TYPED;
-    bool redirected =
-      aTransitionType == nsINavHistoryService::TRANSITION_REDIRECT_TEMPORARY ||
-      aTransitionType == nsINavHistoryService::TRANSITION_REDIRECT_PERMANENT;
-    hidden = GetHiddenState(redirected, aTransitionType);
     transitionType = aTransitionType;
   }
 
@@ -171,6 +168,7 @@ struct VisitData {
   bool typed;
   PRUint32 transitionType;
   PRTime visitTime;
+  PRInt32 frecency;
 
   /**
    * Stores the title.  If this is empty (IsEmpty() returns true), then the
@@ -1057,7 +1055,7 @@ private:
     rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("visit_date"),
                                _place.visitTime);
     NS_ENSURE_SUCCESS(rv, rv);
-    PRInt32 transitionType = _place.transitionType;
+    PRUint32 transitionType = _place.transitionType;
     NS_ASSERTION(transitionType >= nsINavHistoryService::TRANSITION_LINK &&
                  transitionType <= nsINavHistoryService::TRANSITION_FRAMED_LINK,
                  "Invalid transition type!");
@@ -1087,6 +1085,11 @@ private:
    */
   nsresult UpdateFrecency(const VisitData& aPlace)
   {
+    // Don't update frecency if the page should not appear in autocomplete.
+    if (aPlace.frecency == 0) {
+      return NS_OK;
+    }
+
     nsresult rv;
     { // First, set our frecency to the proper value.
       nsCOMPtr<mozIStorageStatement> stmt;
@@ -1116,8 +1119,8 @@ private:
       NS_ENSURE_SUCCESS(rv, rv);
     }
 
-    { // Now, we need to mark the page as not hidden if the frecency is now
-      // nonzero.
+    if (!aPlace.hidden) {
+      // Now, mark the page as not hidden if the frecency is now nonzero.
       nsCOMPtr<mozIStorageStatement> stmt;
       if (aPlace.placeId) {
         stmt = mHistory->GetStatement(
@@ -1561,8 +1564,8 @@ History::InsertPlace(const VisitData& aPlace)
 
   nsCOMPtr<mozIStorageStatement> stmt = GetStatement(
       "INSERT INTO moz_places "
-        "(url, title, rev_host, hidden, typed, guid) "
-      "VALUES (:url, :title, :rev_host, :hidden, :typed, :guid) "
+        "(url, title, rev_host, hidden, typed, frecency, guid) "
+      "VALUES (:url, :title, :rev_host, :hidden, :typed, :frecency, :guid) "
     );
   NS_ENSURE_STATE(stmt);
   mozStorageStatementScoper scoper(stmt);
@@ -1583,6 +1586,8 @@ History::InsertPlace(const VisitData& aPlace)
   NS_ENSURE_SUCCESS(rv, rv);
   rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("typed"), aPlace.typed);
   NS_ENSURE_SUCCESS(rv, rv);
+  rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("frecency"), aPlace.frecency);
+  NS_ENSURE_SUCCESS(rv, rv);
   rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("hidden"), aPlace.hidden);
   NS_ENSURE_SUCCESS(rv, rv);
   nsCAutoString guid(aPlace.guid);
@@ -1868,29 +1873,36 @@ History::VisitURI(nsIURI* aURI,
   // if the visit is toplevel or a non-toplevel followed link, then it can be
   // handled as usual and stored on disk.
 
+  PRUint32 transitionType = nsINavHistoryService::TRANSITION_LINK;
+
   if (!(aFlags & IHistory::TOP_LEVEL) && !isFollowedLink) {
     // A frame redirected to a new site without user interaction.
-    place.SetTransitionType(nsINavHistoryService::TRANSITION_EMBED);
+    transitionType = nsINavHistoryService::TRANSITION_EMBED;
   }
   else if (aFlags & IHistory::REDIRECT_TEMPORARY) {
-    place.SetTransitionType(nsINavHistoryService::TRANSITION_REDIRECT_TEMPORARY);
+    transitionType = nsINavHistoryService::TRANSITION_REDIRECT_TEMPORARY;
   }
   else if (aFlags & IHistory::REDIRECT_PERMANENT) {
-    place.SetTransitionType(nsINavHistoryService::TRANSITION_REDIRECT_PERMANENT);
+    transitionType = nsINavHistoryService::TRANSITION_REDIRECT_PERMANENT;
   }
   else if (recentFlags & nsNavHistory::RECENT_TYPED) {
-    place.SetTransitionType(nsINavHistoryService::TRANSITION_TYPED);
+    transitionType = nsINavHistoryService::TRANSITION_TYPED;
   }
   else if (recentFlags & nsNavHistory::RECENT_BOOKMARKED) {
-    place.SetTransitionType(nsINavHistoryService::TRANSITION_BOOKMARK);
+    transitionType = nsINavHistoryService::TRANSITION_BOOKMARK;
   }
   else if (!(aFlags & IHistory::TOP_LEVEL) && isFollowedLink) {
     // User activated a link in a frame.
-    place.SetTransitionType(nsINavHistoryService::TRANSITION_FRAMED_LINK);
+    transitionType = nsINavHistoryService::TRANSITION_FRAMED_LINK;
   }
-  else {
-    // User was redirected or link was clicked in the main window.
-    place.SetTransitionType(nsINavHistoryService::TRANSITION_LINK);
+
+  place.SetTransitionType(transitionType);
+  place.hidden = GetHiddenState(aFlags & IHistory::REDIRECT_SOURCE,
+                                transitionType);
+
+  // Error pages should never be autocompleted.
+  if (aFlags & IHistory::UNRECOVERABLE_ERROR) {
+    place.frecency = 0;
   }
 
   // EMBED visits are session-persistent and should not go through the database.
@@ -2097,6 +2109,7 @@ History::AddDownload(nsIURI* aSource, nsIURI* aReferrer,
 
   place.visitTime = aStartTime;
   place.SetTransitionType(nsINavHistoryService::TRANSITION_DOWNLOAD);
+  place.hidden = false;
 
   mozIStorageConnection* dbConn = GetDBConn();
   NS_ENSURE_STATE(dbConn);
@@ -2220,6 +2233,7 @@ History::UpdatePlaces(const jsval& aPlaceInfos,
                           nsINavHistoryService::TRANSITION_LINK,
                           nsINavHistoryService::TRANSITION_FRAMED_LINK);
       data.SetTransitionType(transitionType);
+      data.hidden = GetHiddenState(false, transitionType);
 
       // If the visit is an embed visit, we do not actually add it to the
       // database.

toolkit/components/places/nsNavHistory.cpp

@@ -1204,10 +1204,11 @@ nsNavHistory::invalidateFrecencies(const nsCString& aPlaceIdsQueryString)
       "THEN 0 "
       "ELSE -1 "
       "END) "
+    "WHERE frecency > 0 "
   );
 
   if (!aPlaceIdsQueryString.IsEmpty()) {
-    invalideFrecenciesSQLFragment.AppendLiteral("WHERE id IN(");
+    invalideFrecenciesSQLFragment.AppendLiteral("AND id IN(");
     invalideFrecenciesSQLFragment.Append(aPlaceIdsQueryString);
     invalideFrecenciesSQLFragment.AppendLiteral(")");
   }

toolkit/components/places/nsPlacesAutoComplete.js

@@ -1319,6 +1319,7 @@ urlInlineComplete.prototype = {
         + "SELECT host || '/' "
         + "FROM moz_hosts "
         + "WHERE host BETWEEN :search_string AND :search_string || X'FFFF' "
+        + "AND frecency <> 0 "
         + (this._autofillTyped ? "AND typed = 1 " : "")
         + "ORDER BY frecency DESC "
         + "LIMIT 1"

toolkit/components/places/nsPlacesTriggers.h

@@ -91,7 +91,7 @@
     "VALUES (" \
       "(SELECT id FROM moz_hosts WHERE host = fixup_url(get_unreversed_host(NEW.rev_host))), " \
       "fixup_url(get_unreversed_host(NEW.rev_host)), " \
-      "MAX((SELECT frecency FROM moz_hosts WHERE host = fixup_url(get_unreversed_host(NEW.rev_host))), NEW.frecency), " \
+      "MAX(IFNULL((SELECT frecency FROM moz_hosts WHERE host = fixup_url(get_unreversed_host(NEW.rev_host))), -1), NEW.frecency), " \
       "MAX(IFNULL((SELECT typed FROM moz_hosts WHERE host = fixup_url(get_unreversed_host(NEW.rev_host))), 0), NEW.typed) " \
     "); " \
   "END" \