Bug 487809 - Stop using visit_count to invalidate frecencies.

r=dietrich
2011-08-30 16:24:01 +02:00 · 2011-08-30 16:24:01 +02:00 · 638cc44627
--- a/toolkit/components/places/nsNavHistory.cpp
+++ b/toolkit/components/places/nsNavHistory.cpp
@ -1451,9 +1451,8 @@ nsNavHistory::MigrateV7Up(mozIStorageConnection* aDBConn)
    rv = aDBConn->ExecuteSimpleSQL(CREATE_IDX_MOZ_PLACES_FRECENCY);
    NS_ENSURE_SUCCESS(rv, rv);

-    // for place: items and unvisited livemark items, we need to set
-    // the frecency to 0 so that they don't show up in url bar autocomplete
-    rv = FixInvalidFrecenciesForExcludedPlaces();
+    // Invalidate all frecencies, since they need recalculation.
+    rv = invalidateFrecencies(EmptyCString());
    NS_ENSURE_SUCCESS(rv, rv);
  }

@ -2487,35 +2486,41 @@ nsNavHistory::GetHasHistoryEntries(PRBool* aHasEntries)


 nsresult
-nsNavHistory::FixInvalidFrecenciesForExcludedPlaces()
+nsNavHistory::invalidateFrecencies(const nsCString& aPlaceIdsQueryString)
 {
-  // for every moz_place that has an invalid frecency (< 0) and
-  // is an unvisited child of a livemark feed, or begins with "place:",
-  // set frecency to 0 so that it is excluded from url bar autocomplete.
-  nsCOMPtr<mozIStorageStatement> dbUpdateStatement;
-  nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
-      "UPDATE moz_places "
-      "SET frecency = 0 WHERE id IN ("
-        "SELECT h.id FROM moz_places h "
-        "WHERE h.url >= 'place:' AND h.url < 'place;' "
-        "UNION ALL "
-        // Unvisited child of a livemark
-        "SELECT b.fk FROM moz_bookmarks b "
-        "JOIN moz_places h ON b.fk = h.id AND visit_count = 0 AND frecency < 0 "
-        "JOIN moz_bookmarks bp ON bp.id = b.parent "
-        "JOIN moz_items_annos a ON a.item_id = bp.id "
-        "JOIN moz_anno_attributes n ON n.id = a.anno_attribute_id "
-        "WHERE n.name = :anno_name "
-      ")"),
-    getter_AddRefs(dbUpdateStatement));
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  rv = dbUpdateStatement->BindUTF8StringByName(
-    NS_LITERAL_CSTRING("anno_name"), NS_LITERAL_CSTRING(LMANNO_FEEDURI)
+  // Exclude place: queries and unvisited livemark children from autocomplete,
+  // by setting their frecency to zero.
+  nsCAutoString invalideFrecenciesSQLFragment(
+    "UPDATE moz_places SET frecency = (CASE "
+      "WHEN url BETWEEN 'place:' AND 'place;' "
+      "THEN 0 "
+      "WHEN id IN (SELECT b.fk FROM moz_bookmarks b "
+                  "JOIN moz_bookmarks bp ON bp.id = b.parent "
+                  "JOIN moz_items_annos a ON a.item_id = bp.id "
+                  "JOIN moz_anno_attributes n ON n.id = a.anno_attribute_id "
+                  "WHERE b.fk = moz_places.id AND visit_count = 0 "
+                    "AND n.name = :anno_name) "
+      "THEN 0 "
+      "ELSE -1 "
+      "END) "
  );
-  NS_ENSURE_SUCCESS(rv, rv);

-  rv = dbUpdateStatement->Execute();
+  if (!aPlaceIdsQueryString.IsEmpty()) {
+    invalideFrecenciesSQLFragment.AppendLiteral("WHERE id IN(");
+    invalideFrecenciesSQLFragment.Append(aPlaceIdsQueryString);
+    invalideFrecenciesSQLFragment.AppendLiteral(")");
+  }
+
+  nsCOMPtr<mozIStorageStatement> stmt =
+    GetStatementByStoragePool(invalideFrecenciesSQLFragment);
+  NS_ENSURE_STATE(stmt);
+  nsresult rv = stmt->BindUTF8StringByName(
+     NS_LITERAL_CSTRING("anno_name"), NS_LITERAL_CSTRING(LMANNO_FEEDURI)
+   );
+   NS_ENSURE_SUCCESS(rv, rv);
+
+  nsCOMPtr<mozIStoragePendingStatement> ps;
+  rv = stmt->ExecuteAsync(nsnull, getter_AddRefs(ps));
  NS_ENSURE_SUCCESS(rv, rv);

  return NS_OK;
@ -4141,11 +4146,8 @@ nsNavHistory::RemovePagesInternal(const nsCString& aPlaceIdsQueryString)

  mozStorageTransaction transaction(mDBConn, PR_FALSE);

-  nsresult rv = PreparePlacesForVisitsDelete(aPlaceIdsQueryString);
-  NS_ENSURE_SUCCESS(rv, rv);
-
  // Delete all visits for the specified place ids.
-  rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
+  nsresult rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
      "DELETE FROM moz_historyvisits WHERE place_id IN (") +
        aPlaceIdsQueryString +
        NS_LITERAL_CSTRING(")"));
