Last Comment Bug 429926 - Make Cmd+T open a new window when no browser windows are open
: Make Cmd+T open a new window when no browser windows are open
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal with 1 vote (vote)
: seamonkey2.0a1
Assigned To: Stefan [:stefanh] (away until December 6)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-20 08:36 PDT by Stefan [:stefanh] (away until December 6)
Modified: 2008-05-04 12:33 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
BrowserOpenTab() even when no windows are open (2.42 KB, patch)
2008-04-20 08:39 PDT, Stefan [:stefanh] (away until December 6)
jag-mozilla: review-
neil: superreview+
Details | Diff | Splinter Review
New version (2.49 KB, patch)
2008-04-24 13:17 PDT, Stefan [:stefanh] (away until December 6)
jag-mozilla: review+
neil: superreview+
Details | Diff | Splinter Review

Description Stefan [:stefanh] (away until December 6) 2008-04-20 08:36:19 PDT
The firefox eqivalent is bug 425486 where the Safari behavior was copied (just open a blank window). However, I think we should honor the loadOnNewTab prefs here. Camino actually honor the the "Load homepage when opening a new tab" pref even when you hit Cmd+T when no windows are open.
Comment 1 Stefan [:stefanh] (away until December 6) 2008-04-20 08:39:57 PDT
Created attachment 316692 [details] [diff] [review]
BrowserOpenTab() even when no windows are open
Comment 2 jag (Peter Annema) 2008-04-22 22:57:58 PDT
Comment on attachment 316692 [details] [diff] [review]
BrowserOpenTab() even when no windows are open

>Index: suite/browser/navigator.js
>===================================================================
>         case 2:
>-          uriToLoad = getWebNavigation().currentURI.spec;
>+          uriToLoad = Components.classes["@mozilla.org/browser/global-history;2"]
>+                                .getService(Components.interfaces.nsIBrowserHistory).lastPageVisited;
>           break;

Let's say in tab 1 you go to A and in tab 2 you go to B, in that order. Then switch back to tab 1.

|getWebNavigation.currentURI.spec| is A, |C.c[...].gS(...).lastPageVisited| is B.

Maybe something like:

  uriToLoad = gBrowser ? getWebNavigation().currentURI.spec
                       : Components.classes["@mozilla.org/browser/global-history;2"]
                                   .getService(Components.interfaces.nsIBrowserHistory).lastPageVisited;
Comment 3 Stefan [:stefanh] (away until December 6) 2008-04-24 13:17:25 PDT
Created attachment 317598 [details] [diff] [review]
New version

> Let's say in tab 1 you go to A and in tab 2 you go to B, in that order. Then
> switch back to tab 1.
> 
> |getWebNavigation.currentURI.spec| is A, |C.c[...].gS(...).lastPageVisited| is
> B.

Ugh, I should have thought about that :-/ This does what you suggested. If you feel that this is an overkill, I guess we can just open an empty tab. But it'd be nice to honor the prefs, I think (I tend to think that the Safari behavior is just a symptom of "uh, let's just do it this way since we don't know how to do it the right way").
Comment 4 jag (Peter Annema) 2008-04-24 15:59:21 PDT
Personally I don't really see a use case where you always want the most recently loaded page (i.s.o. the most recently viewed page) to be loaded when opening a new window or tab, or on startup.

For startup I can see the usefulness of the more generic case of "restore whatever pages were open last time the browser shut down (or crashed)" (a.k.a. Session Restore).

I can also see wanting to open a copy of the currently shown page in a new tab. Of course, with no tab/window to duplicate from, the best we can do is "most recently loaded", though I'm kinda leaning towards just loading a blank tab, or adding the code to remember the most recently viewed page, either somewhere new (could we get this for free through Session Restore?), or by updating the value of lastPageVisited when switching tabs.

