Bug 487809 - Stop using visit_count to invalidate frecencies.
authorMarco Bonardo <mbonardo@mozilla.com>
Tue, 30 Aug 2011 16:24:01 +0200
changeset 76218 fea3711b548963e6b81e2f94bc63ba9852dce6f7
parent 76217 ed85e33792e086643c1ff66b65e7909525efe34e
child 76219 f64678060d4ad0f3950731f1bf2743f3d765828b
push id3
push userfelipc@gmail.com
push dateFri, 30 Sep 2011 20:09:13 +0000
bugs487809
milestone9.0a1
Bug 487809 - Stop using visit_count to invalidate frecencies. r=dietrich
toolkit/components/places/nsNavHistory.cpp
toolkit/components/places/nsNavHistory.h
toolkit/components/places/tests/unit/test_history_removeAllPages.js
--- a/toolkit/components/places/nsNavHistory.cpp
+++ b/toolkit/components/places/nsNavHistory.cpp
@@ -1446,19 +1446,18 @@ nsNavHistory::MigrateV7Up(mozIStorageCon
         "ALTER TABLE moz_places ADD frecency INTEGER DEFAULT -1 NOT NULL"));
     NS_ENSURE_SUCCESS(rv, rv);
 
     // create index for the frecency column
     // XXX multi column index with typed, and visit_count?
     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);
   }
 
   // Temporary migration code for bug 396300
   nsCOMPtr<mozIStorageStatement> moveUnfiledBookmarks;
   rv = aDBConn->CreateStatement(NS_LITERAL_CSTRING(
       "UPDATE moz_bookmarks "
       "SET parent = ("
@@ -2482,45 +2481,51 @@ nsNavHistory::GetHasHistoryEntries(PRBoo
   NS_ENSURE_SUCCESS(rv, rv);
 
   mHasHistoryEntries = *aHasEntries ? 1 : 0;
   return NS_OK;
 }
 
 
 nsresult
-nsNavHistory::FixInvalidFrecenciesForExcludedPlaces()
-{
-  // 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)
+nsNavHistory::invalidateFrecencies(const nsCString& aPlaceIdsQueryString)
+{
+  // 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;
 }
 
 
 // Call this method before visiting a URL in order to help determine the
 // transition type of the visit.
@@ -4136,79 +4141,34 @@ nsresult
 nsNavHistory::RemovePagesInternal(const nsCString& aPlaceIdsQueryString)
 {
   // Return early if there is nothing to delete.
   if (aPlaceIdsQueryString.IsEmpty())
     return NS_OK;
 
   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(")"));
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = CleanupPlacesOnVisitsDelete(aPlaceIdsQueryString);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Invalidate the cached value for whether there's history or not.
   mHasHistoryEntries = -1;
 
   return transaction.Commit();
 }
 
 
 /**
- * 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
  * 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 just had all its
@@ -4275,20 +4235,18 @@ nsNavHistory::CleanupPlacesOnVisitsDelet
   // Note that we do NOT delete favicons. Any unreferenced favicons will be
   // deleted next time the browser is shut down.
   nsresult rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
       "DELETE FROM moz_places WHERE id IN ( "
         ) + filteredPlaceIds + NS_LITERAL_CSTRING(
       ") "));
   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.
   for (PRInt32 i = 0; i < URIs.Count(); ++i) {
     NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
                      nsINavHistoryObserver,
                      OnDeleteURI(URIs[i], GUIDs[i], nsINavHistoryObserver::REASON_DELETED));
   }
@@ -4565,19 +4523,16 @@ nsNavHistory::RemoveVisitsByTimeframe(PR
     }
   }
 
   // force a full refresh calling onEndUpdateBatch (will call Refresh())
   UpdateBatchScoper batch(*this); // sends Begin/EndUpdateBatch to observers
 
   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(
       "DELETE FROM moz_historyvisits "
       "WHERE :from_date <= visit_date AND visit_date <= :to_date"),
     getter_AddRefs(deleteVisitsStmt));
   NS_ENSURE_SUCCESS(rv, rv);
   rv = deleteVisitsStmt->BindInt64ByName(NS_LITERAL_CSTRING("from_date"), aBeginTime);
@@ -4607,54 +4562,35 @@ nsNavHistory::RemoveVisitsByTimeframe(PR
 //
 //    This function is used to clear history.
 
 NS_IMETHODIMP
 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();
 
   // Invalidate the cached value for whether there's history or not.
   mHasHistoryEntries = -1;
 
   // Expiration will take care of orphans.
   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;
 }
 
 
 // nsNavHistory::HidePage
 //
 //    Sets the 'hidden' column to true. If we've not heard of the page, we
 //    succeed and do nothing.
--- a/toolkit/components/places/nsNavHistory.h
+++ b/toolkit/components/places/nsNavHistory.h
@@ -263,19 +263,24 @@ public:
   nsresult UpdateFrecency(PRInt64 aPlaceId);
 
   /**
    * Calculate frecencies for places that don't have a valid value yet
    */
   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
    * connection object is also used by the annotation service and bookmarks, so
    * that things can be grouped into transactions across these components.
    *
    * NOT ADDREFed.
    *
@@ -723,17 +728,16 @@ protected:
   nsresult CheckAndUpdateGUIDs();
   nsresult MigrateV7Up(mozIStorageConnection *aDBConn);
   nsresult MigrateV8Up(mozIStorageConnection *aDBConn);
   nsresult MigrateV9Up(mozIStorageConnection *aDBConn);
   nsresult MigrateV10Up(mozIStorageConnection *aDBConn);
   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,
                           PRBool aToplevel, nsIURI* aReferrer);
 
   nsresult AddVisitChain(nsIURI* aURI, PRTime aTime,
                          PRBool aToplevel, PRBool aRedirect,
                          nsIURI* aReferrer, PRInt64* aVisitID,
--- a/toolkit/components/places/tests/unit/test_history_removeAllPages.js
+++ b/toolkit/components/places/tests/unit/test_history_removeAllPages.js
@@ -72,20 +72,21 @@ let historyObserver = {
   onDeleteVisits: function() {},
 
   onClearHistory: function() {
     PlacesUtils.history.removeObserver(this, false);
 
     // 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.
         stmt = mDBConn.createStatement(
           "SELECT h.id FROM moz_places h WHERE h.frecency > 0 ");
         do_check_false(stmt.executeStep());
         stmt.finalize();
 
@@ -149,21 +150,18 @@ let historyObserver = {
           "JOIN moz_bookmarks bp ON bp.id = b.parent " +
           "JOIN moz_items_annos t ON t.item_id = bp.id " +
           "JOIN moz_anno_attributes n ON t.anno_attribute_id = n.id " +
           "WHERE n.name = 'livemark/feedURI' AND h.frecency <> 0 LIMIT 1");
         do_check_false(stmt.executeStep());
         stmt.finalize();
 
         do_test_finished();
-      }
-    }
-    Services.obs.addObserver(expirationObserver,
-                             PlacesUtils.TOPIC_EXPIRATION_FINISHED,
-                             false);
+      });
+    }, PlacesUtils.TOPIC_EXPIRATION_FINISHED, false);
   },
 
   QueryInterface: XPCOMUtils.generateQI([
     Ci.nsINavHistoryObserver,
   ]),
 }
 PlacesUtils.history.addObserver(historyObserver, false);