Klaas Heidstra Assignee
	Description • 16 years ago

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.

	Klaas Heidstra Assignee
	Comment 1 • 16 years ago

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

	(no longer active)
	Comment 2 • 16 years ago

Confirming as an enhancement.

	Klaas Heidstra Assignee
	Comment 3 • 16 years ago

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...?

	Klaas Heidstra Assignee
	Comment 4 • 16 years ago

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.

	Klaas Heidstra Assignee
	Comment 5 • 16 years ago

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

	Dão Gottwald [:dao]
	Comment 6 • 16 years ago

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?

	Dão Gottwald [:dao]
	Comment 7 • 16 years ago

(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.

	Dão Gottwald [:dao]
	Comment 8 • 16 years ago

(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.

	Klaas Heidstra Assignee
	Comment 9 • 16 years ago

> 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);

	Dão Gottwald [:dao]
	Comment 10 • 16 years ago

(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.

	Dão Gottwald [:dao]
	Comment 11 • 16 years ago

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

	Klaas Heidstra Assignee
	Comment 12 • 16 years ago

(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#

	Klaas Heidstra Assignee
	Comment 13 • 16 years ago

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).

	Klaas Heidstra Assignee
	Comment 14 • 16 years ago

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.

	Klaas Heidstra Assignee
	Comment 15 • 16 years ago

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#

	Klaas Heidstra Assignee
	Comment 17 • 16 years ago

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 :).

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 20 • 15 years ago

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

Yeah, this makes sense to me.

	Klaas Heidstra Assignee
	Comment 21 • 15 years ago

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).

	Klaas Heidstra Assignee
	Comment 22 • 15 years ago

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).

	Dão Gottwald [:dao]
	Comment 23 • 15 years ago

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);?

	Klaas Heidstra Assignee
	Comment 24 • 15 years ago

(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.

	Klaas Heidstra Assignee
	Comment 25 • 15 years ago

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

	Klaas Heidstra Assignee
	Comment 26 • 15 years ago

(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.

	Dão Gottwald [:dao]
	Comment 27 • 15 years ago

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.

	Klaas Heidstra Assignee
	Comment 28 • 15 years ago

(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.

	Klaas Heidstra Assignee
	Comment 29 • 15 years ago

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.

	Dão Gottwald [:dao]
	Comment 30 • 15 years ago

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();
}

	Ed Lee :Mardak
	Comment 31 • 15 years ago

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())

	Klaas Heidstra Assignee
	Comment 32 • 15 years ago

(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?

	Klaas Heidstra Assignee
	Comment 34 • 14 years ago

(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

	Dão Gottwald [:dao]
	Comment 35 • 14 years ago

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.

	Klaas Heidstra Assignee
	Comment 36 • 14 years ago

(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);

	Klaas Heidstra Assignee
	Comment 37 • 14 years ago

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

	Dão Gottwald [:dao]
	Comment 38 • 14 years ago

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.

	Klaas Heidstra Assignee
	Comment 39 • 14 years ago

(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.

	eyal gruss
	Comment 40 • 14 years ago

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

	Klaas Heidstra Assignee
	Comment 41 • 14 years ago

(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.

	Dão Gottwald [:dao]
	Comment 42 • 14 years ago

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);

	Klaas Heidstra Assignee
	Comment 43 • 14 years ago

Requesting approval for Firefox 4.

	Klaas Heidstra Assignee
	Comment 44 • 14 years ago

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)".

	Dão Gottwald [:dao]
	Comment 45 • 14 years ago

(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.

	Klaas Heidstra Assignee
	Comment 46 • 14 years ago

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.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 47 • 14 years ago

Still not a blocker, no.

	Klaas Heidstra Assignee
	Comment 48 • 14 years ago

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

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 49 • 14 years ago

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?

	Klaas Heidstra Assignee
	Comment 50 • 14 years ago

(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.

	Misak Khachatryan
	Comment 51 • 14 years ago

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

	Misak Khachatryan
	Comment 52 • 14 years ago

Sorry s/somethign/same thing/

	Dão Gottwald [:dao]
	Comment 53 • 14 years ago

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.

	(no longer active)
	Comment 54 • 14 years ago

Should this be marked as fixed?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 55 • 14 years ago

It hasn't landed has it?

	Klaas Heidstra Assignee
	Comment 56 • 14 years ago

(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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 57 • 14 years ago

Generally we put the hg links in the bug.

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 58 • 14 years ago

(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 :-)

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 59 • 14 years ago

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.