487810 - Refactor UpdateFrecency and FixInvalidFrecencies to share frecency updating logic

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Ed Lee :Mardak]

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.

Comment 1

[Ed Lee :Mardak]

Attachment: v1

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..

Comment 2

[Ed Lee :Mardak]

Attachment: v1 more refactoring (broken)

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

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 372067 [details] [diff] [review]
v1

># HG changeset patch
># User Edward Lee <[e-mail]>
># 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.

Comment 4

[Ed Lee :Mardak]

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.