Just thinking out loud. Discuss ;-)
Comment 5 Stefan [:stefanh] (away until December 6) 2008-04-27 14:43:56 PDT
(In reply to comment #4)
> Personally I don't really see a use case where you always want the most
> recently loaded page (i.s.o. the most recently viewed page) to be loaded when
> opening a new window or tab, or on startup.
> 
> For startup I can see the usefulness of the more generic case of "restore
> whatever pages were open last time the browser shut down (or crashed)" (a.k.a.
> Session Restore).
> 
> I can also see wanting to open a copy of the currently shown page in a new tab.
> Of course, with no tab/window to duplicate from, the best we can do is "most
> recently loaded", though I'm kinda leaning towards just loading a blank tab, or
> adding the code to remember the most recently viewed page, either somewhere new
> (could we get this for free through Session Restore?), or by updating the value
> of lastPageVisited when switching tabs.
> 
> Just thinking out loud. Discuss ;-)
> 

Hmm, I wonder if it doesn't makes more sense to use the unmodified lastPageVisited rather than updating it when switching tabs (on startup and when no browse window is open). After all, that is the last visited page, isn't it? Or maybe users don't think like that (difference between last page you looked at and last page you loaded)?
Comment 6 jag (Peter Annema) 2008-04-28 02:07:18 PDT
Well, that's what I'm wondering. Is switching tabs seen as a visit?

Does lastPageVisited get updated when you go back in history? E.g. I search for something, click one of the links, which takes me to a two page article, I go to the second page, then go back twice and then I open a new window. Assuming the new window pref is "Last page visited", would I expect the article's second page to open, or the search results? How about when I open the article in a second tab, go to the second page, then close the tab, which takes me back to the Google search results?

The answer is "the article's second page" to both, which is not what I would've expected (for the first, at least), though I can reason about it and understand why it's doing it, and maybe in certain cases I can make use of it ("Oh, I closed that tab I just opened, what was it again? Let me open a new window...") as a shortcut for checking the top of my history.

On a side note, reload doesn't seem to update lastPageVisited. Hrm.

For this bug perhaps we should just take the second patch, and let's deal with lastPageVisited in a new bug.
Comment 7 Stefan [:stefanh] (away until December 6) 2008-04-28 04:12:33 PDT
Another side-note: In the "Preferences..." --> "Browser" pane you have the choice of Blank Page, Home Page and Last page visited on Browser startup, New window and New tab.. huh.
 
> For this bug perhaps we should just take the second patch, and let's deal with
> lastPageVisited in a new bug.

Sounds good to me. Want to r+ the patch? :-)

Comment 8 jag (Peter Annema) 2008-04-29 01:40:13 PDT
Comment on attachment 317598 [details] [diff] [review]
New version

I was thinking we could just check gBrowser at the beginning of this function and call OpenBrowserWindow(), but that would use the "new window" pref setting instead of the "new tab" pref setting, which is probably not what we want.

>Index: suite/browser/navigator.js
>===================================================================
>+          uriToLoad = gBrowser ? getWebNavigation().currentURI.spec :
>+                      Components.classes["@mozilla.org/browser/global-history;2"]
>+                                .getService(Components.interfaces.nsIBrowserHistory).lastPageVisited;

Please indent like this:

          uriToLoad = gBrowser ? getWebNavigation().currentURI.spec 
                               : Components.classes["@mozilla.org/browser/global-history;2"]
                                           .getService(Components.interfaces.nsIBrowserHistory)
                                           .lastPageVisited;

Or with no wrapping on 80:

          const HISTORY_CONTRACTID = "@mozilla.org/browser/global-history;2";
          const nsIBrowserHistory = Components.interfaces.nsIBrowserHistory;
          uriToLoad = gBrowser ? getWebNavigation().currentURI.spec 
                               : Components.classes[HISTORY_CONTRACTID]
                                           .getService(nsIBrowserHistory)
                                           .lastPageVisited;

r=jag with at least the indentation nit fixed.
Comment 9 Stefan [:stefanh] (away until December 6) 2008-04-29 11:56:40 PDT
Btw, when testing this (before & after the patch) I noticed that if you hit cmd+t a couple of times and the previous tab hasn't finished loading you'll get a tab with about:blank. Happens on branch too. Hmm.
Comment 10 Stefan [:stefanh] (away until December 6) 2008-05-04 11:25:27 PDT
Landed with fixed indentation:

Checking in suite/browser/navigator.js;
/cvsroot/mozilla/suite/browser/navigator.js,v  <--  navigator.js
new revision: 1.622; previous revision: 1.621
done

Note You need to log in before you can comment on or make changes to this bug.