Create a mozmill test for entering permanent PB mode

RESOLVED WONTFIX

Status

Mozilla QA
Mozmill Tests
P3
normal
RESOLVED WONTFIX
5 years ago
2 years ago

People

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

Tracking

(Depends on: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=js], URL)

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

5 years ago
Create a mozmill tests that covers this MozTrap test:
https://moztrap.mozilla.org/manage/caseversion/41243/.

STR:
1. Launch the browser.
2. Open the Options (Prefereces) dialog on the Privacy tab.
3. Select "Use custom settings for history" for "Firefox will:" and check the "Always use private browsing mode" checkbox.
4. Cancel the operation from the Restart Firefox dialog that just appeared.
5. Make sure Firefox keeps using its regular session/mode now.
6. Repeat steps 2 and 3.
7. Click OK on the Restart Firefox dialog that just appeared.
8. After the restart, make sure private browsing mode is being used (even after performing other manual and automatic restarts).
9. Change the Options|Preferences->Privacy settings to "Remeber history". Make sure Firefox works fine back in its regular mode.
10. Repeat steps 1 to 9, but use "Never remeber history" instead of "Use custom settings for history"->"Always use private browsing mode".

Expected Behavior:
Changing from regular mode to permanent private browsing mode, including cancelling it,  works fine. Firefox changes modes according to what the user selected.
I want to work on this test.
Assignee: nobody → mariomihai22
Hi Mario, great! Let me know if you need any help getting started with this.
You can find information about setting up mozmill here:
https://developer.mozilla.org/en-US/docs/Mozmill

Our repository is here: http://hg.mozilla.org/qa/mozmill-tests

And a style guide we're using when writing our tests:
https://developer.mozilla.org/en-US/docs/Mozmill_Tests/Mozmill_Style_Guide
Whiteboard: [mentor=andreeamatei][lang=js]
Priority: -- → P2
(Reporter)

Comment 3

5 years ago
I think bug 851489 should be taken into consideration when creating this test.

If it gets created as one test for both Use Custom Settings and Never Remember History, the test will fail with bugs like 851489 before getting to verify NRH.

I think the best approach would be to create one test for Use Custom Settings and one for Never Remember History. 

