Closed Bug 818456 Opened 7 years ago Closed 7 years ago

Investigate and prepare existing Mozmill tests for per window private browsing

Categories

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

defect

Tracking

(firefox19 unaffected, firefox20 fixed, firefox21 fixed)

RESOLVED FIXED
Tracking Status
firefox19 --- unaffected
firefox20 --- fixed
firefox21 --- fixed

People

(Reporter: whimboo, Assigned: AndreeaMatei)

References

Details

(Whiteboard: [mozmill-test-failure] s=121210 u=enhancement c=private_browsing p=1)

Attachments

(5 files, 29 obsolete files)

9.66 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
4.45 KB, patch
davehunt
: review+
davehunt
: checkin+
Details | Diff | Splinter Review
6.88 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
7.50 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
33.56 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
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.
See Also: → 818732
Priority: -- → P1
Assignee: nobody → alex.lakatos
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.
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.
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.
(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.
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.
Attached patch private-browsing.js patch v1.0 (obsolete) — Splinter Review
The new way of working with Private Browsing would be:
controller = mozmill.getBrowserController();
pb = new privateBrowsing.privateBrowsing(controller);
pbController = pb.open();
pb.close(pbController);
Attachment #691313 - Flags: feedback?(hskupin)
Attachment #691313 - Flags: feedback?(dave.hunt)
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
Attached patch private-browsing.js patch v2.0 (obsolete) — Splinter Review
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
Attachment #691313 - Attachment is obsolete: true
Attachment #691313 - Flags: feedback?(hskupin)
Attachment #691313 - Flags: feedback?(dave.hunt)
Attachment #691906 - Flags: feedback?(hskupin)
Attachment #691906 - Flags: feedback?(dave.hunt)
Attached patch skipPatch v1.0 (obsolete) — Splinter Review
skip patch in case the feature lands and our tests start failing
Attachment #691938 - Flags: review?(hskupin)
Attachment #691938 - Flags: review?(dave.hunt)
Attachment #691938 - Flags: review?(andreea.matei)
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.
Attachment #691906 - Flags: feedback?(hskupin)
Attachment #691906 - Flags: feedback?(dave.hunt)
Attachment #691906 - Flags: feedback-
Attached patch private-browsing.js patch v3.0 (obsolete) — Splinter Review
moved it under ui. used handleWindow. made isWindowPrivate a global method.
Attachment #691906 - Attachment is obsolete: true
Attachment #692276 - Flags: feedback?(hskupin)
Attachment #692276 - Flags: feedback?(dave.hunt)
Comment on attachment 692276 [details] [diff] [review]
private-browsing.js patch v3.0

This looks good to me.
Attachment #692276 - Flags: feedback?(hskupin)
Attachment #692276 - Flags: feedback?(dave.hunt)
Attachment #692276 - Flags: feedback+
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.
(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. :/
Blocks: 821734
Blocks: 821729
Blocks: 821727
Blocks: 821716
Blocks: 821718
Blocks: 821713
Blocks: 821706
(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 :)
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.
Status: NEW → ASSIGNED
Attached patch patch v1.0 (obsolete) — Splinter Review
private-browsing shared module and updated tests
Attachment #692371 - Flags: review?(hskupin)
Attachment #692371 - Flags: review?(dave.hunt)
Attachment #692371 - Flags: review?(andreea.matei)
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
Attachment #692371 - Flags: review?(hskupin)
Attachment #692371 - Flags: review?(dave.hunt)
Attachment #692371 - Flags: review?(andreea.matei)
Attachment #692371 - Flags: review-
Attached patch private-browsing patch v4.0 (obsolete) — Splinter Review
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.
Attachment #692276 - Attachment is obsolete: true
Attachment #692622 - Flags: review?(hskupin)
Attachment #692622 - Flags: review?(dave.hunt)
Blocks: 822188
Blocks: PBnGen
No longer blocks: 821706, 821713, 821716, 821718, 821727, 821729, 821734
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.
Attachment #692622 - Flags: review?(hskupin)
Attachment #692622 - Flags: review?(dave.hunt)
Attachment #692622 - Flags: review-
Blocks: 822121
(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.
Attached patch private-browsing.js patch v5.0 (obsolete) — Splinter Review
Added a callback to the opener. isPrivateBrowsingWindow now throws if aController is null. removed the controller setter.
Attachment #692622 - Attachment is obsolete: true
Attachment #692941 - Flags: review?(hskupin)
Attachment #692941 - Flags: review?(dave.hunt)
Attachment #692941 - Flags: review?(andreea.matei)
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.
Attachment #692941 - Flags: review?(hskupin)
Attachment #692941 - Flags: review?(dave.hunt)
Attachment #692941 - Flags: review?(andreea.matei)
Attachment #692941 - Flags: review+
(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.
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.
Attachment #693363 - Flags: review?(hskupin)
Attachment #693363 - Flags: review?(dave.hunt)
(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.
(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.
(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.
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.
Attachment #693376 - Flags: review?(hskupin)
Attachment #693376 - Flags: review?(dave.hunt)
Attachment #693376 - Flags: review?(andreea.matei)
Attachment #693376 - Attachment description: endurance/testPrivateBrowsing → endurance/testPrivateBrowsing patch v1.0
Attached patch testAboutCache.js (obsolete) — Splinter Review
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.
Attachment #693381 - Flags: review?(hskupin)
Attachment #693381 - Flags: review?(dave.hunt)
Removed testKeyboardShortcut.js and updated testStartStopPBMode.js (also will open PB with shortcut keys).
Attachment #693407 - Flags: review?(hskupin)
Attachment #693407 - Flags: review?(dave.hunt)
Attached patch testDisabledPermissions.js (obsolete) — Splinter Review
Modified testDisabledPermissions with new changes for PB mode
Attachment #693429 - Flags: review?(hskupin)
Attachment #693429 - Flags: review?(dave.hunt)
Attachment #693429 - Flags: review?(andreea.matei)
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.
Attachment #693363 - Flags: review?(hskupin)
Attachment #693363 - Flags: review?(dave.hunt)
Attachment #693363 - Flags: review+
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)
Attachment #693363 - Flags: checkin+
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.
Attachment #693376 - Flags: review?(hskupin)
Attachment #693376 - Flags: review?(dave.hunt)
Attachment #693376 - Flags: review?(andreea.matei)
Attachment #693376 - Flags: review-
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.
Attachment #693381 - Flags: review?(hskupin)
Attachment #693381 - Flags: review?(dave.hunt)
Attachment #693381 - Flags: review-
updated testAboutPrivateBrowsing.js
Attachment #693547 - Flags: review?(hskupin)
Attachment #693547 - Flags: review?(dave.hunt)
Attachment #693547 - Flags: review?(andreea.matei)
Attached patch testGeolocation.js (obsolete) — Splinter Review
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.
Attachment #693556 - Flags: review?(hskupin)
Attachment #693556 - Flags: review?(dave.hunt)
Attachment #693556 - Flags: review?(andreea.matei)
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.
Attachment #693407 - Flags: review?(hskupin)
Attachment #693407 - Flags: review?(dave.hunt)
Attachment #693407 - Flags: review-
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.
Attachment #693429 - Flags: review?(hskupin)
Attachment #693429 - Flags: review?(dave.hunt)
Attachment #693429 - Flags: review?(andreea.matei)
Attachment #693429 - Flags: review-
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.
Attachment #693547 - Flags: review?(hskupin)
Attachment #693547 - Flags: review?(dave.hunt)
Attachment #693547 - Flags: review?(andreea.matei)
Attachment #693547 - Flags: review-
Attached patch testDisabledPermissions.js (obsolete) — Splinter Review
Modified the testDisabledPermissions.js per review
Attachment #693429 - Attachment is obsolete: true
Attachment #693804 - Flags: review?(hskupin)
Attachment #693804 - Flags: review?(dave.hunt)
Attachment #693804 - Flags: review?(andreea.matei)
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.
Attachment #693376 - Flags: review-
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.
Attachment #693804 - Flags: review?(hskupin)
Attachment #693804 - Flags: review?(dave.hunt)
Attachment #693804 - Flags: review?(andreea.matei)
Attachment #693804 - Flags: review-
Addressed review comments.
Attachment #693407 - Attachment is obsolete: true
Attachment #693843 - Flags: review?(hskupin)
Attachment #693843 - Flags: review?(dave.hunt)
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.
Attachment #693556 - Flags: review?(hskupin)
Attachment #693556 - Flags: review?(dave.hunt)
Attachment #693556 - Flags: review?(andreea.matei)
Attachment #693556 - Flags: review-
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?
Attachment #693843 - Flags: review?(hskupin)
Attachment #693843 - Flags: review?(dave.hunt)
Attachment #693843 - Flags: review-
(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.
Attached patch testCloseWindow.js (obsolete) — Splinter Review
Attachment #693867 - Flags: review?(hskupin)
Attachment #693867 - Flags: review?(dave.hunt)
As discussed with Henrik on irc, this test is obsolete on the new PB mode.
Attachment #693867 - Attachment is obsolete: true
Attachment #693867 - Flags: review?(hskupin)
Attachment #693867 - Flags: review?(dave.hunt)
Attachment #693868 - Flags: review?(hskupin)
Attachment #693868 - Flags: review?(dave.hunt)
Attachment #693868 - Flags: review?(andreea.matei)
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
Attachment #693868 - Flags: review?(hskupin)
Attachment #693868 - Flags: review?(dave.hunt)
Attachment #693868 - Flags: review?(andreea.matei)
Attachment #693868 - Flags: review+
Attachment #693868 - Flags: checkin+
Depends on: 823064
Comment on attachment 693922 [details] [diff] [review]
remove tests testDisabledPermissions.js and testGeolocation.js

Removed tests for disable permissions and geoLocation
Attachment #693922 - Flags: review?(andreea.matei)
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.
Attachment #693922 - Flags: review?(hskupin)
Attachment #693922 - Flags: review?(dave.hunt)
Attachment #693922 - Flags: review?(andreea.matei)
Attachment #693922 - Flags: review+
Comment on attachment 693922 [details] [diff] [review]
remove tests testDisabledPermissions.js and testGeolocation.js

http://hg.mozilla.org/qa/mozmill-tests/rev/974d3008f567
Attachment #693922 - Flags: checkin+
Attachment #693556 - Attachment is obsolete: true
Attachment #693804 - Attachment is obsolete: true
Attachment #692371 - Attachment is obsolete: true
Comment on attachment 691938 [details] [diff] [review]
skipPatch v1.0

No, we don't want to skip all the tests. Marking as obsolete.
Attachment #691938 - Attachment is obsolete: true
Attachment #691938 - Flags: review?(hskupin)
Attachment #691938 - Flags: review?(dave.hunt)
Attachment #691938 - Flags: review?(andreea.matei)
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?
Attached patch testAboutCache.js (obsolete) — Splinter Review
Updated the testAboutCache.js.
I've let pb.close() into the teardownModule that Alex will be taking care of today
Attachment #693381 - Attachment is obsolete: true
Attachment #694273 - Flags: review?(hskupin)
Attachment #694273 - Flags: review?(dave.hunt)
(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.
(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...
Updated the manifest file and teardownModule.
Attachment #693843 - Attachment is obsolete: true
Attachment #694282 - Flags: review?(hskupin)
Attachment #694282 - Flags: review?(dave.hunt)
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.
Attachment #692941 - Attachment is obsolete: true
Attachment #693376 - Attachment is obsolete: true
Attachment #693547 - Attachment is obsolete: true
Attachment #694427 - Flags: review?(hskupin)
Attachment #694427 - Flags: review?(dave.hunt)
Attachment #694427 - Flags: review?(andreea.matei)
(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
While this will be nice to have, it's not gonna block us from shipping.
No longer blocks: PBnGen
(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.
(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 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?
Attachment #694427 - Flags: review?(hskupin)
Attachment #694427 - Flags: review?(dave.hunt)
Attachment #694427 - Flags: review?(andreea.matei)
Attachment #694427 - Flags: review-
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.
Attachment #694273 - Flags: review?(hskupin)
Attachment #694273 - Flags: review?(dave.hunt)
Attachment #694273 - Flags: review-
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?
Attachment #694282 - Flags: review?(hskupin)
Attachment #694282 - Flags: review?(dave.hunt)
Attachment #694282 - Flags: review+
updated private browsing library to use a simple if. added Andreeas' patches with her edits.
Attachment #694273 - Attachment is obsolete: true
Attachment #694282 - Attachment is obsolete: true
Attachment #694427 - Attachment is obsolete: true
Attachment #694817 - Flags: review?(hskupin)
Attachment #694817 - Flags: review?(dave.hunt)
Attachment #694817 - Flags: review?(andreea.matei)
Blocks: 823064
No longer depends on: 823064
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.
Attachment #694817 - Flags: review?(hskupin)
Attachment #694817 - Flags: review?(dave.hunt)
Attachment #694817 - Flags: review?(andreea.matei)
Attachment #694817 - Flags: review-
changed the previous patch per the latest review
Attachment #694817 - Attachment is obsolete: true
Attachment #697472 - Flags: review?(hskupin)
Attachment #697472 - Flags: review?(dave.hunt)
Attachment #697472 - Flags: review?(andreea.matei)
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.
Attachment #697472 - Flags: review?(hskupin)
Attachment #697472 - Flags: review?(dave.hunt)
Attachment #697472 - Flags: review?(andreea.matei)
Attachment #697472 - Flags: review-
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..."
Attached patch Skip patch (obsolete) — Splinter Review
Given that we will most likely not be able to fix that soon lets finally skip the private browsing tests for now.
Attachment #697870 - Flags: review?(dave.hunt)
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?
Attachment #697870 - Flags: review?(dave.hunt) → review-
Attached patch Skip patch v2Splinter Review
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.
Attachment #697875 - Flags: review+
Whiteboard: s=121210 u=enhancement c=private_browsing p=1 → [mozmill-test-skipped] s=121210 u=enhancement c=private_browsing p=1
Attachment #697870 - Attachment is obsolete: true
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.
Attachment #697472 - Attachment is obsolete: true
Attachment #697880 - Flags: review?(hskupin)
Attachment #697880 - Flags: review?(dave.hunt)
Attachment #697880 - Flags: review?(andreea.matei)
Due to the fact that the tests were previously skipped, the patch added above did not apply correctly.
Attachment #697880 - Attachment is obsolete: true
Attachment #697880 - Flags: review?(hskupin)
Attachment #697880 - Flags: review?(dave.hunt)
Attachment #697880 - Flags: review?(andreea.matei)
Attachment #697892 - Flags: review?(hskupin)
Attachment #697892 - Flags: review?(dave.hunt)
Attachment #697892 - Flags: review?(andreea.matei)
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).
Just a small change to support:
> pb.open();
without a controller as param.
Attachment #697892 - Attachment is obsolete: true
Attachment #697892 - Flags: review?(hskupin)
Attachment #697892 - Flags: review?(dave.hunt)
Attachment #697892 - Flags: review?(andreea.matei)
Attachment #698678 - Flags: review?(hskupin)
Attachment #698678 - Flags: review?(dave.hunt)
Attachment #698678 - Flags: review?(andreea.matei)
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?
Attachment #698678 - Flags: review?(hskupin)
Attachment #698678 - Flags: review?(dave.hunt)
Attachment #698678 - Flags: review?(andreea.matei)
Attachment #698678 - Flags: review-
(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.
(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.
(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.
(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.
(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.
Attachment #698678 - Attachment is obsolete: true
Attachment #699632 - Flags: review?(hskupin)
Attachment #699632 - Flags: review?(dave.hunt)
Attachment #699632 - Flags: review?(andreea.matei)
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
Attachment #699632 - Attachment is obsolete: true
Attachment #699632 - Flags: review?(hskupin)
Attachment #699632 - Flags: review?(dave.hunt)
Attachment #699632 - Flags: review?(andreea.matei)
Attachment #699864 - Flags: review?(hskupin)
Attachment #699864 - Flags: review?(dave.hunt)
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.
Attachment #699864 - Attachment is obsolete: true
Attachment #699864 - Flags: review?(hskupin)
Attachment #699864 - Flags: review?(dave.hunt)
Attachment #700380 - Flags: review?(hskupin)
Attachment #700380 - Flags: review?(dave.hunt)
Attachment #700380 - Flags: review?(andreea.matei)
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.
Attachment #700380 - Flags: review?(hskupin)
Attachment #700380 - Flags: review?(dave.hunt)
Attachment #700380 - Flags: review?(andreea.matei)
Attachment #700380 - Flags: review-
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
Attachment #700380 - Attachment is obsolete: true
Attachment #702730 - Flags: review?(hskupin)
Attachment #702730 - Flags: review?(dave.hunt)
Attachment #702730 - Flags: review?(hskupin)
Attachment #702730 - Flags: review?(dave.hunt)
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)
Attachment #702899 - Flags: review?(hskupin)
Attachment #702899 - Flags: review?(dave.hunt)
Attachment #702899 - Flags: review+
Attachment #702899 - Flags: checkin+
Can we land this on Aurora? Will it apply cleanly?
Yes, checked now and it's applying cleanly.
Assignee: alex.lakatos.dev → andreea.matei
I would like to see results of a testrun on Aurora too before I want to merge it.
http://hg.mozilla.org/qa/mozmill-tests/rev/ed4ad84a7229 (aurora)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [mozmill-test-skipped] s=121210 u=enhancement c=private_browsing p=1 → [mozmill-test-failure] s=121210 u=enhancement c=private_browsing p=1
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.