Per conversation with Mano online. If that's a string uri or an nsIURI, I don't know though :)

Passes tests too. I do, however, find it concerning that no test cases try using a referrer.

What happens if you try to add a visit with a referrer URL which is not in moz_places?

If we don't get a result from the query, we don't use zero. I have no clue if that's the right behavior, but I made a decision last night when working on this (nobody was around to ask).

The old patch was using the wrong uri. I also placed the referrer code in an if so it doesn't run if we don't have a referring uri.

Comment on attachment 286410 [details] [diff] [review] v1.1 So I think you should create an entry for the referrer as well, as we do for insertBookmark for an item which has no moz_places entry.

Code wise, this does what you want. However, my test case fails, and I'm not really sure why (maybe I found a bug?). I walked through the test case in the debugger, and it's clearly getting added to the db, I even see the place id that the referrer is getting. The test changes I'm referring to are in test_history.js, so please take a look at tell me if I'm just doing something stupid.

I notice bug 399490 seeks to eliminate nsIURI from the places api; I assume that's not relevant here?

yeah, it's not, that bug is mostly about the bookmarks service.

Comment on attachment 286443 [details] [diff] [review] v1.2 > >- rv = InternalAddVisit(pageID, aReferringVisit, aSessionID, aTime, >+ // Get the place id for the referrer, if we have one >+ PRInt64 referringVisitID = 0; >+ if (aReferringURI) { >+ mozStorageStatementScoper scoper(mDBGetPageVisitStats); >+ rv = BindStatementURI(mDBGetPageVisitStats, 0, aReferringURI); >+ PRBool visited = PR_FALSE; >+ if (NS_SUCCEEDED(rv)) >+ rv = mDBGetPageVisitStats->ExecuteStep(&visited); >+ >+ if (NS_SUCCEEDED(rv) && visited) { >+ rv = mDBGetPageVisitStats->GetInt64(0, &referringVisitID); >+ if (NS_FAILED(rv)) >+ referringVisitID = 0; >+ } else { >+ // Add the referrer >+ rv = InternalAddNewPage(aReferringURI, nsXPIDLString(), PR_FALSE, >+ PR_FALSE, 1, &referringVisitID); InternalAddNewPage returns a place Id, not a visit Id. I think you want to call AddVisit itself from there.

Requesting blocking since this bug blocks a bug that blocks a blocker (Bug 390491).

This doesn't pass the expiration tests, but I think that's a fluke.

Still failing - not sure what's up (I didn't touch this code!) ../../../../_tests/xpcshell-simple/test_places/unit/test_expiration.js: FAIL ../../../../_tests/xpcshell-simple/test_places/unit/test_expiration.js.log: >>>>>>> ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "'[JavaScript Error: "Components.classes['@mozilla.org/xre/app-info;1'] has no properties" {file: "file:///Volumes/Mozilla/testtree/mozilla/obj-i386-apple-darwin8.10.1/dist/bin/components/nsBrowserContentHandler.js" line: 750}]' when calling method: [nsIModule::registerSelf]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "<unknown>" data: yes] ************************************************************ *** test pending starting history_expire_visits test *** test pending *** test finished *** running event loop *** exiting *** CHECK FAILED: http://mozilla.com/ == http://filler.com/0 JS frame :: /Volumes/Mozilla/testtree/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_throw :: line 99 JS frame :: /Volumes/Mozilla/testtree/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_check_eq :: line 114 JS frame :: ../../../../_tests/xpcshell-simple/test_places/unit/test_expiration.js :: checkExpireByVisitsTest :: line 488 JS frame :: /Volumes/Mozilla/testtree/mozilla/tools/test-harness/xpcshell-simple/head.js :: anonymous :: line 62 JS frame :: /Volumes/Mozilla/testtree/mozilla/tools/test-harness/xpcshell-simple/head.js :: anonymous :: line 62 done history_expire_visits test starting history_expire_days test *** FAIL *** WARNING: nsExceptionService ignoring thread destruction after shutdown: file /Volumes/Mozilla/testtree/mozilla/xpcom/base/nsExceptionService.cpp, line 191 nsStringStats => mAllocCount: 3508 => mReallocCount: 58 => mFreeCount: 3508 => mShareCount: 388 => mAdoptCount: 27 => mAdoptFreeCount: 27 <<<<<<<

Seth - I think you were the one who was working on the expiration stuff - do you know what's up with the test failures that I'm getting?

Updated to trunk. Looks to me like the tests are still failing. Still no idea why...

I'm rewriting that failing test in bug 402880, so I'll take a look tomorrow at why this is happening.

Hrm, so FYI changing ghist.addURI(uri("http://fizz.com"), false, true, triggerURI); to ghist.addURI(uri("http://fizz.com"), false, true, null); fixes it for me.

So, from nsNavHistory: 3190 if (! FindLastVisit(aReferrer, &referringVisit, aSessionID)) { 3191 // we couldn't find a visit for the referrer, don't set it this means: 1) Your test fails because you've fixed a bug - AddURI ignores aReferrer if the latter hasn't been visited. Meaningfully, passing triggerURI to addURI in the test is a no-op. 2) You can simply the code path in AddVist to use FindLastVist.

And, unless dietrich objects, I'm find with applying the change in comment 16, I don't think the 8th is necessary here.

Passes tests. I have no idea what comment 17 is talking about - sorry.

Addresses past comments and passes tests.

Comment on attachment 294244 [details] [diff] [review] v1.6 You don't need to close the container in uri_in_db, thus you could just return childCount == 1. r=mano otherwise.

Comment on attachment 294244 [details] [diff] [review] v1.6 a=beltzner for 1.9

Yay! Checking in toolkit/components/places/public/nsINavHistoryService.idl; new revision: 1.74; previous revision: 1.73 Checking in toolkit/components/places/src/nsNavHistory.cpp; new revision: 1.218; previous revision: 1.217 Checking in toolkit/components/places/tests/bookmarks/test_bookmarks.js; new revision: 1.39; previous revision: 1.38 Checking in toolkit/components/places/tests/unit/test_385397.js; new revision: 1.2; previous revision: 1.1 Checking in toolkit/components/places/tests/unit/test_399266.js; new revision: 1.2; previous revision: 1.1 Checking in toolkit/components/places/tests/unit/test_expiration.js; new revision: 1.15; previous revision: 1.14 Checking in toolkit/components/places/tests/unit/test_history.js; new revision: 1.8; previous revision: 1.7 Checking in toolkit/components/places/tests/unit/test_nsINavHistoryViewer.js; new revision: 1.4; previous revision: 1.3 Checking in toolkit/components/places/tests/unit/test_resolveNullBookmarkTitles.js; new revision: 1.3; previous revision: 1.2 Checking in toolkit/components/places/tests/unit/test_result_sort.js; new revision: 1.8; previous revision: 1.7

Note - I fixed that test on checkin (tried it locally too).

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