It seems like clicking bookmarks results in visit_type of TRANSITION_LINK (1) instead of TRANSITION_BOOKMARK (3) http://mxr.mozilla.org/seamonkey/source/toolkit/components/places/public/nsINavHistoryService.idl#1166 http://mxr.mozilla.org/mozilla/source/toolkit/components/places/src/nsNavHistory.cpp#3246 Bug 394066 won't really do anything meaningful until this is fixed.

SELECT COUNT(*) FROM moz_historyvisits WHERE visit_type=3 gives me 0 for my profiles, and I've used bookmarks from time to time.

not recording bookmark visits as nsINavHistoryService::TRANSITION_BOOKMARK We need to start call MarkPageAsFollowedBookmark(), like we do for MarkPageAsTyped() we are depending on this for our global frecency algo, see bug #394038 note, we need to fix this typo in MarkPageAsFollowedBookmark(), which I have fixed locally: - mRecentTyped.Put(uriString, GetNow()); + mRecentBookmark.Put(uriString, GetNow());

I think we want to call MarkPageAsFollowedBookmark() from _openTabset() in utils.js and openSelectedNodeIn() in controller.js but, we need to be careful that when clicking on history items in the places organizer, sidebar, or history menu, we don't record those as bookmarked visits, right? additionally, we need to decide what clicking the home button should do (act like a bookmark visit?)

the history menu might already work fine, as it calls openUILink(). as for the history sidebar and history item in the organizer, we need to check before calling MarkPageAsFollowedBookmark() working on a patch...

for history, we should just not call markPageAsFollowedBookmark(). I think we might consider calling markPageAsTyped. If we don't call anything, it will act as link. I think choosing it from history sidebar or in the places organizer (or the History menu, which needs to be fixed) should recorded the visit the same way as when you type a url in the url bar or if you select a url in the autocomplete drop down (which is markPageAsTyped.) I think of "link" as a link in a page. Dietrich, do you agree? When I fix this, I'll also add a unit test and fix this unit test: http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/tests/unit/test_browserhistory.js#128 122 /** 123 * markPageAsTyped 124 * Designate the url as having been explicitly typed in by 125 * the user, so it's okay to be an autocomplete result. 126 */ 127 //XXX how to test this? 128 //bhist.markPageAsTyped(testURI);

