406358 - Location bar drop down ignores frequency of visits

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Ed Lee :Mardak]

Clicking the drop down only shows items that are most recently visited ignoring visit count.

From the patch for bug 389491..
 // nsNavHistory::AutoCompleteTypedSearch
...
 //    visit count (primary) and time since last visited (secondary).
...
+    "ORDER BY v.visit_date DESC LIMIT ");


Which is kinda interesting because bug 329118 changed the query from/to..
-      "WHERE h.typed = 1 ORDER BY visit_count DESC LIMIT ");
+      "WHERE h.typed = 1 ORDER BY visit_date DESC LIMIT ");

Comment 1

[Ed Lee :Mardak]

Attachment: v1 testcase

Testcase that passes after bug 406359 is fixed. Before the fix, "test 3" (4th test) is the one that fails out of all 8 because the results for empty search ignores the visit count.

Comment 2

[Ed Lee :Mardak]

Oh yeah, the note from the testcase..

Because the interactions among *DIFFERENT* visit counts and visit dates is not well defined, this test holds one of the two values constant when modifying the other.

Right now, we do SQL ORDER BY sorting so it prefers the earlier sort over the latter. We might be doing something different for a global ranking, so I didn't want to require fixes to this testcase when future changes define the ordering of "count=2 visit=2 hours ago vs count=1 visit=1 hour ago" (first is older but has more visits.. latter has fewer visits but more recent)

Comment 3

[Dietrich Ayala (:dietrich)]

i think this is fixed by bug 394038 landing. mardak is this testcase still valid for review?

Comment 4

[Ed Lee :Mardak]

Actually.. the testcase doesn't pass for some reason. It seems like my multiple addVisits to increase the visit_count doesn't make a difference.

Comment 5

[Ed Lee :Mardak]

Fyi, I tried changing the count (c1, c2) and the dates (d1, d2) just incase they happened to fall into the same frecency "level", but that didn't seem to make a difference.

I looked at the other test cases and they don't seem to take into account visit_count. Am I increasing visit_count in the wrong way by doing multiple addVisits ?

Comment 6

[Ed Lee :Mardak]

Comment on attachment 291049 [details] [diff] [review]
v1 testcase

I'll hold off on this for now... but potentially there /is/ a bug somewhere because this testcase used to pass when I implemented a fix with bug 406359 back when it wasn't global frecency.

Comment 7

[Ed Lee :Mardak]

Attachment: v2

2 issues from the earlier testcase:

1) Seems like I wasn't setting the visit count correctly.. multiple addVisit != setPageDetails with a visit count.
2) Dates I used didn't fall into different buckets.. 1 hour vs 3 hours ago. I switched the latter to 10 days ago.

Comment 8

[Ed Lee :Mardak]

The testcase tests for both the dropdown and searching (and passes on the current trunk because it was fixed by bug 394038)

Comment 9

[Ed Lee :Mardak]

Checking in toolkit/components/places/tests/unit/test_frecency.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_frecency.js,v  <--  test_frecency.js
initial revision: 1.1
done

Comment 10

[Ed Lee :Mardak]

Comment on attachment 300915 [details] [diff] [review]
v2

Checked on IRC and it was okay to land the testcase.

Comment 11

[Ed Lee :Mardak]

This will be useful to make sure the fix for bug 406359 doesn't break things.

Comment 12

[Gervase Markham [:gerv]]

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