818456 - Investigate and prepare existing Mozmill tests for per window private browsing

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The per window private browsing feature will land soon and we have to investigate which of our mozmill tests will break so we can fix that before it actually lands on mozilla-central. 

So the task is here to run our current tests with an per-window private browsing build enabled. If failures occur a patch has to be provided.

Comment 1

[u279076]

Builds:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-birch/firefox-20.0a1.en-US.win32.installer.exe
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-birch/firefox-20.0a1.en-US.mac.dmg
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-birch/firefox-20.0a1.en-US.linux-x86_64.tar.bz2

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Alex, what's the status of this bug? We have to get it addressed today. Really. We can't wait longer. This feature will most likely be re-landed again today.

Comment 3

[Alex Lakatos[:AlexLakatos]]

I'm still updating the private-browsing shared module, and I'll try to wrap it up today. The way we handle private browsing changed completely. Starting with the menu entry for opening it, and the way we stop private browsing.

Comment 4

[Andreea Matei [:AndreeaMatei]]

On OS X I get the following errors for Private Browsing folder:
* "Private Browsing state has been changed. Expected 'true'" - in testKeyboardShortcut.js

* "Menu entry '#privateBrowsingItem' not found" - in most of the tests, in endurance testPrivateBrowsing_EnterAndLeave and crash testRemoveCookieAfterPrivateBrowsing

* "could not validate element ID: errorShortDescTextNormal with text Nightly is not currently in Provate Brosing mode" - in testAboutPrivateBrowsing.js

I'll work with Alex on this.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Alex Lakatos[:AlexLakatos] from comment #3)
> I'm still updating the private-browsing shared module, and I'll try to wrap
> it up today. The way we handle private browsing changed completely. Starting
> with the menu entry for opening it, and the way we stop private browsing.

Please do not hide all the information you got by investigating that. As we have said a couple of times you should update the bug whenever you find out more details. Only that way we can ensure to share work across people as best as possible. So please remember it for the future and let us know here what else happens beside the items Andreea has posted. Thanks.

Comment 6

[Alex Lakatos[:AlexLakatos]]

Private Browsing now opens up a new window. So there is no prompt anymore, and we have to return the controller of the new window if we want to work with it. Closing private browsing now means we have to close the newly created window. There is no need for functions like reset() in the shared module anymore, but it changes the way we work with private browsing in our tests.

Comment 7

[Alex Lakatos[:AlexLakatos]]

Attachment: private-browsing.js patch v1.0

The new way of working with Private Browsing would be:
controller = mozmill.getBrowserController();
pb = new privateBrowsing.privateBrowsing(controller);
pbController = pb.open();
pb.close(pbController);

Comment 8

[Alex Lakatos[:AlexLakatos]]

Following our discution on IRC with Henrik, we would need only a few methods in the private-browsing shared module:
 - openNewPrivateWindow()
 - getPrivateBrowsingDTSs()
And I think it would be helpful to have a isWindowPrivate() method as well. We can drop the close() method, cause it's a wrapper and simply call pbController.window.close();

As far as the tests are concerned, some can and need to be refactored for the new way we do private browsing, while some are not applicable anymore and I think we should remove them from our repo. Here is a list, but I would like to further discuss it:

Refactor:
tests/endurance/testPrivateBrowsing_EnterAndLeave/test1.js
tests/functional/testPrivateBrowsing/testDisabledPermissions.js
tests/functional/testPrivateBrowsing/testAboutCache.js
tests/functional/testPrivateBrowsing/testDownloadManagerClosed.js
tests/functional/testPrivateBrowsing/testKeyboardShortcut.js
tests/functional/testPrivateBrowsing/testGeolocation.js
tests/functional/testPrivateBrowsing/testAboutPrivateBrowsing.js
tests/functional/testPrivateBrowsing/testStartStopPBMode.js

Not Aplicable:
tests/functional/testPrivateBrowsing/testTabsDismissedOnStop.js
tests/functional/testPrivateBrowsing/testDisabledElements.js
tests/functional/testPrivateBrowsing/testCloseWindow.js
tests/functional/testPrivateBrowsing/testTabRestoration.js
tests/crash/testRemoveCookieAfterPrivateBrowsing.js

Comment 9

[Alex Lakatos[:AlexLakatos]]

Attachment: private-browsing.js patch v2.0

only kept open() and getDtds() in private-browsing. Seeing as how we're going to need to check if any window is a private one, not just one that was opened using privateBrowsing(), I moved isWindowPrivate in utils.js

Comment 10

[Alex Lakatos[:AlexLakatos]]

Attachment: skipPatch v1.0

skip patch in case the feature lands and our tests start failing

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 691906 [details] [diff] [review]
private-browsing.js patch v2.0

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

