Bug 313715: "Open in New Tab" from Bookmark Manager doesn't respect background pref

Stop caching the result of shouldLoadTabInBackground(), there's no point since we need to look at the state of the shift key every time, and we didn't update it in all code paths to begin with.
Pass in |event| from more places in history and bookmarks so we'll actually look at te shift key.
Replace use of "tab_background" with looking at shouldLoadTabInBackground().
r=IanN, sr=Neil

xpfe/components/bookmarks/resources/bookmarks.js

@@ -72,8 +72,6 @@ var kDSContractID;
 var kDSIID;
 var DS;
 
-var gLoadInBackground = false;
-
 // should be moved in a separate file
 function initServices()
 {
@@ -524,16 +522,16 @@ var BookmarksCommand = {
     BookmarksUtils.moveSelection("move", aSelection, target);
   },
 
-  openBookmark: function (aSelection, aTargetBrowser, aDS) 
+  openBookmark: function (aSelection, aTargetBrowser, aDS, aEvent)
   {
     if (!aTargetBrowser)
       return;
     for (var i=0; i<aSelection.length; ++i) {
       var type = aSelection.type[i];
       if (type == "Bookmark" || type == "")
-        this.openOneBookmark(aSelection.item[i].Value, aTargetBrowser, aDS);
+        this.openOneBookmark(aSelection.item[i].Value, aTargetBrowser, aDS, aEvent);
       else if (type == "FolderGroup" || type == "Folder" || type == "PersonalToolbarFolder")
-        this.openGroupBookmark(aSelection.item[i].Value, aTargetBrowser);
+        this.openGroupBookmark(aSelection.item[i].Value, aTargetBrowser, aEvent);
     }
   },
   
@@ -546,18 +544,23 @@ var BookmarksCommand = {
   },
 
   // requires utilityOverlay.js if opening in new window for getTopWin()
-  openOneBookmark: function (aURI, aTargetBrowser, aDS)
+  openOneBookmark: function (aURI, aTargetBrowser, aDS, aEvent)
   {
-    var url = BookmarksUtils.getProperty(aURI, NC_NS+"URL", aDS);
+    var w = getTopWin();
+    if (!w) // no browser window open, so we have to open in new window
+      aTargetBrowser = "window";
+
+    var url = BookmarksUtils.getProperty(aURI, NC_NS + "URL", aDS);
     // Ignore "NC:" and empty urls.
     if (url == "")
       return;
-    var w = aTargetBrowser == "window"? null:getTopWin();
-    if (!w) {
+
+    if (aTargetBrowser == "window") {
       openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no", url);
       return;
     }
-    var browser = w.document.getElementById("content");
+
+    var browser = w.getBrowser();
     switch (aTargetBrowser) {
     case "current":
       browser.loadURI(url);
@@ -565,23 +568,24 @@ var BookmarksCommand = {
       break;
     case "tab":
       var tab = browser.addTab(url);
-      if (!gLoadInBackground)
+      if (!BookmarksUtils.shouldLoadTabInBackground(aEvent))
         browser.selectedTab = tab;
       break;
     }
   },
 
-  openGroupBookmark: function (aURI, aTargetBrowser)
+  openGroupBookmark: function (aURI, aTargetBrowser, aEvent)
   {
     var w = getTopWin();
     if (!w) // no browser window open, so we have to open in new window
-      aTargetBrowser="window";
+      aTargetBrowser = "window";
 
     var resource = RDF.GetResource(aURI);
-    var urlArc   = RDF.GetResource(NC_NS+"URL");
     RDFC.Init(BMDS, resource);
     var containerChildren = RDFC.GetElements();
+
     var URIs = [];
+    var urlArc = RDF.GetResource(NC_NS + "URL");
     while (containerChildren.hasMoreElements()) {
       var res = containerChildren.getNext().QueryInterface(kRDFRSCIID);
       var target = BMDS.GetTarget(res, urlArc, true);
@@ -592,15 +596,20 @@ var BookmarksCommand = {
           URIs.push({ URI: target.QueryInterface(kRDFLITIID).Value });        
       }
     }
+
+    if (URIs.length == 0)
+      return;
+
     if (aTargetBrowser == "window") {
       // This opens the URIs in separate tabs of a new window
       openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no", URIs.join("\n"));
-    } else {
-      var browser = w.getBrowser();
-      var tab = browser.loadGroup(URIs);
-      if (!gLoadInBackground)
-        browser.selectedTab = tab;
+      return;
     }
+
+    var browser = w.getBrowser();
+    var tab = browser.loadGroup(URIs);
+    if (!BookmarksUtils.shouldLoadTabInBackground(aEvent))
+      browser.selectedTab = tab;
   },
 
   findBookmark: function ()
@@ -1581,7 +1590,7 @@ var BookmarksUtils = {
   shouldLoadTabInBackground: function(aEvent)
   {
     var loadInBackground = PREF.getBoolPref("browser.tabs.loadInBackground");
-    if (aEvent.shiftKey)
+    if (aEvent && aEvent.shiftKey)
       loadInBackground = !loadInBackground;
     return loadInBackground;
   },
@@ -1591,8 +1600,6 @@ var BookmarksUtils = {
     if (!aEvent)
       return null;
 
-    gLoadInBackground = this.shouldLoadTabInBackground(aEvent);
-
     switch (aEvent.type) {
     case "click":
     case "dblclick":
@@ -1633,7 +1640,7 @@ var BookmarksUtils = {
 
     var rSource   = RDF.GetResource(aEvent.target.id);
     var selection = BookmarksUtils.getSelectionFromResource(rSource);
-    BookmarksCommand.openBookmark(selection, target, aDS)
+    BookmarksCommand.openBookmark(selection, target, aDS, aEvent)
   }
 }
 

xpfe/components/bookmarks/resources/bookmarksTree.xml

@@ -428,7 +428,7 @@
 
           var target = BookmarksUtils.getBrowserTargetFromEvent(aEvent);
           if (target)
-            BookmarksCommand.openBookmark(selection, target, this.db);
+            BookmarksCommand.openBookmark(selection, target, this.db, aEvent);
         ]]></body>
       </method>
 
@@ -440,7 +440,7 @@
           if (!this._selection.isContainer[0]) {
             var target = BookmarksUtils.getBrowserTargetFromEvent(aEvent);
             if (target)
-              BookmarksCommand.openBookmark(this._selection, target, this.db)
+              BookmarksCommand.openBookmark(this._selection, target, this.db, aEvent)
           }
         ]]></body>
       </method>

xpfe/components/history/resources/history.js

@@ -150,7 +150,7 @@ function historyOnClick(aEvent)
   gHistoryTree.treeBoxObject.getCellAt(aEvent.clientX, aEvent.clientY, row, col, elt);
   if (row.value >= 0 && col.value) {
     if (!isContainer(gHistoryTree, row.value))
-      OpenURL(target);
+      OpenURL(target, aEvent);
     else if (aEvent.button == 0 && elt.value != "twisty")
       gHistoryTree.treeBoxObject.view.toggleOpenState(row.value);
   }
@@ -288,24 +288,29 @@ function collapseExpand()
     gHistoryTree.treeBoxObject.view.toggleOpenState(currentIndex);
 }
 
