If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

make sure that CalculateFrecencyInternal() does not return 0 for places with at least one visit of bonus-able type

RESOLVED FIXED

Status

()

Firefox
Bookmarks & History
P3
normal
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: Seth Spitzer, Unassigned)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 -

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

10 years ago
make sure that CalculateFrecencyInternal() does not return 0 for places with at least one non-embed visit.

as mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=394038#c62, if a place has frecency of 0, we do not show it in the url bar autocomplete results.

additionally, because it has a frecency of zero, on idle, we will not attempt to recalculate frecency (because 0 is a low as you go, and -1 means "visible in ac results, but please recalculate me on idle".)

we need to make sure that if a place has any non embed, non download, non undefined visits that it gets a non-zero frecency.

here's one way this currently happen:

CalculateFrecencyInternal() uses the browser.frecency.numVisits (currently 10) most recent visits.  if those are all visits with browser.frecency.*VisitBonus of zero (like embed or download), our frecency will be zero. but what if the 11th visits was a link, bookmark, or typed?

we could fix this problem by excluding visits NOT IN (0,4,7) from our query to find the last 10 visits, but that makes the assumption that the bonus for embed and download are zero.

here's where we'd fix the query:

  // mDBVisitsForFrecency
  // NOTE: we are not limiting to visits with "visit_type NOT IN (0,4)"
  // because if we do that, mDBVisitsForFrecency would return no visits
  // for places with only embed (or undefined) visits.  That would
  // cause use to estimate a frecency based on what information we do have,
  // see CalculateFrecencyInternal().  that would result in a 
  // non-zero frecency for a place with only
  // embedded visits, instead of a frececny of 0.
  rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
    "SELECT visit_date, visit_type FROM moz_historyvisits " 
    "WHERE place_id = ?1 ORDER BY visit_date DESC LIMIT ") +
     nsPrintfCString("%d", mNumVisitsForFrecency),
    getter_AddRefs(mDBVisitsForFrecency));
  NS_ENSURE_SUCCESS(rv, rv);

I guess we could avoid hard coding and programtically build up the NOT IN(...) based on the pref values.

additionally, this could happen if the weights chosen for any of the buckets was zero:

+// weights for buckets for frecency calculations
+pref("browser.frecency.firstBucketWeight", 100);
+pref("browser.frecency.secondBucketWeight", 70);
+pref("browser.frecency.thirdBucketWeight", 50);
+pref("browser.frecency.fourthBucketWeight", 30);
+pref("browser.frecency.defaultBucketWeight", 10);

note to dietrich:  put a warning about how these weights need to be > 0.

+
+// bonus (in percent) for visit transition types for frecency calculations
+pref("browser.frecency.embedVisitBonus", 0);
+pref("browser.frecency.linkVisitBonus", 120);
+pref("browser.frecency.typedVisitBonus", 200);
+pref("browser.frecency.bookmarkVisitBonus", 140);
+pref("browser.frecency.downloadVisitBonus", 0);
+pref("browser.frecency.permRedirectVisitBonus", 0);
+pref("browser.frecency.tempRedirectVisitBonus", 0);
+pref("browser.frecency.defaultVisitBonus", 0);

note to dietrich:  

are those values for permRedirectVisitBonus and tempRedirectVisitBonus correct?  should they, like embed and download, be worth nothing?
Flags: blocking-firefox3?
(Reporter)

Comment 1

10 years ago
dietrich has already fixed this along with bug #394038

from calculatefrecencyinternal()

+      // fix for bug #412219
+      if (!pointsForSampledVisits) {
+        // For URIs with zero points in the sampled recent visits
+        // but "browsing" type visits outside the sampling range,
+        // set frecency to -1, so that they're still shown in autocomplete.
+        PRInt32 trueVisitCount = 0;
+        rv = CalculateVisitCount(aPlaceId, PR_FALSE /* not for frecency */,
+                                 &trueVisitCount);
+        if (NS_SUCCEEDED(rv) && trueVisitCount)
+          *aFrecency = -1;
+        else
+          *aFrecency = 0;
+      }

the query CalculateVisitCount() will use (if the 2nd param is PR_FALSE) is:

+  // this query is used to calculate the visit_count column
+  // which we use in the UI.  to the end user, we should not count
+  // embedded (or undefined) visits.
+  rv = mDBConn->CreateStatement(
+    NS_LITERAL_CSTRING("SELECT COUNT(*) FROM moz_historyvisits " 
+      "WHERE visit_type NOT IN(0,4) AND place_id = ?1"),
+    getter_AddRefs(mDBTrueVisitCount));
+  NS_ENSURE_SUCCESS(rv, rv);

This works, but does not account for download and redirect visits.

The query is not 100% correct, based on the prefs values, see comment #0.

at the very least, I'd suggest making this:

IN(1,2,3) (link, typed,bookmark) as at least that matches the default pref values.

ideally, we'd build the IN() or NOT IN() based on those pref values.

we still should consider putting a warning about how the browser.frecency.*BucketWeight need to be > 0.
So is this FIXED now?
Flags: blocking-firefox3? → blocking-firefox3+

Updated

10 years ago
Priority: -- → P4
not fixed, need to make the tweak recommended in comment #1.
Priority: P4 → P3
Summary: make sure that CalculateFrecencyInternal() does not return 0 for places with at least one non-embed visit → make sure that CalculateFrecencyInternal() does not return 0 for places with at least one visit of bonus-able type
Dietrich thinks this might be fixed now, but will confirm.

Not blocking, in any case.
Assignee: nobody → dietrich

Updated

10 years ago
Flags: blocking-firefox3+ → blocking-firefox3-

Updated

9 years ago
Depends on: 449406
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
Assignee: dietrich → nobody
I confirmed this is fixed in current code
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.