Create tests for no Restore Previous Session options in private windows

RESOLVED FIXED in Firefox 23

Status

()

Firefox
Private Browsing
P3
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Ioana (away), Assigned: Ioana (away))

Tracking

unspecified
Firefox 23
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

4 years ago
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
(Assignee)

Updated

4 years ago
Assignee: nobody → ioana.budnar
Priority: -- → P3
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

4 years ago
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?
Flags: needinfo?(hskupin)
(Assignee)

Comment 2

4 years ago
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
Component: Mozmill Tests → Private Browsing
Product: Mozilla QA → Firefox
QA Contact: hskupin
(Assignee)

Updated

4 years ago
Flags: needinfo?(hskupin)
(Assignee)

Comment 3

4 years ago
Created attachment 741916 [details] [diff] [review]
testNoSessionRestoreMenuItem

The first test in the list - the test that checks that restoring a session from the main menu is disabled.
Attachment #741916 - Flags: review?(ehsan)
Attachment #741916 - Flags: feedback?(ehsan)
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.
Attachment #741916 - Flags: review?(ehsan)
Attachment #741916 - Flags: review-
Attachment #741916 - Flags: feedback?(ehsan)
(Assignee)

Comment 5

4 years ago
Created attachment 742293 [details] [diff] [review]
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.
Attachment #741916 - Attachment is obsolete: true
Attachment #742293 - Flags: review?(ehsan)
(Assignee)

Comment 6

4 years ago
(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 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.
Attachment #742293 - Flags: review?(ehsan) → review+
(Assignee)

Comment 8

4 years ago
Created attachment 742366 [details] [diff] [review]
testNoSessionRestoreMenuItem-v2

Patch with the changes requested in the previous comment.
Attachment #742293 - Attachment is obsolete: true
Attachment #742366 - Flags: review?(ehsan)
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.  :-)
Attachment #742366 - Flags: review?(ehsan) → review+
(Assignee)

Comment 10

4 years ago
(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 :)
(Assignee)

Comment 11

4 years ago
Created attachment 743657 [details] [diff] [review]
testNoSessionRestoreButtons-v0

The remaining two tests for this bug.
Attachment #743657 - Flags: review?(ehsan)
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
Attachment #743657 - Flags: review?(ehsan) → review+
(Assignee)

Comment 13

4 years ago
Created attachment 744583 [details] [diff] [review]
testNoSessionRestoreButtons-v1
Attachment #743657 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Summary: Create a mozmill test for no Restore Previous Session options in private windows → Create tests for no Restore Previous Session options in private windows
(Assignee)

Updated

4 years ago
Attachment #742366 - Flags: checkin?
(Assignee)

Updated

4 years ago
Attachment #744583 - Flags: checkin?
(Assignee)

Comment 14

4 years ago
Created attachment 746336 [details] [diff] [review]
testNoSessionRestoreButtons-v1

Same patch, but with the bug number in the commit message.
Attachment #744583 - Attachment is obsolete: true
Attachment #744583 - Flags: checkin?
(Assignee)

Comment 15

4 years ago
Created attachment 746354 [details] [diff] [review]
testNoSessionRestoreMenuItem-v2

Same patch, but with the bug number in the commit message.
Attachment #742366 - Attachment is obsolete: true
Attachment #742366 - Flags: checkin?
(Assignee)

Comment 16

4 years ago
Checkin needed for both patches.
Keywords: checkin-needed
Attachment #746336 - Attachment is obsolete: true
Comment on attachment 746336 [details] [diff] [review]
testNoSessionRestoreButtons-v1

gah
Attachment #746336 - Attachment is obsolete: false
https://hg.mozilla.org/integration/mozilla-inbound/rev/330884194efe
https://hg.mozilla.org/integration/mozilla-inbound/rev/56be8e218da0
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/330884194efe
https://hg.mozilla.org/mozilla-central/rev/56be8e218da0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
You need to log in before you can comment on or make changes to this bug.