-function OpenURL(aTarget)
+function OpenURL(aTarget, aEvent)
 {
     var currentIndex = gHistoryTree.currentIndex;     
     var builder = gHistoryTree.builder.QueryInterface(Components.interfaces.nsIXULTreeBuilder);
     var url = builder.getResourceAtIndex(currentIndex).ValueUTF8;
-    var uri = Components.classes["@mozilla.org/network/standard-url;1"].
-                createInstance(Components.interfaces.nsIURI);
-    uri.spec = url;
-    if (uri.schemeIs("javascript") || uri.schemeIs("data")) {
-      var strBundleService = Components.classes["@mozilla.org/intl/stringbundle;1"]
-                                       .getService(Components.interfaces.nsIStringBundleService);
-      var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
-                                    .getService(Components.interfaces.nsIPromptService);
-      var historyBundle = strBundleService.createBundle("chrome://communicator/locale/history/history.properties");
-      var brandBundle = strBundleService.createBundle("chrome://branding/locale/brand.properties");      
-      var brandStr = brandBundle.GetStringFromName("brandShortName");
-      var errorStr = historyBundle.GetStringFromName("load-js-data-url-error");
-      promptService.alert(window, brandStr, errorStr);
+    if (!gIOService)
+      gIOService = Components.classes['@mozilla.org/network/io-service;1']
+                             .getService(Components.interfaces.nsIIOService);
+    try {
+      var scheme = gIOService.extractScheme(url);
+      if (scheme == "javascript" || scheme == "data") {
+        var strBundleService = Components.classes["@mozilla.org/intl/stringbundle;1"]
+                                         .getService(Components.interfaces.nsIStringBundleService);
+        var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
+                                      .getService(Components.interfaces.nsIPromptService);
+        var historyBundle = strBundleService.createBundle("chrome://communicator/locale/history/history.properties");
+        var brandBundle = strBundleService.createBundle("chrome://branding/locale/brand.properties");      
+        var brandStr = brandBundle.GetStringFromName("brandShortName");
+        var errorStr = historyBundle.GetStringFromName("load-js-data-url-error");
+        promptService.alert(window, brandStr, errorStr);
+        return false;
+      }
+    } catch (e) {
       return false;
     }
 
@@ -338,7 +343,7 @@ function OpenURL(aTarget)
         }
       } else {
         if (URLArray.length > 0)
-          OpenURLArrayInTabs(URLArray, aTarget == "tab_background");
+          OpenURLArrayInTabs(URLArray, BookmarksUtils.shouldLoadTabInBackground(aEvent));
       }
     }
     else if (!isContainer(gHistoryTree, currentIndex))