@ -4161,48 +4163,6 @@ nsNavHistory::RemovePagesInternal(const nsCString& aPlaceIdsQueryString)
 }


-/**
- * Prepares for deletion places that are about to have all their visits removed.
- * This is an internal method used by RemovePagesInternal and
- * RemoveVisitsByTimeframe.  This method does not execute in a transaction, so
- * callers should make sure they begin one if needed.
- *
- * @param aPlaceIdsQueryString
- *        A comma-separated list of place IDs, each of which is about to have
- *        all its visits removed
- */
-nsresult
-nsNavHistory::PreparePlacesForVisitsDelete(const nsCString& aPlaceIdsQueryString)
-{
-  // Return early if there is nothing to delete.
-  if (aPlaceIdsQueryString.IsEmpty())
-    return NS_OK;
-
-  // if a moz_place is annotated or was a bookmark,
-  // we won't delete it, but we will delete the moz_visits
-  // so we need to reset the frecency.  Note, we set frecency to
-  // -visit_count, as we use that value in our "on idle" query
-  // to figure out which places to recalculate frecency first.
-  // Pay attention to not set frecency = 0 if visit_count = 0
-  // TODO (bug 487809): we don't need anymore to set frecency here.
-  nsresult rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
-      "UPDATE moz_places "
-      "SET frecency = -(visit_count + 1) "
-      "WHERE id IN ( "
-        "SELECT h.id " 
-        "FROM moz_places h "
-        "WHERE h.id IN ( ") + aPlaceIdsQueryString + NS_LITERAL_CSTRING(") "
-          "AND ( "
-            "EXISTS (SELECT b.id FROM moz_bookmarks b WHERE b.fk =h.id) "
-            "OR EXISTS (SELECT a.id FROM moz_annos a WHERE a.place_id = h.id) "
-          ") "        
-      ")"));
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  return NS_OK;
-}
-
-
 /**
 * Performs cleanup on places that just had all their visits removed, including
 * deletion of those places.  This is an internal method used by
@ -4280,10 +4240,8 @@ nsNavHistory::CleanupPlacesOnVisitsDelete(const nsCString& aPlaceIdsQueryString)
      ") "));
  NS_ENSURE_SUCCESS(rv, rv);

-  // If we have removed all visits to a livemark's child, we need to fix its
-  // frecency, or it would appear in the url bar autocomplete.
-  // XXX this might be dog slow, further degrading delete perf.
-  rv = FixInvalidFrecenciesForExcludedPlaces();
+  // Invalidate frecencies of touched places, since they need recalculation.
+  rv = invalidateFrecencies(aPlaceIdsQueryString);
  NS_ENSURE_SUCCESS(rv, rv);

  // Finally notify about the removed URIs.
@ -4570,9 +4528,6 @@ nsNavHistory::RemoveVisitsByTimeframe(PRTime aBeginTime, PRTime aEndTime)

  mozStorageTransaction transaction(mDBConn, PR_FALSE);

-  rv = PreparePlacesForVisitsDelete(deletePlaceIdsQueryString);
-  NS_ENSURE_SUCCESS(rv, rv);
-
  // Delete all visits within the timeframe.
  nsCOMPtr<mozIStorageStatement> deleteVisitsStmt;
  rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
@ -4612,34 +4567,10 @@ nsNavHistory::RemoveAllPages()
 {
  NS_ASSERTION(NS_IsMainThread(), "This can only be called on the main thread");

-  mozStorageTransaction transaction(mDBConn, PR_FALSE);
-
-  // reset frecency for all items that will _not_ be deleted
-  // Note, we set frecency to -visit_count since we use that value in our
-  // idle query to figure out which places to recalcuate frecency first.
-  // We must do this before deleting visits.
-  // TODO (bug 487809): we don't need anymore to set frecency here.
  nsresult rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
-    "UPDATE moz_places SET frecency = -(visit_count + 1) "
-    "WHERE id IN(SELECT b.fk FROM moz_bookmarks b WHERE b.fk NOTNULL)"));
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  // Expire visits, then let the paranoid functions do the cleanup for us.
-  rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
      "DELETE FROM moz_historyvisits"));
  NS_ENSURE_SUCCESS(rv, rv);

-  // Some of the remaining places could be place: urls or
-  // unvisited livemark items, so setting the frecency to -1
-  // will cause them to show up in the url bar autocomplete
-  // call FixInvalidFrecenciesForExcludedPlaces to handle that scenario.
-  rv = FixInvalidFrecenciesForExcludedPlaces();
-  if (NS_FAILED(rv))
-    NS_WARNING("failed to fix invalid frecencies");
-
-  rv = transaction.Commit();
-  NS_ENSURE_SUCCESS(rv, rv);
-
  // Clear the registered embed visits.
  clearEmbedVisits();

@ -4650,6 +4581,11 @@ nsNavHistory::RemoveAllPages()
  NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
                   nsINavHistoryObserver, OnClearHistory());

+  // Invalidate frecencies for the remaining places.  This must happen
+  // after the notification to ensure it runs enqueued to expiration.
+  rv = invalidateFrecencies(EmptyCString());
+  NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "failed to fix invalid frecencies");
+
  return NS_OK;
 }

--- a/toolkit/components/places/nsNavHistory.h
+++ b/toolkit/components/places/nsNavHistory.h
@ -268,9 +268,14 @@ public:
  nsresult FixInvalidFrecencies();

  /**
-   * Set the frecencies of excluded places so they don't show up in queries
+   * Invalidate the frecencies of a list of places so they will be recalculated
+   * at the first idle-daily notification.
+   *
+   * @param aPlacesIdsQueryString
+   *        Query string containing list of places to be invalidated.  If it's
+   *        an empty string all places will be invalidated.
   */
