fix for bug #409301: Visiting bookmark is recorded as link transition instead of bookmark transition additionally, make it so visits from any history UI (url bar, history menu, history queries in the history sidebar, personal toolbar folder and places organizer) are recorded as "typed" transitions. r=dietrich a=this-bug-blocks-a-firefox-3-+ blocker

browser/base/content/browser-menubar.inc

@@ -324,7 +324,7 @@
             </menu>
 
             <menu id="history-menu"
-                  oncommand="var url = event.target.getAttribute('statustext'); if (url) openUILink(url, event, false, true);"
+                  oncommand="var url = event.target.getAttribute('statustext'); if (url) { PlacesUtils.markPageAsTyped(url); openUILink(url, event, false, true); }"
                   onclick="checkForMiddleClick(this, event);"
                   label="&historyMenu.label;"
                   accesskey="&historyMenu.accesskey;">

browser/base/content/browser.js

@@ -77,7 +77,6 @@ const TYPE_XUL = "application/vnd.mozilla.xul+xml";
 // We use this once, for Clear Private Data
 const GLUE_CID = "@mozilla.org/browser/browserglue;1";
 
-var gGlobalHistory = null;
 var gURIFixup = null;
 var gCharsetMenu = null;
 var gLastBrowserCharset = null;
@@ -2885,17 +2884,9 @@ function addToUrlbarHistory(aUrlToAdd)
   if (aUrlToAdd.search(/[\x00-\x1F]/) != -1) // don't store bad URLs
      return;
 