here are the scenarios to test: should be TRANSITION_TYPED from history menu from the url bar from url autocomplete results from from history sidebar, single and host container, open in tabs (for date container, see bug #409661) from places organizer, history (single and select multiple open in tabs) should be TRANSITION_BOOKMARK from bm menu (single item and folder, open in tabs) from bm toolbar (single item and folder, open in tabs) from bm sidebar (single item and folder, open in tabs) from places organizer, single, select multiple open in tabs, container open in tabs

I've passed the test cases in comment #11, but I cheated by using a printf in nsNavHistory::AddVisitChain(). Someone without that could still verify by doing those tasks and then inspecting places.sqlite. The unit test covers some of the basics, but we could do more with browser chrome tests + some sql queries (or doing RESULTS_AS_FULL_VISITS queries, once they are supported, see bug #409662)

Comment on attachment 294447 [details] [diff] [review] updated patch (improve comments) seeking review from dietrich

The tests haven't been checked-in yet, right? Please only grant in-testsuite once the tests have landed.

Comment on attachment 294447 [details] [diff] [review] updated patch (improve comments) >+ markPageAsTyped: function PU_markPageAsTyped(aURL) { >+ try { >+ this.globalHistory.markPageAsTyped(this.createFixedURI(aURL)); >+ } >+ catch (ex) { >+ } what do you expect to fail here? should at least dump so the failure is visible somehow? >+ }, >+ >+ /** >+ * By calling this before we visit a URL, we will use TRANSITION_BOOKMARK >+ * as the transition for the visit to that URL (if we don't have a referrer). >+ * This is used when visiting pages from the bookmarks menu, >+ * personal toolbar, and bookmarks from within the places organizer. >+ * If we don't call this, we'll treat those visits as TRANSITION_LINK. >+ */ >+ markPageAsFollowedBookmark: function PU_markPageAsFollowedBookmark(aURL) { >+ try { >+ this.history.markPageAsFollowedBookmark(this.createFixedURI(aURL)); >+ } >+ catch (ex) { >+ } >+ }, ditto >@@ -1640,26 +1695,32 @@ var PlacesUtils = { > browserWindow.getBrowser().loadTabs(aURLs, loadInBackground, > replaceCurrentTab); > }, > > openContainerNodeInTabs: function PU_openContainerInTabs(aNode, aEvent) { > var urlsToOpen = this.getURLsForContainerNode(aNode); > if (!this._confirmOpenInTabs(urlsToOpen.length)) > return; >- this._openTabset(urlsToOpen, aEvent); >+ this._openTabset(urlsToOpen, aEvent, >+ !this.nodeIsHost(aNode) && !this.nodeIsDay(aNode)); couldn't you check node.itemId? (non-static containers would have -1), or maybe add a nodeIsBookmarkFolder() which checks itemId != -1 and nodeIsFolder(). > }, > > openURINodesInTabs: function PU_openURINodesInTabs(aNodes, aEvent) { > var urlsToOpen = []; >+ var nodesAreBookmarks = false; > for (var i=0; i < aNodes.length; i++) { >- if (this.nodeIsURI(aNodes[i])) >+ // skip over separators and folders >+ if (this.nodeIsURI(aNodes[i])) { > urlsToOpen.push(aNodes[i].uri); >+ if (!nodesAreBookmarks) >+ nodesAreBookmarks = this.nodeIsBookmark(aNodes[i]); >+ } > } >- this._openTabset(urlsToOpen, aEvent); >+ this._openTabset(urlsToOpen, aEvent, nodesAreBookmarks); is it possible that we'll get a mixed set where some nodes are bookmarks and other not? if so, this seems wrong, since later in openTabset all will get marked as followed-bookmarks. it might be better to either check each URI in openTabset, or always do the marking (as bookmark or typed) outside of it where we're better able to make the determination.

1) > what do you expect to fail here? should at least dump so the failure is visible > somehow? I'm not expecting this to fail, and both nsNavHistory::MarkPageAsTyped() and nsNavHistory::MarkPageAsFollowedBookmark() always returns NS_OK, so I'll remove the try / catch from both. I was being (overly) defensive, as we would not want markPageAsTyped() to throw. 2) > > openContainerNodeInTabs: function PU_openContainerInTabs(aNode, aEvent) { > > var urlsToOpen = this.getURLsForContainerNode(aNode); > > if (!this._confirmOpenInTabs(urlsToOpen.length)) > > return; > >- this._openTabset(urlsToOpen, aEvent); > >+ this._openTabset(urlsToOpen, aEvent, > >+ !this.nodeIsHost(aNode) && !this.nodeIsDay(aNode)); > couldn't you check node.itemId? (non-static containers would have -1), or maybe > add a nodeIsBookmarkFolder() which checks itemId != -1 and nodeIsFolder(). Actually, I can't do what I'm doing, or what you suggest, because for queries I would have itemId != -1, but nodeIsFolder() would be false. I think for both this review comment (and the next), I need to determine for each uri if it is a bookmark or not inside openTabset() Note, for some queries the results are history items and for others they are bookmarks. 3) > >+ this._openTabset(urlsToOpen, aEvent, nodesAreBookmarks); > is it possible that we'll get a mixed set where some nodes are bookmarks and > other not? It isn't currently possible, but if we have a unified query in the organizer, it could be. I'll fix this along with #2 above and attach a new patch.

Comment on attachment 294686 [details] [diff] [review] updated patch, per dietrich's review comments r=me, thanks for revising.

Comment on attachment 294686 [details] [diff] [review] updated patch, per dietrich's review comments seeking approval. we want this as it is part of the global frecency work (a p1 / firefox 3 + blocker). we will be using the transition state (bookmark, typed, link) of visits to weight them.

fixed. note, because this patch includes changes to make it so all history visits get TRANSITION_TYPED, I updated the comment in nsINavHistoryService.idl before checking in: /** * This transition type means that the user typed the page's URL in the - * URL bar. + * URL bar or selected it from URL bar autocomplete results, clicked on + * it from a history query (from the History sidebar, History menu, + * or history query in the personal toolbar or Places organizer. */ const unsigned long TRANSITION_TYPED = 2; Checking in browser/base/content/browser-menubar.inc; /cvsroot/mozilla/browser/base/content/browser-menubar.inc,v <-- browser-menubar.inc new revision: 1.124; previous revision: 1.123 done Checking in browser/base/content/browser.js; /cvsroot/mozilla/browser/base/content/browser.js,v <-- browser.js new revision: 1.920; previous revision: 1.919 done Checking in browser/components/places/content/controller.js; /cvsroot/mozilla/browser/components/places/content/controller.js,v <-- controller.js new revision: 1.191; previous revision: 1.190 done Checking in browser/components/places/content/utils.js; /cvsroot/mozilla/browser/components/places/content/utils.js,v <-- utils.js new revision: 1.91; previous revision: 1.90 done Checking in toolkit/components/places/public/nsINavHistoryService.idl; /cvsroot/mozilla/toolkit/components/places/public/nsINavHistoryService.idl,v <-- nsINavHistoryService.idl new revision: 1.75; previous revision: 1.74 done Checking in toolkit/components/places/src/nsNavHistory.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v <-- nsNavHistory.cpp new revision: 1.221; previous revision: 1.220 done Checking in toolkit/components/places/tests/unit/test_browserhistory.js; /cvsroot/mozilla/toolkit/components/places/tests/unit/test_browserhistory.js,v <-- test_browserhistory.js new revision: 1.2; previous revision: 1.1 done RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_markpageas.js,v done Checking in toolkit/components/places/tests/unit/test_markpageas.js; /cvsroot/mozilla/toolkit/components/places/tests/unit/test_markpageas.js,v <-- test_markpageas.js initial revision: 1.1 done Checking in toolkit/components/history/public/nsIBrowserHistory.idl; /cvsroot/mozilla/toolkit/components/history/public/nsIBrowserHistory.idl,v <-- nsIBrowserHistory.idl new revision: 1.11; previous revision: 1.10 done

??? Uh, this bug doesn't have approval nor is it a blocker. Why did you check-in the patch?

> Uh, this bug doesn't have approval nor is it a blocker. Why did you check-in > the patch? it is part of and blocks the firefox-3-blocker+ / p1 bug for global frecency.

(In reply to comment #22) > > Uh, this bug doesn't have approval nor is it a blocker. Why did you check-in > > the patch? > > it is part of and blocks the firefox-3-blocker+ / p1 bug for global frecency. That has never been a valid reason to check-in during an approval period. http://wiki.mozilla.org/TreeStatus clearly states that the bug itself must be a blocker or else the patch must have approval1.9+, of which neither of those things is true for this bug/patch. I'm not sure why you requested approval in comment #19 and then turned right around and checked it in anyway... That doesn't make sense.

Comment on attachment 294686 [details] [diff] [review] updated patch, per dietrich's review comments ho ho ho, it's a winter vacation approval.

baked in-testsuite long enough

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