Last Comment Bug 716080 - Restore Previous Session does not re-use blank tabs
: Restore Previous Session does not re-use blank tabs
Status: RESOLVED FIXED
[good first bug][mentor=zpao][lang=js]
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 13
Assigned To: Piers
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-06 14:19 PST by Ian Nartowicz
Modified: 2012-02-27 23:48 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to nsSessionStore.js which removes tabs with a blank page when a session is restored. (1.59 KB, patch)
2012-02-17 20:15 PST, Piers
paul: review-
Details | Diff | Splinter Review
Patch to nsSessionStore.js which removes tabs with a blank page when a session is restored. (1.54 KB, patch)
2012-02-22 02:09 PST, Piers
paul: review-
Details | Diff | Splinter Review
Patch to nsSessionStore.js which removes tabs with a blank page when a session is restored. (1.48 KB, patch)
2012-02-24 01:31 PST, Piers
paul: review-
Details | Diff | Splinter Review
Patch to nsSessionStore.js which removes tabs with a blank page when a session is restored. (1.48 KB, patch)
2012-02-24 23:13 PST, Piers
paul: review+
Details | Diff | Splinter Review

Description Ian Nartowicz 2012-01-06 14:19:46 PST
User Agent: Mozilla/5.0 (X11; Linux i686; rv:8.0.1) Gecko/20100101 Firefox/8.0.1
Build ID: 20111120135848

Steps to reproduce:

1. Set "When Firefox starts" to "Show a blank page"
2. Open several tabs or windows
3. Close Firefox
4. Open Firefox (one blank tab)
5. Restore previous session


Actual results:

Previous session is restored by the initial blank tab is not re-used.


Expected results:

Tabs set to the configured home page should be re-used for restored tabs.  This works if the home page is an actual URL.
Comment 1 Virgil Dicu [:virgil] [QA] 2012-01-16 05:54:36 PST
Mozilla/5.0 (X11; Linux x86_64; rv:12.0a1) Gecko/20120115 Firefox/12.0a1

