856043 - Create tests for no Restore Previous Session options in private windows

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect, P3)

Description

[Ioana (away)]

STR:
1. Launch Firefox.
2. Open a private window (File menu or Ctrl+Shift+P / Cmd+Shift+P).
3. Load some web pages in new tabs, in the private window.
4. Close the private window (all, if you had more already opened).
5. Open a new private window.
6. Try to restore the previous private session.

Expected Results:
There are no Restore Previous Session options in the private window (in home page, history, about:sessionrestore).

Notes:
MozTrap test #6729.
https://bugzilla.mozilla.org/show_bug.cgi?id=819274#c13

Comment 1

[Ioana (away)]

I ran into a problem while trying to verify that the Restore Previous Session option is disabled in the History menu. The 'disabled' property for this item is only set to 'true' after the History menu has been opened once.

The problem is that on Mac OSX, you can't click the main menu main items. You can click the History menu items (Show History, Restore Previous Session etc), but that does not initialize the 'disabled' property I need.

I had a short discussion with Gavin Sharp, which has worked on this, and he suggests we skip this part of the test on Mac.

I also posted this issue on dev-automation, but didn't get any suggestions.

Henrik, do you have any ideas whether I can open the History menu some way, or do you agree with Gavin?

Comment 2

[Ioana (away)]

Considering bug 862371 comment 7, I am going to create a browser test for this test case too.

Considering that the 3 options are quite different, I think it would be better to create separate tests for them:
- 1 browser chrome test for the menu option
- 1 mochitest for the about:home option
- 1 mochitest for the about:sessionrestore option

Comment 3

[Ioana (away)]

Attachment: testNoSessionRestoreMenuItem

The first test in the list - the test that checks that restoring a session from the main menu is disabled.

Comment 4

[(no longer active)]

Comment on attachment 741916 [details] [diff] [review]
testNoSessionRestoreMenuItem

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

::: browser/components/privatebrowsing/test/browser/Makefile.in
@@ +31,5 @@
>  		browser_privatebrowsing_localStorage_before_after_page2.html \
>  		browser_privatebrowsing_localStorage_page1.html \
>  		browser_privatebrowsing_localStorage_page2.html \
>  		browser_privatebrowsing_nonbrowser.js \
> +               browser_privatebrowsing_noSessionRestoreMenuOption.js \

Nit: indentation.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_noSessionRestoreMenuOption.js
@@ +16,5 @@
> +  }, false);
> +
> +  //Open a new PB window 
> +  //and check that the Session Restore menu option is not enabled
> +  let win2 = OpenBrowserWindow({private: true});

This creates two windows right after each other, and it's possible for each one of them to open up before the other, I think.  You probably want to make sure that the first window is fully opened and loaded before loading the second one.

Comment 5

[Ioana (away)]

Attachment: testNoSessionRestoreMenuItem-v1

Added the following:
- check that the first private window gets loaded
- wait for the first private window to get closed and confirm that the second one got loaded before testing the Session Restore option (as the manual test requires)

I ran the test locally (single and with the whole suite) several times and it passed each time.

Comment 6

[Ioana (away)]

(In reply to Ioana Budnar, QA [:ioana] from comment #5)
> Added the following:
> - check that the first private window gets loaded
> - wait for the first private window to get closed and confirm that the
> second one got loaded before testing the Session Restore option (as the
> manual test requires)

Just to make sure it's clear:
These are the only checks that matter for my test. The two windows can be opened in parallel without issues. The purpose of the test is to make sure that the Session Store option doesn't show up in private mode after creating little history (fully loading and closing a page from the same type of session).

Comment 7

[(no longer active)]

Comment on attachment 742293 [details] [diff] [review]
testNoSessionRestoreMenuItem-v1

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

r=me with the comment below fixed.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_noSessionRestoreMenuOption.js
@@ +52,5 @@
> +  }, false);
> +
> +  //Wait until the first private window is closed, 
> +  //then check that the Session Restore menu option is not enabled the second one
> +  waitForWinClose(windowsCount()==1);

Hmm, this is a bit weird, why not just call testNoSessionRestoreMenuItem from the load handler after you call win.close()?  Then you should be able to just test to make sure windowsCount() returns 1, and get rid of the polling logic inside waitForWinClose.

Comment 8

[Ioana (away)]

Attachment: testNoSessionRestoreMenuItem-v2

Patch with the changes requested in the previous comment.

Comment 9

[(no longer active)]

Comment on attachment 742366 [details] [diff] [review]
testNoSessionRestoreMenuItem-v2

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

(FWIW when somebody r+'es one of your patches with comments, it means that they trust you to make those changes, so you don't need to ask for review again.  :-)

Comment 10

[Ioana (away)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #9)
> Comment on attachment 742366 [details] [diff] [review]
> testNoSessionRestoreMenuItem-v2
> 
> Review of attachment 742366 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (FWIW when somebody r+'es one of your patches with comments, it means that
> they trust you to make those changes, so you don't need to ask for review
> again.  :-)

Sorry, didn't know that... Thanks for pointing it out :)

Comment 11

[Ioana (away)]

Attachment: testNoSessionRestoreButtons-v0

The remaining two tests for this bug.

Comment 12

[(no longer active)]

Comment on attachment 743657 [details] [diff] [review]
testNoSessionRestoreButtons-v0

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

::: browser/components/privatebrowsing/test/browser/Makefile.in
@@ +41,5 @@
>  		browser_privatebrowsing_placesTitleNoUpdate.html \
>  		browser_privatebrowsing_popupblocker.js \
>  		browser_privatebrowsing_protocolhandler.js \
>  		browser_privatebrowsing_protocolhandler_page.html \
> +                browser_privatebrowsing_ssAboutHomeButton.js \

Nit: call this browser_privatebrowsing_aboutHomeButtonAfterWindowClose.js, please.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_aboutSessionRestore.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// This test checks that the session restore button from about:sessionrestore
> +//is disabled in private mode

Nit: please use a space after // everywhere.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_ssAboutHomeButton.js
@@ +23,5 @@
> +            info("about:home got loaded");
> +            let sessionRestoreButton = win.gBrowser
> +                                          .contentDocument
> +                                          .getElementById("restorePreviousSession");
> +            is(window.getComputedStyle(sessionRestoreButton).display, 

win.getComputedStyle

Comment 13

[Ioana (away)]

Attachment: testNoSessionRestoreButtons-v1

Comment 14

[Ioana (away)]

Attachment: testNoSessionRestoreButtons-v1

Same patch, but with the bug number in the commit message.

Comment 15

[Ioana (away)]

Attachment: testNoSessionRestoreMenuItem-v2

Same patch, but with the bug number in the commit message.

Comment 16

[Ioana (away)]

Checkin needed for both patches.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 746336 [details] [diff] [review]
testNoSessionRestoreButtons-v1

gah

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/330884194efe
https://hg.mozilla.org/integration/mozilla-inbound/rev/56be8e218da0

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/330884194efe
https://hg.mozilla.org/mozilla-central/rev/56be8e218da0