-  nsresult FixInvalidFrecenciesForExcludedPlaces();
+  nsresult invalidateFrecencies(const nsCString& aPlaceIdsQueryString);

  /**
   * Returns a pointer to the storage connection used by history. This
@ -728,7 +733,6 @@ protected:
  nsresult MigrateV11Up(mozIStorageConnection *aDBConn);

  nsresult RemovePagesInternal(const nsCString& aPlaceIdsQueryString);
-  nsresult PreparePlacesForVisitsDelete(const nsCString& aPlaceIdsQueryString);
  nsresult CleanupPlacesOnVisitsDelete(const nsCString& aPlaceIdsQueryString);

  nsresult AddURIInternal(nsIURI* aURI, PRTime aTime, PRBool aRedirect,
--- a/toolkit/components/places/tests/unit/test_history_removeAllPages.js
+++ b/toolkit/components/places/tests/unit/test_history_removeAllPages.js
@ -77,10 +77,11 @@ let historyObserver = {
    // check browserHistory returns no entries
    do_check_eq(0, PlacesUtils.bhistory.count);

-    let expirationObserver = {
-      observe: function (aSubject, aTopic, aData) {
-        Services.obs.removeObserver(this, aTopic, false);
+    Services.obs.addObserver(function observeExpiration(aSubject, aTopic, aData)
+    {
+      Services.obs.removeObserver(observeExpiration, aTopic, false);

+      waitForAsyncUpdates(function () {
        // Check that frecency for not cleared items (bookmarks) has been converted
        // to -MAX(visit_count, 1), so we will be able to recalculate frecency
        // starting from most frecent bookmarks.
@ -154,11 +155,8 @@ let historyObserver = {
        stmt.finalize();

        do_test_finished();
-      }
-    }
-    Services.obs.addObserver(expirationObserver,
-                             PlacesUtils.TOPIC_EXPIRATION_FINISHED,
-                             false);
+      });
+    }, PlacesUtils.TOPIC_EXPIRATION_FINISHED, false);
  },

  QueryInterface: XPCOMUtils.generateQI([