401980 - use a simpler query than mDBGetURLPageInfo for IsURIStringVisited

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[(not reading, please use [e-mail] instead)]

use a simpler query than mDBGetURLPageInfo for IsURIStringVisited

all we care about is if visit count is > 0, but our query is:

"SELECT h.id, h.url, h.title, h.rev_host, h.visit_count FROM moz_places h WHERE h.url = ?1"

Why not:

"SELECT h.visit_count FROM moz_places h WHERE h.url = ?1"

Comment 1

[(not reading, please use [e-mail] instead)]

maybe with a with tons and tons of links (http://lxr.mozilla.org/seamonkey/source/layout/base/nsCSSFrameConstructor.cpp) fixing this could have a measurable performance impact?

Comment 2

[Marco Bonardo [:mak]]

Attachment: use mDBGetPageVisitStats instead of mDBGetURLPageInfo for visit count

from an sql perspective there is no difference, both queries takes about 0,22ms, the only difference will be in memory consumption due to h.url, h.title and h.rev_host that are (could be) long text fields. 
For this reason a good choice could be use mDBGetPageVisitStats instead of mDBGetURLPageInfo

      "SELECT id, visit_count, typed, hidden "
      "FROM moz_places "
      "WHERE url = ?1"

this will add only typed and hidden that are int, should be memory friendly enough without adding a new statement

Comment 3

[(not reading, please use [e-mail] instead)]

Comment on attachment 293490 [details] [diff] [review]
use mDBGetPageVisitStats instead of mDBGetURLPageInfo for visit count

r=sspitzer, thanks marco.

if we get approval, I'll land for you.

it sounds like, which this might not show up in Tp, it could affect MH test.

Comment 4

[(not reading, please use [e-mail] instead)]

Attachment: as checked in

Comment 5

[(not reading, please use [e-mail] instead)]

fixed, thanks again marko.

Checking in nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.215; previous revision: 1.214
done

while we can't test IsURIStringVisited() directly, we can and should test nsNavHistory::IsVisited(), so adding in test suite ?.

Comment 6

[(not reading, please use [e-mail] instead)]

> it sounds like, which this might not show up in Tp, it could affect MH test.

it didn't show up anywhere, but I'm still glad we fixed it, thanks marco.

see also bug #408790 which I found while testing your patch.

Comment 7

[(not reading, please use [e-mail] instead)]

Attachment: test

note, we do have one test for isVisited() already, see http://lxr.mozilla.org/mozilla/source/docshell/test/unit/test_nsIDownloadHistory.js, but this tests something else.

we really should test isVisited() in places.

note, part of this test will fail because of bug #408790 so it is commented out.

once we fix that bug, we can uncomment (and complete) the test of uris that we block from history.

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 293736 [details] [diff] [review]
test

r=me, thanks for adding this.

yes, we need to go though and find other areas of the API that also do not have breadth testing like this.

Comment 9

[(not reading, please use [e-mail] instead)]

test checked in, thanks dietrich

once I have a fix for bug #408790, I'll re-enable the commented out parts (and finish testing other schemes which should be blocked from history.)

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

Comment 10

[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