1014323 - Under private mode, every new blank tab closed is recorded in Recent Closed Tabs entry

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Yeuk Hon Wong [:yeukhon]]

Open a private window, open new tab (CMD+T on Mac), close the new tab, check Recent Closed Tabs, the tab just closed is recorded in the list.

There is no point of keeping them. New blank private tab should have been treated as normal blank page about:blank.

Very minor problem.

Similar ticket: https://bugzilla.mozilla.org/show_bug.cgi?id=581937

Comment 1

[scipio134]

I seem to be inept with nightly, because I can not find the "Recent Closed Tabs" menu. Where is it?

Comment 2

[scipio134]

Disregard previous comment.

Comment 3

[J A]

Attachment: Patch.diff

I thought I'd take a stab at this to try to get familiar with contributing to FireFox. Was not sure who to assign the review to, so I just selected what was recommended by Bugzilla.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8436415 [details] [diff] [review]
Patch.diff

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

Hey Jacob, thanks for your patch! The fix itself looks great but we'll need to address some minors details in the test - although it already does all the right things. That's a very solid first contribution, thanks again :)

::: browser/components/sessionstore/test/browser.ini
@@ +191,5 @@
>  [browser_625016.js]
>  skip-if = os == "mac" || e10s
>  
>  [browser_911547.js]
> +[browser_1014323.js]

Please give it a more descriptive name like browser_about_privatebrowsing.js and insert it a little higher above.

::: browser/components/sessionstore/test/browser_1014323.js
@@ +5,5 @@
> +// Tests that an about:privatebrowsing tab with no history will not 
> +// be saved into session store and thus, it will not show up in 
> +// Recently Closed Tabs.
> +
> +function test() {

Please use add_task(), that makes it a lot easier to write asynchronous tests. Just take a look at the other (newer) tests for how to do that.

@@ +9,5 @@
> +function test() {
> +  waitForExplicitFinish();
> +
> +  gPrefService.setIntPref("browser.sessionstore.max_tabs_undo", 0);
> +  gPrefService.clearUserPref("browser.sessionstore.max_tabs_undo");

If you want to clear the list of closed tabs, please call .forgetClosedTab() until .getClosedTabCount() returns 0.

@@ +16,5 @@
> +
> +  var tab = gBrowser.addTab("about:privatebrowsing");
> +  var browser = gBrowser.getBrowserForTab(tab);
> +  browser.addEventListener("load", function() {
> +    browser.removeEventListener("load", arguments.callee, true);

You can use promiseBrowserLoaded() in a task to wait for a browser to load.

Comment 5

[J A]

Attachment: Patch2.diff

Hey Tim, thanks for reviewing my patch. I've gone through your suggestions and have an updated patch attached. Thanks!

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8436533 [details] [diff] [review]
Patch2.diff

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

Thanks, that looks great! r=ttaubert with the test renamed. Please attach the new version of the patch and add the "checkin-needed" keyword. And don't forget to set a proper patch description with bug number and reviewer name :)

::: browser/components/sessionstore/test/browser.ini
@@ +61,1 @@
>  [browser_aboutSessionRestore.js]

Sorry, I should have looked at the list before. I think we should name it browser_aboutPrivateBrowsing.js, like we did with the test for about:sessionrestore.

Comment 7

[J A]

Attachment: Patch2.diff

Thanks Tim!

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/f4d6559cc232

This is now on the fx-team branch and should be merged to mozilla-central today or tomorrow and then be in tomorrow's (or the day after that) Nightly.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Jacob, thanks for your contribution again! If you're interested in helping with some more sessionstore stuff please feel free to talk to me on IRC or over email. We have a few other mentored bugs that just wait for someone to show up! :) There are of course lots of other mentored bugs all over Firefox if you're interested in other areas of code. Bugs Ahoy[1] is a good resource.

[1] http://www.joshmatthews.net/bugsahoy/

Comment 10

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/f4d6559cc232

Comment 11

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF Nightly 33
Build Id:20140624030200
OS: Win 7 x64, Mac Os 10.7.5, Ubuntu 13.04 x64