Closed Bug 412110 Opened 17 years ago Closed 10 years ago

don't bother calculating frecencies for place: urls (they should be 0)

Categories

(Toolkit :: Places, defect, P5)

defect

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: sspitzer, Unassigned)

References

Details

don't both calculating frecencies for place: urls the fix will be to nsNavHistory::InternalAddNewPage() we currently do: // frecency PRInt32 frecency = -1; if (aCalculateFrecency) { rv = CalculateFrecency(aTyped, aVisitCount, -1 /* no page id, since this page doesn't exist */, url, &frecency); NS_ENSURE_SUCCESS(rv, rv); } I think we should do something like: PRInt32 frecency = -1; if (IsQueryURL(url)) frecency = 0; // place: bookmarks should not show up in autocomplete else if (aCalculateFrecency) { rv = CalculateFrecency(aTyped, aVisitCount, -1 /* no page id, since this page doesn't exist */, url, &frecency); NS_ENSURE_SUCCESS(rv, rv); } Note that if aCalculateFrecency is false, we use -1 for frecency to show that it is invalid, and need to be calculated (on idle) but it will still show up in autocomplete. for place: urls, frececny should always be 0, to prevent them from showing up in autocomplete. note, even when we fix this, we still need the code in FixInvalidFrecenciesForExcludedPlaces() to fix place: queries, as we might have set a place: query to -1 on clear all private data or from migration from fx 3b2. another note: it might be possible that something is calling CalculateFrecencyInternal() directly with a place: url. Here's a stack showing how this can happen: places.dll!nsNavHistory::CalculateFrecencyInternal(int aTyped=0, int aVisitCount=0, __int64 aPlaceId=-1, int aIsBookmarked=0, int * aFrecency=0x0012c468) Line 6123 C++ places.dll!nsNavHistory::CalculateFrecency(int aTyped=0, int aVisitCount=0, __int64 aPlaceId=-1, nsCAutoString & aURL={...}, int * aFrecency=0x0012c468) Line 6196 + 0x20 bytes C++ > places.dll!nsNavHistory::InternalAddNewPage(nsIURI * aURI=0x00fb5c10, const nsAString_internal & aTitle={...}, int aHidden=1, int aTyped=0, int aVisitCount=0, int aCalculateFrecency=1, __int64 * aPageID=0x0012c740) Line 1481 + 0x25 bytes C++ places.dll!nsNavHistory::GetUrlIdFor(nsIURI * aURI=0x00fb5c10, __int64 * aEntryID=0x0012c740, int aAutoCreate=1) Line 1408 + 0x1c bytes C++ places.dll!nsNavBookmarks::InsertBookmark(__int64 aFolder=8, nsIURI * aItem=0x00fb5c10, int aIndex=-1, const nsACString_internal & aTitle={...}, __int64 * aNewBookmarkId=0x0012c988) Line 874 + 0x19 bytes C++
Summary: don't both calculating frecencies for place: urls → doncalculating frecencies for place: urls
Summary: doncalculating frecencies for place: urls → don't bother calculating frecencies for place: urls (they should be 0)
here's an example of where we call CalculateFrecenyInternal() directly: places.dll!nsNavHistory::CalculateFrecencyInternal(int aTyped=0, int aVisitCount=0, __int64 aPlaceId=51, int aIsBookmarked=0, int * aFrecency=0x0012c55c) Line 6121 C++ places.dll!nsNavHistory::UpdateFrecency(__int64 aPlaceId=51, int aIsBookmarked=0) Line 5999 + 0x23 bytes C++ > places.dll!nsNavBookmarks::InsertBookmark(__int64 aFolder=60, nsIURI * aItem=0x069d2488, int aIndex=-1, const nsACString_internal & aTitle={...}, __int64 * aNewBookmarkId=0x0012c988) Line 947 + 0x1b bytes C++ That happens when creating a bookmark.
this will also happen when you change a place: bookmark places.dll!nsNavHistory::CalculateFrecencyInternal(int aTyped=0, int aVisitCount=0, __int64 aPlaceId=53, int aIsBookmarked=1, int * aFrecency=0x00129478) Line 6121 C++ places.dll!nsNavHistory::UpdateFrecency(__int64 aPlaceId=53, int aIsBookmarked=1) Line 5999 + 0x23 bytes C++ > places.dll!nsNavBookmarks::ChangeBookmarkURI(__int64 aBookmarkId=10, nsIURI * aNewURI=0x06c8a1b0) Line 2197 + 0x19 bytes C++
Priority: -- → P5
Product: Firefox → Toolkit
QA Contact: places → places
the pointed out code is quite old, I think nowadays (I quickly went through the code and looks like I'm recalling correctly) we are doing the right thing (we set frecency to 0 and never update 0 frecencies).
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.