::: lib/private-browsing.js
@@ +20,5 @@
>   * @param {MozMillController} controller
>   *        MozMillController to use for the modal entry dialog
>   */
>  function privateBrowsing(controller) {
>    this._controller = controller;

As we have agreed on it should be an ui class. I have suggested PrivateBrowsingWindow. And it has to live under lib/ui.

@@ +76,4 @@
>  
> +    var pbWindow = mozmill.wm.getMostRecentWindow("navigator:browser");
> +    assert.ok(utils.isWindowPrivate(pbWindow),
> +              "Private Browsing window has been opened");

Please use utils.handleWindow() here.

::: lib/utils.js
@@ +434,5 @@
> + * @return {boolean}
> + */
> +function isWindowPrivate(aWindow) {
> +  return PrivateBrowsingUtils.isWindowPrivate(aWindow);
> +}

This is private browsing related. Why have you added it to utils.js? Please move it back and make it a global function.

Comment 12

[Alex Lakatos[:AlexLakatos]]

Attachment: private-browsing.js patch v3.0

moved it under ui. used handleWindow. made isWindowPrivate a global method.

Comment 13

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 692276 [details] [diff] [review]
private-browsing.js patch v3.0

This looks good to me.

Comment 14

[Andreea Matei [:AndreeaMatei]]

I'm afraid this test is also related somehow:
http://mozmill-ci.blargon7.com/#/functional/failure?branch=All&platform=All&from=2012-12-07&to=&test=%2FtestTabbedBrowsing%2FtestNewTab.js&func=testNewTab.js%3A%3AtestNewTab

It should open a new tab, but somehow it's replaced with "about:privatebrowsing", could be a bug in the new Nightly.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andreea Matei [:AndreeaMatei] from comment #14)
> It should open a new tab, but somehow it's replaced with
> "about:privatebrowsing", could be a bug in the new Nightly.

Right, because I think we spawn a new pb window and don't close it. That's why about:privatebrowsing is listed.

Alex, sorry when I have to make pressure but we really have to make progress here. I'm afraid to see that nothing will be fixed before Monday. :/

Comment 16

[Alex Lakatos[:AlexLakatos]]

(In reply to Henrik Skupin (:whimboo) from comment #15)
> Alex, sorry when I have to make pressure but we really have to make progress
> here. I'm afraid to see that nothing will be fixed before Monday. :/
I'm working through the tests right now. I've got about 5 left. I won't leave the office without uploading a patch, so it's up to the review process. Hopefully it's going to be a slam dunk, r+ from the first iteration :)

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Alex! If it gets a r- Dave or myself would have to continue. Seeing all those broken tests I feel that we most likely see some regressions in Firefox.

Comment 18

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v1.0

private-browsing shared module and updated tests

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 692371 [details] [diff] [review]
patch v1.0

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

Lots of comments here. Also would you mind to give feedback regarding your talk to the QA people which tests we need? I would kinda like to hear that in the next days. Otherwise good start for the transition.

::: lib/private-browsing.js
@@ +27,1 @@
>    this._controller = controller;

I would make the controller parameter optional and add a controller setter to the class. Reason is that you will not always use the browser controller here, but only if you want to create a PrivateBrowsingWindow instance from an already existent window. In all other cases open() will have to be used with the original controller.

@@ +46,5 @@
>     *
>     * @returns Array of external DTD urls
>     * @type [string]
>     */
> +  getDtds : function PrivateBrowsingWindow_getDtds() {

nit: For prefix of the internal function name don't use the full name of the class but an abbreviation. Here please 'PBW'.

@@ +59,2 @@
>     *
>     * @param {boolean} useShortcut

nit: while we are at it please update for the a prefix.

@@ +59,4 @@
>     *
>     * @param {boolean} useShortcut
>     *        Use the keyboard shortcut if true otherwise the menu entry is used
> +   * @returns {MozMillController} Mozmill Controller

This method will not return the controller for the new pb window but assign it to the internal variable.

@@ +76,2 @@
>  
> +      return (windows.length === (windowCount + 1));

This would fit perfectly into a single line.

@@ +81,3 @@
>  
> +    assert.ok(isWindowPrivate(this._controller.window),
> +              "A new private window has been opened");

I would also add a wrapper to the class. It would call the global function of this module. That makes it easier for tests to run those checks.

@@ +86,5 @@
>  
> +/**
> + * Checks a window for being a Private Window.
> + *
> + * @param {object} aWindow

You want to use the MozmillController class here.

@@ +91,5 @@
> + *        Window to check
> + *
> + * @return {boolean}
> + */
> +function isWindowPrivate(aWindow) {

Please name it isPrivateBrowsingWindow. The class internal getter could be isPrivate.

::: tests/endurance/testPrivateBrowsing_EnterAndLeave/test1.js
@@ +10,5 @@
>  function setupModule() {
>    controller = mozmill.getBrowserController();
>    enduranceManager = new Endurance.EnduranceManager(controller);
>  
> +  pb = new privateBrowsing.PrivateBrowsingWindow(controller);

You want to instantiate this class when you

@@ +21,2 @@
>    enduranceManager.run(function () {
> +    pb.open();

You will have to pass in the original controller here.

@@ +21,4 @@
>    enduranceManager.run(function () {
> +    pb.open();
> +    enduranceManager.addCheckpoint("Opened a private browsing window");
> +    pb.controller.window.close();

You want to add a close() method to the PrivateBrowsingWindow class. Also it has to check the window count. Otherwise we will have test failures.

::: tests/functional/testPrivateBrowsing/manifest.ini
@@ -4,1 @@
>  run-if = os == 'mac'

Why are you removing this test? It's not what we have agreed on in our ask an expert session.

@@ -6,2 @@
>  [testDisabledPermissions.js]
> -[testDownloadManagerClosed.js]

Same here. I thought we have discovered a regression in Firefox here. So why do you remove the test?

@@ -8,2 @@
>  [testGeolocation.js]
> -[testKeyboardShortcut.js]

Same here. Please leave and for the other tests use the menu as default action.

@@ -8,3 @@
>  [testGeolocation.js]
> -[testKeyboardShortcut.js]
> -[testStartStopPBMode.js]

For this test do we really need the 'close' part in the name? We cannot hit the same keyboard shortcut or menu entry to close the window. So it would not be necessary I believe.

::: tests/functional/testPrivateBrowsing/testAboutCache.js
@@ +11,5 @@
>                        "http://domain2.mozqa.com"];
>  
>  function setupModule() {
>    controller = mozmill.getBrowserController();
> +  pb = new privateBrowsing.PrivateBrowsingWindow(controller);

the current window is not a pb window.

@@ -27,5 @@
>  }
>  
> -function teardownModule() {
> -  pb.reset();
> -}

You want to be sure that the second window has been closed. So I don't think we can remove teardownModule().

@@ +74,5 @@
>    });
>  
>    expect.notEqual(memoryEntriesCount, 0, "Memory cache contains entries in PB");
>  
> +  pb.controller.window.close()

See my above comment. We will not reach this point if the test fails before.

::: tests/functional/testPrivateBrowsing/testAboutPrivateBrowsing.js
@@ +17,4 @@
>    controller = mozmill.getBrowserController();
>  
>    // Create Private Browsing instance and set handler
> +  pb = new privateBrowsing.PrivateBrowsingWindow(controller);

the current window is not a pb window.

@@ +26,1 @@
>    prefs.preferences.clearUserPref(PREF_PRIVATE_BROWSING_SUPPORT);

Same here for closing the additional window.

@@ +53,2 @@
>  
> +  pbController = utils.handleWindow("type", "navigator:browser", undefined, false);

I think we should allow a callback for the open() method of the pb window class, so we do not have to duplicate such an amount of code. In the callback we simply trigger the action which open the pb window.

::: tests/functional/testPrivateBrowsing/testCloseWindow.js
@@ +14,2 @@
>    controller = mozmill.getBrowserController();
> +  pb = new privateBrowsing.PrivateBrowsingWindow(controller);

Same as for the other tests.

@@ -25,5 @@
>  }
>  
> -var teardownModule = function(module) {
> -  pb.reset();
> -}

ditto.

@@ +31,3 @@
>    var cmdKey = utils.getEntity(tabBrowser.getDtds(), "closeCmd.key");
>    controller.keypress(null, cmdKey, {accelKey: true});
> +  pb.controller.window.close();

I would use controller.window.close() here too. The DTD entity we already check in another test.

@@ +38,2 @@
>  
>    utils.handleWindow("type", "navigator:browser", checkWindowOpen, false);

We have closed both windows. So no other window is open at this time. You will have to open a new browser window first. Not sure if you missed to add the triggering code.

@@ +38,5 @@
>  
>    utils.handleWindow("type", "navigator:browser", checkWindowOpen, false);
>  }
>  
>  function checkWindowOpen(controller) {

please follow the coding styles and add the 'a' prefix.

@@ +45,5 @@
>    // for now. It has to be changed once the test is updated for Mozmill 2
>    controller.sleep(TIMEOUT_SESSION_STORE);
>  
> +  assert.ok(privateBrowsing.isWindowPrivate(controller.window),
> +            "A private browsing window is opened");

Hm. I don't think we want to use session restore here but open a normal browser window which should not be a private browsing one. That would align what the test was testing before. What you are testing here is worth another test.

::: tests/functional/testPrivateBrowsing/testDisabledElements.js
@@ -44,5 @@
> - * @param {MozMillController} controller
> - *        MozMillController of the window to operate on
> - */
> -function checkImportMenu(controller) {
> -  var maintenanceButton = new elementslib.ID(controller.window.document,

I would like to know if we really have to remove this test and that this is the expected behavior. Please point me to a comment of a bug where it has been discussed.

::: tests/functional/testPrivateBrowsing/testDisabledPermissions.js
@@ +24,4 @@
>    controller = mozmill.getBrowserController();
>  
>    // Create Private Browsing instance
> +  pb = new privateBrowsing.PrivateBrowsingWindow(controller);

snip

@@ +35,5 @@
>    // Reset the user cookie pref
>    prefs.preferences.clearUserPref(PREF_COOKIE_BEHAVIOR);
>    prefs.preferences.clearUserPref(PREF_COOKIE_LIFETIME_POLICY);
> +
> +  pb.controller.window.close();

Danger zone.... it could have already been closed. So don't call close if window is not valid anymore. As mentioned earlier use an internal close() method.

@@ +46,1 @@
>    tabs.closeAllTabs(controller);

nit: can you move this up into setupModule?

@@ +63,5 @@
>                                      '/*[name()="menupopup"][4]' +
>                                      '/*[name()="menuitem"][1]');
>  
> +  pb.controller.waitForElement(allow);
> +  pb.controller.assertJSProperty(allow, "disabled", true);

This code doesn't exist anymore. So you are working with an outdated hg checkout.

@@ +75,5 @@
>  
>  /**
>   * Just in case we open a modal dialog we have to mark the test as failed
>   */
> +function cookieHandler(controller) {

That's an orphaned function and can be removed.

::: tests/functional/testPrivateBrowsing/testGeolocation.js
@@ +17,3 @@
>    controller = mozmill.getBrowserController();
>  
> +  pb = new privateBrowsing.PrivateBrowsingWindow(controller);

snip

@@ +21,2 @@
>  
> +  tabBrowser.closeAllTabs();

nit: please no empty line before.

@@ -26,2 @@
>  
> -var teardownModule = function(module)

snip. Why the removal?

::: tests/functional/testPrivateBrowsing/testStartStopPBMode.js
@@ +21,5 @@
>    modifier = controller.window.document.documentElement.
>               getAttribute("titlemodifier_privatebrowsing");
>  
> +  // Create Private Browsing instance
> +  pb = new privateBrowsing.PrivateBrowsingWindow(controller);

snip.

@@ -30,5 @@
>  }
>  
> -function teardownModule() {
> -  pb.reset();
> -}

snip

Comment 20

[Alex Lakatos[:AlexLakatos]]

Attachment: private-browsing patch v4.0

Seems what I feared most did happen. So here is just the private-browsing library again. I would love to finish this first and then edit the test files again, instead of editing all in the same iteration.
I've hopefully updated the shared library to comply with Henriks last comment.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 692622 [details] [diff] [review]
private-browsing patch v4.0

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

::: lib/private-browsing.js
@@ +23,5 @@
>   * @param {MozMillController} controller
>   *        MozMillController to use for the modal entry dialog
>   */
> +function PrivateBrowsingWindow(controller) {
> +  this._controller = controller ? controller : mozmill.getBrowserController();

We do not want to set a default controller if none has been specified. In such a case the internal property should be null. controller will only be used in cases when you want to use the class for an already opened window and existing controller.

@@ +49,2 @@
>     */
> +  set controller(aController) {

I know that I have mentioned that we should add a setter, but thinking more about it over the weekend I'm not sure we really need this. When we want to cover another window a new instance of the privateBrowsingWindow should be created.

@@ +67,5 @@
>    /**
> +   * Returns the private state of the window of the class instance
> +   *
> +   * @returns isWindowPrivate
> +   * @type {boolean}

This needs an update: http://code.google.com/p/jsdoc-toolkit/wiki/TagReturns

@@ +81,3 @@
>     *        Use the keyboard shortcut if true otherwise the menu entry is used
>     */
> +  open: function PBW_open(aUseShortcut) {

This method still misses the proposed callback solution. Best option here is to have a boolean or function as argument. Given what has been specified we run the appropriate code.

@@ +110,2 @@
>  
> +    assert.ok(this.isPrivate(), "A window is being closed");

Why an assert? Expect sounds perfect to me.

@@ +116,3 @@
>  
> +      return (windows.length === (windowCount - 1));
> +    }, "A private browsing window has been closed");

Fine for now. But please file a bug so we can flip to an event listener in the waitFor. This would apply to all of our tests and not only that one here.

@@ +127,5 @@
> + *
> + * @return {boolean}
> + */
> +function isPrivateBrowsingWindow(aController) {
> +  return PrivateBrowsingUtils.isWindowPrivate(aController.window);

Keep in mind that aController can be null.

Comment 22

[Alex Lakatos[:AlexLakatos]]

(In reply to Henrik Skupin (:whimboo) from comment #19)
> Lots of comments here. Also would you mind to give feedback regarding your
> talk to the QA people which tests we need? I would kinda like to hear that
> in the next days. Otherwise good start for the transition.
I talked with :ioana as she was responsible for organizing a testday around the feature. There are about 30 testcase in the spreadsheet used there but they are not prioritized. I'm cc'ing :ioana and :juanb for more details.

Comment 23

[Alex Lakatos[:AlexLakatos]]

Attachment: private-browsing.js patch v5.0

Added a callback to the opener. isPrivateBrowsingWindow now throws if aController is null. removed the controller setter.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 692941 [details] [diff] [review]
private-browsing.js patch v5.0

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

Only some nits but looks good to go on with updating the set of tests.

::: lib/private-browsing.js
@@ +23,4 @@
>   * @param {MozMillController} controller
>   *        MozMillController to use for the modal entry dialog
>   */
> +function PrivateBrowsingWindow(controller) {

It still misses the a prefix in all cases.

@@ +23,5 @@
>   * @param {MozMillController} controller
>   *        MozMillController to use for the modal entry dialog
>   */
> +function PrivateBrowsingWindow(controller) {
> +  this._controller = controller ? controller : null;

This can be 'controller || null'.

@@ +54,5 @@
>      return dtds;
>    },
>  
>    /**
> +   * Returns the private state of the window of the class instance

nit: private browsing state

@@ +68,2 @@
>     *
> +   * @param {String} aMethod

nit: string

@@ +109,2 @@
>  
> +    expect.ok(this.isPrivate(), "A window is being closed");

I don't think that check is necessary.

@@ +126,5 @@
> + *
> + * @return {boolean}
> + */
> +function isPrivateBrowsingWindow(aController) {
> +  if (aController && aController.window)

No need for the second check. When we have a controller, we also have a window.

Comment 25

[Alex Lakatos[:AlexLakatos]]

(In reply to Henrik Skupin (:whimboo) from comment #24)
> @@ +109,2 @@
> >  
> > +    expect.ok(this.isPrivate(), "A window is being closed");
> 
> I don't think that check is necessary.
If we don't check to see if the window we are closing is private, we could be closing a regular window.

Comment 26

[Alex Lakatos[:AlexLakatos]]

Attachment: removed tests patch v1.0

4 tests we are sure we want to remove. If we had doubts about tests/functional/testPrivateBrowsing/testDisabledElements.js, I talked with Ehsan and Josh and the current behavior is the expected one, so the test should be removed.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Alex Lakatos[:AlexLakatos] from comment #25)
> > > +    expect.ok(this.isPrivate(), "A window is being closed");
> > 
> > I don't think that check is necessary.
> If we don't check to see if the window we are closing is private, we could
> be closing a regular window.

We are not going to close any kind of window but the one which is covered by the current controller object. So this doesn't matter.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Alex Lakatos[:AlexLakatos] from comment #26)
> tests/functional/testPrivateBrowsing/testDisabledElements.js, I talked with
> Ehsan and Josh and the current behavior is the expected one, so the test
> should be removed.

Where did this talk happen? Please always try to do it on our mailing list or on the appropriate bug. I would love to see their statements for future questions.

Comment 29

[Alex Lakatos[:AlexLakatos]]

(In reply to Henrik Skupin (:whimboo) from comment #28)
> (In reply to Alex Lakatos[:AlexLakatos] from comment #26)
> > tests/functional/testPrivateBrowsing/testDisabledElements.js, I talked with
> > Ehsan and Josh and the current behavior is the expected one, so the test
> > should be removed.
> 
> Where did this talk happen? Please always try to do it on our mailing list
> or on the appropriate bug. I would love to see their statements for future
> questions.
I talked with :ehsan on #devtools and with :jdm on #developers I think. But I'm cc'ing them here for their statements.

Comment 30

[Alex Lakatos[:AlexLakatos]]

Attachment: endurance/testPrivateBrowsing patch v1.0

Updated and renamed the old test to reflect the new behavior.
(In reply to Henrik Skupin (:whimboo) from comment #19)
> Comment on attachment 692371 [details] [diff] [review]
> You want to add a close() method to the PrivateBrowsingWindow class. Also it
> has to check the window count. Otherwise we will have test failures.
There is no need to do a window count in the test, pb.close() takes care of that.

Comment 31

[Andreea Matei [:AndreeaMatei]]

Attachment: testAboutCache.js

Patch for testAboutCache.js. As talked in bug 822121, we are opening 2 PB windows and check in the second one that the PB cache is maintained. If we close the first PB window before opening the second one, the cache is not maintained, that is why we close it only after the second one is opened.

Comment 32

[Andreea Matei [:AndreeaMatei]]

Attachment: testStartStopPBKeyboardShortcut.js

Removed testKeyboardShortcut.js and updated testStartStopPBMode.js (also will open PB with shortcut keys).

Comment 33

[Daniela Petrovici]

Attachment: testDisabledPermissions.js

Modified testDisabledPermissions with new changes for PB mode

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 693363 [details] [diff] [review]
removed tests patch v1.0

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

Everything looks fine except the commit message. Please make sure to keep our working style. I will land the patch now so we have lesser failures until the remaining tests get fixed.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 693363 [details] [diff] [review]
removed tests patch v1.0

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

http://hg.mozilla.org/qa/mozmill-tests/rev/fbc581feb8f0 (default)

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 693376 [details] [diff] [review]
endurance/testPrivateBrowsing patch v1.0

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

In general I would like to see Dave's comments on this patch given that he maintains the endurance tests. Beside that there is one thing I would propose to change.

::: tests/endurance/testPrivateBrowsing_EnterAndLeave/test1.js
@@ +19,2 @@
>    enduranceManager.run(function () {
> +    var pb = new privateBrowsing.PrivateBrowsingWindow(controller);

We operate on a single main window. So we can set this up already in setupModule.

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 693381 [details] [diff] [review]
testAboutCache.js

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

::: tests/functional/testPrivateBrowsing/testAboutCache.js
@@ -27,5 @@
>  }
>  
> -function teardownModule() {
> -  pb.reset();
> -}

Same as for my last review. Why do you remove the teardownModule method? Please address all of my comments before asking review on a newly uploaded patch.

@@ +54,5 @@
>    }
>  
>    // Get entries information for both disk and memory devices
>    cs.visitEntries(visitor);
> +  assert.equal(diskEntriesCount, 0, "Disk cache has no entries");

why have you removed the second assert?

@@ +59,2 @@
>  
> +  pb = new privateBrowsing.PrivateBrowsingWindow(controller);

Please create that variable in setupModule() and name it correctly so it's name is similar to the second instance.

@@ +72,5 @@
>    });
>  
>    expect.notEqual(memoryEntriesCount, 0, "Memory cache contains entries in PB");
>  
> +  secondPb = new privateBrowsing.PrivateBrowsingWindow(controller);

Same here.

@@ +86,5 @@
> +
> +  expect.notEqual(memoryEntriesCount, 0,
> +                  "Memory cache in second PB window contains the entries from the first PB window");
> +
> +  secondPb.close();

I can't comment on until we have the right steps hashed out on bug 822121.

@@ +92,5 @@
>    cs.visitEntries(visitor);
>    assert.equal(memoryEntriesCount, 0, "Memory cache has no entries after PB mode");
>  
>    TEST_DOMAINS.forEach(function (aPage) {
>      assert.ok(diskEntries.indexOf(aPage) === -1,

Change those checks all to expect. There is no need for assert. It was a failure when we landed the initial test.

Comment 38

[Alex Lakatos[:AlexLakatos]]

Attachment: testAboutPrivateBrowsing patch v1.0

updated testAboutPrivateBrowsing.js

Comment 39

[Alex Lakatos[:AlexLakatos]]

Attachment: testGeolocation.js

updated testGeolocation.js. This test is always going to pass, because of the try/catch block and the fact that nothing happens on the catch branch. I'm having issues with this test: if the correct page is loaded, the shortcut for sharing location does nothing. in some runs, about:privatebrowsing is loaded just after the correct page is loaded, and if I add locationBar for debugging, locationBar.value still points to the correct page and not about:privatebrowsing. When running the test you can clearly see that the loaded page is about:privatebrowsing, because the waitFor is failing, thus waiting for 5 seconds. Hitting the back button takes you to the correct test page.

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 693407 [details] [diff] [review]
testStartStopPBKeyboardShortcut.js

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

I do not see an update of the manifest in this patch which is necessary. Also the second test should not mention start/stop in it's filename given that we open/close a private browsing window. More comments inline.

::: tests/functional/testPrivateBrowsing/testStartStopPBMode.js
@@ +19,5 @@
>    modifier = controller.window.document.documentElement.
>               getAttribute("titlemodifier_privatebrowsing");
>  
>    // Create Private Browsing instance and set handler
> +  pb = new privateBrowsing.PrivateBrowsingWindow(controller);

This should be done in the setupModule() instead.

@@ -30,5 @@
>  }
>  
> -function teardownModule() {
> -  pb.reset();
> -}

Why do you remove teardownModule()? We have to cleanup after the test ran.

@@ +31,2 @@
>   */
>  function testStartStopPrivateBrowsingMode() {

Update the name accordingly.

@@ +46,2 @@
>  
> +  expect.contain(pb.controller.window.document.title, modifier,

nit: Please squash the above empty line.

@@ +55,5 @@
> +  pb.controller.waitForElement(longDescElem);
> +  expect.equal(longDescElem.getNode().textContent, description,
> +               "Private Browsing description is correct");
> +  expect.equal(moreInfoElem.getNode().textContent, learnMore,
> +               "The 'Learn More' link text is correct");

Please leave assertText() in here. This is a change which has to be made in bug 803088.

Comment 41

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 693429 [details] [diff] [review]
testDisabledPermissions.js

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

::: tests/functional/testPrivateBrowsing/testDisabledPermissions.js
@@ +49,3 @@
>    // Make sure we are not in PB mode and don't show a prompt
>    pb.enabled = false;
>    pb.showPrompt = false;

What's with those lines? Those properties don't exist anymore.

Comment 42

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 693547 [details] [diff] [review]
testAboutPrivateBrowsing patch v1.0

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

::: tests/functional/testPrivateBrowsing/testAboutPrivateBrowsing.js
@@ +24,2 @@
>  
> +  pb.close();

This call will fail with an unexpected exception if there is no pb window open. You will have to update the private-browsing module to handle such a case more sanely.

@@ +32,5 @@
>    controller.open("about:privatebrowsing");
>    controller.waitForPageLoad();
>  
>    // Check descriptions on the about:privatebrowsing page
> +  pb = new privateBrowsing.PrivateBrowsingWindow(controller);

Move this line into setupModule().

@@ +39,5 @@
>    var statusText = new elementslib.ID(controller.tabs.activeTab, "errorShortDescTextNormal");
>    controller.waitForElement(statusText);
>    controller.assertText(statusText, issueDesc);
>  
> +  pb.open("callback", openPBWindowCallback);

The callback is short so change it to an inline function.

Comment 43

[Daniela Petrovici]

Attachment: testDisabledPermissions.js

Modified the testDisabledPermissions.js per review

Comment 44

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 693376 [details] [diff] [review]
endurance/testPrivateBrowsing patch v1.0

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

This works for me to get the tests running again, however I would prefer to follow the open new window tests model now that private browsing is per window and can accumulate entities.

The test would be called testPrivateBrowsing_OpenNewWindow, and it would open as many windows as there are entities before closing them. r- for now, but would be happy to have this filed as a separate bug to be worked on later.

::: tests/endurance/testPrivateBrowsing_EnterAndLeave/test1.js
@@ +19,2 @@
>    enduranceManager.run(function () {
> +    var pb = new privateBrowsing.PrivateBrowsingWindow(controller);

Moving this to setupModule causes this to fail with multiple iterations.

Comment 45

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 693804 [details] [diff] [review]
testDisabledPermissions.js

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

::: tests/functional/testPrivateBrowsing/testDisabledPermissions.js
@@ +20,5 @@
>  const PREF_COOKIE_LIFETIME_POLICY = "network.cookie.lifetimePolicy";
>  
>  const TIMEOUT = 5000;
>  
> +function setupModule(module) {

module isn't needed here

@@ +34,3 @@
>  }
>  
> +function teardownModule(module) {

module isn't needed here

@@ +49,1 @@
>  

nit: remove this empty line

@@ +72,2 @@
>  
>    // No cookie dialog should show up

The modal dialog handler has been removed, so we would no longer fail if it appeared. We should check the steps for this test and make sure we're still covering the required functionality.

Comment 46

[Andreea Matei [:AndreeaMatei]]

Attachment: testOpenClosePBKeyboardShortcut.js

Addressed review comments.

Comment 47

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 693556 [details] [diff] [review]
testGeolocation.js

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

(In reply to Alex Lakatos[:AlexLakatos] from comment #39)
> Created attachment 693556 [details] [diff] [review]
> testGeolocation.js
> 
> updated testGeolocation.js. This test is always going to pass, because of
> the try/catch block and the fact that nothing happens on the catch branch.
> I'm having issues with this test: if the correct page is loaded, the
> shortcut for sharing location does nothing. in some runs,
> about:privatebrowsing is loaded just after the correct page is loaded, and
> if I add locationBar for debugging, locationBar.value still points to the
> correct page and not about:privatebrowsing. When running the test you can
> clearly see that the loaded page is about:privatebrowsing, because the
> waitFor is failing, thus waiting for 5 seconds. Hitting the back button
> takes you to the correct test page.

I am seeing these issues too. I do not think this is ready for review. Please address these issues first.

::: tests/functional/testPrivateBrowsing/testGeolocation.js
@@ -24,4 @@
>    tabBrowser = new tabs.tabBrowser(controller);
>  }
>  
> -var teardownModule = function(module)

Why remove teardown? We would still need to ensure that we clean up.

Comment 48

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 693843 [details] [diff] [review]
testOpenClosePBKeyboardShortcut.js

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

This is looking good, however there's a conflict when applying it, and I've left a question regarding the teardown.

::: tests/functional/testPrivateBrowsing/manifest.ini
@@ -11,1 @@
>  [testTabRestoration.js]

This conflicts because these last two tests no longer appear in the manifest file. Please update your local branch.

::: tests/functional/testPrivateBrowsing/testStartStopPBMode.js
@@ +24,5 @@
>    tabBrowser.closeAllTabs();
>  }
>  
>  function teardownModule() {
> +  if (mozmill.utils.getWindows("navigator:browser").length > 1 &&

as the pb.close() would throw if it's already closed, could we just use a try/catch here?

Comment 49

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Dave Hunt (:davehunt) from comment #48)
> >  function teardownModule() {
> > +  if (mozmill.utils.getWindows("navigator:browser").length > 1 &&
> 
> as the pb.close() would throw if it's already closed, could we just use a
> try/catch here?

See my reply in comment 42 too. This is nothing we want to see in our tests but has to be covered in the pb module.

Comment 50

[Andrei Eftimie]

Attachment: testCloseWindow.js

Comment 51

[Andrei Eftimie]

Attachment: testCloseWindow.js

As discussed with Henrik on irc, this test is obsolete on the new PB mode.

Comment 52

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 693868 [details] [diff] [review]
testCloseWindow.js

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

Looks good to me. Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/db52715317eb

Comment 53

[Daniela Petrovici]

Attachment: remove tests testDisabledPermissions.js and testGeolocation.js

Comment 54

[Daniela Petrovici]

Comment on attachment 693922 [details] [diff] [review]
remove tests testDisabledPermissions.js and testGeolocation.js

Removed tests for disable permissions and geoLocation

Comment 55

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 693922 [details] [diff] [review]
remove tests testDisabledPermissions.js and testGeolocation.js

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

Whatever has been discussed please ensure to add a reasonable comment in the future when you attach a patch which refactors or removes a test. In a bug like that it's hard to keep track of what has been decided / agreed on and it always helps to see what's done in the patch when you have to review it. Thanks

Looks good to me otherwise.

Comment 56

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 693922 [details] [diff] [review]
remove tests testDisabledPermissions.js and testGeolocation.js

http://hg.mozilla.org/qa/mozmill-tests/rev/974d3008f567

Comment 57

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 691938 [details] [diff] [review]
skipPatch v1.0

No, we don't want to skip all the tests. Marking as obsolete.

Comment 58

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Alex and Andreea, what's left to do on this bug? Can you please give us a status update? Can we finally finish up with this bug today?

Comment 59

[Andreea Matei [:AndreeaMatei]]

Attachment: testAboutCache.js

Updated the testAboutCache.js.
I've let pb.close() into the teardownModule that Alex will be taking care of today

Comment 60

[Alex Lakatos[:AlexLakatos]]

(In reply to Henrik Skupin (:whimboo) [away 12/21 - 01/01] from comment #58)
> Alex and Andreea, what's left to do on this bug? Can you please give us a
> status update? Can we finally finish up with this bug today?
I'm finishing up the shared module so we can land that and start landing tests. Andreea will finish up testOpenClosePBKeyboardShortcut in the meantime.

Comment 61

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Alex Lakatos[:AlexLakatos] from comment #60)
> I'm finishing up the shared module so we can land that and start landing
> tests. Andreea will finish up testOpenClosePBKeyboardShortcut in the
> meantime.

I will not land this as a separate checkin. As I said it has to land all at once. Just to refresh everyones mind...

Comment 62

[Andreea Matei [:AndreeaMatei]]

Attachment: testOpenClosePBKeyboardShortcut.js

Updated the manifest file and teardownModule.

Comment 63

[Alex Lakatos[:AlexLakatos]]

Attachment: private-browsing and tests patch v1.0

updated nits in both tests and the shared library. wrapped PBW_close() in a try/catch. Left the endurance test as it was, and will log an additional bug to refactor it as Dave wants.

Comment 64

[Alex Lakatos[:AlexLakatos]]

(In reply to Dave Hunt (:davehunt) from comment #44)
> Comment on attachment 693376 [details] [diff] [review]
> endurance/testPrivateBrowsing patch v1.0
> The test would be called testPrivateBrowsing_OpenNewWindow, and it would
> open as many windows as there are entities before closing them. r- for now,
> but would be happy to have this filed as a separate bug to be worked on
> later.
Logged bug 823576

Comment 65

[(no longer active)]

While this will be nice to have, it's not gonna block us from shipping.

Comment 66

[Daniela Petrovici]

Related to the removal of testDisabledPermissions.js. There are two mochitests that cover this functionality:
http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/test/browser/global/browser_privatebrowsing_popupblocker.js
http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/test/browser/global/browser_privatebrowsing_cookieacceptdialog.js

Comment 67

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Daniela Petrovici from comment #66)
> Related to the removal of testDisabledPermissions.js. There are two
> mochitests that cover this functionality:
> http://mxr.mozilla.org/mozilla-central/source/browser/components/
> privatebrowsing/test/browser/global/browser_privatebrowsing_popupblocker.js
> http://mxr.mozilla.org/mozilla-central/source/browser/components/
> privatebrowsing/test/browser/global/
> browser_privatebrowsing_cookieacceptdialog.js

Thanks Daniela. As far as I can tell, this does not check that the modal dialog does not appear when in PB. That said, the testDisabledPermissions didn't directly test this either. Perhaps we need a new test to explicitly check that the modal dialog is not displayed when in PB, and it might make sense to have a check to ensure it does display when not in PB. Could you confirm that such a test does not already exist, and if not please file a bug to create it.

Comment 68

[Daniela Petrovici]

(In reply to Dave Hunt (:davehunt) from comment #67)
> (In reply to Daniela Petrovici from comment #66)
> > Related to the removal of testDisabledPermissions.js. There are two
> > mochitests that cover this functionality:
> > http://mxr.mozilla.org/mozilla-central/source/browser/components/
> > privatebrowsing/test/browser/global/browser_privatebrowsing_popupblocker.js
> > http://mxr.mozilla.org/mozilla-central/source/browser/components/
> > privatebrowsing/test/browser/global/
> > browser_privatebrowsing_cookieacceptdialog.js
> 
> Thanks Daniela. As far as I can tell, this does not check that the modal
> dialog does not appear when in PB. That said, the testDisabledPermissions
> didn't directly test this either. Perhaps we need a new test to explicitly
> check that the modal dialog is not displayed when in PB, and it might make
> sense to have a check to ensure it does display when not in PB. Could you
> confirm that such a test does not already exist, and if not please file a
> bug to create it.

Logged bug https://bugzilla.mozilla.org/show_bug.cgi?id=823869 for adding a new test since we don't have one that verifies this.

Comment 69

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 694427 [details] [diff] [review]
private-browsing and tests patch v1.0

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

::: lib/private-browsing.js
@@ +10,4 @@
>   */
>  
>  // Include required modules
> +var { assert, expect } = require("../assertions");

Expect is no longer used.

@@ +106,3 @@
>     */
> +  close: function PBW_close() {
> +    try {

I don't like the blanket try/catch approach here. Can we not use simple if logic to only attempt to close the window if it is still open?

Comment 70

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 694273 [details] [diff] [review]
testAboutCache.js

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

::: tests/functional/testPrivateBrowsing/testAboutCache.js
@@ -87,5 @@
>  
>    cs.visitEntries(visitor);
> -  assert.equal(memoryEntriesCount, 0, "Memory cache has no entries after PB mode");
> -
> -  TEST_DOMAINS.forEach(function (aPage) {

Why are we removing this? According to bug 822121 comment 6, we still care that the disk entries do not contain any sites that we visit during private browsing.

Comment 71

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 694282 [details] [diff] [review]
testOpenClosePBKeyboardShortcut.js

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

This looks good, but is dependant on the private browsing library refactor. Could we get this added to the revision for the library and tests patch?

Comment 72

[Alex Lakatos[:AlexLakatos]]

Attachment: private-browsing and tests patch v2.0

updated private browsing library to use a simple if. added Andreeas' patches with her edits.

Comment 73

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 694817 [details] [diff] [review]
private-browsing and tests patch v2.0

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

::: lib/private-browsing.js
@@ +15,3 @@
>  
> +// Import PrivateBrowsingUtils to gain acces to its' API
> +Components.utils.import("resource://gre/modules/PrivateBrowsingUtils.jsm");

Please use Cu as we do for all the other places. Also fix the nit in the comment above.

@@ +23,5 @@
>   * @param {MozMillController} controller
>   *        MozMillController to use for the modal entry dialog
>   */
> +function PrivateBrowsingWindow(aController) {
> +  this._controller = aController || null;

nit: update the docstring for the parameter name.

@@ +36,3 @@
>     *
>     * @returns Mozmill Controller
>     * @type {MozMillController}

Please do that as given by http://code.google.com/p/jsdoc-toolkit/wiki/TagReturns

@@ +56,5 @@
>  
>    /**
> +   * Returns the private browsing state of the window of the class instance
> +   *
> +   * @returns {boolean} Returns the private state of the window from the controller

nit: No need to use 'Returns' here. Just use 'Private state of the window'.

@@ +83,5 @@
> +        var cmdKey = utils.getEntity(this.getDtds(), "privateBrowsingCmd.commandkey");
> +        this._controller.keypress(null, cmdKey, {accelKey: true, shiftKey: true});
> +        break;
> +      case "callback":
> +        aCallback(this._controller);

If aCallback is set please sanity check that it is a function before calling it. Also why are you passing the controller into the callback? There should be passed nothing to the callback.

@@ +101,4 @@
>    },
>  
>    /**
> +   * Close a Private Browsing window

nit: 'the Private...'

@@ +106,3 @@
>     */
> +  close: function PBW_close() {
> +    if (!this._controller.window.closed) {

As what I have mentioned before, do not try to access a property of the window here which might be null. This can also cause a dead object failure. A check for the window first should solve it.

@@ +123,5 @@
> + *
> + * @param {MozMillController} aController
> + *        Controller of the window to check
> + *
> + * @return {boolean}

Please be compliant to the jsdoc specs.

@@ +129,5 @@
> +function isPrivateBrowsingWindow(aController) {
> +  if (aController)
> +    return PrivateBrowsingUtils.isWindowPrivate(aController.window);
> +  else
> +    throw new Error(arguments.callee.name + ": aController is null");

I would refactor that code to check for the controller first and throw. That will save us the else and aligns us to the other code we have in Mozmill.

::: tests/functional/testPrivateBrowsing/testAboutCache.js
@@ +11,5 @@
>                        "http://domain2.mozqa.com"];
>  
>  function setupModule() {
>    controller = mozmill.getBrowserController();
> +  pb = new privateBrowsing.PrivateBrowsingWindow(controller);

Why are you initializing the window with the current controller? It's not an active PB window. Only for those you can specify the controller as parameter.

::: tests/functional/testPrivateBrowsing/testAboutPrivateBrowsing.js
@@ +17,2 @@
>    controller = mozmill.getBrowserController();
> +  pb = new privateBrowsing.PrivateBrowsingWindow(controller);

Again, this is not a private browsing window so controller can't be specified as parameter.

@@ +39,5 @@
>    var statusText = new elementslib.ID(controller.tabs.activeTab, "errorShortDescTextNormal");
>    controller.waitForElement(statusText);
>    controller.assertText(statusText, issueDesc);
>  
> +  pb.open("callback", function (aController) {

aController needs to be removed. See my comment for the lib module. You will have to use 'controller' inside the callback.

::: tests/functional/testPrivateBrowsing/testStartStopPBMode.js
@@ +17,5 @@
>  function setupModule() {
>    controller = mozmill.getBrowserController();
>    modifier = controller.window.document.documentElement.
>               getAttribute("titlemodifier_privatebrowsing");
> +  pb = new privateBrowsing.PrivateBrowsingWindow(controller);

Again, this is not a pb window initially.

@@ +29,4 @@
>  }
>  
>  /**
> + * Start Private Browsing Mode through Keyboard shortcut

nit: Only start or also close?

@@ +35,2 @@
>    // Open local pages in separate tabs and wait for each to finish loading
> +  LOCAL_TEST_PAGES.forEach(function(aPage) {

Why do we have to do that? It's similar to other tests where we don't have to load any pages in tabs anymore.

Comment 74

[Daniela Petrovici]

Attachment: private-browsing and tests patch v2.1

changed the previous patch per the latest review

Comment 75

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 697472 [details] [diff] [review]
private-browsing and tests patch v2.1

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

::: lib/private-browsing.js
@@ +24,3 @@
>   *        MozMillController to use for the modal entry dialog
>   */
> +function PrivateBrowsingWindow() {

Why have you removed the aController parameter? That's necessary and I haven't mentioned somewhere that this should be removed.

@@ +36,2 @@
>     *
> +   * @returns {MozMillController} Controller of the private browing window

it's enough to say 'window'.

@@ +72,2 @@
>     */
> +  open: function PBW_open(aController, aMethod, aCallback) {

I do not see a jsdoc entry for the newly added aController parameter.

Also if this parameter is null the internal controller should be used. Also what should happen if both are null?

@@ +82,5 @@
> +        var cmdKey = utils.getEntity(this.getDtds(), "privateBrowsingCmd.commandkey");
> +        aController.keypress(null, cmdKey, {accelKey: true, shiftKey: true});
> +        break;
> +      case "callback":
> +        if (typeof aCallback != "function")

Triple operator and brackets around as usual please.

@@ +84,5 @@
> +        break;
> +      case "callback":
> +        if (typeof aCallback != "function")
> +          throw new Error("No callback given for opening the private browsing window");
> +        aCallback();

Hm. Lets revert my decision from last time and pass in the specified controller from above. It would help a lot depending on how the callback is implemented (inline or not).

@@ +107,4 @@
>     */
> +  close: function PBW_close() {
> +    if(!this._controller.window)
> +      throw new Error("The private window exists");

We do not want to throw here. This check should have been part of the same if block. Also keep in mind that this will fail if _controller is null. Most likely it's better to update the value of _controller based on the window close event and reset it to null too.

@@ +131,5 @@
> + */
> +function isPrivateBrowsingWindow(aController) {
> +  if (!aController)
> +    throw new Error(arguments.callee.name + ": aController is null");
> +  return PrivateBrowsingUtils.isWindowPrivate(aController.window);

nit: I miss brackets and an blank line before the return line. Please keep our style guide.

Comment 76

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 697472 [details] [diff] [review]
private-browsing and tests patch v2.1

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

::: tests/functional/testPrivateBrowsing/testStartStopPBMode.js
@@ +17,4 @@
>  }
>  
>  /**
> + * Start and close Private Browsing Mode through Keyboard shortcut

This should be: 'Open and Close..."

Comment 77

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Skip patch

Given that we will most likely not be able to fix that soon lets finally skip the private browsing tests for now.

Comment 78

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 697870 [details] [diff] [review]
Skip patch

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

Looks good, just the outstanding question on the crash manifest.

::: tests/crash/manifest.ini
@@ +1,1 @@
> +disabled = Bug 818456 - Investigate and prepare existing Mozmill tests for per window private browsing

Why are we adding this line to an otherwise empty file?

Comment 79

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Skip patch v2

Updated skip patch by removing the line in the crash manifest which is not necessary anymore given that the related test has already been removed earlier on that bug.

Comment 80

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 697875 [details] [diff] [review]
Skip patch v2

http://hg.mozilla.org/qa/mozmill-tests/rev/094bf69b3a5f (default)

Comment 81

[Daniela Petrovici]

Attachment: private-browsing and tests patch v2.2

Related to the controllers being null when opening a private browsing window: In case both are null, we can initialize aController with mozmill.getBrowserController(); since we need this controller to open the PB window.

Comment 82

[Daniela Petrovici]

Attachment: private-browsing and tests patch v2.3

Due to the fact that the tests were previously skipped, the patch added above did not apply correctly.

Comment 83

[u279076]

I'd just like to add a note to this bug that Per-Window Private Browsing is tentatively scheduled to ship in Firefox 20 (moving to Aurora next week).

Comment 84

[Andrei Eftimie]

Attachment: private-browsing and tests patch v2.4

Just a small change to support:
> pb.open();
without a controller as param.

Comment 85

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 698678 [details] [diff] [review]
private-browsing and tests patch v2.4

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

::: lib/private-browsing.js
@@ +79,5 @@
> +    if (!aController) {
> +      if (this._controller !== null) {
> +        aController = this._controller;
> +      } else {
> +        aController = mozmill.getBrowserController();

We cannot use this construct  because Mozmill will automatically open a new browser window. Also I don't think we should make it that complicated. Just do:

var controller = aController || this._controller;
assert.ok(controller, "A controller has to be specified");

Also in the future please do not overwrite values of parameters which have been passed to that function.

@@ +93,5 @@
> +        aController.keypress(null, cmdKey, {accelKey: true, shiftKey: true});
> +        break;
> +      case "callback":
> +        if (typeof aCallback !== "function") {
> +          throw new Error("No callback given for opening the private browsing window");

I would say that we should change as much as possible throw statements into assert.* calls. Would you mind filing a followup but on it?

@@ +130,2 @@
>      }
> +    this._controller = null;

This code doesn't look that stable to me. It wouldn't set this._controller to null if one of the conditions hasn't been met. It's not unreliable. As I have mentioned in my last review we really should rely on the window close event.

::: tests/endurance/manifest.ini
@@ +10,1 @@
>  disabled = Bug 818456 - Investigate and prepare existing Mozmill tests for per window private browsing

You want to re-enable all the skipped tests.

::: tests/endurance/testPrivateBrowsing_EnterAndLeave/test1.js
@@ +19,2 @@
>    enduranceManager.run(function () {
> +    var pb = new privateBrowsing.PrivateBrowsingWindow();

It's a private browsing window. To make the variable more descriptive would you mind to update it to pbWindow across the tests?

Comment 86

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #85)
> Comment on attachment 698678 [details] [diff] [review]
> private-browsing and tests patch v2.4
> 
> Review of attachment 698678 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/private-browsing.js
> @@ +79,5 @@
> > +    if (!aController) {
> > +      if (this._controller !== null) {
> > +        aController = this._controller;
> > +      } else {
> > +        aController = mozmill.getBrowserController();
> 
> We cannot use this construct  because Mozmill will automatically open a new
> browser window. Also I don't think we should make it that complicated. Just
> do:
> 
> var controller = aController || this._controller;
> assert.ok(controller, "A controller has to be specified");

This will not allow us to do:

pb = new privateBrowsing.PrivateBrowsingWindow();
pb.open();
pb.controller // is the controller for the Private Browsing WIndow

We do not know the controller for the PB until we open the window...
pb.controller should return the Private Window controller, not the controller used to open that window.

Comment 87

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andrei Eftimie from comment #86)
> > var controller = aController || this._controller;
> > assert.ok(controller, "A controller has to be specified");
> 
> This will not allow us to do:
> 
> pb = new privateBrowsing.PrivateBrowsingWindow();
> pb.open();
> pb.controller // is the controller for the Private Browsing WIndow
> 
> We do not know the controller for the PB until we open the window...
> pb.controller should return the Private Window controller, not the
> controller used to open that window.

No, there are two use cases here. You create a wrapper for a new private browsing window or an existing one:

new:      pb = new privateBrowsing.PrivateBrowsingWindow();
existing: pb = new privateBrowsing.PrivateBrowsingWindow(controller);

If you call the open() method later you could specify a controller or not. 

But you are right in terms that this flow will not work. But the reason is that we would overwrite the internally stored controller for the current window by the newly opened window:

pb1 = new privateBrowsing.PrivateBrowsingWindow();
pb1.open(controller);
pb1.open();

The code I have proposed in my last comment is not different to the code in question under review. It was just the removal of the complex if condition and the getBrowserController() call.

I think we have to force that the controller has to be specified in the open() method.

Comment 88

[Daniela Petrovici]

(In reply to Henrik Skupin (:whimboo) from comment #85)
> Comment on attachment 698678 [details] [diff] [review]
> private-browsing and tests patch v2.4
> 
> Review of attachment 698678 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +130,2 @@
> >      }
> > +    this._controller = null;
> 
> This code doesn't look that stable to me. It wouldn't set this._controller
> to null if one of the conditions hasn't been met. It's not unreliable. As I
> have mentioned in my last review we really should rely on the window close
> event.

I have added now an event listener to the private browsing close function, but there is an issue.

When I am trying to run the newly created testOpenClosePBKeyboardShortcut.js, it fails in the tearDown due to the fact that the pbWindow.close() is called again there. 

Please tell me if I should remove the tearDown from that test (since it is only trying to close the pb window that was already closed). In case I shouldn't remove it, I can add a check to see if the _controller is not null before attaching an event listener to it.

Comment 89

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Daniela Petrovici from comment #88)
> I have added now an event listener to the private browsing close function,
> but there is an issue.
> 
> When I am trying to run the newly created
> testOpenClosePBKeyboardShortcut.js, it fails in the tearDown due to the fact
> that the pbWindow.close() is called again there. 

I don't think you set this._controller to null when the underlying window has been successfully closed. I'm not sure why you propose to remove pb.close() in teardownModule given that we have it already discussed that this call is important to clean up the test if it gets aborted to early.

Comment 90

[Daniela Petrovici]

Attachment: private-browsing and tests patch v2.5

(In reply to Henrik Skupin (:whimboo) from comment #85)
> Comment on attachment 698678 [details] [diff] [review]
> private-browsing and tests patch v2.4
> 
> Review of attachment 698678 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +93,5 @@
> > +        aController.keypress(null, cmdKey, {accelKey: true, shiftKey: true});
> > +        break;
> > +      case "callback":
> > +        if (typeof aCallback !== "function") {
> > +          throw new Error("No callback given for opening the private browsing window");
> 
> I would say that we should change as much as possible throw statements into
> assert.* calls. Would you mind filing a followup but on it?

Logged bug 828185 for this

> @@ +130,2 @@
> >      }
> > +    this._controller = null;
> 
> This code doesn't look that stable to me. It wouldn't set this._controller
> to null if one of the conditions hasn't been met. It's not unreliable. As I
> have mentioned in my last review we really should rely on the window close
> event.

Changed this code by adding an event listener in the close function. The tests are running properly now: http://mozmill-crowd.blargon7.com/#/functional/report/23d8fbdd0190d4b0496d6b129fd7e95c and http://mozmill-crowd.blargon7.com/#/functional/report/23d8fbdd0190d4b0496d6b129fd7eb9a

Please tell me in case this is not the correct approach.

Comment 91

[Daniela Petrovici]

Attachment: private-browsing and tests patch v2.6

Added an observer in the close function of the private-browsing library and tested the following:
- if test fails, tearDown is executed and window is properly closed
- if test passes, the tearDown is executed, but we are not trying to close the window again

Comment 92

[Daniela Petrovici]

Attachment: private-browsing and tests patch v2.7

Based on the discussion from Ask an expert meeting (today), I have changed to close function so that the observer is not added in case the window was already closed.

Comment 93

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 700380 [details] [diff] [review]
private-browsing and tests patch v2.7

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

::: lib/private-browsing.js
@@ +81,2 @@
>  
> +    var controller = aController || this._controller;

Why is this code still in here? See the last comment's why it will not work.

@@ +124,4 @@
>        }
>      }
> +    //Try to close the window in case it is not already closed
> +    if (this._controller && this._controller.window && !this._controller.window.closed) {

Now that we set _controller to null we do not need all those additional checks. If it's null the window doesn't exist anymore.

@@ +151,5 @@
> + * @returns {boolean} True if the window is a private browsing window
> + */
> +function isPrivateBrowsingWindow(aController) {
> +  if (!aController) {
> +    throw new Error(arguments.callee.name + ": aController is null");

Instead of the name of the parameter please say that a controller is needed.

::: tests/functional/testPrivateBrowsing/testDownloadManagerClosed.js
@@ +15,5 @@
>                     LOCAL_TEST_FOLDER + "downloading/unknown_type.mtdl",
>                     LOCAL_TEST_FOLDER + "downloading/unknown_type.fmtd"
>                    ];
>  
> +const DOWNLOAD_PB = LOCAL_TEST_FOLDER + "downloading/unknown_type_pbWindow.stbf";

I don't expect any work been done in this test given that bug 823064 handles this case. Why are you working on it in parallel?

::: tests/functional/testPrivateBrowsing/testStartStopPBMode.js
@@ -46,5 @@
> -    controller.open(page.url);
> -    controller.waitForPageLoad();
> -
> -    var elem = new elementslib.ID(controller.tabs.activeTab, page.id);
> -    controller.assertNode(elem);

The removal of this line will collide with the patch on bug 809431. Please get the other bug fixed and base this patch ontop of its landing.

@@ -83,5 @@
> -
> -    // waitForElement is used on exit of PB mode because pages are loaded from bfcache
> -    var elem = new elementslib.ID(controller.tabs.getTab(i), LOCAL_TEST_PAGES[i].id);
> -    controller.waitForElement(elem);
> -    controller.assertNode(elem);

Same here.

Comment 94

[Daniela Petrovici]

Attachment: private-browsing and tests patch v2.8

Removed: var controller = aController || this._controller; since we need to enforce that the open function has to be given a controller

Removed additional checks from:   if (this._controller && this._controller.window && !this._controller.window.closed) {

Changed this line: throw new Error(arguments.callee.name + ": aController is null"); - so we say that the controller is needed

Removed changes for testDownloadManager test case since they are done in another bug, but modified the import of private browsing library since the test was failing with <TOP_LEVEL> error even though it was skipped

Created patch on top of changes for bug 809431

Comment 95

[Daniela Petrovici]

Attachment: private-browsing and tests patch v2.9

Performed a change to the testAboutPrivateBrowsing. Reports are below:

http://mozmill-crowd.blargon7.com/#/functional/report/9e41582ed5e806fa373351d9d30b5568
http://mozmill-crowd.blargon7.com/#/functional/report/9e41582ed5e806fa373351d9d30b71d7

Comment 96

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 702899 [details] [diff] [review]
private-browsing and tests patch v2.9

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

I think that looks pretty good now. Lets get it landed so we will have results for todays nightly build:

http://hg.mozilla.org/qa/mozmill-tests/rev/2ff03ef4ab5e (default)

Comment 97

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Can we land this on Aurora? Will it apply cleanly?

Comment 98

[Andreea Matei [:AndreeaMatei]]

Yes, checked now and it's applying cleanly.

Comment 99

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I would like to see results of a testrun on Aurora too before I want to merge it.

Comment 100

[Andreea Matei [:AndreeaMatei]]

OS X:
http://mozmill-crowd.blargon7.com/#/functional/report/7752524a41e6fe70d5ec0e3208003d0e

Linux:
http://mozmill-crowd.blargon7.com/#/functional/report/7752524a41e6fe70d5ec0e3208002e7a

Window:
http://mozmill-crowd.blargon7.com/#/functional/report/7752524a41e6fe70d5ec0e3208005543

Comment 101

[Henrik Skupin [:whimboo][⌚️UTC+1]]

http://hg.mozilla.org/qa/mozmill-tests/rev/ed4ad84a7229 (aurora)