-  if (!gGlobalHistory)
-    gGlobalHistory = Components.classes["@mozilla.org/browser/global-history;2"]
-                               .getService(Components.interfaces.nsIBrowserHistory);
-
-  if (!gURIFixup)
-    gURIFixup = Components.classes["@mozilla.org/docshell/urifixup;1"]
-                          .getService(Components.interfaces.nsIURIFixup);
    try {
      if (aUrlToAdd.indexOf(" ") == -1) {
-       var fixedUpURI = gURIFixup.createFixupURI(aUrlToAdd, 0);
-       gGlobalHistory.markPageAsTyped(fixedUpURI);
+       PlacesUtils.markPageAsTyped(aUrlToAdd);
      }
    }
    catch(ex) {

browser/components/places/content/controller.js

@@ -638,9 +638,16 @@ PlacesController.prototype = {
   openSelectedNodeIn: function PC_openSelectedNodeIn(aWhere) {
     var node = this._view.selectedURINode;
     if (node && PlacesUtils.checkURLSecurity(node)) {
-       // Check whether the node is a bookmark which should be opened as
-       // a web panel
-      if (aWhere == "current" && PlacesUtils.nodeIsBookmark(node)) {
+      var isBookmark = PlacesUtils.nodeIsBookmark(node);
+
+      if (isBookmark)
+        PlacesUtils.markPageAsFollowedBookmark(node.uri);
+      else
+        PlacesUtils.markPageAsTyped(node.uri);
+
+      // Check whether the node is a bookmark which should be opened as
+      // a web panel
+      if (aWhere == "current" && isBookmark) {
         if (PlacesUtils.annotations
                        .itemHasAnnotation(node.itemId, LOAD_IN_SIDEBAR_ANNO)) {
           var w = getTopWin();

browser/components/places/content/utils.js

@@ -111,6 +111,12 @@ var PlacesUtils = {
                           getService(Ci.nsINavHistoryService);
   },
 
+  get globalHistory() {
+    delete this.globalHistory;
+    return this.globalHistory = Cc["@mozilla.org/browser/global-history;2"].
+                                getService(Ci.nsIBrowserHistory);
+  },
+
   /**
    * The Live Bookmark Service.
    */
@@ -1241,6 +1247,29 @@ var PlacesUtils = {
     return null;
   },
 
+  /**
+   * By calling this before we visit a URL, we will use TRANSITION_TYPED
+   * as the transition for the visit to that URL (if we don't have a referrer).
+   * This is used when visiting pages from the history menu, history sidebar,
+   * url bar, url autocomplete results, and history searches from the places
+   * organizer.  If we don't call this, we'll treat those visits as
+   * TRANSITION_LINK.
+   */
+  markPageAsTyped: function PU_markPageAsTyped(aURL) {
+    this.globalHistory.markPageAsTyped(this.createFixedURI(aURL));
+  },
+
+  /**
+   * 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) {
+    this.history.markPageAsFollowedBookmark(this.createFixedURI(aURL));
+  },
+
   /**
    * Allows opening of javascript/data URI only if the given node is
    * bookmarked (see bug 224521).
@@ -1552,7 +1581,7 @@ var PlacesUtils = {
       for (let i = 0; i < contents.childCount; ++i) {
         let child = contents.getChild(i);
         if (this.nodeIsURI(child))
-          urls.push(child.uri);
+          urls.push({uri: child.uri, isBookmark: this.nodeIsBookmark(child)});
       }
     }
     else {
@@ -1568,7 +1597,7 @@ var PlacesUtils = {
         for (let i = 0; i < aNode.childCount; ++i) {
           let child = aNode.getChild(i);
           if (this.nodeIsURI(child))
-            urls.push(child.uri);
+            urls.push({uri: child.uri, isBookmark: this.nodeIsBookmark(child)});
         }
         if (!wasOpen)
           aNode.containerOpen = false;
@@ -1625,19 +1654,32 @@ var PlacesUtils = {
     return reallyOpen;
   },
 
-  _openTabset: function PU__openTabset(aURLs, aEvent) {
+  /** aItemsToOpen needs to be an array of objects of the form:
+    * {uri: string, isBookmark: boolean}
+    */
+  _openTabset: function PU__openTabset(aItemsToOpen, aEvent) {
+    var urls = [];
+    for each (var item in aItemsToOpen) {
+      if (item.isBookmark)
+        this.markPageAsFollowedBookmark(item.uri);
+      else
+        this.markPageAsTyped(item.uri);
+
+      urls.push(item.uri);
+    }
+
     var browserWindow = getTopWin();
     var where = browserWindow ?
                 whereToOpenLink(aEvent, false, true) : "window";
     if (where == "window") {
       window.openDialog(getBrowserURL(), "_blank",
-                        "chrome,all,dialog=no", aURLs.join("|"));
+                        "chrome,all,dialog=no", urls.join("|"));
       return;
     }
 
     var loadInBackground = where == "tabshifted" ? true : false;
     var replaceCurrentTab = where == "tab" ? false : true;
-    browserWindow.getBrowser().loadTabs(aURLs, loadInBackground,
+    browserWindow.getBrowser().loadTabs(urls, loadInBackground,
                                         replaceCurrentTab);
   },
 
@@ -1645,14 +1687,16 @@ var PlacesUtils = {
     var urlsToOpen = this.getURLsForContainerNode(aNode);
     if (!this._confirmOpenInTabs(urlsToOpen.length))
       return;
+
     this._openTabset(urlsToOpen, aEvent);
   },
 
   openURINodesInTabs: function PU_openURINodesInTabs(aNodes, aEvent) {
     var urlsToOpen = [];
     for (var i=0; i < aNodes.length; i++) {
+      // skip over separators and folders
       if (this.nodeIsURI(aNodes[i]))
-        urlsToOpen.push(aNodes[i].uri);
+        urlsToOpen.push({uri: aNodes[i].uri, isBookmark: this.nodeIsBookmark(aNodes[i])});
     }
     this._openTabset(urlsToOpen, aEvent);
   },

toolkit/components/history/public/nsIBrowserHistory.idl

@@ -94,9 +94,15 @@ interface nsIBrowserHistory : nsIGlobalHistory2
     void hidePage(in nsIURI aURI);
 
     /**
-     * markPageAsTyped
-     * Designate the url as having been explicitly typed in by
-     * the user, so it's okay to be an autocomplete result.
+     * This is just like markPageAsFollowedBookmark (in nsINavHistory, 
+     * also implemented by the history service), but for URLs that a
+     * user visits from the chrome that are not bookmarks, such as a
+     * URL that is typed in the URL bar or clicking on a link in the 
+     * history menu or history sidebar.   It declares that the given URI
+     * is treated as if they typed the URL into the URL bar (which
+     * get more weight in our URL bar autocomplete algorithm.)
+     * If this URI is loaded soon after this message has been received, 
+     * that transition will be marked as typed.
      */
     void markPageAsTyped(in nsIURI aURI);
 };

toolkit/components/places/public/nsINavHistoryService.idl

@@ -1171,7 +1171,9 @@ interface nsINavHistoryService : nsISupports
 
   /**
    * 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;
 

toolkit/components/places/src/nsNavHistory.cpp

@@ -1760,7 +1760,13 @@ nsNavHistory::GetHasHistoryEntries(PRBool* aHasEntries)
 
 // nsNavHistory::MarkPageAsFollowedBookmark
 //
-//    @see MarkPageAsTyped
+// We call MarkPageAsFollowedBookmark() before visiting a URL in order to 
+// help determine the transition type of the visit.  
+// We keep track of the URL so that later, in AddVisitChain() 
+// we can use TRANSITION_BOOKMARK as the transition.
+// Note, AddVisitChain() is not called immediately when we are doing LAZY_ADDs
+//
+// @see MarkPageAsTyped
 
 NS_IMETHODIMP
 nsNavHistory::MarkPageAsFollowedBookmark(nsIURI* aURI)
@@ -1780,7 +1786,7 @@ nsNavHistory::MarkPageAsFollowedBookmark(nsIURI* aURI)
   if (mRecentBookmark.Count() > RECENT_EVENT_QUEUE_MAX_LENGTH)
     ExpireNonrecentEvents(&mRecentBookmark);
 
-  mRecentTyped.Put(uriString, GetNow());
+  mRecentBookmark.Put(uriString, GetNow());
   return NS_OK;
 }
 
@@ -3030,17 +3036,13 @@ nsNavHistory::HidePage(nsIURI *aURI)
 
 // nsNavHistory::MarkPageAsTyped
 //
-//    Just sets the typed column to true, which will make this page more likely
-//    to float to the top of autocomplete suggestions.
+// We call MarkPageAsTyped() before visiting a URL in order to 
+// help determine the transition type of the visit.  
+// We keep track of the URL so that later, in AddVisitChain() 
+// we can use TRANSITION_TYPED as the transition.
+// Note, AddVisitChain() is not called immediately when we are doing LAZY_ADDs
 //
-//    We can get this notification for pages that have not yet been added to the
-//    DB. This happens when you type a new URL. The AddURI is called only when
-//    the page is successfully found. If we don't have an entry yet, we add
-//    one for this page, marking it as typed but hidden, with a 0 visit count.
-//    This will get updated when AddURI is called, and it will clear the hidden
-//    flag for typed URLs.
-//
-//    @see MarkPageAsFollowedBookmark
+// @see MarkPageAsFollowedBookmark
 
 NS_IMETHODIMP
 nsNavHistory::MarkPageAsTyped(nsIURI *aURI)

toolkit/components/places/tests/unit/test_browserhistory.js

@@ -118,12 +118,4 @@ function run_test() {
   //bhist.addPageWithDetails(testURI, "testURI", Date.now());
   //bhist.hidePage(testURI);
   //do_check_eq(0, bhist.count);
-
-  /**
-   * markPageAsTyped
-   * Designate the url as having been explicitly typed in by
-   * the user, so it's okay to be an autocomplete result.
-   */
-  //XXX how to test this?
-  //bhist.markPageAsTyped(testURI);
 }

toolkit/components/places/tests/unit/test_markpageas.js

@@ -0,0 +1,121 @@
+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
+ *
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is mozilla.org code.
+ *
+ * The Initial Developer of the Original Code is Google Inc.
+ * Portions created by the Initial Developer are Copyright (C) 2005
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *  Darin Fisher <darin@meer.net>
+ *  Dietrich Ayala <dietrich@mozilla.com>
+ *  Dan Mills <thunder@mozilla.com>
+ *  Seth Spitzer <sspitzer@mozilla.org>
+ *
+ * Alternatively, the contents of this file may be used under the terms of
+ * either the GNU General Public License Version 2 or later (the "GPL"), or
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+// Get global history service
+try {
+  var gh = Cc["@mozilla.org/browser/global-history;2"].getService(Ci.nsIBrowserHistory);
+} catch(ex) {
+  do_throw("Could not get global history service\n");
+} 
+
+// Get history service
+try {
+  var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].getService(Ci.nsINavHistoryService);
+} catch(ex) {
+  do_throw("Could not get history service\n");
+} 
+
+function add_uri_to_history(aURI) {
+  gh.addURI(aURI,
+            false, // not redirect
+            true, // top level
+            null); // no referrer, so that we'll use the markPageAs hint
+}
+
+var gVisits = [{url: "http://www.mozilla.com/",
+                transition: histsvc.TRANSITION_TYPED},
+               {url: "http://www.google.com/", 
+                transition: histsvc.TRANSITION_BOOKMARK},
+               {url: "http://www.espn.com/",
+                transition: histsvc.TRANSITION_LINK}];
+
+// main
+function run_test() {
+  for each (var visit in gVisits) {
+    if (visit.transition == histsvc.TRANSITION_TYPED)
+      gh.markPageAsTyped(uri(visit.url));
+    else if (visit.transition == histsvc.TRANSITION_BOOKMARK)
+      gh.markPageAsFollowedBookmark(uri(visit.url))
+    else {
+     // because it is a top level visit with no referrer,
+     // it will result in TRANSITION_LINK
+    }
+    add_uri_to_history(uri(visit.url));
+  }
+
+  do_test_pending();
+}
+
+// create and add history observer
+var observer = {
+  _visitCount: 0,
+  onBeginUpdateBatch: function() {
+  },
+  onEndUpdateBatch: function() {
+  },
+  onVisit: function(aURI, aVisitID, aTime, aSessionID, aReferringID, aTransitionType) {
+    do_check_eq(aURI.spec, gVisits[this._visitCount].url);
+    do_check_eq(aTransitionType, gVisits[this._visitCount].transition);
+    this._visitCount++;
+
+    if (this._visitCount == gVisits.length)
+      do_test_finished();
+  },
+  onTitleChanged: function(aURI, aPageTitle) {
+  },
+  onDeleteURI: function(aURI) {
+  },
+  onClearHistory: function() {
+  },
+  onPageChanged: function(aURI, aWhat, aValue) {
+  },
+  onPageExpired: function(aURI, aVisitTime, aWholeEntry) {
+  },
+  QueryInterface: function(iid) {
+    if (iid.equals(Ci.nsINavBookmarkObserver) ||
+        iid.equals(Ci.nsISupports)) {
+      return this;
+    }
+    throw Cr.NS_ERROR_NO_INTERFACE;
+  }
+};
+
+histsvc.addObserver(observer, false);