xpfe/components/history/resources/historyTreeOverlay.xul

@@ -76,7 +76,7 @@
       <menuitem id="miOpenInNewWindow" label="&openLinkCmd.label;" accesskey="&openLinkCmd.accesskey;"
                 oncommand="OpenURL('window');"/>
       <menuitem id="miOpenInNewTab" label="&openLinkCmdInTab.label;" accesskey="&openLinkCmdInTab.accesskey;"
-                oncommand="OpenURL(BookmarksUtils.shouldLoadTabInBackground(event) ? 'tab_background' : 'tab');"/>
+                oncommand="OpenURL('tab', event);"/>
       <menuseparator id="pre-bookmarks-separator"/>
       <menuitem id="miAddBookmark" label="&bookmarkLinkCmd.label;" accesskey="&bookmarkLinkCmd.accesskey;" oncommand="historyAddBookmarks();"/>
       <menuitem id="miCopyLinkLocation" label="&copyLinkCmd.label;" accesskey="&copyLinkCmd.accesskey;"
@@ -93,12 +93,12 @@
   <tree id="historyTree" flex="1" enableColumnDrag="true" class="plain"
             context="historyMenu"
             datasources="rdf:history" ref="NC:HistoryByDate" flags="dont-build-content"
-            onkeypress="if (event.keyCode == 13) OpenURL(BookmarksUtils.getBrowserTargetFromEvent(event));"
+            onkeypress="if (event.keyCode == 13) OpenURL(BookmarksUtils.getBrowserTargetFromEvent(event), event);"
             onselect="this.view.selectionChanged();
                       historyOnSelect();"
             onclick="historyOnClick(event);"
             ondraggesture="if (event.originalTarget.localName == 'treechildren') nsDragAndDrop.startDrag(event, historyDNDObserver);"
-            ondblclick="if (validClickConditions(event)) OpenURL(BookmarksUtils.getBrowserTargetFromEvent(event));">
+            ondblclick="if (validClickConditions(event)) OpenURL(BookmarksUtils.getBrowserTargetFromEvent(event), event);">
     <template>
       <rule>
         <treechildren>