448546 - When middle-clicking back/forward/reload, the new tab should inherit history from the tab that opened it (using duplicateTab)

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement)

Description

[Klaas Heidstra]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a2pre) Gecko/2008073002 Minefield/3.1a2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a2pre) Gecko/2008073002 Minefield/3.1a2pre

From bug 263942 comment #48 :
> Hang on - there are much better ways to duplicate a tab!
> 
> Bug 393716 added the duplicateTab method (documented at
> http://developer.mozilla.org/en/docs/nsISessionStore ) which will duplicate a
> tab including it's history, scroll position, form contents, etc.
> 
> This method can be used directly for the "tab" and "tabshifted" cases (for the
> foreground tab case you'll have to set selectedTab manually). For the window
> case, while duplicateTab can copy tabs to different windows, it currently
> requires the window to already be open. The cleanest approach is probably to
> extend duplicateTab so that it creates a new window if the passed window is
> null. This would also be useful to Bug 225680 / extensions wanting to allow
> tabs to be "detached" as new windows.
> 
> Note however that the session service requires the browser.sessionstore.enabled
> preference to be true (the default); if not we should fall back on your
> existing code which just copies the uri (and this can continue to handle the
> "save" case).
> 
> I'm happy to have a go at implementing this if you want.

After the tab is duplicated it should reload/go back/go forward(depends on which button was middle/ctrl-clicked). This behavior could also be applied to other items such as home-button/bookmarks/hyperlinks in webpages, but that is for another bug.  

Reproducible: Always

Steps to Reproduce:
1. Go to a page and click on a link.
2. Now middle-click the back-button.
Actual Results:  
A new tab is opened with the url of the "back"-page without restoring history, scroll position, form contents, etc.

Expected Results:  
Middle-click on the back-button should clone the current tab and then go back one page.

Comment 1

[Klaas Heidstra]

When this bug gets fixed bug 225680 can very easily be fixed so I made that bug depend on this one.

Comment 2

[(no longer active)]

Confirming as an enhancement.

Comment 3

[Klaas Heidstra]

Would it be an idea to add a "duplicateTabIn" wich works the same way as openUILinkIn in utilityOverlay.js, but instead duplicates a tab to a new window, tab etc...?

Comment 4

[Klaas Heidstra]

Attachment: patch - add a duplicateTabIn() to utilityOverlay.js for use with back/forward/reload

This adds a duplicateTabIn to utilityOverlay.js it works mostly the same as openUiLinkIn, but duplictaes a tab in a place specified by the parameter "where" (instead of just opening a link). After the tab is duplicated, a function is executed which can do "stuff" with the newly created tab (e.g. when you want to reload the new tab you can attach a function that does something like: getBrowserForTab(newTab).reload()). You can also provide a fallbackUrl which in this case is used for the back and forward buttons, when win.getBrowserForTab(newTab).goBack(); fails it opens the "back"-url in "fallbackUrl".

There is a problem when you middle-click reload pages with anchors in it's address(e.g. http://www.bla.com/#bla), it scrolls to the last position where it should scroll to the anchor. This is because the load event fires before page scrolling happens, I'm not sure how to solve this.

Comment 5

[Klaas Heidstra]

Dão could you say if I'm going somewhat in the right direction with this patch?

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 332001 [details] [diff] [review]
patch - add a duplicateTabIn() to utilityOverlay.js for use with back/forward/reload

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -1345,37 +1345,45 @@ function gotoHistoryIndex(aEvent)

>     var sessionHistory = getWebNavigation().sessionHistory;
>     var entry = sessionHistory.getEntryAtIndex(index, false);
>     var url = entry.URI.spec;
>-    openUILinkIn(url, where);
>+    var gotoIndexDuplicatedTab = function(newTab) {
>+      var win = newTab.ownerDocument.defaultView.getBrowser();
>+      win.getBrowserForTab(newTab).gotoIndex(index);
>+    };
>+    duplicateTabIn(getBrowser().selectedTab, where, gotoIndexDuplicatedTab, url);

> 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);
>+    var goForwardDuplicatedTab = function(newTab) {
>+      var win = newTab.ownerDocument.defaultView.getBrowser();
>+      win.getBrowserForTab(newTab).goForward();
>+    };
>+    duplicateTabIn(getBrowser().selectedTab, where, goForwardDuplicatedTab, url);

> 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);
>+    var goBackDuplicatedTab = function(newTab) {
>+      var win = newTab.ownerDocument.defaultView.getBrowser();
>+      win.getBrowserForTab(newTab).goBack();  
>+    };
>+    duplicateTabIn(getBrowser().selectedTab, where, goBackDuplicatedTab, url);