This is the default behavior for both the home page and the blank. None of them are re-used after session restore.
Are you sure this worked for you with a URL as home page?
Can you reproduce that in Safe mode?
https://support.mozilla.com/en-US/kb/Safe+Mode
Comment 2 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-01-20 17:19:52 PST
(In reply to Virgil Dicu [QA] from comment #1)
> Mozilla/5.0 (X11; Linux x86_64; rv:12.0a1) Gecko/20120115 Firefox/12.0a1
> 
> This is the default behavior for both the home page and the blank. None of
> them are re-used after session restore.

That's not true.

Session restore will attempt to overwrite your home page(s) - if all open tabs when you press "restore previous session" match your home page prefs exactly, then we'll overwrite them. However we didn't handle the blank tab preference.
Comment 3 Ian Nartowicz 2012-01-27 07:15:43 PST
Indeed.  Session restore is quite brutal about overwriting pages that match the configured home page(s), I was quite surprised.  Yet it doesn't overwrite a blank page when the setting is "Show a blank page".  Reproducible in all the recent versions I've tried, even with new profiles or safe mode.
Comment 4 Piers 2012-02-17 20:15:50 PST
Created attachment 598486 [details] [diff] [review]
Patch to nsSessionStore.js which removes tabs with a blank page when a session is restored.

The problem stems from the fact that the home page setting and a blank start page are two different things. If a user's start page preference is for a blank page (which has a URI of about:blank), this blank tab will never be removed on session restore as 'about:blank' will never match any of the home page URIs. This is the case even when no home page is specified because the home page URI will default to 'about:home'. The only exception is if you explicitly set the home page to 'about:blank', then the blank start tab will be removed.

In my patch I have added a condition to the comparison of open tabs and home pages which checks if the user's start page preference is for a blank page and if the current tab is a blank page. If this condition is true the tab is removed.
Comment 5 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-02-21 11:39:54 PST
Comment on attachment 598486 [details] [diff] [review]
Patch to nsSessionStore.js which removes tabs with a blank page when a session is restored.

Review of attachment 598486 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Piers, thanks for taking this on!

I think you're on the right path, but I'd like to make it more specific. If the homepages are open when the startup pref is to show blank, then I would consider that a deliberate action, and we should keep them open. Only about:blank should go away then. So with that in mind...

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +1685,2 @@
>      let homePages = aWindow.gHomeButton.getHomePage().split("|");
> +    let startupPref = this._prefBranch.getIntPref("startup.page"); 

If we set homePages conditionally then we don't need to change anything in the loop below.
> if (startupPref = blank)
>   homePages = [about:blank]
> else if startupPref = homepage
>   homePages = [homepages]
Comment 6 Ian Nartowicz 2012-02-21 14:28:50 PST
Or to put it a different way, the homepages pref value is irrelevant when startup.page is set to blank.  It is only stored as a relic memory, but is not used as the startup page and should not be treated as a startup page in the session restore code.

I hadn't actually realised that it did this, overwriting the homepages URL even if the startup.page is set to be blank.  I would have considered that to also be a bug and well worth changing.
Comment 7 Piers 2012-02-22 02:09:06 PST
Created attachment 599543 [details] [diff] [review]
Patch to nsSessionStore.js which removes tabs with a blank page when a session is restored.

Thanks for the clarification Paul and Ian. I have attached a new patch for this bug.

I was unsure about the relationship between home pages and the blank page startup option and thought they were two separate functions. Just to understand this a little further, how should the home page button behave if the startup option is set to a blank page? From my testing, the home page button (and its tooltip) will use the URIs specified in the home page text box (or about:home if blank) as opposed to using about:blank. Is this another bug?

Cheers
Comment 8 Ian Nartowicz 2012-02-22 03:12:46 PST
You are correct.  The home page button navigates to the configured URL even if "When Firefox starts" is set to blank.  I believe this is intentional.  There would be very little use in having a button that took you to the place you already were on startup or with a new window.

I think what Paul described is still correct.  If startup.page is blank but someone is on the configured home page then it is because they explicitly pressed a button and they want to be on that page.  So we should leave them there and open the restored session elsewhere.

Is this going to need addressing again for the new tab page?  Presumably that page should be overwritten.  Always?
Comment 9 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-02-22 11:33:55 PST
(In reply to Ian Nartowicz from comment #8)
> You are correct.  The home page button navigates to the configured URL even
> if "When Firefox starts" is set to blank.  I believe this is intentional. 
> There would be very little use in having a button that took you to the place
> you already were on startup or with a new window.

Right.

> Is this going to need addressing again for the new tab page?  Presumably
> that page should be overwritten.  Always?

Yea, it will need to be. I didn't want to pile on & do it here though - it's certainly related but it didn't make sense to handle newtab in some sessionstore cases but not all. The fix for that will happen in bug 717044.
Comment 10 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-02-22 11:41:21 PST
Comment on attachment 599543 [details] [diff] [review]
Patch to nsSessionStore.js which removes tabs with a blank page when a session is restored.

Review of attachment 599543 [details] [diff] [review]:
-----------------------------------------------------------------

After thinking about this some more, I've changed my mind a little bit (sorry!). We should just always overwrite about:blank tabs. With this we'll ignore multiple about:blank tabs only if your startup pref == blank. But if your startup pref == homepages, then we'll never close about:blank. That inconsistency doesn't feel right.

So let's seed homePages with about:blank and then just add pref.homepages if appropriate. You may find Array#concat (https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Array/concat) to be helpful.

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +1687,5 @@
>      let tabbrowser = aWindow.gBrowser;
>      let normalTabsLen = tabbrowser.tabs.length - tabbrowser._numPinnedTabs;
> +    let startupPref = this._prefBranch.getIntPref("startup.page"); 
> +    if (startupPref == 0)
> +      homePages = ['about:blank']

Nit: Use double quotes for consistency with other string literals in the file.
Comment 11 Piers 2012-02-22 21:02:50 PST
Ok great, not a problem Paul.

> So let's seed homePages with about:blank and then just add pref.homepages if
> appropriate. You may find Array#concat

Sorry to drag this on however I just wanted to clarify what you meant by "if appropriate". You are referring to when the user's startup preference is to load their homepages? E.g. when their preference is to load their homepages the array will contain their homepages plus blank, otherwise just blank.

> Nit: Use double quotes for consistency with other string literals in the
> file.

Thanks for the pickup!
Comment 12 Ian Nartowicz 2012-02-23 03:54:43 PST
That's right.  Always add "about:blank" and then add more if they are actually configured to load on startup (ie. startup.page NOT 0).
Comment 13 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-02-23 11:19:23 PST
(In reply to Piers Biddlestone from comment #11)
> > So let's seed homePages with about:blank and then just add pref.homepages if
> > appropriate. You may find Array#concat
> 
> Sorry to drag this on however I just wanted to clarify what you meant by "if
> appropriate". You are referring to when the user's startup preference is to
> load their homepages? E.g. when their preference is to load their homepages
> the array will contain their homepages plus blank, otherwise just blank.

Correct.

Ian: you're totally on the ball. Thanks for being active here!
Comment 14 Piers 2012-02-24 01:31:16 PST
Created attachment 600332 [details] [diff] [review]
Patch to nsSessionStore.js which removes tabs with a blank page when a session is restored.

Thanks again Paul and Ian for the clarification.

The code change is now rather minimal.

Cheers
Comment 15 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-02-24 15:47:38 PST
Comment on attachment 600332 [details] [diff] [review]
Patch to nsSessionStore.js which removes tabs with a blank page when a session is restored.

So close! Just one thing that I should have mentioned last time.

>+    if (startupPref != 0)
>+      homePages = homePages.concat(aWindow.gHomeButton.getHomePage().split("|"));

Sorry to be nitpicky, but I think we should be explicit here and only add the homepages if pref == 1. It's not only clearer of our intentions, but it will also help prevent issues if we ever add any options to startup.page in the future).
Comment 16 Piers 2012-02-24 23:13:47 PST
Created attachment 600626 [details] [diff] [review]
Patch to nsSessionStore.js which removes tabs with a blank page when a session is restored.

Thanks Paul, I have made this change and attached the new patch.
Comment 17 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-02-27 13:38:33 PST
Comment on attachment 600626 [details] [diff] [review]
Patch to nsSessionStore.js which removes tabs with a blank page when a session is restored.

Review of attachment 600626 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. Thanks Piers!
Comment 18 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-02-27 13:44:42 PST
Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/782a97166d7c
Comment 19 Tim Taubert [:ttaubert] 2012-02-27 23:48:56 PST
https://hg.mozilla.org/mozilla-central/rev/782a97166d7c

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