Each of these tests would be a restart test with 3 parts/tests and the only difference between them would be the way you move to permanent PB in the first part/test.
(In reply to Ioana Budnar [QA] from comment #3)

I'd started to work on Use Custom Settings test. I will create another test for Never Remember History after finishing first one and if needed I will concatenate them before adding the patch/es.
Flags: needinfo?(hskupin)
(In reply to Ioana Budnar [QA] from comment #3)
> I think the best approach would be to create one test for Use Custom
> Settings and one for Never Remember History. 

Absolutely. We shouldn't mix up that different handling in a single test module.
Flags: needinfo?(hskupin)
Status: NEW → ASSIGNED
Priority: P2 → P3
Created attachment 732798 [details] [diff] [review]
Patch v1 - Use Custom Settings

Created test for enabling Permanent Private Browsing from Use Custom Settings.
Attachment #732798 - Flags: review?(andreea.matei)
Comment on attachment 732798 [details] [diff] [review]
Patch v1 - Use Custom Settings

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

Thanks for your work, it's quite a big test/patch :)

While it tests well and does what it's supposed to, I have a few suggestions.

Overall, please add js doc to the functions so others can know more of what a specific method should do. Different names depending on the test would also be nice (see testPermanentBrowsing()).
The style guide and our tests are a good source for how the code format should be.

::: tests/functional/restartTests/testPermanentBrowsing/test1.js
@@ +3,5 @@
> +// Include required modules
> +var { assert, expect } = require("../../../../lib/assertions");
> +var prefs = require("../../../../lib/prefs");
> +var utils = require("../../../../lib/utils");
> +var modalDialog = require("../../../../lib/modal-dialog");

We sort these alphabetically, please reorder them and add a blank line after these declarations and the consts ones. Same applies for all test files.

@@ +4,5 @@
> +var { assert, expect } = require("../../../../lib/assertions");
> +var prefs = require("../../../../lib/prefs");
> +var utils = require("../../../../lib/utils");
> +var modalDialog = require("../../../../lib/modal-dialog");
> +const TIMEOUT = 5000;

I don't see this used anywhere, we can remove it (also if it's used for methods that by default wait for 5000ms)

@@ +6,5 @@
> +var utils = require("../../../../lib/utils");
> +var modalDialog = require("../../../../lib/modal-dialog");
> +const TIMEOUT = 5000;
> +const TIMEOUT_USER_SHUTDOWN = 2000;
> +const PREF_PB_AUTOSTART = "browser.privatebrowsing.autostart";

We should separate these 2 constants with a blank line since are not connected.

@@ +8,5 @@
> +const TIMEOUT = 5000;
> +const TIMEOUT_USER_SHUTDOWN = 2000;
> +const PREF_PB_AUTOSTART = "browser.privatebrowsing.autostart";
> +
> +var setupModule = function () {

Please add aModule here and in teardownModule, as a parameter, and then use it inside the functions (e.g controller should be aModule.controller).
Same applies for all test files.

@@ +10,5 @@
> +const PREF_PB_AUTOSTART = "browser.privatebrowsing.autostart";
> +
> +var setupModule = function () {
> +  controller = mozmill.getBrowserController();
> +  persisted.untouched_pref = prefs.preferences.getPref(PREF_PB_AUTOSTART, true);

I think we can move this one on the main function.

@@ +17,5 @@
> +var testPermanentBrowsing = function () {
> +  // Call preferences dialog - UseCustomSettings (cancel the operation and keep using it's regular session)
> +  prefs.openPreferencesDialog(controller, useCustomSettings);
> +  // Call preferences dialog - UseCostomSetiings (enable the operation and restart FF)
> +  prefs.openPreferencesDialog(controller, useCustomSettingsEnabled);

These 2 methods (useCustomSettings) have duplicate code, I think it would be best if we add a new method in the pref library that we then use it here. The differences as "check/uncheck" and ok/cancel button can be used as parameters. If there are more options in the dropdown, a switch would be nice.

@@ +49,5 @@
> +  prefDialog.close(true);
> +
> +  //Verify if the pref didn't change
> +  var curentPref = prefs.preferences.getPref(PREF_PB_AUTOSTART,true);
> +  assert.equal(curentPref,persisted.untouched_pref, "The pref has changed");

These checks can be moved in testPermanentBrowsing(), after the call.

::: tests/functional/restartTests/testPermanentBrowsing/test2.js
@@ +16,5 @@
> +var testPermanentBrowsing = function () {
> +
> +  // Call preferences dialog - RememberHistory (cancel the operation and
> +  // keep using it's regular session)
> +  prefs.openPreferencesDialog(controller, useCustomSettings);

Same as in the first test, we can use these custom settings in a new method from the library.

@@ +25,5 @@
> +var useCustomSettings = function (aController) {
> +
> +  //Verify if brower is in private mode after restart
> +  var curentPref = prefs.preferences.getPref(PREF_PB_AUTOSTART, true);
> +  assert.notEqual(curentPref, persisted.untouched_pref, "The " + PREF_PB_AUTOSTART + " has not changed");

These can be moved above in testPermanentBrowsing before calling the preferences dialog.

@@ +42,5 @@
> +  //Close pref window
> +  prefDialog.close(true);
> +  //Verify you're still in PrivateBrowsing after canceling RestartFirefoxDialog.
> +  var curentPref = prefs.preferences.getPref(PREF_PB_AUTOSTART, true);
> +  assert.notEqual(curentPref, persisted.untouched_pref, "The " + PREF_PB_AUTOSTART + " has changed");

These ones also can be moved above, after the calling of prefs.

::: tests/functional/restartTests/testPermanentBrowsing/test3.js
@@ +3,5 @@
> +// Include required modules
> +var { assert, expect } = require("../../../../lib/assertions");
> +var prefs = require("../../../../lib/prefs");
> +var utils = require("../../../../lib/utils");
> +var modalDialog = require("../../../../lib/modal-dialog");

utils and modalDialog modules are not used in this test.

@@ +10,5 @@
> +const PREF_PB_AUTOSTART = "browser.privatebrowsing.autostart";
> +
> +var setupModule = function () {
> +  controller = mozmill.getBrowserController();
> +}

We need a teardownModule() as well, resetting and cleaning everything done in these tests.
Attachment #732798 - Flags: review?(andreea.matei) → review-
(In reply to Andreea Matei [:AndreeaMatei] from comment #7)

I've done most updates requested in Comment 7, except the one for adding a new method in the pref library. I'm not sure how to do this but I'll try next week to reach a way to complete this. Thanks for review!
Hi Mario,
Do you have any questions regarding that method, can I help? You can find me on IRC also, on #automation channel.
Hi Andreea, I am working to abstract the test but got some issues. I'll contact you on IRC
Created attachment 740752 [details] [diff] [review]
Patch v2 - Use Custom Settings

Hi Andreea, I have just finished updating v1 patch, as requested in Comment 7.
Attachment #740752 - Flags: review?(andreea.matei)
Comment on attachment 740752 [details] [diff] [review]
Patch v2 - Use Custom Settings

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

Finally done, sorry it took that long :)
Great progress, I can see how clean the tests look now, with all the code in the library. I was wondering if we should push it even further in the library code, but I came to the conclusion is easier to understand the way it is now. 
But when adding the other test, if there's duplicate code there, please integrate it in the current methods.
There are some comments inline, most repeat themselves, for comments, style guide, js documentation needed, but it's going well, we're getting close. 
Thanks for your work so far!

::: lib/prefs.js
@@ +372,5 @@
>      utils.handleWindow("type", prefWindowType, callback);
>    }
>  }
>  
> +function useCustomSettings(aController, handler) {

All these new methods need js doc added so we can explain what the method should do, which are the parameters, if they're optional or not and so on.
handler here would mean that we want to set custom settings and it's value is true by default. So lets call it aSetSuccess. If we use false, then it's clear we don't successfuly set the setting.

@@ +383,5 @@
> +  assert.waitFor(function () {
> +    return historyMode.getNode().value === "custom";
> +  }, "History mode is set to custom");
> +
> +  var alwaysUsePrivateMode = new elementslib.ID(aController.window.document,

These kind of elements would be best added in getElements() method.

@@ +396,5 @@
> +  md.waitForDialog();
> +
> +  assert.waitFor( function () {
> +    return alwaysUsePrivateMode.getNode().checked === handler;
> +    }, "Checked");

Indentation is wrong here, needs 2 spaces shifted to left and there's no need for the extra space after waitFor.

@@ +403,5 @@
> +    prefDialog.close(true);
> +  }
> +}
> +
> +function restoreRegularSession (aController, handler){

Please remove the space between function name and parameters, they're connected. handler should also have a more suggestive name, aRestoreSuccess maybe?

@@ +416,5 @@
> +  });
> +
> +  aController.waitForElement(historyMode);
> +  aController.select(historyMode, null, null, "remember");
> +

We need to wait for the dialog after we trigger it.

@@ +417,5 @@
> +
> +  aController.waitForElement(historyMode);
> +  aController.select(historyMode, null, null, "remember");
> +
> +  if (!handler){

Please add a space between last brackets.

@@ +422,5 @@
> +    prefDialog.close(true);
> +  }
> +}
> +
> +function handleModalDialog (controller, restart) {

Same as above for the space and we use parameter with 'a' prefix e.g. aController, aRestart.

@@ +425,5 @@
> +
> +function handleModalDialog (controller, restart) {
> +  if (restart) {
> +    var button = new elementslib.Lookup(controller.window.document,
> +                                        '/id("commonDialog")/anon({"anonid":"buttons"})/{"dlgtype":"accept"}');

This and the button in the else block exceeds 80 characters per line rule, could you please split the string? 
Even more, these 2 buttons should be in the getElements() method under one case, with subtypes, if they're not already.

::: tests/functional/restartTests/testPermanentBrowsing/test1.js
@@ +11,5 @@
> +const PREF_PB_AUTOSTART = "browser.privatebrowsing.autostart";
> +
> +var setupModule = function (aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  persisted.untouched_pref = prefs.preferences.getPref(PREF_PB_AUTOSTART, true);

We need a blank line between those two, are not connected. I would call the pref persisted.default_pref

@@ +14,5 @@
> +  aModule.controller = mozmill.getBrowserController();
> +  persisted.untouched_pref = prefs.preferences.getPref(PREF_PB_AUTOSTART, true);
> +}
> +
> +var testPermanentBrowsing = function () {

Here we test the use custom setting, so the name should reflect that. Also please add js doc for all the tests/methods.

@@ +15,5 @@
> +  persisted.untouched_pref = prefs.preferences.getPref(PREF_PB_AUTOSTART, true);
> +}
> +
> +var testPermanentBrowsing = function () {
> +  // Call preferences dialog - UseCustomSettings (cancel the operation and keep

Lets leave only the explanation from the round brackets. The rest is readable in the methods name, that we call the pref dialog and useCustomSettings.

@@ +21,5 @@
> +  prefs.openPreferencesDialog(controller, function (aController) {
> +    prefs.useCustomSettings(aController, false);
> +  });
> +
> +  // Verify if the pref didn't change

No need for this comment as it's clear from the assert call and message.

@@ +22,5 @@
> +    prefs.useCustomSettings(aController, false);
> +  });
> +
> +  // Verify if the pref didn't change
> +  var curentPref = prefs.preferences.getPref(PREF_PB_AUTOSTART, true);

nit: current, but since we're not using it again, we could call it inside the assert call.

@@ +23,5 @@
> +  });
> +
> +  // Verify if the pref didn't change
> +  var curentPref = prefs.preferences.getPref(PREF_PB_AUTOSTART, true);
> +  assert.equal(curentPref, persisted.untouched_pref, "The pref has not changed");

Our messages should be positive and reflect the expected state, here the preference remained as default.

@@ +25,5 @@
> +  // Verify if the pref didn't change
> +  var curentPref = prefs.preferences.getPref(PREF_PB_AUTOSTART, true);
> +  assert.equal(curentPref, persisted.untouched_pref, "The pref has not changed");
> +
> +  // Call preferences dialog - UseCostomSettings (enable the operation and restart FF)

Same as above, comment should only explain the method.

::: tests/functional/restartTests/testPermanentBrowsing/test2.js
@@ +13,5 @@
> +var setupModule = function(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +}
> +
> +var testPermanentBrowsing = function () {

Here we test the remeber setting, so the name should reflect that.

@@ +15,5 @@
> +}
> +
> +var testPermanentBrowsing = function () {
> +  //Verify if brower is in private mode after restart
> +  var curentPref = prefs.preferences.getPref(PREF_PB_AUTOSTART, true);

Space after // and the comment and nit: current

@@ +17,5 @@
> +var testPermanentBrowsing = function () {
> +  //Verify if brower is in private mode after restart
> +  var curentPref = prefs.preferences.getPref(PREF_PB_AUTOSTART, true);
> +  assert.notEqual(curentPref, persisted.untouched_pref,
> +                  "The " + PREF_PB_AUTOSTART + " has changed");

I think we're more interested in the behaviour the change has, like "The browser's private mode has been changed"

@@ +20,5 @@
> +  assert.notEqual(curentPref, persisted.untouched_pref,
> +                  "The " + PREF_PB_AUTOSTART + " has changed");
> +
> +  // Call preferences dialog - RememberHistory (cancel the operation and
> +  // keep using it's private session)

As before, please leave the explaining part only for all the methods.

@@ +26,5 @@
> +    prefs.restoreRegularSession(aController, false);
> +  });
> +
> +  //Verify you're still in PrivateBrowsing after canceling RestartFirefoxDialog.
> +  var curentPref = prefs.preferences.getPref(PREF_PB_AUTOSTART, true);

The variable is already declared above, you're just checking again it's value, so no need for var here.

@@ +28,5 @@
> +
> +  //Verify you're still in PrivateBrowsing after canceling RestartFirefoxDialog.
> +  var curentPref = prefs.preferences.getPref(PREF_PB_AUTOSTART, true);
> +  assert.notEqual(curentPref, persisted.untouched_pref,
> +                  "The " + PREF_PB_AUTOSTART + " has not changed");

Same as above.

::: tests/functional/restartTests/testPermanentBrowsing/test3.js
@@ +20,5 @@
> +
> +var testPermanentBrowsing = function () {
> +  var curentPref = prefs.preferences.getPref(PREF_PB_AUTOSTART, true);
> +  assert.equal(curentPref,persisted.untouched_pref,
> +               "The " + PREF_PB_AUTOSTART + " has not changed");

Same comments as for the other test.
Attachment #740752 - Flags: review?(andreea.matei) → review-
Attachment #732798 - Attachment is obsolete: true
Thanks for review Andreea, I'm on my way to update tests as requested, I'll probably have a final version Thursday afternoon, in worst case on Friday.
I'll also attach the test suite for the second way of entering PB, called neverRemember.
Created attachment 754453 [details] [diff] [review]
Patch_v3

Andreea please take a look when have time at the updated patch that I had attached.
Attachment #754453 - Flags: review?(andreea.matei)
Comment on attachment 754453 [details] [diff] [review]
Patch_v3

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

Hi Mario, thanks for the patch! There are still some things to change, but overall looks way better, being so compact!

I'm also missing the manifest file from the restartTests folder.
Test Use custom settings fails on OS X, it appears it's not able to handle the modal dialog and the other test also fails, not sure if the preference is changed.

::: lib/prefs.js
@@ +373,5 @@
>    }
>  }
>  
> +/**
> + * [remember_dontRemember description]

Let's use here a description of the function, like "Change browser preferences to remember or not remember  history" or a better one.

@@ +380,5 @@
> + *         If true, the pref is changed from regular to private and reverse.
> + *         If false, the pref remains in it's current status.
> + * @param  {boolean} aRemember
> + *         (Optional)
> + *         The preference to get the value of. (remember / dontRemember)

We would need here also the description for "optional", what happens when we don't give this parameter.

@@ +386,5 @@
> +
> +function permanentPrivateBrowsing(aController, aRestoreSucces, aRemember) {
> +  var prefDialog = new this.preferencesDialog(aController);
> +  prefDialog.paneId = "panePrivacy";
> +  var historyMode = new elementslib.ID(aController.window.document, "historyMode");

Usually we separate blocks of code that are not related by an empty line. In this case, we open Preferences, switch to a pane. That's a block. Then we do some stuff on that pane (another block).

Please move the historyMode element to getElements()

@@ +390,5 @@
> +  var historyMode = new elementslib.ID(aController.window.document, "historyMode");
> +  aController.waitForElement(historyMode);
> +  var md = new modalDialog.modalDialog(aController.window);
> +
> +  //If aRemember is not null, NeverRememberHistory method is used to enable PB

This comment doesn't reflect the code process, I don't see a neverRememberHistory method, instead it will go to "use custom settings".

@@ +392,5 @@
> +  var md = new modalDialog.modalDialog(aController.window);
> +
> +  //If aRemember is not null, NeverRememberHistory method is used to enable PB
> +  if (aRemember) {
> +     md.start(function(aController) {

This line has to be shifted one space to the left.

@@ +400,5 @@
> +    if (aRemember == "remember") {
> +      aController.select(historyMode, null, null, "remember");
> +    }
> +    else {
> +    aController.select(historyMode, null, null, "dontremember");

Indentation issue

@@ +404,5 @@
> +    aController.select(historyMode, null, null, "dontremember");
> +    }
> +  }
> +
> +  //If aRemember = null, UseCustom settings method is used to enable PB.

As mentioned above, this should be explained in the jsdoc.

@@ +412,5 @@
> +      handleModalDialog(aController, aRestoreSucces);
> +    });
> +
> +    var alwaysUsePrivateMode = new elementslib.ID(aController.window.document,
> +                                                  "privateBrowsingAutoStart");

Lets move this in getElement()

@@ +418,5 @@
> +    md.waitForDialog();
> +
> +    assert.waitFor(function () {
> +      return alwaysUsePrivateMode.getNode().checked === aRestoreSucces;
> +    }, "Checked");

We would need a better message here.

@@ +422,5 @@
> +    }, "Checked");
> +  }
> +
> +  if (!aRestoreSucces) {
> +      prefDialog.close(true);

Indentation here should be 2 spaces from the parent.

@@ +443,5 @@
> +    aController.startUserShutdown(TIMEOUT_USER_SHUTDOWN, true);
> +  } else {
> +    var button = new elementslib.Lookup(aController.window.document,
> +                                        '/id("commonDialog")/anon({"anonid":"buttons"})' +
> +                                        '/{"dlgtype":"cancel"}');

I think I've mentioned this before, could you please move these elements to the getElements() method?

::: tests/functional/restartTests/testPreferences_NeverRememberHistoryPB/test1.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +Cu.import("resource://gre/modules/Services.jsm");

Are we using this anywhere? (for all tests)

@@ +4,5 @@
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +// Include required modules
> +var { assert, expect } = require("../../../../lib/assertions");

We aren't using expect (for all tests)

@@ +16,5 @@
> +  persisted.default_pref = prefs.preferences.getPref(PREF_PB_AUTOSTART, true);
> +}
> +
> +/**
> + * Enabling PB from neverRemember pref.

Wouldn't be better "Enable private browsing mode with "Never remember history" preference"?

@@ +25,5 @@
> +    prefs.permanentPrivateBrowsing(aController, false, "dontremember");
> +  });
> +
> +  assert.equal(prefs.preferences.getPref(PREF_PB_AUTOSTART, true), persisted.default_pref,
> +               "The browser's private mode has not changed");

The error message appears when the condition is not true, so the message has to reflect that. The browser PB mode shouldn't have changed, so the message should be that it has changed, because it appears when the assert fails. This is available for all instances, more than that, we usually use positive messages.

::: tests/functional/restartTests/testPreferences_NeverRememberHistoryPB/test2.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +Cu.import("resource://gre/modules/Services.jsm");

Are we using this anywhere?

::: tests/functional/restartTests/testPreferences_UseCustomSettingPB/test1.js
@@ +21,5 @@
> + */
> +var testEnableUseCustomSettings = function () {
> +  // Cancel the operation and keep using it's regular session
> +  prefs.openPreferencesDialog(controller, function (aController) {
> +    prefs.permanentPrivateBrowsing(aController, false, null, true);

This has too many parameters, the method has only 3.

@@ +29,5 @@
> +               "The browser's private mode has not changed");
> +
> +  // Enable the operation and restart FF in pb mode.
> +  prefs.openPreferencesDialog(controller, function (aController) {
> +    prefs.permanentPrivateBrowsing(aController, true, null, true);

Same here.
Attachment #754453 - Flags: review?(andreea.matei) → review-
Thanks Andreea for review. I'll update the test as mentioned in Comment 16 as soon as possible.
Created attachment 771290 [details] [diff] [review]
finalVersion
Attachment #771290 - Flags: review?(andreea.matei)
Comment on attachment 771290 [details] [diff] [review]
finalVersion

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

Please add a commit message to the patch and obsolete the other ones.

You're also missing the manifest from restartTests. 

These tests fail for me on OS X.

::: lib/prefs.js
@@ +176,5 @@
>          break;
> +      case "md_button":
> +        elem = new elementslib.Lookup(this._controller.window.document, PREF_MODAL_DIALOG_BUTTONS +
> +                                      '/{"dlgtype":"' + spec.subtype + '"}');
> +        break;

We should keep this sorted alphabetically.

@@ +200,5 @@
>                                        '/{"pane":"' + spec.value + '"}');
>          break;
> +      case "id":
> +        elem = new elementslib.ID(this._controller.window.document, spec.id);
> +        break;

Why id and not have the elements here like we do on the other libraries?

@@ +397,5 @@
> + * @param  {boolean} aRemember
> + *         (Optional)
> + *         If aRemember is null, useCustomSettings method is used to enable PB
> +           If aRemember is not null, NeverRememberHistory method is used to enable PB
> + */

Please see bug 882137 regarding how the jsdoc should look like, when we have optional parameters. Also we can remove the [] from the first line.

@@ +405,5 @@
> +  prefDialog.paneId = "panePrivacy";
> +  var historyMode = prefDialog.getElement({
> +    type: "id",
> +    id: "historyMode"
> +  });

If we'll have the element as a case as mentioned above, only type will be necessary, looking more compact. And please have this on a single line, I think it won't exceed the 80 chars limit. Same applies for all instances in this file.

@@ +421,5 @@
> +      aController.select(historyMode, null, null, "dontremember");
> +    }
> +  }
> +
> +  else {

No need for the extra blank line, if/else are bonded.

@@ +436,5 @@
> +    md.waitForDialog();
> +
> +    assert.waitFor(function () {
> +      return alwaysUsePrivateMode.getNode().checked === aRestoreSucces;
> +    }, "Checked");

I think I already mentioned this message in the previous reviews, please address all requests.

::: tests/functional/restartTests/testPreferences_NeverRememberHistoryPB/test1.js
@@ +14,5 @@
> +  persisted.default_pref = prefs.preferences.getPref(PREF_PB_AUTOSTART, true);
> +}
> +
> +/**
> + * Enable private browsing mode with "Never remember history" preference".

Extra quote

::: tests/functional/restartTests/testPreferences_NeverRememberHistoryPB/test2.js
@@ +16,5 @@
> + * Verify PB is enabled and then restore regular session.
> + */
> +var testRestoreRegularSession = function () {
> +  assert.notEqual(prefs.preferences.getPref(PREF_PB_AUTOSTART, true), persisted.default_pref,
> +                  "The browser's private mode has not been changed");

Positive message, "has been changed" here as well.

::: tests/functional/restartTests/testPreferences_NeverRememberHistoryPB/test3.js
@@ +13,5 @@
> +}
> +
> +var teardownModule = function (aModule) {
> +  prefs.preferences.clearUserPref(PREF_PB_AUTOSTART);
> +}

We might also need to delete the persisted object here.

@@ +20,5 @@
> + * Verify regular session was enabled.
> + */
> +var testDefaultSession = function () {
> +  assert.equal(prefs.preferences.getPref(PREF_PB_AUTOSTART, true), persisted.default_pref,
> +               "The browser's private mode has not been changed");

Same here, please create a positive message.

::: tests/functional/restartTests/testPreferences_UseCustomSettingPB/test2.js
@@ +15,5 @@
> +/**
> + * Verify PB is enabled and then restore regular session.
> + */
> +var testRestoreRegularSession = function () {
> +  var currentPref = prefs.preferences.getPref(PREF_PB_AUTOSTART, true)

Semicolon needed.

@@ +17,5 @@
> + */
> +var testRestoreRegularSession = function () {
> +  var currentPref = prefs.preferences.getPref(PREF_PB_AUTOSTART, true)
> +  assert.notEqual(currentPref, persisted.default_pref,
> +                  "The browser's private mode has not been changed");

Same here.

::: tests/functional/restartTests/testPreferences_UseCustomSettingPB/test3.js
@@ +13,5 @@
> +}
> +
> +var teardownModule = function (aModule) {
> +  prefs.preferences.clearUserPref(PREF_PB_AUTOSTART);
> +}

We might also need to delete the persisted object here.

@@ +20,5 @@
> + * Verify regular session was enabled.
> + */
> +var testDefaultSession = function () {
> +  assert.equal(prefs.preferences.getPref(PREF_PB_AUTOSTART, true), persisted.default_pref,
> +              "The browser's private mode has not been changed");

Indentation, one space to the right.
Attachment #771290 - Flags: review?(andreea.matei) → review-
I think we can leave this one for someone else to continue.
Assignee: mihai.morar → nobody
Status: ASSIGNED → NEW
(Assignee)

Updated

4 years ago
Attachment #740752 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #754453 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Assignee: nobody → xabolcs
Status: NEW → ASSIGNED
(Assignee)

Comment 21

4 years ago
Hey, the updated patch is now applies cleanly, but doesn't work yet. :)

I get "No item selected for element ID: historyMode" error near |aController.select(historyMode ...)|,
but I don't know why. The historyMode element exists, and its a menulist, the selectable values also exist.

Could you help me?


(In reply to Andreea Matei [:AndreeaMatei] from comment #19)
> Comment on attachment 771290 [details] [diff] [review]
> finalVersion
> 
> Review of attachment 771290 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please add a commit message to the patch and obsolete the other ones.
> 
> You're also missing the manifest from restartTests. 
> 
> These tests fail for me on OS X.
> 
> ::: lib/prefs.js
> @@ +176,5 @@
> >          break;
> > +      case "md_button":
> > +        elem = new elementslib.Lookup(this._controller.window.document, PREF_MODAL_DIALOG_BUTTONS +
> > +                                      '/{"dlgtype":"' + spec.subtype + '"}');
> > +        break;
> 
> We should keep this sorted alphabetically.

Done.

> 
> @@ +200,5 @@
> >                                        '/{"pane":"' + spec.value + '"}');
> >          break;
> > +      case "id":
> > +        elem = new elementslib.ID(this._controller.window.document, spec.id);
> > +        break;
> 
> Why id and not have the elements here like we do on the other libraries?

Sorry, I don't get what you wrote here. Please explain a little bit how do you imagine the new case!

> 
> @@ +397,5 @@
> > + * @param  {boolean} aRemember
> > + *         (Optional)
> > + *         If aRemember is null, useCustomSettings method is used to enable PB
> > +           If aRemember is not null, NeverRememberHistory method is used to enable PB
> > + */
> 
> Please see bug 882137 regarding how the jsdoc should look like, when we have
> optional parameters. Also we can remove the [] from the first line.

Done.

> 
> @@ +405,5 @@
> > +  prefDialog.paneId = "panePrivacy";
> > +  var historyMode = prefDialog.getElement({
> > +    type: "id",
> > +    id: "historyMode"
> > +  });
> 
> If we'll have the element as a case as mentioned above, only type will be
> necessary, looking more compact. And please have this on a single line, I
> think it won't exceed the 80 chars limit. Same applies for all instances in
> this file.

Will do if I understand what you expect here about the "element" stuff.
The 80 chars limit is clear, for all instances in this file too, but not yet done. :)


> 
> @@ +421,5 @@
> > +      aController.select(historyMode, null, null, "dontremember");
> > +    }
> > +  }
> > +
> > +  else {
> 
> No need for the extra blank line, if/else are bonded.

Done by Mario.

> 
> @@ +436,5 @@
> > +    md.waitForDialog();
> > +
> > +    assert.waitFor(function () {
> > +      return alwaysUsePrivateMode.getNode().checked === aRestoreSucces;
> > +    }, "Checked");
> 
> I think I already mentioned this message in the previous reviews, please

Not yet addressed.


> address all requests.
> 
> :::
> tests/functional/restartTests/testPreferences_NeverRememberHistoryPB/test1.js
> @@ +14,5 @@
> > +  persisted.default_pref = prefs.preferences.getPref(PREF_PB_AUTOSTART, true);
> > +}
> > +
> > +/**
> > + * Enable private browsing mode with "Never remember history" preference".
> 
> Extra quote

Done.

> 
> :::
> tests/functional/restartTests/testPreferences_NeverRememberHistoryPB/test2.js
> @@ +16,5 @@
> > + * Verify PB is enabled and then restore regular session.
> > + */
> > +var testRestoreRegularSession = function () {
> > +  assert.notEqual(prefs.preferences.getPref(PREF_PB_AUTOSTART, true), persisted.default_pref,
> > +                  "The browser's private mode has not been changed");
> 
> Positive message, "has been changed" here as well.

Are you sure?
Currently it is failing, and it says:
> "The browser's private mode has not been changed - 'false' should not equal 'false'"

A failing test with this message is clear to me.

Regarding to your "positive message" nit, should I use assert.equal here instead assert.notEqual?

> 
> :::
> tests/functional/restartTests/testPreferences_NeverRememberHistoryPB/test3.js
> @@ +13,5 @@
> > +}
> > +
> > +var teardownModule = function (aModule) {
> > +  prefs.preferences.clearUserPref(PREF_PB_AUTOSTART);
> > +}
> 
> We might also need to delete the persisted object here.

Done, if you referred to |persisted.default_pref|.


> ::: tests/functional/restartTests/testPreferences_UseCustomSettingPB/*

Not worked yet with those files.
(Assignee)

Comment 22

4 years ago
Created attachment 8396035 [details] [diff] [review]
Patch v4.1 - adressing nits in progress, tests still failing

Just for reference. This is still a work in progress, not all nits were addressed.

Andreea feel free to write your thoughts again, or just simply put an f- on it. :)
Attachment #771290 - Attachment is obsolete: true
Attachment #8396035 - Flags: feedback?(andreea.matei)
(Assignee)

Comment 23

4 years ago
(In reply to Szabolcs Hubai (:xabolcs) from comment #21)
> Hey, the updated patch is now applies cleanly, but doesn't work yet. :)
> 
> I get "No item selected for element ID: historyMode" error near
> |aController.select(historyMode ...)|,
> but I don't know why. The historyMode element exists, and its a menulist,
> the selectable values also exist.
> 
> Could you help me?
> 
>

Why do I get this? Why can't Mozmill select something from a menulist?
Thanks, for helping!

> ERROR | Test Failure | {
>   "exception": {
>     "message": "No item selected for element ID: historyMode", 
>     "lineNumber": 1059, 
>     "name": "Error", 
>     "fileName": "resource://mozmill/driver/mozelement.js"
>   }
> }
Flags: needinfo?(andreea.matei)
(In reply to Szabolcs Hubai (:xabolcs) from comment #21)
> Hey, the updated patch is now applies cleanly, but doesn't work yet. :)
> 
> I get "No item selected for element ID: historyMode" error near
> |aController.select(historyMode ...)|,
> but I don't know why. The historyMode element exists, and its a menulist,
> the selectable values also exist.
> 
> Could you help me?
> 

It might be a time issue, the test happens so fast that if we don't wait enough, we might not see the element. 
You're on Mozmill 2.0.6 right? I'll test the patch and check more.

> > > +      case "id":
> > > +        elem = new elementslib.ID(this._controller.window.document, spec.id);
> > > +        break;
> > 
> > Why id and not have the elements here like we do on the other libraries?
> 
> Sorry, I don't get what you wrote here. Please explain a little bit how do
> you imagine the new case!

I mean to have the id specified in the elem itself, like:
elem = new elementslib.ID(this._controller.window.document, "historyMode);
If you see below, it's overcomplicated how it is right now, the code below will then be just:
var historyMode = prefDialog.getElement({type: "historyMode" });
Where "historyMode" as type is the case name of the switch.

> > @@ +405,5 @@
> > > +  prefDialog.paneId = "panePrivacy";
> > > +  var historyMode = prefDialog.getElement({
> > > +    type: "id",
> > > +    id: "historyMode"
> > > +  });

> > :::
> > tests/functional/restartTests/testPreferences_NeverRememberHistoryPB/test2.js
> > @@ +16,5 @@
> > > + * Verify PB is enabled and then restore regular session.
> > > + */
> > > +var testRestoreRegularSession = function () {
> > > +  assert.notEqual(prefs.preferences.getPref(PREF_PB_AUTOSTART, true), persisted.default_pref,
> > > +                  "The browser's private mode has not been changed");
> > 
> > Positive message, "has been changed" here as well.
> 
> Are you sure?
> Currently it is failing, and it says:
> > "The browser's private mode has not been changed - 'false' should not equal 'false'"
> 
> A failing test with this message is clear to me.
> 
> Regarding to your "positive message" nit, should I use assert.equal here
> instead assert.notEqual?

As requested in our style guide, we usually want positive messages. assert.notEqual is fine, the items shouldn't be equal. 
https://developer.mozilla.org/en-US/docs/Mozmill_Tests/Mozmill_Style_Guide#Error_Messages

> 
> > 
> > :::
> > tests/functional/restartTests/testPreferences_NeverRememberHistoryPB/test3.js
> > @@ +13,5 @@
> > > +}
> > > +
> > > +var teardownModule = function (aModule) {
> > > +  prefs.preferences.clearUserPref(PREF_PB_AUTOSTART);
> > > +}
> > 
> > We might also need to delete the persisted object here.
> 
> Done, if you referred to |persisted.default_pref|.
Yes, that's correct.

When you have a working version please also add andrei.eftimie as a reviewer. Thanks!
Flags: needinfo?(andreea.matei)
(Assignee)

Comment 25

4 years ago
(In reply to Andreea Matei [:AndreeaMatei] from comment #24)
> (In reply to Szabolcs Hubai (:xabolcs) from comment #21)
> > Hey, the updated patch is now applies cleanly, but doesn't work yet. :)
> > 
> > I get "No item selected for element ID: historyMode" error near
> > |aController.select(historyMode ...)|,
> > but I don't know why. The historyMode element exists, and its a menulist,
> > the selectable values also exist.
> > 
> > Could you help me?
> > 
> 
> It might be a time issue, the test happens so fast that if we don't wait
> enough, we might not see the element. 
> You're on Mozmill 2.0.6 right? I'll test the patch and check more.
> 

Sorry, but I can't get forward with this |menuitem.select()|.

Only with the following patch:

> diff -r f1b6348bde9f mozmill/mozmill/extension/resource/driver/mozelement.js
> --- a/mozmill/mozmill/extension/resource/driver/mozelement.js	Tue Mar 11 12:20:32 2014 +0100
> +++ b/mozmill/mozmill/extension/resource/driver/mozelement.js	Thu Mar 27 00:59:18 2014 +0100
> @@ -1035,6 +1035,7 @@
>          // Click the item
>          try {
>            item.click();
> +          this.element.selectedItem = item;
>  
>            var self = this;
>            var selected = index || option || value;


I'm trying to finish the patch till weekend.
Does 'item' actually receive the click? You could check this by enhancing the click() call here for the expected event parameter.
Comment on attachment 8396035 [details] [diff] [review]
Patch v4.1 - adressing nits in progress, tests still failing

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

We still need to figure out the failure, but as feedback this is looking good!

::: firefox/lib/prefs.js
@@ +392,5 @@
> + * Change browser preferences to remember or not remember history
> + * @param  {MozMillController} aController
> + * @param  {boolean} aRestoreSuccess
> + *         If true, the pref is changed from regular to private and reverse.
> + *         If false, the pref remains in it's current status.

nit: its

@@ +393,5 @@
> + * @param  {MozMillController} aController
> + * @param  {boolean} aRestoreSuccess
> + *         If true, the pref is changed from regular to private and reverse.
> + *         If false, the pref remains in it's current status.
> + * @param  {boolean} [aRemember]

If this is optional, we want to also add the default value it has, like: [aRemember=default_value]

::: firefox/tests/functional/restartTests/testPreferences_NeverRememberHistoryPB/test1.js
@@ +16,5 @@
> +
> +/**
> + * Enable private browsing mode with "Never remember history" preference.
> + */
> +var testEnableNeverRemember = function () {

Please use function testEnableNeverRemeber(), same for all the tests.

@@ +17,5 @@
> +/**
> + * Enable private browsing mode with "Never remember history" preference.
> + */
> +var testEnableNeverRemember = function () {
> +  // Cancel the operation and keep using it's regular session.

nit: its

@@ +31,5 @@
> +    prefs.permanentPrivateBrowsing(aController, true, "dontremember");
> +  });
> +}
> +
> +

Please remove these extra lines.
Attachment #8396035 - Flags: feedback?(andreea.matei) → feedback+
(Assignee)

Comment 28

4 years ago
Created attachment 8398222 [details] [diff] [review]
Patch v5 - addressed nits

Just finished with the fifth version, I hope all the nits were addressed.
Attachment #8396035 - Attachment is obsolete: true
Attachment #8398222 - Flags: feedback?(andrei.eftimie)
Attachment #8398222 - Flags: feedback?(andreea.matei)
(Assignee)

Comment 29

4 years ago
(In reply to Andreea Matei [:AndreeaMatei] from comment #16)
> ...
> Test Use custom settings fails on OS X, it appears it's not able to handle
> the modal dialog and the other test also fails, not sure if the preference
> is changed.
> 

Sadly I'm unable to test with Mac OS X, I need help in that issue!
Therefor is the feedback request instead of r?.


(In reply to Henrik Skupin (:whimboo) from comment #26)
> Does 'item' actually receive the click? You could check this by enhancing
> the click() call here for the expected event parameter.

I changed the click() to click(null, null, {type: "click"}), and to {type: "select"},
but didn't notice anything special.
I also tried this.click(), this.element.click() before and after the item.click(),
but nothing helps just setting the selectedItem after(!) the click.
Does a sleep(0) right before the item selection help here?

Comment 31

4 years ago
Comment on attachment 8398222 [details] [diff] [review]
Patch v5 - addressed nits

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

I remember working with this particular dropdown (historyMode) in the past and having issues with it (maybe even particularly on OSX).
But I can't find where or when this was. I'll try searching more deeply.

::: firefox/lib/prefs.js
@@ +411,5 @@
> +  });
> +
> +  if (typeof aRemember === "boolean") {
> +    if (aRemember) {
> +      aController.select(historyMode, null, null, "remember");

We should update these to mozmill 2.0 style: el.action instead of controller.action(el)
`historyMode.select`

@@ +417,5 @@
> +    else {
> +      aController.select(historyMode, null, null, "dontremember");
> +    }
> +  }
> +  else {

This is a bit hard to follow.
I see that in previous versions `aRemember` had the value we wanted directly.

Since we 3 possible states here "remember", "dontremember", "custom", I would argue that it would be easier to pass in the string directly. And instead of having these nested if statemenets we could simply call:
> aController.select(historyMode, null, null, aRemember);
This would cover all 3 states.

@@ +428,5 @@
> +    alwaysUsePrivateMode.click();
> +
> +    md.waitForDialog();
> +
> +    assert.waitFor(function () {

This should use fat arrow function syntax:
> () => {}

@@ +453,5 @@
> +      type: "md_button", subtype: "accept"
> +    });
> +
> +    aController.startUserShutdown(TIMEOUT_USER_SHUTDOWN, true);
> +  } else {

All else clauses should be on a newline.
> }
> else {
Attachment #8398222 - Flags: feedback?(andrei.eftimie) → feedback-
(Assignee)

Comment 32

4 years ago
(In reply to Henrik Skupin (:whimboo) from comment #30)
> Does a sleep(0) right before the item selection help here?

Element.click() already have one.


(In reply to Andrei Eftimie from comment #31)
> Comment on attachment 8398222 [details] [diff] [review]
> Patch v5 - addressed nits
> 
> Review of attachment 8398222 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I remember working with this particular dropdown (historyMode) in the past
> and having issues with it (maybe even particularly on OSX).
> But I can't find where or when this was. I'll try searching more deeply.
> 

Thanks!


> ::: firefox/lib/prefs.js
> @@ +411,5 @@
> > +  });
> > +
> > +  if (typeof aRemember === "boolean") {
> > +    if (aRemember) {
> > +      aController.select(historyMode, null, null, "remember");
> 
> We should update these to mozmill 2.0 style: el.action instead of
> controller.action(el)
> `historyMode.select`

Thanks for all the examples! It's more easier to address the nits with them.
Done.

> 
> @@ +417,5 @@
> > +    else {
> > +      aController.select(historyMode, null, null, "dontremember");
> > +    }
> > +  }
> > +  else {
> 
> This is a bit hard to follow.
> I see that in previous versions `aRemember` had the value we wanted directly.
> 
> Since we 3 possible states here "remember", "dontremember", "custom", I
> would argue that it would be easier to pass in the string directly. And
> instead of having these nested if statemenets we could simply call:
> > aController.select(historyMode, null, null, aRemember);
> This would cover all 3 states.

Yeah, done, the winner is |aHistoryMode|.
Could you propose a good jsdoc for it?

> 
> @@ +428,5 @@
> > +    alwaysUsePrivateMode.click();
> > +
> > +    md.waitForDialog();
> > +
> > +    assert.waitFor(function () {
> 
> This should use fat arrow function syntax:
> > () => {}
> 

Done.


> @@ +453,5 @@
> > +      type: "md_button", subtype: "accept"
> > +    });
> > +
> > +    aController.startUserShutdown(TIMEOUT_USER_SHUTDOWN, true);
> > +  } else {
> 
> All else clauses should be on a newline.
> > }
> > else {

Nothin to do. ;)
(Assignee)

Comment 33

4 years ago
Created attachment 8399000 [details] [diff] [review]
Patch v6

Still requesting just f? due the new jsdoc and the historyMode selection issue.
Attachment #8398222 - Attachment is obsolete: true
Attachment #8398222 - Flags: feedback?(andreea.matei)
Attachment #8399000 - Flags: feedback?(andrei.eftimie)
(Assignee)

Comment 34

4 years ago
(In reply to Szabolcs Hubai (:xabolcs) from comment #33)
> Created attachment 8399000 [details] [diff] [review]
> Patch v6
> 
> Still requesting just f? due the new jsdoc and the historyMode selection
> issue.

Ping for feedback.


(In reply to Andrei Eftimie from comment #31)
> Comment on attachment 8398222 [details] [diff] [review]
> Patch v5 - addressed nits
> 
> Review of attachment 8398222 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I remember working with this particular dropdown (historyMode) in the past
> and having issues with it (maybe even particularly on OSX).
> But I can't find where or when this was. I'll try searching more deeply.
> 

Any update on this?

Comment 35

4 years ago
(In reply to Szabolcs Hubai (:xabolcs) from comment #34)
> (In reply to Szabolcs Hubai (:xabolcs) from comment #33)
> > Created attachment 8399000 [details] [diff] [review]
> > Patch v6
> > 
> > Still requesting just f? due the new jsdoc and the historyMode selection
> > issue.
> 
> Ping for feedback.
> 
> 
> (In reply to Andrei Eftimie from comment #31)
> > Comment on attachment 8398222 [details] [diff] [review]
> > Patch v5 - addressed nits
> > 
> > Review of attachment 8398222 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I remember working with this particular dropdown (historyMode) in the past
> > and having issues with it (maybe even particularly on OSX).
> > But I can't find where or when this was. I'll try searching more deeply.
> > 
> 
> Any update on this?

Seems to be a bug in mozmill. I'm checking this more in depth.

Comment 36

4 years ago
Actually I don't think it's a bug from mozmill.

Here's what I've found, broken down in steps of what happens:
in test2 we:

1. check the the Private Mode is no enabled // pass
2. open the pref window
3. call permanentPrivateBrowsing with the intent to set "remember" and hit "Cancel" on the dialog that asks for a restart
4. "historyMode"s value now is "dontremember" // since we set it up in the previous test to "custom", should it be "custom" at this point?
5. we click on the "historyMode" dropdown's option "remember"
6. the dialog listener fires and we click on "cancel"
7. due to "Cancel" the value reverts to "dontremember"
8. we timeout in the "select" method waiting for the value to be set to "remember"

I'm not sure what the solution here should be. The waitFor in mozelement MozMillDropList.select method _should_ finish before we close the modal dialog... but that code appears not to run until the modal dialog is handled.

Comment 37

4 years ago
Comment on attachment 8399000 [details] [diff] [review]
Patch v6

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

I'm not sure how to fix the issue mentioned in comment 36.

However, the new preference window will land soon, so we will need to update this test to use that.
See bug 738797 for when that will land.

I do see the same behaviour of the historyMode dropdown in the new preference page as well, so it will probably not fix the issue we're having.

::: firefox/lib/prefs.js
@@ +404,5 @@
> +  prefDialog.paneId = "panePrivacy";
> +  var historyMode = prefDialog.getElement({type: "historyMode"});
> +  var md = new modalDialog.modalDialog(aController.window);
> +
> +  md.start(function(aController) {

You can use the fat arrow syntax for anonymous functions.

> aController => {}

::: firefox/tests/functional/restartTests/testPreferences_neverRememberHistoryPB/test1.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Include required modules
> +var { assert } = require("../../../../../lib/assertions");

the Assertion module is included by default now in test files. You can remove this require call.
Attachment #8399000 - Flags: feedback?(andrei.eftimie) → feedback-
(Assignee)

Comment 38

4 years ago
(In reply to Andrei Eftimie from comment #37)
> Comment on attachment 8399000 [details] [diff] [review]
> Patch v6
> 

Thanks for the feedback.
Will push the updated patch till weekend.


> Review of attachment 8399000 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure how to fix the issue mentioned in comment 36.
> 

It's OK to me to defer this fix until this historyMode dropdown + Mac issue get fixed.
But it's also OK to me if you slice that to a new bug to fix later (and skip this test on Mac).


> However, the new preference window will land soon, so we will need to update
> this test to use that.
> See bug 738797 for when that will land.
> 
> I do see the same behaviour of the historyMode dropdown in the new
> preference page as well, so it will probably not fix the issue we're having.
> 

It's landed, so I'm going to update the test.


> ::: firefox/lib/prefs.js
> @@ +404,5 @@
> > +  prefDialog.paneId = "panePrivacy";
> > +  var historyMode = prefDialog.getElement({type: "historyMode"});
> > +  var md = new modalDialog.modalDialog(aController.window);
> > +
> > +  md.start(function(aController) {
> 
> You can use the fat arrow syntax for anonymous functions.
> 
> > aController => {}
> 

Will do.


> :::
> firefox/tests/functional/restartTests/testPreferences_neverRememberHistoryPB/
> test1.js
> @@ +2,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +// Include required modules
> > +var { assert } = require("../../../../../lib/assertions");
> 
> the Assertion module is included by default now in test files. You can
> remove this require call.

Will do. Thanks for letting me know about that!
Mentor: andreea.matei@softvision.ro
Whiteboard: [mentor=andreeamatei][lang=js] → [lang=js]
Hi Szabolcs, do you think you can finish this off in the near future? Did you have any issues while updating the patch? Thanks!
(Assignee)

Comment 40

3 years ago
(In reply to Andreea Matei [:AndreeaMatei] from comment #39)
> Hi Szabolcs, do you think you can finish this off in the near future? Did
> you have any issues while updating the patch? Thanks!

I try to refresh this patch and my environment late tonight.
I hope I woudn't have any issues with the patch. ... Of course I need help with the OSX problem.
(Assignee)

Comment 41

3 years ago
(In reply to Andreea Matei [:AndreeaMatei] from comment #39)
> Hi Szabolcs, do you think you can finish this off in the near future? Did
> you have any issues while updating the patch? Thanks!

As I see, the "firefox/lib/prefs.js" isn't in-content preferences compatible.
I think it would be if Bug 1005811 got fixed.

So I simply did a workaround with
"controller.open(IN_CONTENT_PREFERENCES_URL)" in the test1.js (for now)
and "new elementslib.ID()" + controller.waitForElement() in pref.js,
but I got "historyMode.select is not a function" error, even if I open
the privacy pane directly with "about:preferences#privacy".

Andreea, how could I deal with this historyMode.select issue?

See partially updated (so still failing) patch for the details!
(Assignee)

Comment 42

3 years ago
Created attachment 8477083 [details] [diff] [review]
Patch v7 - in-content Prefs hacks

The patch referenced in comment #41.
Attachment #8399000 - Attachment is obsolete: true
(Assignee)

Comment 43

3 years ago
(In reply to Szabolcs Hubai (:xabolcs) from comment #41)
> [...]
> 
> Andreea, how could I deal with this historyMode.select issue?
> 
> See partially updated (so still failing) patch for the details!

Andreea please help me! I have no clue how to step over about this historyMode.select.
|findElement.ID| gives me an object like below:

> Object { _locatorType: "ID", _locator: "historyMode", _element: <menulist#historyMode>, _owner: undefined, _document: XULDocument → browser.xul, _defaultView: ChromeWindow → browser.xul, isElement: true } prefs.js:422

|historyMode| doesn't have |select|, |historyMode.getNode()| doesn't have |select| also.


Sorry, it seems like, I'm unable to refresh this patch to support in-content preferences.
Please help me, or please reassign this bug to somebody else.
Flags: needinfo?(andreea.matei)
Yes, I will debug that issue today and come back with an answer asap. Thanks for the updates!
Flags: needinfo?(andreea.matei)
(Assignee)

Comment 45

3 years ago
(In reply to Andreea Matei [:AndreeaMatei] from comment #44)
> Yes, I will debug that issue today and come back with an answer asap. Thanks
> for the updates!

I'm testing on Linux x86_64, if this is relevant.
Thanks for helping.
(In reply to Szabolcs Hubai (:xabolcs) from comment #41)
> As I see, the "firefox/lib/prefs.js" isn't in-content preferences compatible.
> I think it would be if Bug 1005811 got fixed.

That's correct. As a workaround for now we fallback to the old preferences dialog in all the other preferences related tests. You might wanna do the same by setting the appropriate preference.
(Assignee)

Comment 47

3 years ago
(In reply to Henrik Skupin (:whimboo) from comment #46)
> (In reply to Szabolcs Hubai (:xabolcs) from comment #41)
> > As I see, the "firefox/lib/prefs.js" isn't in-content preferences compatible.
> > I think it would be if Bug 1005811 got fixed.
> 
> That's correct. As a workaround for now we fallback to the old preferences
> dialog in all the other preferences related tests. You might wanna do the
> same by setting the appropriate preference.

So you are overriding Andrei guidance from comment #37?
It's OK to me, just checking you are on the same opinion.
> However, the new preference window will land soon, so we will need to update
> this test to use that.
> See bug 738797 for when that will land.
> 

Of course the fallback method will speed up my progress.
Yeah. Please see http://hg.mozilla.org/qa/mozmill-tests/file/03dd84d656d3/firefox/tests/functional/testPreferences/testSetToCurrentPage.js#l17.
(Assignee)

Comment 49

3 years ago
(In reply to Andreea Matei [:AndreeaMatei] from comment #39)
> Hi Szabolcs, do you think you can finish this off in the near future? Did
> you have any issues while updating the patch? Thanks!

I'm done with addressing nits (from comment #37).

But the |menuitem.select()| still failing for me, and I need the workaround
from comment #25 to pass the tests.

:whimboo, Andrei; any thoughts about this issue?
Andrei's last result is in comment #36.
(Assignee)

Comment 50

3 years ago
Created attachment 8478674 [details] [diff] [review]
Patch v8 - workaround for in-content Prefs

Asking for feedback now. 
In case of f+ please delegate r? to somebody!
Attachment #8477083 - Attachment is obsolete: true
Attachment #8478674 - Flags: feedback?(andreea.matei)
(Assignee)

Comment 51

3 years ago
(In reply to Szabolcs Hubai (:xabolcs) from comment #49)
> ...
> 
> But the |menuitem.select()| still failing for me, and I need the workaround
> from comment #25 to pass the tests.
> 
> :whimboo, Andrei; any thoughts about this issue?
> Andrei's last result is in comment #36.

In the mean time should I give a try to the new Preferences class from Bug 1005811 comment #46?
Andreea, can you please do the feedback request from Szabolcs? It would be good when we can continue on this bug.

Comment 53

3 years ago
To quickly reiterate the issue here (summarize the problem mentioned in comment 36):

If the preference is set to "Custom" (but the options are the same as "Remember") after Firefox restarts, it checks the options and since the values are the same as "Remember" the selected option will be "Remember".

So in `testPreferences_useCustomSettingPB/test2.js` we fail the change to "Remember" because we already are on "Remember".

We could change to "Custom" before cancelling the change to "Remember" like:
> // Cancel the operation and keep using it's private session.
> prefs.openPreferencesDialog(controller, function (aController) {
>   prefs.permanentPrivateBrowsing(aController, true, "custom");
>   prefs.permanentPrivateBrowsing(aController, false, "remember");
> });
(Assignee)

Updated

3 years ago
(Assignee)

Updated

3 years ago
Depends on: 1005811

Updated

3 years ago
Depends on: 1077356

Updated

3 years ago
No longer depends on: 1005811

Updated

3 years ago
Depends on: 1005811
(Assignee)

Updated

2 years ago
Attachment #8478674 - Flags: feedback?(matei.andreea89)
(Assignee)

Comment 54

2 years ago
I think this bug is a wontfix for Mozmill.

:whimboo, what about firefox-ui-tests? Should this bug refiled against it?
Flags: needinfo?(hskupin)
That's correct. But fr Firefox UI Tests we miss a lot of libraries still, so it might take a while until we can get started with this test. Anyhow, please file a new bug under the Firefox UI Tests component, so we can track the creation of the test - which indeed seems to be important. Thanks.
Flags: needinfo?(hskupin)
(Assignee)

Updated

2 years ago
See Also: → bug 1176469
(Assignee)

Comment 56

2 years ago
This MozTrap test shouldn't implemented for Mozmill, but for Marionette in Firefox UI Test.
That's why bug 1176469 was created for.

Closing this as WONTFIX.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.