529240 - Make drop down menus for back and forward buttons middle/Ctrl-clickable

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, enhancement)

Description

[Matt A. Tobin]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.4) Gecko/20091114 Lightning/1.0pre SeaMonkey/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.4) Gecko/20091114 Lightning/1.0pre SeaMonkey/2.0

Firefox supports middle-clicking web pages in the back and forward buttons drop down lists to open that site in a new tab, Seamonkey does not.

Reproducible: Always

Steps to Reproduce:
1. Go to 2 or more web pages to generate a list in the back or forward button's history drop down list.
2. Right Click on the back or forward button or use the drop down arrow.
3. Middle-click one of the pages.
Actual Results:  
Nothing happens

Expected Results:  
Selected Page should open in a new tab

Comment 1

[Karsten Düsterloh]

Actually, it should probably adhere to the "Tabbed Browsing: Open Tabs instead of Windows for" preference settings.

Comment 2

[Matt A. Tobin]

Possibly however a few users including the irc user PP asked if its possible to enable and if not asked if it could be added.

However I believe that it should operate in the same manor as firefox. ie Clicking opens in same tab and middle-clicking opens in new tab.

Comment 3

[Matt A. Tobin]

I should note that this is a feature i use on a regular basis as well.

Comment 4

[Philip Chee]

Middle clicking works in Firefox because they have fixed:
Bug 246719 (Make link modifiers work consistently throughout Firefox)
We probably need the bits of:
Bug 302575 (URL bar gets confused about what URI is loaded)
that didn't get ported to SeaMonkey.

Comment 5

[Philip Chee]

Plan of action:

