From e642011f5280d6f1b9e38465e1bd2c921bc6338f Mon Sep 17 00:00:00 2001 From: Marco Bonardo Date: Tue, 30 Aug 2011 16:24:01 +0200 Subject: [PATCH] Bug 487809 - Stop using visit_count to invalidate frecencies. r=dietrich --- toolkit/components/places/nsNavHistory.cpp | 148 +++++------------- toolkit/components/places/nsNavHistory.h | 10 +- .../tests/unit/test_history_removeAllPages.js | 14 +- 3 files changed, 55 insertions(+), 117 deletions(-) diff --git a/toolkit/components/places/nsNavHistory.cpp b/toolkit/components/places/nsNavHistory.cpp index 81e3e8510c0..ef8fa786282 100644 --- 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 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 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 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 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; } diff --git a/toolkit/components/places/nsNavHistory.h b/toolkit/components/places/nsNavHistory.h index 90f71dbb0ab..84d73e99c2e 100644 --- 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, diff --git a/toolkit/components/places/tests/unit/test_history_removeAllPages.js b/toolkit/components/places/tests/unit/test_history_removeAllPages.js index 2477b1199fa..d46a561ee25 100644 --- 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([