History sidebar "By Site" should use visit_count to avoid display of empty sites

RESOLVED FIXED in Firefox 3

Status

()

P2
normal
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: ondrej, Assigned: ondrej)

Tracking

Trunk
Firefox 3
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
wanted-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

11 years ago
Created attachment 306133 [details] [diff] [review]
Use visit_count to filter out empty hosts

When history sidebar shows results "By Site", it does not join with history visits to be superfast. However, if you import bookmarks or clear your history, the view will show all the sites.

We can stay superfast but avoid displaying those hosts by using visit_count from the moz_places table. This could speed up "By Date and Site" view too.
Flags: blocking-firefox3?
Attachment #306133 - Flags: review?(dietrich)

Comment 1

11 years ago
actually visit_count is not realiable so this probably depends on Bug 416313

Updated

11 years ago
Depends on: 416313
This will not block the final release of Firefox 3.
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-

Comment 3

11 years ago
in the second query (the one with the additional conditions):

-                   "WHERE h.hidden <> 1 AND h.rev_host = '.' "
+                   "WHERE h.hidden <> 1 AND h.rev_host = '.' AND visit_count > 0 "

-                  "WHERE h.hidden <> 1 AND h.rev_host <> '.' "
+                  "WHERE h.hidden <> 1 AND h.rev_host <> '.' AND visit_count > 0 "

h.visit_count would be more consistent, although it does not make any difference.
(Assignee)

Comment 4

11 years ago
Created attachment 312243 [details] [diff] [review]
Use visit_count to filter out empty hosts (nit and unbitrot)

(In reply to comment #3)
> h.visit_count would be more consistent, although it does not make any
> difference.

You are right, and because unbitrotting was necessary anyway, here comes the fixed patch.
Attachment #306133 - Attachment is obsolete: true
Attachment #312243 - Flags: review?(dietrich)
Attachment #306133 - Flags: review?(dietrich)
Blocks: 432197
Comment on attachment 312243 [details] [diff] [review]
Use visit_count to filter out empty hosts (nit and unbitrot)

r=me thanks.
Attachment #312243 - Flags: review?(dietrich) → review+
Comment on attachment 312243 [details] [diff] [review]
Use visit_count to filter out empty hosts (nit and unbitrot)

fixes blocker bug 432197.
Attachment #312243 - Flags: approval1.9?
Created attachment 319658 [details] [diff] [review]
with the test from bug 432197
Attachment #312243 - Attachment is obsolete: true
Attachment #319658 - Flags: approval1.9?
Attachment #312243 - Flags: approval1.9?
OS: Windows XP → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → Firefox 3
Comment on attachment 319658 [details] [diff] [review]
with the test from bug 432197

a1.9=beltzner
Attachment #319658 - Flags: approval1.9? → approval1.9+

Comment 9

11 years ago
Marking a blocker since it blocks bug 432197 and we are approving the patch.
Flags: blocking-firefox3- → blocking-firefox3+

Comment 10

11 years ago
Dietrich/Marco any performance concerns here? Looks like just an extra part of the where clause and no additional joins - but want to make sure...
Whiteboard: [has patch][has review][has approval]
Since we're now able to use moz_places.visit_count instead of joining against moz_historyvisits, this should not have a significant effect on performance.
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.302; previous revision: 1.301
done
Checking in toolkit/components/places/tests/unit/test_history_sidebar.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_history_sidebar.js,v  <--  test_history_sidebar.js
new revision: 1.4; previous revision: 1.3
done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has review][has approval]
backed out, tree is closed
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [has patch][has review][has approval]
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.304; previous revision: 1.303
done
Checking in toolkit/components/places/tests/unit/test_history_sidebar.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_history_sidebar.js,v  <--  test_history_sidebar.js
new revision: 1.6; previous revision: 1.5
done
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has review][has approval]
(In reply to comment #10)
> Dietrich/Marco any performance concerns here? Looks like just an extra part of
> the where clause and no additional joins - but want to make sure...

should not be a problem it's a simple check on a column already joined

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
You need to log in before you can comment on or make changes to this bug.