You should be able to share more of this stuff in the helper function. I think this should also eliminate the need to pass a callback function.

Always use gBrowser instead of getBrowser().

> function BrowserHandleBackspace()

>-    openUILinkIn(getWebNavigation().currentURI.spec, where);
>+    var reloadDuplicatedTab = function(newTab) {
>+      var win = newTab.ownerDocument.defaultView.getBrowser();
>+      win.getBrowserForTab(newTab).reload();
>+    };
>+    duplicateTabIn(getBrowser().selectedTab, where, reloadDuplicatedTab);

Why are you reloading here?

Comment 7

[Dão Gottwald [:dao]]

(In reply to comment #6)
> > function BrowserHandleBackspace()
> 
> >-    openUILinkIn(getWebNavigation().currentURI.spec, where);
> >+    var reloadDuplicatedTab = function(newTab) {
> >+      var win = newTab.ownerDocument.defaultView.getBrowser();
> >+      win.getBrowserForTab(newTab).reload();
> >+    };
> >+    duplicateTabIn(getBrowser().selectedTab, where, reloadDuplicatedTab);
> 
> Why are you reloading here?

Oops. That's BrowserReloadOrDuplicate, not BrowserHandleBackspace. Still, why are you reloading? It's reload /or/ duplicate.

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #6)
So... pass an index instead of a callback (null/undefined for doing nothing).
Then, if you drop the "save" case, you can remove the url parameter.

Comment 9

[Klaas Heidstra]

> Oops. That's BrowserReloadOrDuplicate, not BrowserHandleBackspace. Still, why
> are you reloading? It's reload /or/ duplicate.

When people middle-click the reload button they expect that a duplicated "reloaded" tab will appear. Just duplicating a tab is something else, that can be provide by a shortcut key. 

> So... pass an index instead of a callback (null/undefined for doing nothing).
> Then, if you drop the "save" case, you can remove the url parameter.

Besides the save case a fallback url is also needed when gotoIndex fails (e.g. when browser.sessionstore.enabled is false), otherwise it would just load the current url in a new tab instead of the url from the index.

The main reason I'm using a callback function is because in the future duplicateTabIn could also be used for other ui-links, for example when you middle-click on a bookmark you could use something like this:
> var loadURIDuplicatedTab = function(newTab) {
>   var win = newTab.ownerDocument.defaultView.gBrowser;
>   win.getBrowserForTab(newTab).loadURI(bookmarkUrl);
> };
> duplicateTabIn(gBrowser.selectedTab, where, loadURIDuplicatedTab);

Comment 10

[Dão Gottwald [:dao]]

(In reply to comment #9)
> > Oops. That's BrowserReloadOrDuplicate, not BrowserHandleBackspace. Still, why
> > are you reloading? It's reload /or/ duplicate.
> 
> When people middle-click the reload button they expect that a duplicated
> "reloaded" tab will appear.

Well, I wouldn't. I don't think users would be too fond of the word "load" when intentionally using a modifier key or the middle mouse button to duplicate a tab. In fact, making the page not reload seems to be the best part of this bug to me; I'm not sure I like the idea of duplicating the history.

> Besides the save case a fallback url is also needed when gotoIndex fails (e.g.
> when browser.sessionstore.enabled is false), otherwise it would just load the
> current url in a new tab instead of the url from the index.

In that case you can still determine the url in the function as currently done outside of it, right?

> The main reason I'm using a callback function is because in the future
> duplicateTabIn could also be used for other ui-links, for example when you
> middle-click on a bookmark you could use something like this:
> > var loadURIDuplicatedTab = function(newTab) {
> >   var win = newTab.ownerDocument.defaultView.gBrowser;
> >   win.getBrowserForTab(newTab).loadURI(bookmarkUrl);
> > };
> > duplicateTabIn(gBrowser.selectedTab, where, loadURIDuplicatedTab);

I don't see why someone would want to have the history of a potentially unrelated tab inherited when opening a bookmark in a new tab. Let's not try to make this work for use cases that don't exist yet.

Comment 11

[Dão Gottwald [:dao]]

Anyway, as for the implementation details, I don't think now is the time to discuss this any further. Please get ui-review first.

Comment 12

[Klaas Heidstra]

(In reply to comment #11)
> Anyway, as for the implementation details, I don't think now is the time to
> discuss this any further. Please get ui-review first.

You're right, implementation details can be discussed here:
http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/76f02c8fb692701b#

Comment 13

[Klaas Heidstra]

Attachment: patch v2 - add a historyIndex to the duplicateTabIn from bug 225680

This patch applies on top of the one from bug 225680, I'm not sure this functionality is really wanted (maybe I should start a discussion about that somewhere).

Comment 14

[Klaas Heidstra]

For some reason bugs that ask for the same thing as this bug were RESOLVED DUPLICATE of bug 18808 (which is in fact about cloning an entire window), so I made that bug depend on this one.

Comment 15

[Klaas Heidstra]

There is a discussion about wether or not this is a good idea in:
http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/701b4f54f135573d#

Comment 17

[Klaas Heidstra]

Attachment: patch v2.1 - update to latest patch in 225680

This is just an update to the new patch in bug 225680, there are no functional changes. Oh, and Gavin Sharp thanks for the editbugs rights :).

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 337354 [details] [diff] [review]
patch v2.1 - update to latest patch in 225680

Yeah, this makes sense to me.

Comment 21

[Klaas Heidstra]

Attachment: patch v3 - updated patch opens new tabs next to current

It seems this bug got ui-review+, so here is a new patch.

The duplicateTabIn function from some patch in bug 225680 is now in this patch. Instead of opening newly created tabs on the last position of the tabbar, they now get moved next to the current one (that's the current behaviour for new tabs in the latest nightlies).

Comment 22

[Klaas Heidstra]

Attachment: patch v4- use _lastRelatedTab from bug 465673 for new tabs

Instead of just placing the tab next to the current one, this patch now places it next to the last related tab (see: bug 465673).

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 400463 [details] [diff] [review]
patch v4- use _lastRelatedTab from bug 465673 for new tabs

>+function duplicateTabIn(aTab, where, historyIndex) {
>+  var browserWin; // parent window of the new tab
>+  var win; // browserWin.gBrowser

This variable is unused. Also, it doesn't make sense to call a reference to gBrowser "win".

>+    var newTabPos = win.mTabs.length - 1;
>+    // New tabs get moved next to last related tab.
>+    if (win.mPrefs.getBoolPref("browser.tabs.insertRelatedAfterCurrent") &&
>+        (where != "window"))
>+      newTabPos = (win._lastRelatedTab || aTab)._tPos + 1;
>+    win.moveTabTo(newTab, newTabPos);
>+    win._lastRelatedTab = newTab;

This shouldn't be implemented here. Instead, I think duplicateTab should pass {relatedToCurrent: true} to addTab, in case the original tab is selected.

>+      newTab.addEventListener("load", newTabOnload, false);

This doesn't make sense. The tab doesn't have a load event, except maybe for the favicon.


>+    case "tab":
>+      browserWin = aTab.ownerDocument.defaultView;
>+      duplicateATab(browserWin.gBrowser, loadInBackground);

Any reason why this isn't just duplicateATab(gBrowser, loadInBackground);?

Comment 24

[Klaas Heidstra]

Attachment: patch v5 - address review comments

(In reply to comment #23)
> (From update of attachment 400463 [details] [diff] [review])
> >+function duplicateTabIn(aTab, where, historyIndex) {
> >+  var browserWin; // parent window of the new tab
> >+  var win; // browserWin.gBrowser
> 
> This variable is unused. Also, it doesn't make sense to call a reference to
> gBrowser "win".

This is removed now, as it was indeed useless.  
 
> This shouldn't be implemented here. Instead, I think duplicateTab should pass
> {relatedToCurrent: true} to addTab, in case the original tab is selected.

relatedToCurrent is now passed via duplicateTab.

> >+      newTab.addEventListener("load", newTabOnload, false);
> 
> This doesn't make sense. The tab doesn't have a load event, except maybe for
> the favicon.

The load eventListener is now attached to the gBrowser object of the new tabs parent window.

> >+    case "tab":
> >+      browserWin = aTab.ownerDocument.defaultView;
> >+      duplicateATab(browserWin.gBrowser, loadInBackground);
> 
> Any reason why this isn't just duplicateATab(gBrowser, loadInBackground);?

There is not a real good reason, so this is now changed to "duplicateATab(gBrowser, loadInBackground);" in this patch.

Comment 25

[Klaas Heidstra]

Attachment: patch v5.1 - forgotten to add aRelatedToCurrent to nsISessionStore.idl

In the previous patch I didn't add the new aRelatedToCurrent param to duplicateTab in nsISessionStore.idl. This is fixed with this patch.

Comment 26

[Klaas Heidstra]

(In reply to comment #25)
> Created an attachment (id=401585) [details]
> patch v5.1 - forgotten to add aRelatedToCurrent to nsISessionStore.idl

> +          let aTabGBrowser = aTab.ownerDocument.defaultView.gBrowser;
> +          let sessionHistory = aTabWin.webNavigation.sessionHistory;
> +          let entry = sessionHistory.getEntryAtIndex(historyIndex, false);

In the above code aTabWin ofcourse must be aTabGBrowser.

Comment 27

[Dão Gottwald [:dao]]

Comment on attachment 401585 [details] [diff] [review]
patch v5.1 - forgotten to add aRelatedToCurrent to nsISessionStore.idl

>       <method name="duplicateTab">
>         <parameter name="aTab"/><!-- can be from a different window as well -->
>+        <parameter name="aRelatedToCurrent"/>

>-  duplicateTab: function sss_duplicateTab(aWindow, aTab) {
>+  duplicateTab: function sss_duplicateTab(aWindow, aTab, aRelatedToCurrent) {

I don't think these changes are needed. Just decide that it's related when aTab == selectedTab.

>+function duplicateTabIn(aTab, where, historyIndex) {

That function belongs in browser.js, I think.

>+  switch (where) {
>+    case "window":
>+      let sa = Components.classes["@mozilla.org/supports-array;1"].
>+               createInstance(Components.interfaces.nsISupportsArray);
>+      sa.AppendElement(null); // wuri
>+      sa.AppendElement(null); // charset
>+      sa.AppendElement(null); // referrerUrl
>+      sa.AppendElement(null); // postData
>+      sa.AppendElement(null); // allowThirdPartyFixup
>+
>+      let ww = Components.classes["@mozilla.org/embedcomp/window-watcher;1"].
>+               getService(Components.interfaces.nsIWindowWatcher);
>+      const browserWin = ww.openWindow(getTopWin(),
>+                                       getBrowserURL(),
>+                                       null,
>+                                       "chrome,dialog=no,all",
>+                                       sa);
>+
>+      function browserWinOnload(event) {
>+        browserWin.removeEventListener("load", browserWinOnload, false);
>+        setTimeout(function() {
>+          duplicateATab(browserWin.gBrowser);
>+        }, 0);
>+      }
>+      browserWin.addEventListener("load", browserWinOnload, false);

This is all a huge hack. I think we should either duplicate the tab and then use gBrowser.replaceTabWithWindow or pass the tab to the new window and let that window duplicate it.

Comment 28

[Klaas Heidstra]

Attachment: patch v6 - addresses issues from comment 27

(In reply to comment #27)
> (From update of attachment 401585 [details] [diff] [review])
> I don't think these changes are needed. Just decide that it's related when aTab
> == selectedTab.
Done.

> >+function duplicateTabIn(aTab, where, historyIndex) {
> 
> That function belongs in browser.js, I think.
The reason I put it in utilityOverlay.js is because similar functions like openUILinkIn are also in there. But I now have moved it to browser.js.

> This is all a huge hack. I think we should either duplicate the tab and then
> use gBrowser.replaceTabWithWindow or pass the tab to the new window and let
> that window duplicate it.
In this patch I have chosen for the "gBrowser.replaceTabWithWindow" approach.

Comment 29

[Klaas Heidstra]

Attachment: patch v6.1 - fix problem with new windows

The previous patch didn't go back when you shift middle clicked the back button, to open in a new window. To solve this problem I now call the replaceTabWithWindow function after the tab has navigated to a history index.

Comment 30

[Dão Gottwald [:dao]]

Comment on attachment 403156 [details] [diff] [review]
patch v6.1 - fix problem with new windows

>+    if (where != "window" && !loadInBackground) {
>+      // If the new tab gets closed, select the previous selected tab.
>+      newTab.owner = gBrowser.selectedTab;
>+      function attrChanged(event) {
>+        if (event.attrName == "selectedIndex" &&
>+            event.prevValue != event.newValue)
>+          gBrowser.resetOwner(parseInt(event.prevValue));
>+      }
>+      if (!gBrowser.mTabChangedListenerAdded) {
>+        gBrowser.mTabBox.addEventListener("DOMAttrModified", attrChanged,
>+                                          false);
>+        gBrowser.mTabChangedListenerAdded = true;
>+      }
>+      // Select the newly created tab.
>+      gBrowser.selectedTab = newTab;
>+    }

This whole block looks like it shouldn't be needed...

>--- a/browser/components/sessionstore/src/nsSessionStore.js	Thu Sep 10 09:10:30 2009 +0100
>+++ b/browser/components/sessionstore/src/nsSessionStore.js	Sun Sep 27 18:59:51 2009 -0500
>@@ -947,8 +947,9 @@
>     var tabState = this._collectTabData(aTab, true);
>     var sourceWindow = aTab.ownerDocument.defaultView;
>     this._updateTextAndScrollDataForTab(sourceWindow, aTab.linkedBrowser, tabState, true);
>-    
>-    var newTab = aWindow.getBrowser().addTab();
>+
>+    let aRelatedToCurrent = aTab == aWindow.gBrowser.selectedTab ? true : false;
>+    var newTab = aWindow.gBrowser.addTab(null, {relatedToCurrent: aRelatedToCurrent});

I think you need to set the owner here, e.g. like this:

if (aTab == aWindow.gBrowser.selectedTab) {
  var newTab = aWindow.gBrowser.addTab(null, {relatedToCurrent: true,
                                              ownerTab: aTab});
} else {
  newTab = aWindow.gBrowser.addTab();
}

Comment 31

[Ed Lee :Mardak]

Does the patch here also fix bug 18808 for the case of opening links in a new tab and not just for back/forward? Or it's for a separate ui-review decision for that behavior?

Klaas, do you need any help updating the patch perhaps from some sessionstore people? What's the status of it now?

"Tab History" add-on https://addons.mozilla.org/en-US/firefox/addon/1859 seems to implement this pretty briefly with a loop to to.addEntry(from.getEntry())

Comment 32

[Klaas Heidstra]

(In reply to comment #31)
> Does the patch here also fix bug 18808 for the case of opening links in a new
> tab and not just for back/forward? Or it's for a separate ui-review decision
> for that behavior?
No it doesn't. That behavior could be achieved with something like ctrl+shift+n, see: bug 455722.

> Klaas, do you need any help updating the patch perhaps from some sessionstore
> people? What's the status of it now?
> 
> "Tab History" add-on https://addons.mozilla.org/en-US/firefox/addon/1859 seems
> to implement this pretty briefly with a loop to to.addEntry(from.getEntry())
The patch should be updated with the comments from comment #30 addressed. Unfortunately I haven't really got time to work on this anytime soon, so maybe someone else could finish this patch?

Comment 34

[Klaas Heidstra]

Attachment: patch v7 - address review comments and update to tip

(In reply to comment #30)
> This whole block looks like it shouldn't be needed...
This is removed now. Also "gBrowser.addEventListener("load", gBrowserOnload, false);" isn't needed anymore, so that is removed too. 

> I think you need to set the owner here, e.g. like this:
> 
> if (aTab == aWindow.gBrowser.selectedTab) {
>   var newTab = aWindow.gBrowser.addTab(null, {relatedToCurrent: true,
>                                               ownerTab: aTab});
> } else {
>   newTab = aWindow.gBrowser.addTab();
> }
Done (although I've used a little different syntax).

DuplicateTab in tabbrowser.xml is modified in a similar way:
> +              return aTab == this.mCurrentBrowser.selectedTab ?
> +                this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec,
> +                                {relatedToCurrent: true, ownerTab: aTab}) :
> +                this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec);

The helper function from the previous patch (duplicateATab) wasn't really needed and is removed.

There are some problems caused by limitations of SessionStore:
* The state of HTML5 video isn't restored when duplicating a tab (the same applies to plugins)
* Only the scroll position of the current page gets restored. This means that when you middle-click the back button, the new tab's scroll position will always be at the top

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 463323 [details] [diff] [review]
patch v7 - address review comments and update to tip

>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -1881,22 +1881,26 @@
>       </method>
> 
>       <method name="duplicateTab">
>         <parameter name="aTab"/><!-- can be from a different window as well -->
>         <body>
>           <![CDATA[
>             // try to have SessionStore duplicate the given tab
>             try {
>-              var ss = Components.classes["@mozilla.org/browser/sessionstore;1"]
>-                                 .getService(Components.interfaces.nsISessionStore);
>+              const ss = Components.classes["@mozilla.org/browser/sessionstore;1"]
>+                                   .getService(Components.interfaces.nsISessionStore);
>               return ss.duplicateTab(window, aTab);
>-            } catch (ex) {
>+            }
>+            catch (ex) {

Avoid these changes.

>               // fall back to basic URL copying
>-              return this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec);
>+              return aTab == this.mCurrentBrowser.selectedTab ?
>+                this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec,
>+                                {relatedToCurrent: true, ownerTab: aTab}) :
>+                this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec);

aTab == this.mCurrentBrowser.selectedTab is never going to be true, as mCurrentBrowser doesn't have such a property. Also, loadOneTab doesn't have an "ownerTab" parameter. It sets the owner tab automatically instead.

Comment 36

[Klaas Heidstra]

Attachment: patch v7.1 - address issues from comment #35

(In reply to comment #35)

> Avoid these changes.
Okay.

> aTab == this.mCurrentBrowser.selectedTab is never going to be true, as
> mCurrentBrowser doesn't have such a property. Also, loadOneTab doesn't have an
> "ownerTab" parameter. It sets the owner tab automatically instead.
I've changed it to:
+              return aTab == this.selectedTab ?
+                this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec,
+                                {relatedToCurrent: true}) :
+                this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec);

Comment 37

[Klaas Heidstra]

Dão what's your opinion about the latest patch?

Comment 38

[Dão Gottwald [:dao]]

Comment on attachment 464388 [details] [diff] [review]
patch v7.1 - address issues from comment #35

>+function duplicateTabIn(aTab, where, historyIndex) {
>+  let newTab = gBrowser.duplicateTab(aTab);
>+
>+  // Go to index if it's provided, fallback to loadURI if there's no history.
>+  if (historyIndex != null) {
>+      try {

fix indentation

>+      catch (ex) {
>+        let aTabGBrowser = aTab.ownerDocument.defaultView.gBrowser;
>+        let sessionHistory = aTabGBrowser.webNavigation.sessionHistory;

aTab.linkedBrowser.sessionHistory

>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -1886,17 +1886,20 @@
>           <![CDATA[
>             // try to have SessionStore duplicate the given tab
>             try {
>               var ss = Components.classes["@mozilla.org/browser/sessionstore;1"]
>                                  .getService(Components.interfaces.nsISessionStore);
>               return ss.duplicateTab(window, aTab);
>             } catch (ex) {
>               // fall back to basic URL copying
>-              return this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec);
>+              return aTab == this.selectedTab ?
>+                this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec,
>+                                {relatedToCurrent: true}) :
>+                this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec);
>             }
>           ]]>
>         </body>
>       </method>

It doesn't look like this should be using loadOneTab at all, as this would select the tab depending on browser.tabs.loadInBackground, whereas ss.duplicateTab would never select the tab.
More importantly, the whole try/catch thing seems bogus -- ss.duplicateTab shouldn't fail here.

Comment 39

[Klaas Heidstra]

Attachment: patch v8 - address issues from comment #38

(In reply to comment #38)
> fix indentation
Done.

> aTab.linkedBrowser.sessionHistory
Done.

> It doesn't look like this should be using loadOneTab at all, as this would
> select the tab depending on browser.tabs.loadInBackground, whereas
> ss.duplicateTab would never select the tab.
> More importantly, the whole try/catch thing seems bogus -- ss.duplicateTab
> shouldn't fail here.
I've removed the try/catch and only kept the part that was in the try block.

Comment 40

[eyal gruss]

does this work also for middle-clicking on a link from the back-forward drop-down history menu?

Comment 41

[Klaas Heidstra]

(In reply to comment #40)
> does this work also for middle-clicking on a link from the back-forward
> drop-down history menu?

Yes, it does.

Comment 42

[Dão Gottwald [:dao]]

Comment on attachment 469242 [details] [diff] [review]
patch v8 - address issues from comment #38

>+function gotoHistoryIndex(aEvent) {
>+  let index = aEvent.target.getAttribute("index");
>   if (!index)
>     return false;
> 
>-  var where = whereToOpenLink(aEvent);
>+  let where = whereToOpenLink(aEvent);
> 
>   if (where == "current") {
>-    // Normal click.  Go there in the current tab and update session history.
>+    // Normal click. Go there in the current tab and update session history.
> 
>     try {
>       gBrowser.gotoIndex(index);
>     }
>     catch(ex) {
>       return false;
>     }
>     return true;
>   }
>   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});
>+    // Modified click. Go there in a new tab/window.
>+
>+    duplicateTabIn(gBrowser.selectedTab, where, index);
>     return true;
>   }

Remove 'else {' here, as the function exits in the if branch.

>+function duplicateTabIn(aTab, where, historyIndex) {
>+  let newTab = gBrowser.duplicateTab(aTab);
...
>+  switch (where) {
>+    case "window":
>+      gBrowser.replaceTabWithWindow(newTab);

This performs pretty badly. Can you call gBrowser.hideTab before calling replaceTabWithWindow so that the tab won't sit in the current window apparently waiting for the new window to open?

>       <method name="duplicateTab">
>         <parameter name="aTab"/><!-- can be from a different window as well -->
>         <body>
>           <![CDATA[
>-            // try to have SessionStore duplicate the given tab
>-            try {
>-              var ss = Components.classes["@mozilla.org/browser/sessionstore;1"]
>-                                 .getService(Components.interfaces.nsISessionStore);
>-              return ss.duplicateTab(window, aTab);
>-            } catch (ex) {
>-              // fall back to basic URL copying
>-              return this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec);
>-            }
>+            var ss = Components.classes["@mozilla.org/browser/sessionstore;1"]
>+                               .getService(Components.interfaces.nsISessionStore);
>+            return ss.duplicateTab(window, aTab);

You can still trim this a bit:

return Cc["@mozilla.org/browser/sessionstore;1"]
         .getService(Ci.nsISessionStore)
         .duplicateTab(window, aTab);

Comment 43

[Klaas Heidstra]

Attachment: patch v8.1 - final patch address issues from comment #42

Requesting approval for Firefox 4.

Comment 44

[Klaas Heidstra]

Comment on attachment 470611 [details] [diff] [review]
patch v8.1 - final patch address issues from comment #42

(In reply to comment #42)
> This performs pretty badly. Can you call gBrowser.hideTab before calling
> replaceTabWithWindow so that the tab won't sit in the current window apparently
> waiting for the new window to open?

Apparently you can't move a hidden (by gBrowser.hideTab(newTab)) tab to a new window. Maybe the performance problem could be handled in a different bug (by making it possible to replace a hidden tab with a window)?

This means the latest patch should be without "gBrowser.hideTab(newTab)".

Comment 45

[Dão Gottwald [:dao]]

(In reply to comment #44)
> Apparently you can't move a hidden (by gBrowser.hideTab(newTab)) tab to a new
> window.

Why would it fail? It worked for me when I tested it.

Comment 46

[Klaas Heidstra]

Comment on attachment 470611 [details] [diff] [review]
patch v8.1 - final patch address issues from comment #42

(In reply to comment #45)
> (In reply to comment #44)
> > Apparently you can't move a hidden (by gBrowser.hideTab(newTab)) tab to a new
> > window.
> 
> Why would it fail? It worked for me when I tested it.

I've just discovered something went wrong with building the source. Which meant that I was somehow testing with the previous patch applied. But still strange shift+back button didn't create a new window. It did duplicate the tab cause I could see it using the new tab groups icon.

But re-asking approval because you've tested it :P.

Comment 47

[Mike Beltzner [:beltzner, not reading bugmail]]

Still not a blocker, no.

Comment 48

[Klaas Heidstra]

Is there still a chance to get this in for Firefox 4?

Comment 49

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 470611 [details] [diff] [review]
patch v8.1 - final patch address issues from comment #42

a=beltzner, but it'll bounce if there's a regression

Klaas: can you write tests for this, too?

Comment 50

[Klaas Heidstra]

Attachment: Updated to tip for check-in [r=dao, ui-r+a=beltzner]

(In reply to comment #49)
> Comment on attachment 470611 [details] [diff] [review]
> patch v8.1 - final patch address issues from comment #42
> 
> a=beltzner, but it'll bounce if there's a regression
> 
> Klaas: can you write tests for this, too?

Dão, do you have an idea what sort of tests need to be written for this. Setting checkin-needed, but this bug shouldn't be resolved until tests are added.

Comment 51

[Misak Khachatryan]

Bug 529240 does somethign for SeaMonkey, but slightly different way. You might be interested how duplicateTab was modified in this patch https://bugzilla.mozilla.org/attachment.cgi?id=470672

Comment 52

[Misak Khachatryan]

Sorry s/somethign/same thing/

Comment 53

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/dfbf5d853b89

(In reply to comment #50)
> Dão, do you have an idea what sort of tests need to be written for this.

A basic test would create a tab with history, call duplicateTabIn with tab/tabshifted and see what it does.

(In reply to comment #51)
> Bug 529240 does somethign for SeaMonkey, but slightly different way. You might
> be interested how duplicateTab was modified in this patch

Might be worth filing a new bug to see if we can simplify our code.

Comment 54

[(no longer active)]

Should this be marked as fixed?

Comment 55

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

It hasn't landed has it?

Comment 56

[Klaas Heidstra]

(In reply to comment #54)
> Should this be marked as fixed?

I guess not until tests are added, see comment #50.

(In reply to comment #55)
> It hasn't landed has it?

Yes it has.

Comment 57

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Generally we put the hg links in the bug.

http://hg.mozilla.org/mozilla-central/rev/dfbf5d853b89

Comment 58

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #57)
> Generally we put the hg links in the bug.
> 
> http://hg.mozilla.org/mozilla-central/rev/dfbf5d853b89

Oh, apparently I'm just blind :-)

Comment 59

[:Gavin Sharp [email: [e-mail]]]

Is someone working on tests for this? Leaving the bug open indefinitely while the patch has landed is confusing. We need to resolve this bug, and also get some tests landed.