Refactor UpdateFrecency and FixInvalidFrecencies to share frecency updating logic

RESOLVED FIXED in mozilla1.9.2a1

Status

()

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Mardak, Assigned: Mardak)

Tracking

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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.
Posted patch v1Splinter Review
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..
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #372067 - Flags: review?(dietrich)
Just FYI what I had going on before I stopped from test failures :p
Attachment #372067 - Attachment is patch: true
Attachment #372067 - Attachment mime type: application/octet-stream → text/plain
Attachment #372067 - Flags: review?(dietrich)
Attachment #372067 - Flags: review?(dietrich)
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.
Attachment #372067 - Flags: review?(dietrich) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.