While doing bug 476298, I noticed the latter half of UpdateFrecency and FixInvalidFrecencies basically do the same thing, and divergent code could lead to issues.

At one point I was changing UpdateFrecency to just take the placeId and instead of everything having aIsBookmarked, CalculateFrecency would take a aMaybeBookmarked (which could be given !IsQueryURI or placeId != -1, etc) and then CalculateFrecency would do IsRealBookmarked. And this is after getting rid of CalculateFrecency and making Internal the main thing. But I ran into issues with navBookmarks..

Just FYI what I had going on before I stopped from test failures :p

Comment on attachment 372067 [details] [diff] [review] v1 ># HG changeset patch ># User Edward Lee <edilee@mozilla.com> ># Date 1239381480 18000 ># Node ID eeab39d088edadc1e411d488bd56b21a1fd2879c ># Parent 0bf1c26438e1c50c7199826669a8b36dd21db991 >Bug 487810 - Refactor UpdateFrecency and FixInvalidFrecencies to share frecency updating logic >Separate the last half of UpdateFrecency into UpdateFrecencyInternal to be reused by FixInvalidFrecencies. > >diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp >--- a/toolkit/components/places/src/nsNavHistory.cpp >+++ b/toolkit/components/places/src/nsNavHistory.cpp >@@ -7254,52 +7254,63 @@ nsNavHistory::UpdateFrecency(PRInt64 aPl > PRInt32 hidden = 0; > rv = mDBGetPlaceVisitStats->GetInt32(1, &hidden); > NS_ENSURE_SUCCESS(rv, rv); > > PRInt32 oldFrecency = 0; > rv = mDBGetPlaceVisitStats->GetInt32(2, &oldFrecency); > NS_ENSURE_SUCCESS(rv, rv); > >+ rv = UpdateFrecencyInternal(aPlaceId, typed, hidden, oldFrecency, >+ aIsBookmarked); >+ NS_ENSURE_SUCCESS(rv, rv); nit: line up input param on second line r=me otherwise, either with a test, or confirmation that the codepaths that have changed are executed in pre-existing tests.

http://hg.mozilla.org/mozilla-central/rev/b8da67f93ea4 Separate the last half of UpdateFrecency into UpdateFrecencyInternal to be reused by FixInvalidFrecencies. Existing tests touch both paths, e.g., test_000_frecency and test_migrateFrecency.