Step 1: Port the following inter-related bugs:

  [Bug 246719] Make link modifiers work consistently throughout Firefox.
  [Bug 232172] Make link modifiers work on the home button.
  [Bug 263942] Reload button should have middle click support (open same URL in new tab, clone tab).
  [Bug 302575] URL bar gets confused about what URI is loaded.
    (Some parts were not ported since we didn't have middle-click functionality then)
  [Bug 440702] Link modifiers should use the command attribute instead of oncommand(and remove observes elements).
    (Only port the relevant parts)
  [Bug 539594] Middle-clicking back/forward/reload should open the new tab next to the current one.

  Some related one-liners can come along for the ride:

  Bug 291768 - Middle click on proxy icon in location bar should paste URL
   ("if middlemouse.paste isn't enabled, middle click shouldn't paste....")
  Bug 216899 - middle-clicking functions ContentLoadURL and autoscroll conflict.


Step 2: Possibly file followup bugs/patches to port the following:

  [Bug 538536] allow middle-clicks on some additional Help menu items
  [Bug 538536] Middle Clicking on "Firefox Help" and "For Internet Users" does nothing.
  [Bug 565772] Accept middle-click for new tab button.

I have a WIP patch for step 1. Just needs testing.

Comment 7

[Philip Chee]

Attachment: Patch v1.0 Sprinkle MiddleLinkClick fairy dust over navigator.

This patch ports the following inter-related bugs:

  [Bug 246719] Make link modifiers work consistently throughout Firefox.
  [Bug 232172] Make link modifiers work on the home button.
  [Bug 263942] Reload button should have middle click support (open same URL in new tab, clone tab).
  [Bug 302575] URL bar gets confused about what URI is loaded.
    (Some parts were not ported since we didn't have middle-click functionality then)
  [Bug 440702] Link modifiers should use the command attribute instead of oncommand(and remove observes elements).
    (Only port the relevant parts)
  [Bug 539594] Middle-clicking back/forward/reload should open the new tab next to the current one.

  Some related one-liners coming along for the ride:

  Bug 291768 - Middle click on proxy icon in location bar should paste URL
   ("if middlemouse.paste isn't enabled, middle click shouldn't paste....")
  Bug 216899 - middle-clicking functions ContentLoadURL and autoscroll conflict.

Comment 8

[Philip Chee]

Comment on attachment 460490 [details] [diff] [review]
Patch v1.0 Sprinkle MiddleLinkClick fairy dust over navigator.

># HG changeset patch
># User Philip Chee <[e-mail]>
># Date 1280227276 -28800
># Node ID 1c215e088b5477950c39b47db797704530064e25
># Parent  9f4d6e54923df1f7a1a9b4b833a031cae53b5d10
>Bug 529240 Make drop down menus for back and forward buttons middle/Ctrl-clickable.
>
>diff --git a/suite/browser/navigator.js b/suite/browser/navigator.js
>--- a/suite/browser/navigator.js
>+++ b/suite/browser/navigator.js
>@@ -782,59 +782,95 @@ function Translate()
> }
> 
> function gotoHistoryIndex(aEvent)
> {
>   var index = aEvent.target.getAttribute("index");
>   if (!index)
>     return false;
> 
>-  if (index == "back")
>-    gBrowser.goBackGroup();
>-  else if (index ==  "forward")
>-    gBrowser.goForwardGroup();
>+  var where = whereToOpenLink(aEvent);
>+
>+  if (where == "current") {
>+    // Normal click.  Go there in the current tab and update session history.
>+
>+    if (index == "back")
>+      gBrowser.goBackGroup();
>+    else if (index ==  "forward")
>+      gBrowser.goForwardGroup();
>+    else {
>+      try {
>+        getWebNavigation().gotoIndex(index);
>+      }
>+      catch(ex) {
>+        return false;
>+      }
>+    }
>+    return true;
>+  }
>   else {
>-    try {
>-      getWebNavigation().gotoIndex(index);
>-    }
>-    catch(ex) {
>-      return false;
>-    }
>+    // Modified click.  Go there in a new tab/window.
>+    // This code doesn't copy history or work well with framed pages.
>+
>+    var sessionHistory = getWebNavigation().sessionHistory;
>+    var entry = sessionHistory.getEntryAtIndex(index, false);
>+    var url = entry.URI.spec;
>+    openUILinkIn(url, where, {relatedToCurrent: true});
>+    return true;
>   }
>-  return true;
>-
> }
> 
>-function BrowserBack()
>+function BrowserBack(aEvent)
> {
>-  try {
>-    getBrowser().goBack();
>+  var where = whereToOpenLink(aEvent, false, true);
>+
>+  if (where == "current") {
>+    try {
>+      getBrowser().goBack();
>+    }
>+    catch(ex) {}
>   }
>-  catch(ex) {
>+  else {
>+    var sessionHistory = getWebNavigation().sessionHistory;
>+    var currentIndex = sessionHistory.index;
>+    var entry = sessionHistory.getEntryAtIndex(currentIndex - 1, false);
>+    var url = entry.URI.spec;
>+    openUILinkIn(url, where, {relatedToCurrent: true});
>   }
> }
> 
> function BrowserHandleBackspace()
> {
>   switch (Services.prefs.getIntPref("browser.backspace_action")) {
>     case 0:
>       BrowserBack();
>       break;
>     case 1:
>       goDoCommand("cmd_scrollPageUp");
>       break;
>   }
> }
> 
>-function BrowserForward()
>+function BrowserForward(aEvent)
> {
>-  try {
>-    getBrowser().goForward();
>+  var where = whereToOpenLink(aEvent, false, true);
>+
>+  if (where == "current") {
>+    try {
>+      getBrowser().goForward();
>+    }
>+    catch(ex) {
>+    }
>   }
>-  catch(ex) {
>+  else {
>+    var sessionHistory = getWebNavigation().sessionHistory;
>+    var currentIndex = sessionHistory.index;
>+    var entry = sessionHistory.getEntryAtIndex(currentIndex + 1, false);
>+    var url = entry.URI.spec;
>+    openUILinkIn(url, where, {relatedToCurrent: true});
>   }
> }
> 
> function BrowserUp()
> {
>   loadURI(getBrowser().currentURI.spec.replace(/[#?].*$/, "").replace(/\/[^\/]*.$/, "/"));
> }
> 
>@@ -890,59 +926,53 @@ function BrowserStop()
>   }
>   catch(ex) {
>   }
> }
> 
> function BrowserReload()
> {
>   const reloadFlags = nsIWebNavigation.LOAD_FLAGS_NONE;
>-  return BrowserReloadWithFlags(reloadFlags);
>+  BrowserReloadWithFlags(reloadFlags);
> }
> 
> function BrowserReloadSkipCache()
> {
>   // Bypass proxy and cache.
>   const reloadFlags = nsIWebNavigation.LOAD_FLAGS_BYPASS_PROXY | nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE;
>-  return BrowserReloadWithFlags(reloadFlags);
>+  BrowserReloadWithFlags(reloadFlags);
>+}
>+
>+function BrowserReloadOrDuplicate(aEvent)
>+{
>+  var key = /Mac/.test(navigator.platform) ? aEvent.metaKey : aEvent.ctrlKey;
>+  if (aEvent.shiftKey && !(aEvent.button == 1 || key)) {
>+    BrowserReloadSkipCache();
>+    return;
>+  }
>+
>+  var where = whereToOpenLink(aEvent, false, true);
>+  if (where == "current")
>+    BrowserReload();
>+  else
>+    openUILinkIn(getWebNavigation().currentURI.spec, where,
>+                 {relatedToCurrent: true});
> }
> 
> function BrowserHome(aEvent)
> {
>   var tab;
>   var homePage = getHomePage();
>-  var target = !gBrowser ? "window": !aEvent ? "current" : BookmarksUtils.getBrowserTargetFromEvent(aEvent);
>+  //var where = whereToOpenLink(aEvent, false, true);
>+  var where = !gBrowser ? "window": !aEvent ? "current" : BookmarksUtils.getBrowserTargetFromEvent(aEvent);
> 
>-  if (homePage.length == 1) {
>-    switch (target) {
>-    case "current":
>-      loadURI(homePage[0]);
>-      break;
>-    case "tab":
>-      tab = gBrowser.addTab(homePage[0]);
>-      if (!BookmarksUtils.shouldLoadTabInBackground(aEvent))
>-        gBrowser.selectedTab = tab;
>-      break;
>-    case "window":
>-      openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no", homePage[0]);
>-    }
>-  } else {
>-    if (target == "window")
>-      openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no", homePage.join("\n"));
>-    else {
>-      var URIs = [];
>-      for (var i in homePage)
>-        URIs.push({URI: homePage[i]});
>-
>-      tab = gBrowser.loadGroup(URIs);
>-      
>-      if (!BookmarksUtils.shouldLoadTabInBackground(aEvent))
>-        gBrowser.selectedTab = tab;
>-    }
>-  }
>+  if (homePage.length == 1)
>+    openUILinkIn(homePage[0], where);
>+  else
>+    openUILinkArrayIn(homePage, where)
> }
> 
> function addBookmarkAs()
> {
>   const browsers = gBrowser.browsers;
>   if (browsers.length > 1)
>     BookmarksUtils.addBookmarkForTabBrowser(gBrowser);
>   else
>@@ -2225,20 +2255,20 @@ function handlePageProxyClick(aEvent)
> {
>   switch (aEvent.button) {
>   case 0:
>     // bug 52784 - select location field contents
>     gURLBar.select();
>     break;
>   case 1:
>     // bug 111337 - load url/keyword from clipboard
>-    return middleMousePaste(aEvent);
>+    if (Services.prefs.getBoolPref("middlemouse.paste"))
>+      middleMousePaste(aEvent);
>     break;
>   }
>-  return true;
> }
> 
> function updateComponentBarBroadcaster()
> { 
>   var compBarBroadcaster = document.getElementById('cmd_viewcomponentbar');
>   var taskBarBroadcaster = document.getElementById('cmd_viewtaskbar');
>   var compBar = document.getElementById('component-bar');
>   if (taskBarBroadcaster.getAttribute('checked') == 'true') {
>diff --git a/suite/browser/navigator.xul b/suite/browser/navigator.xul
>--- a/suite/browser/navigator.xul
>+++ b/suite/browser/navigator.xul
>@@ -127,21 +127,23 @@
> 
>   <popupset id="bookmarksPopupset"/>
>   <template id="bookmarksMenuTemplate"/>
> 
>   <popupset id="mainPopupSet">
>     <menupopup id="backMenu"
>            position="after_start"
>            onpopupshowing="return BrowserBackMenu(event);"
>-           oncommand="gotoHistoryIndex(event);"/>
>+           oncommand="gotoHistoryIndex(event);"
>+           onclick="checkForMiddleClick(this, event);"/>
>     <menupopup id="forwardMenu"
>            position="after_start"
>            onpopupshowing="return BrowserForwardMenu(event);"
>-           oncommand="gotoHistoryIndex(event);"/>
>+           oncommand="gotoHistoryIndex(event);"
>+           onclick="checkForMiddleClick(this, event);"/>
>     <tooltip id="aHTMLTooltip"
>              onpopupshowing="return FillInHTMLTooltip(document.tooltipNode);"/>
>     <menupopup id="sidebarPopup"/>
> 
>     <tooltip  id="btTooltip"/>
> 
>     <tooltip id="home-button-tooltip" noautohide="true">
>       <vbox id="home-button-tooltip-inner" flex="1"/>
>@@ -225,37 +227,40 @@
>     </toolbar>
> 
>     <toolbarpalette id="BrowserToolbarPalette">
> 
>       <!-- Nav bar buttons -->
>       <toolbarbutton id="back-button" type="menu-button"
>                      class="toolbarbutton-1 chromeclass-toolbar-additional"
>                      label="&backButton.label;"
>-                     oncommand="if (event.target==this) BrowserBack(); else gotoHistoryIndex(event);"
>+                     oncommand="if (event.target==this) BrowserBack(event); else gotoHistoryIndex(event);"
>+                     onclick="checkForMiddleClick(this, event);"
>                      context="backMenu"
>                      tooltiptext="&backButton.tooltip;">
>         <observes element="canGoBack" attribute="disabled"/>
>         <menupopup context="" onpopupshowing="BrowserBackMenu(event);"/>
>       </toolbarbutton>
> 
>       <toolbarbutton id="forward-button" type="menu-button"
>                      class="toolbarbutton-1 chromeclass-toolbar-additional"
>                      label="&forwardButton.label;"
>-                     oncommand="if (event.target==this) BrowserForward(); else gotoHistoryIndex(event);"
>+                     oncommand="if (event.target==this) BrowserForward(event); else gotoHistoryIndex(event);"
>+                     onclick="checkForMiddleClick(this, event);"
>                      context="forwardMenu"
>                      tooltiptext="&forwardButton.tooltip;">
>         <observes element="canGoForward" attribute="disabled"/>
>         <menupopup context="" onpopupshowing="BrowserForwardMenu(event);"/>
>       </toolbarbutton>
> 
>       <toolbarbutton id="reload-button"
>                      class="toolbarbutton-1 chromeclass-toolbar-additional"
>                      label="&reloadButton.label;"
>-                     oncommand="if (event.shiftKey) BrowserReloadSkipCache(); else BrowserReload();"
>+                     oncommand="BrowserReloadOrDuplicate(event);"
>+                     onclick="checkForMiddleClick(this, event);"
>                      tooltiptext="&reloadButton.tooltip;"/>
> 
>       <toolbarbutton id="stop-button"
>                      class="toolbarbutton-1 chromeclass-toolbar-additional"
>                      label="&stopButton.label;"
>                      oncommand="BrowserStop();" observes="canStop"
>                      disabled="true"
>                      tooltiptext="&stopButton.tooltip;">
>@@ -357,17 +362,19 @@
>           <menuitem id="printMenuItemToolbar" label="&printCmd.label;" accesskey="&printCmd.accesskey;"
>                     default="true" oncommand="PrintUtils.print()"/>
>           <menuitem id="printPreviewMenuItemToolbar" label="&printPreviewCmd.label;" accesskey="&printPreviewCmd.accesskey;"
>                     oncommand="setTimeout(BrowserPrintPreview, 0);"/>
>         </menupopup>
>       </toolbarbutton>
> 
>       <toolbaritem id="throbber-box" align="center">
>-        <button id="navigator-throbber" oncommand="goClickThrobber('browser.throbber.url')"
>+        <button id="navigator-throbber"
>+                oncommand="goClickThrobber('browser.throbber.url', event)"
>+                onclick="checkForMiddleClick(this, event);"
>                 tooltiptext="&throbber.tooltip;"/>
>       </toolbaritem>
> 
>       <!-- "Bookmarks" button on personal toolbar -->                           
>       <toolbarbutton type="menu" id="bookmarks-button" class="bookmark-item"
>                      label="&bookmarksButton.label;"
>                      datasources="rdf:bookmarks rdf:files rdf:localsearch rdf:internetsearch"
>                      ref="NC:BookmarksRoot" container="true" flags="dont-test-empty"
>diff --git a/suite/browser/navigatorOverlay.xul b/suite/browser/navigatorOverlay.xul
>--- a/suite/browser/navigatorOverlay.xul
>+++ b/suite/browser/navigatorOverlay.xul
>@@ -190,21 +190,23 @@
>              accesskey="&addCurPageAsCmd.accesskey;"
>              oncommand="addBookmarkAs();"/>
>     <command id="Browser:AddGroupmarkAs" label="&addCurTabsAsCmd.label;"
>              accesskey="&addCurTabsAsCmd.accesskey;"
>              oncommand="addGroupmarkAs(); event.stopPropagation();"/>
>     <command id="Browser:ManageBookmark" label="&manBookmarksCmd.label;" accesskey="&manBookmarksCmd.accesskey;" 
>              oncommand="toBookmarksManager();"/>
>   
>-    <!-- Go Menu -->  
>+    <!-- Go Menu -->
>     <command id="Browser:Home"    oncommand="BrowserHome(event);"/>
>     <command id="Browser:Back"    oncommand="BrowserBack();"    observes="canGoBack"/>
>     <command id="Browser:Forward" oncommand="BrowserForward();" observes="canGoForward"/>
>     <command id="Browser:Up"      oncommand="BrowserUp();"      observes="canGoUp"/>
>+    <command id="Browser:BackOrBackDuplicate"       oncommand="BrowserBack(event);"    observes="canGoBack"/>
>+    <command id="Browser:ForwardOrForwardDuplicate" oncommand="BrowserForward(event);" observes="canGoForward"/>
>     <commandset id="viewZoomCommands"/>
>     <commandset id="tasksCommands"/>
> 
>     <!-- Tools Menu -->
>     <command id="Browser:SearchInternet" oncommand="BrowserSearchInternet();"/>
>     <command id="Tools:Sanitize"
>              oncommand="Components.classes['@mozilla.org/suite/suiteglue;1'].getService(Components.interfaces.nsISuiteGlue).sanitize(window);"/>
> 
>@@ -398,17 +400,22 @@
>             <menuitem label="&componentbarCmd.label;" accesskey="&componentbarCmd.accesskey;" class="menuitem-iconic" type="checkbox" observes="cmd_viewcomponentbar"/>
>           </menupopup>
>         </menu>   
>         <menuitem id="menuitem_fullScreen" accesskey="&fullScreenCmd.accesskey;"
>                   label="&fullScreenCmd.label;" key="key_fullScreen"
>                   command="View:FullScreen" hidden="true"/>
>         <menuseparator />
>         <menuitem label="&stopCmd.label;" accesskey="&stopCmd.accesskey;" id="menuitem-stop" disabled="true" oncommand="BrowserStop();" key="key_stop"/>
>-        <menuitem id="menuitem_reload" accesskey="&reloadCmd.accesskey;" key="key_reload" label="&reloadCmd.label;" oncommand="BrowserReload();"/>
>+        <menuitem id="menuitem_reload"
>+                  label="&reloadCmd.label;"
>+                  accesskey="&reloadCmd.accesskey;"
>+                  key="key_reload"
>+                  oncommand="BrowserReloadOrDuplicate(event);"
>+                  onclick="checkForMiddleClick(this, event);"/>
>         <menuseparator />
>   
>         <!-- overlayed from viewZoomOverlay.xul -->
>         <menu id="menu_zoom"/>
> 
>         <menu id="menu_UseStyleSheet" label="&useStyleSheetMenu.label;" accesskey="&useStyleSheetMenu.accesskey;" disabled="false" observes="isImage">
>           <menupopup onpopupshowing="stylesheetFillPopup(this);"
>                      oncommand="stylesheetSwitchAll(window.content, event.target.getAttribute('data')); setStyleDisabled(false);" type="radio">
>@@ -441,38 +448,42 @@
>           </menupopup>
>         </menu>
>       </menupopup>
>     </menu>
> 
>     <menu id="history-menu"
>           label="&goMenu.label;"
>           accesskey="&goMenu.accesskey;"
>-          oncommand="gotoHistoryIndex(event);">
>+          oncommand="gotoHistoryIndex(event);"
>+          onclick="checkForMiddleClick(this, event);">
>       <menupopup id="goPopup"
>                  onpopupshowing="updateGoMenu(event);">
>         <menuitem id="historyMenuBack"
>                   label="&goBackCmd.label;"
>                   accesskey="&goBackCmd.accesskey;"
>                   key="goBackKb"
>-                  command="Browser:Back"/>
>+                  command="Browser:BackOrBackDuplicate"
>+                  onclick="checkForMiddleClick(this, event);"/>
>         <menuitem id="historyMenuForward"
>                   label="&goForwardCmd.label;"
>                   accesskey="&goForwardCmd.accesskey;"
>                   key="goForwardKb"
>-                  command="Browser:Forward"/>
>+                  command="Browser:ForwardOrForwardDuplicate"
>+                  onclick="checkForMiddleClick(this, event);"/>
>         <menuitem id="historyMenuUp"
>                   label="&goUpCmd.label;"
>                   accesskey="&goUpCmd.accesskey;"
>                   key="goUpKb"
>                   command="Browser:Up"/>
>         <menuitem id="historyMenuHome"
>                   label="&goHomeCmd.label;"
>                   accesskey="&goHomeCmd.accesskey;"
>                   command="Browser:Home"
>+                  onclick="checkForMiddleClick(this, event);"
>                   key="goHome"/>
>         <menuseparator/>
>         <menuitem id="menu_showAllHistory"
>                   label="&historyCmd.label;"
>                   accesskey="&historyCmd.accesskey;"
>                   oncommand="toHistory()"
>                   key="key_gotoHistory"/>
>         <menuseparator id="startHistorySeparator" hidden="true"/>
>diff --git a/suite/common/contentAreaClick.js b/suite/common/contentAreaClick.js
>--- a/suite/common/contentAreaClick.js
>+++ b/suite/common/contentAreaClick.js
>@@ -116,20 +116,19 @@
>             isPhishingURL(ceParams.linkNode, false, href))
>           return false;
>         handleLinkClick(event, href, ceParams.linkNode);
>       }
>       return true;
>     }
> 
>     if (pref && !isKeyCommand && event.button == 1 &&
>-        pref.getBoolPref("middlemouse.contentLoadURL")) {
>-      if (middleMousePaste(event)) {
>-        event.stopPropagation();
>-      }
>+        pref.getBoolPref("middlemouse.contentLoadURL") &&
>+        !pref.getBoolPref("general.autoScroll")) {
>+      middleMousePaste(event);
>     }
>     return true;
>   }
> 
>   function openNewTabOrWindow(event, href, doc)
>   {
>     // should we open it in a new tab?
>     if (pref && pref.getBoolPref("browser.tabs.opentabfor.middleclick")) {
>@@ -192,56 +191,19 @@
>   function middleMousePaste( event )
>   {
>     var url = readFromClipboard();
>     if (!url)
>       return false;
>     addToUrlbarHistory(url);
>     url = getShortcutOrURI(url);
> 
>-    // On ctrl-middleclick, open in new window or tab.  Do not send referrer.
>-    if (event.ctrlKey) {
>-      // fix up our pasted URI in case it is malformed.
>-      const nsIURIFixup = Components.interfaces.nsIURIFixup;
>-      if (!gURIFixup)
>-        gURIFixup = Components.classes["@mozilla.org/docshell/urifixup;1"]
>-                              .getService(nsIURIFixup);
>-
>-      url = gURIFixup.createFixupURI(url, nsIURIFixup.FIXUP_FLAGS_MAKE_ALTERNATE_URI).spec;
>-
>-      return openNewTabOrWindow(event, url, null);
>-    }
>-
>-    // If ctrl wasn't down, then just load the url in the targeted win/tab.
>-    var browser = getBrowser();
>-    var tab = event.originalTarget;
>-    if (tab.localName == "tab" &&
>-        tab.parentNode == browser.tabContainer) {
>-      tab.linkedBrowser.userTypedValue = url;
>-      if (tab == browser.mCurrentTab && url != "about:blank") {
>-          gURLBar.value = url;
>-      }
>-      tab.linkedBrowser.loadURI(url);
>-      if (event.shiftKey != (pref && pref.getBoolPref("browser.tabs.loadInBackground")))
>-        browser.selectedTab = tab;
>-    }
>-    else if (event.target == browser) {
>-      tab = browser.addTab(url);
>-      if (event.shiftKey != (pref && pref.getBoolPref("browser.tabs.loadInBackground")))
>-        browser.selectedTab = tab;
>-    }
>-    else {
>-      if (url != "about:blank") {
>-        gURLBar.value = url;
>-      }
>-      loadURI(url);
>-    }
>-
>-    event.stopPropagation();
>-    return true;
>+  // ignore the fact this is a middle click.
>+  openUILink(url, event, true);
>+  event.stopPropagation();
>   }
> 
>   function addToUrlbarHistory(aUrlToAdd)
>   {
>     // Remove leading and trailing spaces first
>     aUrlToAdd = aUrlToAdd.trim();
> 
>     if (!aUrlToAdd)
>diff --git a/suite/common/contentAreaContextOverlay.xul b/suite/common/contentAreaContextOverlay.xul
>--- a/suite/common/contentAreaContextOverlay.xul
>+++ b/suite/common/contentAreaContextOverlay.xul
>@@ -150,25 +150,27 @@
>                 oncommand="gContextMenu.toggleImageSize();"/>
>       <menuitem id="context-reloadimage"
>                 label="&reloadImageCmd.label;"
>                 accesskey="&reloadImageCmd.accesskey;"
>                 oncommand="gContextMenu.reloadImage();"/>
>       <menuitem id="context-viewimage"
>                 label="&viewImageCmd.label;"
>                 accesskey="&viewImageCmd.accesskey;"
>-                oncommand="gContextMenu.viewMedia();"/>
>+                oncommand="gContextMenu.viewMedia(event);"
>+                onclick="checkForMiddleClick(this, event);"/>
>       <menuitem id="context-copyimage"
>                 label="&copyImageCmd.label;"
>                 accesskey="&copyImageCmd.accesskey;"
>                 command="cmd_copyImage"/>
>       <menuitem id="context-viewvideo"
>                 label="&viewVideoCmd.label;"
>                 accesskey="&viewVideoCmd.accesskey;"
>-                oncommand="gContextMenu.viewMedia();"/>
>+                oncommand="gContextMenu.viewMedia(event);"
>+                onclick="checkForMiddleClick(this, event);"/>
>       <menuitem id="context-copyvideourl"
>                 label="&copyVideoURLCmd.label;"
>                 accesskey="&copyVideoURLCmd.accesskey;"
>                 oncommand="gContextMenu.copyMediaLocation();"/>
>       <menuitem id="context-copyaudiourl"
>                 label="&copyAudioURLCmd.label;"
>                 accesskey="&copyAudioURLCmd.accesskey;"
>                 oncommand="gContextMenu.copyMediaLocation();"/>
>@@ -189,25 +191,28 @@
>       <menuitem id="context-saveaudio"
>                 label="&saveAudioCmd.label;"
>                 accesskey="&saveAudioCmd.accesskey;"
>                 oncommand="gContextMenu.saveMedia();"/>
>       <menuseparator id="context-sep-image"/>
>       <menuitem id="context-back"
>                 label="&goBackCmd.label;"
>                 accesskey="&goBackCmd.accesskey;"
>-                oncommand="BrowserBack()"/>
>+                oncommand="BrowserBack(event)"
>+                onclick="checkForMiddleClick(this, event);"/>
>       <menuitem id="context-forward"
>                 label="&goForwardCmd.label;"
>                 accesskey="&goForwardCmd.accesskey;"
>-                oncommand="BrowserForward()"/>
>+                oncommand="BrowserForward(event)"
>+                onclick="checkForMiddleClick(this, event);"/>
>       <menuitem id="context-reload"
>                 label="&reloadCmd.label;"
>                 accesskey="&reloadCmd.accesskey;"
>-                oncommand="BrowserReload();"/>
>+                oncommand="BrowserReloadOrDuplicate(event);"
>+                onclick="checkForMiddleClick(this, event);"/>
>       <menuitem id="context-stop"
>                 label="&stopCmd.label;"
>                 accesskey="&stopCmd.accesskey;"
>                 disabled="true"
>                 oncommand="BrowserStop();"/>
>       <menuseparator id="context-sep-stop"/>
>       <menuitem id="context-bookmarkpage"
>                 label="&bookmarkPageCmd.label;"
>@@ -217,17 +222,18 @@
>                 valueSaveAs="&savePageAsCmd.label;"
>                 valueSave="&savePageCmd.label;"
>                 accesskey="&savePageCmd.accesskey;"
>                 oncommand="saveDocument(window.content.document, true);"/>
>       <menuseparator id="context-sep-viewbgimage"/>
>       <menuitem id="context-viewbgimage"
>                 label="&viewBGImageCmd.label;"
>                 accesskey="&viewBGImageCmd.accesskey;"
>-                oncommand="gContextMenu.viewBGImage();"/>
>+                oncommand="gContextMenu.viewBGImage(event);"
>+                onclick="checkForMiddleClick(this, event);"/>
>       <menuitem id="context-undo"
>                 label="&undoCmd.label;"
>                 accesskey="&undoCmd.accesskey;"
>                 command="cmd_undo"/>
>       <menuitem id="context-redo"
>                 label="&redoCmd.label;"
>                 accesskey="&redoCmd.accesskey;"
>                 command="cmd_redo"/>
>diff --git a/suite/common/nsContextMenu.js b/suite/common/nsContextMenu.js
>--- a/suite/common/nsContextMenu.js
>+++ b/suite/common/nsContextMenu.js
>@@ -795,42 +795,44 @@ nsContextMenu.prototype = {
>   reloadImage: function() {
>     urlSecurityCheck(this.mediaURL, this.target.nodePrincipal,
>                      Components.interfaces.nsIScriptSecurityManager.ALLOW_CHROME);
>     if (this.target instanceof Components.interfaces.nsIImageLoadingContent)
>       this.target.forceReload();
>   },
> 
>   // Change current window to the URL of the image, video, or audio.
>-  viewMedia: function() {
>+  viewMedia: function(aEvent) {
>     var viewURL;
>     if (this.onCanvas)
>       viewURL = this.target.toDataURL();
>     else {
>       viewURL = this.mediaURL;
>       urlSecurityCheck(viewURL, this.target.nodePrincipal,
>                        Components.interfaces.nsIScriptSecurityManager.ALLOW_CHROME);
>     }
>-    openTopWin(viewURL, this.target.ownerDocument.defaultView);
>+    var doc = this.target.ownerDocument;
>+    openUILink(viewURL, aEvent, null, null, null, null, doc.documentURIObject );
>   },
> 
>   // Full screen video playback
>   fullScreenVideo: function() {
>     var isPaused = this.target.paused && this.target.currentTime > 0;
>     this.target.pause();
> 
>     openDialog("chrome://communicator/content/fullscreen-video.xhtml",
>                "", "chrome,dialog=no", this.target, isPaused);
>   },
> 
>   // Change current window to the URL of the background image.
>-  viewBGImage: function() {
>+  viewBGImage: function(aEvent) {
>     urlSecurityCheck(this.bgImageURL, this.target.nodePrincipal,
>                      Components.interfaces.nsIScriptSecurityManager.ALLOW_CHROME);
>-    openTopWin(this.bgImageURL, this.target.ownerDocument.defaultView);
>+    var doc = this.target.ownerDocument;
>+    openUILink(this.bgImageURL, aEvent, null, null, null, null, doc.documentURIObject );
>   },
> 
>   setWallpaper: function() {
>     // Confirm since it's annoying if you hit this accidentally.
>     var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>                                   .getService(Components.interfaces.nsIPromptService);
>     var navigatorBundle = document.getElementById("bundle_navigator");
>     var promptTitle = navigatorBundle.getString("wallpaperConfirmTitle");
>diff --git a/suite/common/utilityOverlay.js b/suite/common/utilityOverlay.js
>--- a/suite/common/utilityOverlay.js
>+++ b/suite/common/utilityOverlay.js
>@@ -511,29 +511,29 @@ function toolboxCustomizeChange(toolbox,
>     let primary = /toolbar-primary/.test(toolbar.getAttribute("class"));
>     if (primary) {
>       toolbar.removeAttribute("ignoremodepref");
>       document.persist(toolbar.id, "ignoremodepref");
>     }
>   }
> }
> 
>-function goClickThrobber( urlPref )
>+function goClickThrobber(urlPref, aEvent)
> {
>   var url;
>   try {
>     url = Services.prefs.getComplexValue(urlPref, nsIPrefLocalizedString).data;
>   }
> 
>   catch(e) {
>     url = null;
>   }
> 
>-  if ( url )
>-    openUILink(url);
>+  if (url)
>+    openUILink(url, aEvent, false, true);
> }
> 
> function getTopWin()
> {
>   return Services.wm.getMostRecentWindow("navigator:browser");
> }
> 
> function isRestricted( url )
>@@ -1129,20 +1129,20 @@ function getBoolPref(prefname, def)
>     return Services.prefs.getBoolPref(prefname);
>   }
>   catch (er) {
>     return def;
>   }
> }
> 
> // openUILink handles clicks on UI elements that cause URLs to load.
>-function openUILink(url, e, ignoreButton, ignoreSave, allowKeywordFixup)
>+function openUILink(url, e, ignoreButton, ignoreSave, allowKeywordFixup, postData, referrerUrl)
> {
>   var where = whereToOpenLink(e, ignoreButton, ignoreSave);
>-  return openUILinkIn(url, where, allowKeywordFixup);
>+  return openUILinkIn(url, where, allowKeywordFixup, postData, referrerUrl);
> }
> 
> /* whereToOpenLink() looks at an event to decide where to open a link.
>  *
>  * The event may be a mouse event (click, double-click, middle-click) or keypress event (enter).
>  *
>  * The logic for modifiers is as following:
>  * If browser.tabs.opentabfor.middleclick is true, then Ctrl (or Meta) and middle-click

Comment 9

[Philip Chee]

Fooey, sorry for the bugspam.

> function BrowserHome(aEvent)
[....]
>-  var target = !gBrowser ? "window": !aEvent ? "current" : BookmarksUtils.getBrowserTargetFromEvent(aEvent);
>+  //var where = whereToOpenLink(aEvent, false, true);
>+  var where = !gBrowser ? "window": !aEvent ? "current" : BookmarksUtils.getBrowserTargetFromEvent(aEvent);

I thought that whereToOpenLink() changed the way this behaved too much but I left this commented out in case anyone wants to test how that works.

Comment 10

[Philip Chee]

Note I didn't touch bookmarks since Places Bookmarks is going to land RSN.

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

Some feedback, as requested (next time with f?=me please so I don't forget):
- Overall works very well. Tested Back/Forward/Reload/Home buttons and context/menu items, including individual page entries, with single and group home page, with loading tabs in the back-/foreground (pref), with left and right mouse buttons (to ensure that functionality is not affected), View Background Image, View Image, throbber; confirming that new tabs are opened next to originating ones.
- Issues: throbber middle click doesn't respect load tabs in the background. Or is that on purpose?
- Didn't test: proxy icon (not using a proxy here), view other media (video/audio? How?), middlemouse.paste, middlemouse.contentLoadURL.
- Nits: white space before closing bracket of openUILink call (twice), now obsolete curly braces (once).

Thanks! :-)

Comment 12

[Robert Kaiser]

Comment on attachment 460490 [details] [diff] [review]
Patch v1.0 Sprinkle MiddleLinkClick fairy dust over navigator.

Too many conflicts with my own patches that I can easily apply it and give feedback. Not sure what feedback you'd expect from me anyhow.

Comment 13

[Ian Neal]

(In reply to comment #11)
> - Issues: throbber middle click doesn't respect load tabs in the background. Or
> is that on purpose?
Seems to respect loads tabs in background for me on linux.

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #13)
> (In reply to comment #11)
> > - Issues: throbber middle click doesn't respect load tabs in the background. Or
> > is that on purpose?
> Seems to respect loads tabs in background for me on linux.

Sorry, upon further checking I found there's an incompatibility with MR Tech Toolkit 6.0.4. I should really have checked with a fresh profile... So, no issue there.

Comment 15

[Ian Neal]

Comment on attachment 460490 [details] [diff] [review]
Patch v1.0 Sprinkle MiddleLinkClick fairy dust over navigator.

>diff --git a/suite/browser/navigator.js b/suite/browser/navigator.js
> function gotoHistoryIndex(aEvent)
>+  var where = whereToOpenLink(aEvent);
>+
>+  if (where == "current") {
>+    // Normal click.  Go there in the current tab and update session history.
>+
>+    if (index == "back")
>+      gBrowser.goBackGroup();
>+    else if (index ==  "forward")
>+      gBrowser.goForwardGroup();
>+    else {
>+      try {
>+        getWebNavigation().gotoIndex(index);
>+      }
>+      catch(ex) {
>+        return false;
>+      }
>+    }
>+    return true;
If you add this line, then you don't need to put the following code in an else clause. Alternatively don't add the return line in this and the else and leave the return at the end of the function. I'm sure switching on strict javascript warnings will have shown this up ;)
>+  }
>   else {
>+    // Modified click.  Go there in a new tab/window.
>+    // This code doesn't copy history or work well with framed pages.
>+
>+    var sessionHistory = getWebNavigation().sessionHistory;
>+    var entry = sessionHistory.getEntryAtIndex(index, false);
>+    var url = entry.URI.spec;
>+    openUILinkIn(url, where, {relatedToCurrent: true});
>+    return true;
>   }
>-  return true;
>-
> }

Is it worth having a helper function that you pass where, index and false to, that does the above, which would remove some code duplication?
e.g.
function OpenSessionHistoryIn(aWhere, aIndex, aAddSessionHistoryIndex)
{
  var sessionHistory = getWebNavigation().sessionHistory;
  if (aAddSessionHistoryIndex)
    aIndex += sessionHistory.index;
  var entry = sessionHistory.getEntryAtIndex(aIndex, false);
  var url = entry.URI.spec;
  openUILinkIn(url, aWhere, {relatedToCurrent: true});
}

>+function BrowserBack(aEvent)
>+    var sessionHistory = getWebNavigation().sessionHistory;
>+    var currentIndex = sessionHistory.index;
>+    var entry = sessionHistory.getEntryAtIndex(currentIndex - 1, false);
>+    var url = entry.URI.spec;
>+    openUILinkIn(url, where, {relatedToCurrent: true});
Here passing where, -1 and true.

>+function BrowserForward(aEvent)
>+    var sessionHistory = getWebNavigation().sessionHistory;
>+    var currentIndex = sessionHistory.index;
>+    var entry = sessionHistory.getEntryAtIndex(currentIndex + 1, false);
>+    var url = entry.URI.spec;
>+    openUILinkIn(url, where, {relatedToCurrent: true});
Here passing where, +1 and true.

> function BrowserHome(aEvent)
> {
>   var tab;
>   var homePage = getHomePage();
>-  var target = !gBrowser ? "window": !aEvent ? "current" : BookmarksUtils.getBrowserTargetFromEvent(aEvent);
>+  //var where = whereToOpenLink(aEvent, false, true);
>+  var where = !gBrowser ? "window": !aEvent ? "current" : BookmarksUtils.getBrowserTargetFromEvent(aEvent);
How about:
var where = !gBrowser ? "window": whereToOpenLink(aEvent, false, true);
Either way getBrowserTargetFromEvent will be vanishing soon and you will bitrot KaiRo's bookmark places patch.

> // openUILink handles clicks on UI elements that cause URLs to load.
>-function openUILink(url, e, ignoreButton, ignoreSave, allowKeywordFixup)
>+function openUILink(url, e, ignoreButton, ignoreSave, allowKeywordFixup, postData, referrerUrl)
In theory should be using the format of aVariableName here, but not going to fall out over it.
> {
>   var where = whereToOpenLink(e, ignoreButton, ignoreSave);
>-  return openUILinkIn(url, where, allowKeywordFixup);
>+  return openUILinkIn(url, where, allowKeywordFixup, postData, referrerUrl);
> }
r=me with those points addressed.

Comment 16

[Philip Chee]

Attachment: Patch v1.1 Fix OpenUILinkArrayIn() as well.

> Ian Neal      2010-07-31 08:10:34 PDT
> 
> >diff --git a/suite/browser/navigator.js b/suite/browser/navigator.js
> > function gotoHistoryIndex(aEvent)

> >+    return true;
> If you add this line, then you don't need to put the following code in an else
> clause. Alternatively don't add the return line in this and the else and leave
> the return at the end of the function. I'm sure switching on strict javascript
> warnings will have shown this up ;)
Fixed.

> Is it worth having a helper function that you pass where, index and false to,
> that does the above, which would remove some code duplication?
> e.g.
> function OpenSessionHistoryIn(aWhere, aIndex, aAddSessionHistoryIndex)
> {
>   var sessionHistory = getWebNavigation().sessionHistory;
>   if (aAddSessionHistoryIndex)
>     aIndex += sessionHistory.index;
>   var entry = sessionHistory.getEntryAtIndex(aIndex, false);
>   var url = entry.URI.spec;
>   openUILinkIn(url, aWhere, {relatedToCurrent: true});
> }
Fixed. But I used aIsOffset instead of aAddSessionHistoryIndex.

> > function BrowserHome(aEvent)
> >-  var target = !gBrowser ? "window": !aEvent ? "current" : BookmarksUtils.getBrowserTargetFromEvent(aEvent);
> >+  //var where = whereToOpenLink(aEvent, false, true);
> >+  var where = !gBrowser ? "window": !aEvent ? "current" : BookmarksUtils.getBrowserTargetFromEvent(aEvent);
> How about:
> var where = !gBrowser ? "window": whereToOpenLink(aEvent, false, true);
> Either way getBrowserTargetFromEvent will be vanishing soon and you will bitrot
> KaiRo's bookmark places patch.
Fixed.

> > // openUILink handles clicks on UI elements that cause URLs to load.
> >-function openUILink(url, e, ignoreButton, ignoreSave, allowKeywordFixup)
> >+function openUILink(url, e, ignoreButton, ignoreSave, allowKeywordFixup, postData, referrerUrl)
> In theory should be using the format of aVariableName here, but not going to
> fall out over it.
I'll leave it as it is since changing all these will just add unnecessary white noise to this patch.

> r=me with those points addressed.

New in this patch. Fixed a slight preexisting problem with openUILinkArrayIn() exposed by my changes to BrowserHome()

I've noticed a minor regression in the behaviour with left clicking the home button ignoring browser.tabs.loadGroup although I suspect that once KaiRo's places bookmarks lands the last remaining caller for gBrower.loadGroup() will be gone.

I think we should ignore browser.tabs.loadGroup here if we are going to make use of the openUILink* family consistently. My take is that the load/replace/appendGroup() functions are there (only two callers) only because we didn't have openUILinkArrayIn() then. With the ability to middle click the home button to open a group of home pages in new tabs, the browser.tabs.loadGroup pref is now redundant.

Aha. Found Bug 203960 (Make bookmark groups conditionally replace existing tabs instead of appending).

Comment 17

[Robert Kaiser]

Comment on attachment 463163 [details] [diff] [review]
Patch v1.1 Fix OpenUILinkArrayIn() as well.

> function BrowserHome(aEvent)
> {
>   var tab;
>   var homePage = getHomePage();
>-  var target = !gBrowser ? "window": !aEvent ? "current" : BookmarksUtils.getBrowserTargetFromEvent(aEvent);
>+  var where = !gBrowser ? "window" :
>+                          !aEvent ? "current" :
>+                                    whereToOpenLink(aEvent, false, true);

Let me forward Neil's nit: "whereToOpenLink already knows how to deal with a null event."

So, as Ian said, make this just:

var where = !gBrowser ? "window" : whereToOpenLink(aEvent, false, true);

Comment 18

[Philip Chee]

Attachment: Patch v1.1a Fix nits plus minor problem with OpenUILinkArrayIn().

> Ian Neal      2010-07-31 08:10:34 PDT
> 
> >diff --git a/suite/browser/navigator.js b/suite/browser/navigator.js
> > function gotoHistoryIndex(aEvent)

> >+    return true;
> If you add this line, then you don't need to put the following code in an else
> clause. Alternatively don't add the return line in this and the else and leave
> the return at the end of the function. I'm sure switching on strict javascript
> warnings will have shown this up ;)
Fixed.

> Is it worth having a helper function that you pass where, index and false to,
> that does the above, which would remove some code duplication?
> e.g.
> function OpenSessionHistoryIn(aWhere, aIndex, aAddSessionHistoryIndex)
> {
>   var sessionHistory = getWebNavigation().sessionHistory;
>   if (aAddSessionHistoryIndex)
>     aIndex += sessionHistory.index;
>   var entry = sessionHistory.getEntryAtIndex(aIndex, false);
>   var url = entry.URI.spec;
>   openUILinkIn(url, aWhere, {relatedToCurrent: true});
> }
Fixed. But I used aIsOffset instead of aAddSessionHistoryIndex.

> > function BrowserHome(aEvent)
> >-  var target = !gBrowser ? "window": !aEvent ? "current" : BookmarksUtils.getBrowserTargetFromEvent(aEvent);
> >+  //var where = whereToOpenLink(aEvent, false, true);
> >+  var where = !gBrowser ? "window": !aEvent ? "current" : BookmarksUtils.getBrowserTargetFromEvent(aEvent);
> How about:
> var where = !gBrowser ? "window": whereToOpenLink(aEvent, false, true);
> Either way getBrowserTargetFromEvent will be vanishing soon and you will bitrot
> KaiRo's bookmark places patch.
Fixed.

> > // openUILink handles clicks on UI elements that cause URLs to load.
> >-function openUILink(url, e, ignoreButton, ignoreSave, allowKeywordFixup)
> >+function openUILink(url, e, ignoreButton, ignoreSave, allowKeywordFixup, postData, referrerUrl)
> In theory should be using the format of aVariableName here, but not going to
> fall out over it.
I'll leave it as it is since changing all these will just add unnecessary white noise to this patch.

> r=me with those points addressed.

New in this patch. Fixed a slight preexisting problem with openUILinkArrayIn() exposed by my changes to BrowserHome()

I've noticed a minor regression in the behaviour with left clicking the home button ignoring browser.tabs.loadGroup although I suspect that once KaiRo's places bookmarks lands the last remaining caller for gBrower.loadGroup() will be gone.

I think we should ignore browser.tabs.loadGroup here if we are going to make use of the openUILink* family consistently. My take is that the load/replace/appendGroup() functions are there (only two callers) only because we didn't have openUILinkArrayIn() then. With the ability to middle click the home button to open a group of home pages in new tabs, the browser.tabs.loadGroup pref is now redundant.

Aha. Found Bug 203960 (Make bookmark groups conditionally replace existing tabs instead of appending).

> Robert Kaiser      2010-08-05 08:55:06 PDT
> 
> Comment on attachment 463163 [details] [diff] [review]
> Patch v1.1 Fix OpenUILinkArrayIn() as well.
> 
> > function BrowserHome(aEvent)
> > {
> >   var tab;
> >   var homePage = getHomePage();
> >-  var target = !gBrowser ? "window": !aEvent ? "current" : BookmarksUtils.getBrowserTargetFromEvent(aEvent);
> >+  var where = !gBrowser ? "window" :
> >+                          !aEvent ? "current" :
> >+                                    whereToOpenLink(aEvent, false, true);
> 
> Let me forward Neil's nit: "whereToOpenLink already knows how to deal with a
> null event."
> 
> So, as Ian said, make this just:
> 
> var where = !gBrowser ? "window" : whereToOpenLink(aEvent, false, true);
Fixed. Cheers!

Comment 19

[[e-mail]]

Comment on attachment 463219 [details] [diff] [review]
Patch v1.1a Fix nits plus minor problem with OpenUILinkArrayIn().

>+function OpenSessionHistoryIn(aWhere, aIndex, aIsOffset)
>+{
>+  var sessionHistory = getWebNavigation().sessionHistory;
>+  if (aIsOffset)
>+    aIndex += sessionHistory.index;
>+  var entry = sessionHistory.getEntryAtIndex(aIndex, false);
>+  var url = entry.URI.spec;
>+  openUILinkIn(url, aWhere, {relatedToCurrent: true});
Any chance we can send the referrerURI (property of nsISHEntry) too?
(Don't send postData though, that's dangerous.)

>-  if (index == "back")
>-    gBrowser.goBackGroup();
>-  else if (index ==  "forward")
>-    gBrowser.goForwardGroup();
Back/forward group only make sense on the current browser.
Also, Kairo is probably removing the last consumer of back/forward group and we should probably kill that off entirely, so it would be nice if you could write the patch so that we don't have to reindent everything when we do.

>+    // This code doesn't copy history or work well with framed pages.
Ask Misak whether Session Restore could be used to clone the tab.

>+  var key = /Mac/.test(navigator.platform) ? aEvent.metaKey : aEvent.ctrlKey;
>+  if (aEvent.shiftKey && !(aEvent.button == 1 || key)) {
What is this actually testing for? where == "current" && aEvent.shiftKey?

>+  if (homePage.length == 1)
>+    openUILinkIn(homePage[0], where);
>+  else
>+    openUILinkArrayIn(homePage, where)
Doesn't openUILinkArrayIn work for an array of one element?

>-    return middleMousePaste(aEvent);
>+    if (Services.prefs.getBoolPref("middlemouse.paste"))
>+      middleMousePaste(aEvent);
What's this for? middlemouse.paste is for clicking the middle mouse button into a content textfield. The page proxy icon has independent behaviour.

>-                     oncommand="if (event.target==this) BrowserBack(); else gotoHistoryIndex(event);"
>+                     oncommand="if (event.target==this) BrowserBack(event); else gotoHistoryIndex(event);"
[Interesting that we already passed the event to gotoHistoryIndex.]

>+        <button id="navigator-throbber"
>+                oncommand="goClickThrobber('browser.throbber.url', event)"
>+                onclick="checkForMiddleClick(this, event);"
What about non-browser throbbers?

>     <command id="Browser:Home"    oncommand="BrowserHome(event);"/>
>     <command id="Browser:Back"    oncommand="BrowserBack();"    observes="canGoBack"/>
>     <command id="Browser:Forward" oncommand="BrowserForward();" observes="canGoForward"/>
>     <command id="Browser:Up"      oncommand="BrowserUp();"      observes="canGoUp"/>
>+    <command id="Browser:BackOrBackDuplicate"       oncommand="BrowserBack(event);"    observes="canGoBack"/>
>+    <command id="Browser:ForwardOrForwardDuplicate" oncommand="BrowserForward(event);" observes="canGoForward"/>
Why different commands?

>-        pref.getBoolPref("middlemouse.contentLoadURL")) {
>-      if (middleMousePaste(event)) {
>-        event.stopPropagation();
>-      }
>+        pref.getBoolPref("middlemouse.contentLoadURL") &&
>+        !pref.getBoolPref("general.autoScroll")) {
>+      middleMousePaste(event);
What does this change achieve (and why)?

>-    openTopWin(viewURL, this.target.ownerDocument.defaultView);
This is needed to avoid XSS from specially crafted resource URIs into the existing window.

>+  return openUILinkIn(url, where, allowKeywordFixup, postData, referrerUrl);
Sadly openUILinkIn mostly ignores the referrerURI, except for new tabs...

>+  var relatedToCurrent = where == "current";
>   for (var i = 1; i < urlArray.length; i++)
>-    browser.addTab(urlArray[i], {allowThirdPartyFixup: allowThirdPartyFixup});
>+    browser.addTab(urlArray[i], {allowThirdPartyFixup: allowThirdPartyFixup, relatedToCurrent: relatedToCurrent});
Doesn't this assume that insertRelatedAfterCurrent is true?

Comment 20

[Philip Chee]

Attachment: Patch v1.2 back to WIP.

> [e-mail]      2010-08-05 14:24:39 PDT
> 
> Comment on attachment 463219 [details] [diff] [review]
> Patch v1.1a Fix nits plus minor problem with OpenUILinkArrayIn().
> 
> >+function OpenSessionHistoryIn(aWhere, aIndex, aIsOffset)
> >+{
> >+  var sessionHistory = getWebNavigation().sessionHistory;
> >+  if (aIsOffset)
> >+    aIndex += sessionHistory.index;
> >+  var entry = sessionHistory.getEntryAtIndex(aIndex, false);
> >+  var url = entry.URI.spec;
> >+  openUILinkIn(url, aWhere, {relatedToCurrent: true});
> Any chance we can send the referrerURI (property of nsISHEntry) too?
> (Don't send postData though, that's dangerous.)

Hmm. Looks like I don't need to QI to nsISHEntry at all.
Fixed.

> >-  if (index == "back")
> >-    gBrowser.goBackGroup();
> >-  else if (index ==  "forward")
> >-    gBrowser.goForwardGroup();
> Back/forward group only make sense on the current browser.
> Also, Kairo is probably removing the last consumer of back/forward group and we
> should probably kill that off entirely, so it would be nice if you could write
> the patch so that we don't have to reindent everything when we do.
I've removed all the Group navigation stuff on the assumption that Places Booksmarks will land RSN.
 
> >+    // This code doesn't copy history or work well with framed pages.
> Ask Misak whether Session Restore could be used to clone the tab.
Hmm. Good idea.
 
> >+  var key = /Mac/.test(navigator.platform) ? aEvent.metaKey : aEvent.ctrlKey;
> >+  if (aEvent.shiftKey && !(aEvent.button == 1 || key)) {
> What is this actually testing for? where == "current" && aEvent.shiftKey?

Firefox does this:

function BrowserReloadOrDuplicate(aEvent) {
  var backgroundTabModifier = aEvent.button == 1 ||
#ifdef XP_MACOSX
    aEvent.metaKey;
#else
    aEvent.ctrlKey;
#endif
  if (aEvent.shiftKey && !backgroundTabModifier) {
    BrowserReloadSkipCache();
    return;
  }
.....

> >+  if (homePage.length == 1)
> >+    openUILinkIn(homePage[0], where);
> >+  else
> >+    openUILinkArrayIn(homePage, where)
> Doesn't openUILinkArrayIn work for an array of one element?

Oops. Fixed.

> >-    return middleMousePaste(aEvent);
> >+    if (Services.prefs.getBoolPref("middlemouse.paste"))
> >+      middleMousePaste(aEvent);
> What's this for? middlemouse.paste is for clicking the middle mouse button into
> a content textfield. The page proxy icon has independent behaviour.

Firefox Bug 291768 (Middle click on proxy icon in location bar should paste URL).

> Traditional behaviour on Unix is to allow pasting a URL into the content area
> to load a URL from the current X11 selection. This feature tends to confuse
> new users (see the heated discussion in  bug 96972 for example) and conflicts
> with autoscroll (which is also activated by middle click in the content area).
> Autoscroll has been disabled by default on Unix because of this (bug 216899).
> 
> In Mozilla there is another way to load a URL from the X11 selection: middle
> click paste on the proxy icon in the location bar. This allows quick paste of
> URLs without the need for middlemouse.contentLoadURL and thus can be used even
> if autoscroll is enabled. I would like to see this in Firefox as well.
> 
> Relevant discussion:
> 
> bug 85677 - middle click in content behaviour
> bug 111337 - corresponding bug for Mozilla
> bug 216899 - ContentLoadURL vs autoscroll

> >-                     oncommand="if (event.target==this) BrowserBack(); else gotoHistoryIndex(event);"
> >+                     oncommand="if (event.target==this) BrowserBack(event); else gotoHistoryIndex(event);"
> [Interesting that we already passed the event to gotoHistoryIndex.]

When KaiRo ported Places History he also ported the middleLinkClick() bits as well. Of course that doesn't do anything until this patch which ports the rest is applied.

> >+        <button id="navigator-throbber"
> >+                oncommand="goClickThrobber('browser.throbber.url', event)"
> >+                onclick="checkForMiddleClick(this, event);"
> What about non-browser throbbers?

Most other windows with throbbers don't have a tabbrowser, and the mailnews tabbrowser has a different implementation.

> >     <command id="Browser:Home"    oncommand="BrowserHome(event);"/>
> >     <command id="Browser:Back"    oncommand="BrowserBack();"    observes="canGoBack"/>
> >     <command id="Browser:Forward" oncommand="BrowserForward();" observes="canGoForward"/>
> >     <command id="Browser:Up"      oncommand="BrowserUp();"      observes="canGoUp"/>
> >+    <command id="Browser:BackOrBackDuplicate"       oncommand="BrowserBack(event);"    observes="canGoBack"/>
> >+    <command id="Browser:ForwardOrForwardDuplicate" oncommand="BrowserForward(event);" observes="canGoForward"/>
> Why different commands?

I didn't want to risk changing the behaviour of the forward/backward navigation keys.

> >-        pref.getBoolPref("middlemouse.contentLoadURL")) {
> >-      if (middleMousePaste(event)) {
> >-        event.stopPropagation();
> >-      }
> >+        pref.getBoolPref("middlemouse.contentLoadURL") &&
> >+        !pref.getBoolPref("general.autoScroll")) {
> >+      middleMousePaste(event);
> What does this change achieve (and why)?

Firefox Bug 216899 (middle-clicking functions ContentLoadURL and autoscroll conflict).
This should also fix the related SeaMonkey Bug 534248 (Middle-clicking to stop autoscroll may activate contentLoadURL)

> >-    openTopWin(viewURL, this.target.ownerDocument.defaultView);
> This is needed to avoid XSS from specially crafted resource URIs into the
> existing window.

Firefox does not believe that this is necessary.
I looked at Bug 329521 and Attachment 225478 [details] [diff] and it appears that the only reason we use openTopWin() is that we didn't have openUILink(). The other difference is that we use nsIScriptSecurityManager.ALLOW_CHROME and Firefox uses nsIScriptSecurityManager.DISALLOW_SCRIPT. The IDL for nsIScriptSecurityManager comes with dire warnings about using ALLOW_CHROME, so I think DISALLOW_SCRIPT is actually correct.

> >+  return openUILinkIn(url, where, allowKeywordFixup, postData, referrerUrl);
> Sadly openUILinkIn mostly ignores the referrerURI, except for new tabs...

I assume that this is a parenthetical comment and that it isn't a nit I have to fix.

> >+  var relatedToCurrent = where == "current";
> >   for (var i = 1; i < urlArray.length; i++)
> >-    browser.addTab(urlArray[i], {allowThirdPartyFixup: allowThirdPartyFixup});
> >+    browser.addTab(urlArray[i], {allowThirdPartyFixup: allowThirdPartyFixup, relatedToCurrent: relatedToCurrent});
> Doesn't this assume that insertRelatedAfterCurrent is true?

If the user flips the preference I would assume that they would want to ignore relatedToCurrent here as well.

Comment 21

[[e-mail]]

(In reply to comment #20)
> > Any chance we can send the referrerURI (property of nsISHEntry) too?
> > (Don't send postData though, that's dangerous.)
> Hmm. Looks like I don't need to QI to nsISHEntry at all.
Maybe something else is doing it for you, but you can't rely on it.

> I've removed all the Group navigation stuff on the assumption that Places
> Booksmarks will land RSN.
All the group navigation stuff? Including all the callers? (Not read patch yet.)

> > >+  var key = /Mac/.test(navigator.platform) ? aEvent.metaKey : aEvent.ctrlKey;
> > >+  if (aEvent.shiftKey && !(aEvent.button == 1 || key)) {
> > What is this actually testing for? where == "current" && aEvent.shiftKey?
> Firefox does this:
Which means you don't know why... basically, my question was, are you only bypassing the cache when the reload would have affected the current page?

> > >-    return middleMousePaste(aEvent);
> > >+    if (Services.prefs.getBoolPref("middlemouse.paste"))
> > >+      middleMousePaste(aEvent);
> > What's this for? middlemouse.paste is for clicking the middle mouse button into
> > a content textfield. The page proxy icon has independent behaviour.
> Firefox Bug 291768 (Middle click on proxy icon in location bar should paste
> URL).
Sure, and for whatever reason they wanted this to depend on middlemouse.paste, and I see no reason to copy them.

> > >+        <button id="navigator-throbber"
> > >+                oncommand="goClickThrobber('browser.throbber.url', event)"
> > >+                onclick="checkForMiddleClick(this, event);"
> > What about non-browser throbbers?
> Most other windows with throbbers don't have a tabbrowser, and the mailnews
> tabbrowser has a different implementation.
But the throbber still calls goClickThrobber, doesn't it?

> > Why different commands?
> I didn't want to risk changing the behaviour of the forward/backward navigation keys.
Hmm, I need to check that.

> This should also fix the related SeaMonkey Bug 534248 (Middle-clicking to stop
> autoscroll may activate contentLoadURL)
Well to avoid confusion that part of the patch belongs in that bug.

> > >-    openTopWin(viewURL, this.target.ownerDocument.defaultView);
> > This is needed to avoid XSS from specially crafted resource URIs into the
> > existing window.
> Firefox does not believe that this is necessary.
Well that's because it blocks all sorts of URIs such as -moz-icon: ...

> > >+  return openUILinkIn(url, where, allowKeywordFixup, postData, referrerUrl);
> > Sadly openUILinkIn mostly ignores the referrerURI, except for new tabs...
> I assume that this is a parenthetical comment and that it isn't a nit I have to fix.
No, it's a bug you need to file ;-)

> > >+  var relatedToCurrent = where == "current";
> > >   for (var i = 1; i < urlArray.length; i++)
> > >-    browser.addTab(urlArray[i], {allowThirdPartyFixup: allowThirdPartyFixup});
> > >+    browser.addTab(urlArray[i], {allowThirdPartyFixup: allowThirdPartyFixup, relatedToCurrent: relatedToCurrent});
> > Doesn't this assume that insertRelatedAfterCurrent is true?
> If the user flips the preference I would assume that they would want to ignore
> relatedToCurrent here as well.
I guess since it didn't work before the pref existed that it's not unexpected.

Comment 22

[Philip Chee]

> > I've removed all the Group navigation stuff on the assumption that Places
> > Booksmarks will land RSN.
> All the group navigation stuff? Including all the callers? (Not read patch
> yet.)

Err I meant I remove the callers from the parts of the code I touched. Removing the callees would be done in the last step when Bookmarks Places removes the last callers.

I'll look at all the other issues on Monday.

Comment 23

[[e-mail]]

(In reply to comment #22)
> > > I've removed all the Group navigation stuff on the assumption that Places
> > > Booksmarks will land RSN.
> > All the group navigation stuff? Including all the callers? (Not read patch
> > yet.)
> Err I meant I remove the callers from the parts of the code I touched. Removing
> the callees would be done in the last step when Bookmarks Places removes the
> last callers.
No, that doesn't make sense, since you can't remove the back/forward group navigation until you've removed all the loadGroup callers. So you would have to wait until Places lands, or remove the back/forward group code separately.

Comment 24

[Philip Chee]

> No, that doesn't make sense, since you can't remove the back/forward group
> navigation until you've removed all the loadGroup callers. So you would have to
> wait until Places lands, or remove the back/forward group code separately.

I think that's what I said. I'm only removing the callers in navigator.js and not touching the actual back/forward group navigation code in tabbrowser.xml. KaiRo's patch will remove the remaining callers.

Comment 25

[[e-mail]]

(In reply to comment #24)
> > No, that doesn't make sense, since you can't remove the back/forward group
> > navigation until you've removed all the loadGroup callers. So you would have to
> > wait until Places lands, or remove the back/forward group code separately.
> I think that's what I said. I'm only removing the callers in navigator.js and
> not touching the actual back/forward group navigation code in tabbrowser.xml.
> KaiRo's patch will remove the remaining callers.
Well, places has now landed, so it doesn't matter, but the point was that Bookmarks calls loadGroup at which point back/forward would have been broken.

Comment 26

[Philip Chee]

>>>>-    openTopWin(viewURL, this.target.ownerDocument.defaultView);
>>> This is needed to avoid XSS from specially crafted resource URIs into the
>>> existing window.
>> Firefox does not believe that this is necessary.
> Well that's because it blocks all sorts of URIs such as -moz-icon: ...
OK. I'm stuck here. I've looked at openTopWin() and I have no idea what it's doing.

If I read the function correctly (highly unlikely) what I should do is something like:

if the URL (!isRestricted())
  do openUILink(.....)
else
  do not pass the referrer and use loadURIWithFlags(url, Components.interfaces.nsIWebNavigation.LOAD_FLAGS_FROM_EXTERNAL)
  either in the current tab or in a new tab depending on middleClickLink.


What does the _top here do in openTopWin? Isn't _top the default anyway?

        else if (topWindowOfType.content == opener.top)
            opener.open(url, "_top");

Comment 27

[[e-mail]]

(In reply to comment #26)
> What does the _top here do in openTopWin? Isn't _top the default anyway?
> 
>         else if (topWindowOfType.content == opener.top)
>             opener.open(url, "_top");
No, _blank is the default.

The easiest thing to do is to call openToWin if whereToOpenLink == "current".

Comment 28

[Philip Chee]

Attachment: Patch v1.3 Address issues.

Carrying forward r=IanN

> (In reply to comment #20)
>> Hmm. Looks like I don't need to QI to nsISHEntry at all.
> Maybe something else is doing it for you, but you can't rely on it.
Fixed.

>> I've removed all the Group navigation stuff on the assumption that Places
>> Booksmarks will land RSN.
> All the group navigation stuff? Including all the callers? (Not read patch
> yet.)

Mooted now that KaiRo's places bookmarks patch has landed.

>>> What is this actually testing for? where == "current" && aEvent.shiftKey?
>> Firefox does this:
> Which means you don't know why... basically, my question was, are you only
> bypassing the cache when the reload would have affected the current page?

If I read the code correctly I'm bypassing the cache only if the reload would have affected the current page && the shift key is pressed && no other modifier keys/middleclicks occur. I don't know if openUILinkIn() bypasses the cache or not.

>>> What's this for? middlemouse.paste is for clicking the middle mouse button into
>>> a content textfield. The page proxy icon has independent behaviour.
>> Firefox Bug 291768 (Middle click on proxy icon in location bar should paste
>> URL).
> Sure, and for whatever reason they wanted this to depend on middlemouse.paste,
> and I see no reason to copy them.
OK. Removed.

>>> What about non-browser throbbers?
>> Most other windows with throbbers don't have a tabbrowser, and the mailnews
>> tabbrowser has a different implementation.
> But the throbber still calls goClickThrobber, doesn't it?
Hmmm. OK. I've added MiddleLinkClick functionality to the three mailnews throbbers that I could find.

>> This should also fix the related SeaMonkey Bug 534248 (Middle-clicking to stop
>> autoscroll may activate contentLoadURL)
> Well to avoid confusion that part of the patch belongs in that bug.
OK. Removed.

>>> This is needed to avoid XSS from specially crafted resource URIs into the
>>> existing window.
>> Firefox does not believe that this is necessary.
> Well that's because it blocks all sorts of URIs such as -moz-icon: ...

OK. I'm stuck here. I've looked at openTopWin() and I have no idea what it's doing.

If I read the function correctly (highly unlikely) what I should do is something like:

  if the URL (!isRestricted())
    do openUILink(.....)
  else
    do not pass the referrer and use loadURIWithFlags(url, ...LOAD_FLAGS_FROM_EXTERNAL)
  either in the current tab or in a new tab depending on middleClickLink.

> The easiest thing to do is to call openToWin if whereToOpenLink == "current".
OK Fixed.

>>> Sadly openUILinkIn mostly ignores the referrerURI, except for new tabs...
>> I assume that this is a parenthetical comment and that it isn't a nit I have to fix.
> No, it's a bug you need to file ;-)
/sigh/

>>>>-    browser.addTab(urlArray[i], {allowThirdPartyFixup: allowThirdPartyFixup});
>>>>+    browser.addTab(urlArray[i], {allowThirdPartyFixup: allowThirdPartyFixup, relatedToCurrent: relatedToCurrent});
>>> Doesn't this assume that insertRelatedAfterCurrent is true?
>> If the user flips the preference I would assume that they would want to ignore
>> relatedToCurrent here as well.
> I guess since it didn't work before the pref existed that it's not unexpected.
Yeah, that too.

Comment 29

[[e-mail]]

(In reply to comment #28)
>>>> What is this actually testing for? where == "current" && aEvent.shiftKey?
>>> Firefox does this:
>> Which means you don't know why... basically, my question was, are you only
>> bypassing the cache when the reload would have affected the current page?
> If I read the code correctly I'm bypassing the cache only if the reload would
> have affected the current page && the shift key is pressed && no other modifier
> keys/middleclicks occur.
Well the other modifier keys/middleclicks would mean it wouldn't have affected the current page, don't they?

>>>> What about non-browser throbbers?
>>> Most other windows with throbbers don't have a tabbrowser, and the mailnews
>>> tabbrowser has a different implementation.
>> But the throbber still calls goClickThrobber, doesn't it?
>Hmmm. OK. I've added MiddleLinkClick functionality to the three mailnews
>throbbers that I could find.
Great. That just leaves Composer ;-)

>>> This should also fix the related SeaMonkey Bug 534248 (Middle-clicking to stop
>>> autoscroll may activate contentLoadURL)
>> Well to avoid confusion that part of the patch belongs in that bug.
> OK. Removed.
Doesn't look removed to me.

Comment 30

[[e-mail]]

Comment on attachment 465800 [details] [diff] [review]
Patch v1.3 Address issues.

>+  else {
>+  	OpenSessionHistoryIn(where, -1, true);
Nit: incorrect indentation (twice)

>+function BrowserReloadOrDuplicate(aEvent)
Don't need this, since you can make the button and menuitem call BrowserReload(event) and leave the key calling BrowserReload()

>+  var key = /Mac/.test(navigator.platform) ? aEvent.metaKey : aEvent.ctrlKey;
>+  if (aEvent.shiftKey && !(aEvent.button == 1 || key)) {
>+    BrowserReloadSkipCache();
My point is, that if you move this to after whereToOpenLink, will where always equal "current" at this point?

>+  var where = !gBrowser ? "window" : whereToOpenLink(aEvent, false, true);
>+  openUILinkArrayIn(homePage, where)
Actually openUILinkArrayIn will create a window if necessary, so you don't need to test for gBrowser.

>+    <command id="Browser:BackOrBackDuplicate"       oncommand="BrowserBack(event);"    observes="canGoBack"/>
>+    <command id="Browser:ForwardOrForwardDuplicate" oncommand="BrowserForward(event);" observes="canGoForward"/>
[Still trying to think up a better way to do this.]

>+    var where = whereToOpenLink(aEvent, false, true);
>+    if (/^tab/.test(where))
>+      where = "tabfocused";
I wonder whether we need an aIgnoreBackground flag for whereToOpenLink.

Comment 31

[[e-mail]]

(In reply to comment #19)
> (From update of attachment 463219 [details] [diff] [review])
> >+    // This code doesn't copy history or work well with framed pages.
> Ask Misak whether Session Restore could be used to clone the tab.
Misak, is it possible to use some of the Session Restore code to duplicate a tab and its session history either as a new tab or as a new window?

Comment 32

[[e-mail]]

Comment on attachment 465800 [details] [diff] [review]
Patch v1.3 Address issues.

>+  openUILinkArrayIn(homePage, where)
Missing semicolon.

>+    <command id="Browser:BackOrBackDuplicate"       oncommand="BrowserBack(event);"    observes="canGoBack"/>
>+    <command id="Browser:ForwardOrForwardDuplicate" oncommand="BrowserForward(event);" observes="canGoForward"/>
So rather than creating two new elements just so that the two menuitems can observe them, you might as well set the oncommand and observes attributes directly on the menuitems in the first places (and remove command).

Comment 33

[Philip Chee]

Attachment: Patch v1.4 fix nits.

>>>>> What is this actually testing for? where == "current" && aEvent.shiftKey?
>>>> Firefox does this:
>>> Which means you don't know why... basically, my question was, are you only
>>> bypassing the cache when the reload would have affected the current page?
>> If I read the code correctly I'm bypassing the cache only if the reload would
>> have affected the current page && the shift key is pressed && no other modifier
>> keys/middleclicks occur.
> Well the other modifier keys/middleclicks would mean it wouldn't have affected
> the current page, don't they?
[....]
>>+  var key = /Mac/.test(navigator.platform) ? aEvent.metaKey : aEvent.ctrlKey;
>>+  if (aEvent.shiftKey && !(aEvent.button == 1 || key)) {
>>+    BrowserReloadSkipCache();
> My point is, that if you move this to after whereToOpenLink, will where always
> equal "current" at this point?

OK I've simplified the test and moved it to after whereToOpenLink() seems to work.

>>>>> What about non-browser throbbers?
>>>> Most other windows with throbbers don't have a tabbrowser, and the mailnews
>>>> tabbrowser has a different implementation.
>>> But the throbber still calls goClickThrobber, doesn't it?
>>Hmmm. OK. I've added MiddleLinkClick functionality to the three mailnews
>>throbbers that I could find.
> Great. That just leaves Composer ;-)
Fixed.

>>>> This should also fix the related SeaMonkey Bug 534248 (Middle-clicking to stop
>>>> autoscroll may activate contentLoadURL)
>>> Well to avoid confusion that part of the patch belongs in that bug.
>> OK. Removed.
> Doesn't look removed to me.
I guess I forgot to hg qrefresh.

>>+  else {
>>+  	OpenSessionHistoryIn(where, -1, true);
> Nit: incorrect indentation (twice)
Fixed.

>>+function BrowserReloadOrDuplicate(aEvent)
> Don't need this, since you can make the button and menuitem call
> BrowserReload(event) and leave the key calling BrowserReload()
Fixed.

>>+  var where = !gBrowser ? "window" : whereToOpenLink(aEvent, false, true);
>>+  openUILinkArrayIn(homePage, where)
> Actually openUILinkArrayIn will create a window if necessary, so you don't need
> to test for gBrowser.
Fixed.

>>+    var where = whereToOpenLink(aEvent, false, true);
>>+    if (/^tab/.test(where))
>>+      where = "tabfocused";
> I wonder whether we need an aIgnoreBackground flag for whereToOpenLink.

I could put this together with the other flags but this would change the order of parameters with respect to Firefox. Tacking it on the end would be odd. I could make it take an optional object but that seems overkill for this use case.

>>+  openUILinkArrayIn(homePage, where)
> Missing semicolon.
Fixed.

>>+    <command id="Browser:BackOrBackDuplicate"       oncommand="BrowserBack(event);"    observes="canGoBack"/>
>>+    <command id="Browser:ForwardOrForwardDuplicate" oncommand="BrowserForward(event);" observes="canGoForward"/>
> So rather than creating two new elements just so that the two menuitems can
> observe them, you might as well set the oncommand and observes attributes
> directly on the menuitems in the first places (and remove command).
Fixed.

Comment 34

[[e-mail]]

(In reply to comment #33)
> > My point is, that if you move this to after whereToOpenLink, will where always
> > equal "current" at this point?
> OK I've simplified the test and moved it to after whereToOpenLink() seems to
> work.
Ah, now I understand! Unlike the old code which we used to use in, say, Bookmarks, whereToOpenLink returns null if you press shift. We could do with a flag in whereToOpenLink ;-)

> > Doesn't look removed to me.
> I guess I forgot to hg qrefresh.
Still doesn't look removed to me.

>>>+    var where = whereToOpenLink(aEvent, false, true);
>>>+    if (/^tab/.test(where))
>>>+      where = "tabfocused";
>> I wonder whether we need an aIgnoreBackground flag for whereToOpenLink.
> I could put this together with the other flags but this would change the order
> of parameters with respect to Firefox. Tacking it on the end would be odd. I
> could make it take an optional object but that seems overkill for this use
> case.
Well, if we only do it once it's OK. If we end up duplicating code though...

Comment 35

[[e-mail]]

(In reply to comment #31)
> (In reply to comment #19)
> > (From update of attachment 463219 [details] [diff] [review])
> > >+    // This code doesn't copy history or work well with framed pages.
> > Ask Misak whether Session Restore could be used to clone the tab.
> Misak, is it possible to use some of the Session Restore code to duplicate a
> tab and its session history either as a new tab or as a new window?
I see that there is already a duplicate tab function in Session Restore, however it can only duplicate into a new tab in an existing window and it can't change the history index. I wonder whether we can improve on that.

Comment 36

[[e-mail]]

Attachment: Proof of concept

This enhances duplicateTab so that the duplicate a) can open in a new window, not just a new tab b) can open to a relative history index. c) is backward compatible (except that it now allows a null window parameter).

Note that I used a relative delta because session store uses a slightly different session history indexing mechanism.

Comment 37

[[e-mail]]

Comment on attachment 467416 [details] [diff] [review]
Patch v1.4 fix nits.

>+  if (where == "current")
>+    BrowserReloadWithFlags(nsIWebNavigation.LOAD_FLAGS_NONE);
>+  else if (where == null && aEvent.shiftKey &&
>+           !(aEvent.metaKey || aEvent.ctrlKey))
I don't think you need to check meta/ctrlKey because where == null, and this would only be true if the user had turned off all the tabbed browsing prefs, and therefore they probably want ctrl+shift+reload to force reload, which is what it would have done before in such a case anyway.

Comment 38

[Philip Chee]

Attachment: Patch v1.5 use sessionStore::duplicateTab() to clone history when opening new tabs/windows.

>>> Ask Misak whether Session Restore could be used to clone the tab.
> I see that there is already a duplicate tab function in Session Restore,
> however it can only duplicate into a new tab in an existing window and it can't
> change the history index. I wonder whether we can improve on that.

> Created attachment 467724 [details] [diff] [review]
> Proof of concept
> 
> This enhances duplicateTab so that the duplicate a) can open in a new window,
> not just a new tab b) can open to a relative history index. c) is backward
> compatible (except that it now allows a null window parameter).
> 
> Note that I used a relative delta because session store uses a slightly
> different session history indexing mechanism.

This patch should be applied on top of the patch in attachment 467724 [details] [diff] [review] as it depends on the enhanced duplicateTab() function.

> (In reply to comment #33)
>>> My point is, that if you move this to after whereToOpenLink, will where always
>>> equal "current" at this point?
>> OK I've simplified the test and moved it to after whereToOpenLink() seems to
>> work.
> Ah, now I understand! Unlike the old code which we used to use in, say,
> Bookmarks, whereToOpenLink returns null if you press shift. We could do with a
> flag in whereToOpenLink ;-)
Hmm. What sort of flag would we need and what would it do?
 
>>> Doesn't look removed to me.
>> I guess I forgot to hg qrefresh.
> Still doesn't look removed to me.
As discussed over IRC the changes to the middleMousePaste() function should be addressed in a separate patch. I've made a slight change to handle event.stopPropagation() as needed for the other changes though.
 
>>>>+    var where = whereToOpenLink(aEvent, false, true);
>>>>+    if (/^tab/.test(where))
>>>>+      where = "tabfocused";
>>> I wonder whether we need an aIgnoreBackground flag for whereToOpenLink.
>> I could put this together with the other flags but this would change the order
>> of parameters with respect to Firefox. Tacking it on the end would be odd. I
>> could make it take an optional object but that seems overkill for this use
>> case.
> Well, if we only do it once it's OK. If we end up duplicating code though...
I've made a minimal change in whereToOpenLink() to take an additional ignoreBackground flag. If Firefox changes we can follow suite as we would only have one internal caller that uses this flag.
 
>>+  if (where == "current")
>>+    BrowserReloadWithFlags(nsIWebNavigation.LOAD_FLAGS_NONE);
>>+  else if (where == null && aEvent.shiftKey &&
>>+           !(aEvent.metaKey || aEvent.ctrlKey))
> I don't think you need to check meta/ctrlKey because where == null, and this
> would only be true if the user had turned off all the tabbed browsing prefs,

Yeah, the reason I checked for meta/ctrlKey is that I noticed that it was possible to turn off all those prefs.

> and therefore they probably want ctrl+shift+reload to force reload, which is
> what it would have done before in such a case anyway.
Ah, good point. Fixed.

Comment 39

[[e-mail]]

Comment on attachment 469001 [details] [diff] [review]
Patch v1.5 use sessionStore::duplicateTab() to clone history when opening new tabs/windows.

[Not tested yet]

>+function OpenSessionHistoryIn(aWhere, aIndex, aIsOffset)
>+{
Should the duplicate tab code go here instead of gotoHistoryIndex?

>+    ss.duplicateTab(win, gBrowser.selectedTab, delta);
This opens in the background, I think; is that ideal?

>-      if (middleMousePaste(event)) {
>-        event.stopPropagation();
>-      }
>+      middleMousePaste(event);
I see, so sometimes middleMousePaste returns false, and sometimes it returns true, and sometimes it calls stopPropagation.

>-      return openNewTabOrWindow(event, url, null);
>+      if (openNewTabOrWindow(event, url, null))
>+        event.stopPropagation();
This doesn't return any more, which seems wrong.

>-    return true;
You'd have to check all the other returns not to return a value.

Comment 40

[Philip Chee]

>>-      return openNewTabOrWindow(event, url, null);
>>+      if (openNewTabOrWindow(event, url, null))
>>+        event.stopPropagation();
> This doesn't return any more, which seems wrong.
Oops right I accidentally deleted one return too many.

Comment 41

[Philip Chee]

Attachment: Patch v1.6 Address review comments.

>>+function OpenSessionHistoryIn(aWhere, aIndex, aIsOffset)
>>+{
> Should the duplicate tab code go here instead of gotoHistoryIndex?

Moved.

>>+    ss.duplicateTab(win, gBrowser.selectedTab, delta);
> This opens in the background, I think; is that ideal?

Fixed using some code borrowed from openUILinkIn().

>>-      if (middleMousePaste(event)) {
>>-        event.stopPropagation();
>>-      }
>>+      middleMousePaste(event);
> I see, so sometimes middleMousePaste returns false, and sometimes it returns
> true, and sometimes it calls stopPropagation.

Erm. Sorry about this.

>>-      return openNewTabOrWindow(event, url, null);
>>+      if (openNewTabOrWindow(event, url, null))
>>+        event.stopPropagation();
> This doesn't return any more, which seems wrong.

Oops right I accidentally deleted one return too many. Fixed.

>>-    return true;
> You'd have to check all the other returns not to return a value.

Fixed so all the returns are void.

Comment 42

[[e-mail]]

Comment on attachment 469828 [details] [diff] [review]
Patch v1.6 Address review comments.

>+function OpenSessionHistoryIn(aWhere, aIndex, aIsOffset)
>+{
>+  if (!aIsOffset)
>+    aIndex -= getWebNavigation().sessionHistory.index;
Please pass in aDelta and compute it in the relevant caller instead.

>+    openUILinkIn(getWebNavigation().currentURI.spec, where,
>+                 {relatedToCurrent: true});
Can we not use OpenSessionHistoryIn(where); here?
[Should nsISessionStore.duplicateTab set relatedToCurrent?]

Comment 43

[Philip Chee]

Attachment: Patch v1.7 r=IanN, sr=Neil [Checked in: Comment 48]

Carrying forward r=IanN sr=Neil

>>+function OpenSessionHistoryIn(aWhere, aIndex, aIsOffset)
[...]
> Please pass in aDelta and compute it in the relevant caller instead.
Fixed.

>>+    openUILinkIn(getWebNavigation().currentURI.spec, where,
>>+                 {relatedToCurrent: true});
> Can we not use OpenSessionHistoryIn(where); here?
Yes we can. Fixed. Now passes isRelated to ss.duplicateTab.

Comment 44

[Philip Chee]

Attachment: Sessionstore Patch Sv1.1 Additional parameter aRelated. r=Misak

Carrying forward r=Misak.

Requesting sr for the additional aRelated parameter.

> [Should nsISessionStore.duplicateTab set relatedToCurrent?]
Now adds an additional optional boolean parameter to duplicateTab() aRelated which is passed directly to addTab(). Note addTab() ANDs aRelatedToCurrent with "browser.tabs.insertRelatedAfterCurrent".

Comment 45

[[e-mail]]

Comment on attachment 470516 [details] [diff] [review]
Sessionstore Patch Sv1.1 Additional parameter aRelated. r=Misak

>+                          .addTab(null, {relatedToCurrent: aRelated});
Nit: file style is { relatedToCurrent: aRelated }

Comment 46

[Philip Chee]

Attachment: Sessionstore Patch Sv1.1a Additional parameter aRelated. r=Misak (sr=Neil for relatedToCurrent) [Checked in: Comment 47]

Patch for checkin. Carrying forward r=Misak sr=Neil

I'm going on a plane trip later today and the tree is on fire at the moment so can't checkin. I'll try to checkin in a day or two unless someone checks these two patches in for me in the meantime.

Comment 47

[Ian Neal]

Comment on attachment 470672 [details] [diff] [review]
Sessionstore Patch Sv1.1a Additional parameter aRelated. r=Misak   (sr=Neil for relatedToCurrent) [Checked in: Comment 47]

http://hg.mozilla.org/comm-central/rev/b6f89d5356df

Comment 48

[Ian Neal]

Comment on attachment 470513 [details] [diff] [review]
Patch v1.7 r=IanN, sr=Neil [Checked in: Comment 48]

http://hg.mozilla.org/comm-central/rev/338c1695a64b

Comment 49

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #48)
> Comment on attachment 470513 [details] [diff] [review]
> Patch v1.7 r=IanN, sr=Neil [Checked in: Comment 48]
> 
> http://hg.mozilla.org/comm-central/rev/338c1695a64b

Oh well, wrong checkin comment. That'll make my next SMTT blog post harder.

Comment 50

[Philip Chee]

> Oh well, wrong checkin comment. That'll make my next SMTT blog post harder.

Mea Culpa. Sorry about that.