883860 - Create metro mozmill test for Pop-up blocker

Status: RESOLVED FIXED

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

Description

[Andrei Eftimie]

Create a Mozmill test for Firefox Metro which implements attachement 723719 from bug 831947

Comment 1

[Andrei Eftimie]

Typo: attachment 723719 [details]

Comment 2

[Cosmin Malutan, [:cosmin-malutan]]

I will take this bug.
Thanks

Comment 3

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v1.0

Hi Andreea here are the pop-up blocker tests.
I added the patch only for tracking work, as it depends on bug 880417 at the moment.

Comment 4

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8363811 [details] [diff] [review]
patch_v1.0

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

The indentation is off and it has changes in more files than it should, I think you applied another patch before or something. 
Also I think we could merge all those 3 tests into one with 3 functions maybe. You could try to clear the cache between each one.

::: metrofirefox/tests/functional/testPopups/testPopupsAllowOnce.js
@@ +19,5 @@
> +  aModule.locationBar = new toolbars.LocationBar(aModule.controller);
> +  aModule.tabs = new tabs.Tabs(aModule.controller);
> +}
> +
> +function testPopupsAllowOnce() {

js doc

::: metrofirefox/tests/functional/testPopups/testPopupsAlwaysAllow.js
@@ +57,5 @@
> +		return (tabs.length - 1) === parseInt(POPUPS_COUNT);
> +	}, POPUPS_COUNT + " tabs has been opened.");
> +
> +	assert.ok(tabs.selectedIndex ===  parseInt(POPUPS_COUNT),
> +					  "The last opened pop-up is selected");

assert.equal

::: metrofirefox/tests/functional/testPopups/testPopupsNeverWarn.js
@@ +26,5 @@
> +														 getService(Ci.nsIPermissionManager);
> +	nsIPermissionManager.removeAll();
> +}
> +
> +function testPopupsNeverWarn() {

js doc

@@ +34,5 @@
> +	controller.open(TEST_DATA);
> +  controller.waitForPageLoad();
> +
> +  expect.equal(tabsCount, tabs.length,
> +               "The window count has not changed");

If tabsCount = tabs.length, there's no much sense in having this expect.

Comment 5

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.0

Thanks for review Andreea, I moved all the tests in one file and it looks more neat.

(In reply to Andreea Matei [:AndreeaMatei] from comment #4)
> > +  expect.equal(tabsCount, tabs.length,
> > +               "The window count has not changed");
> 
> If tabsCount = tabs.length, there's no much sense in having this expect.
Actually that is what test dose, we check that no pup-us where open.

Comment 6

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8371322 [details] [diff] [review]
patch_v2.0

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

::: metrofirefox/tests/functional/testPopups/testPopupBlocker.js
@@ +39,5 @@
> +  controller.open(TEST_DATA)
> +  controller.waitForPageLoad();
> +
> +  expect.equal(tabsCount, tabs.length,
> +               "The tabs count has not changed");

I'd say "No new tabs have been opened yet"

@@ +46,5 @@
> +  // Get the Allow once Button and click on it
> +  var allowOnceLabel = utils.getProperty("chrome://browser/locale/browser.properties",
> +                                         "popupButtonAllowOnce2");
> +  var allowOnceButton = toolBar.locationBar.getNotificationElement("popup-blocked",
> +                        {type: "label", value: allowOnceLabel});

I'd indent this at least at .getNotificationElement, everywhere.

@@ +49,5 @@
> +  var allowOnceButton = toolBar.locationBar.getNotificationElement("popup-blocked",
> +                        {type: "label", value: allowOnceLabel});
> +  assert.ok(allowOnceButton.getNode(),
> +            "'" + allowOnceLabel + "' button has been found");
> +  allowOnceButton.click();

tap() please.

@@ +56,5 @@
> +  assert.waitFor(function () {
> +    return (tabs.length - 1) === parseInt(POPUPS_COUNT);
> +  }, POPUPS_COUNT + " tabs has been opened.");
> +
> +  //Close all tabs and refresh and notification will appear again

I would comment "Checking the popup was allowed only once by revisiting the page"

@@ +58,5 @@
> +  }, POPUPS_COUNT + " tabs has been opened.");
> +
> +  //Close all tabs and refresh and notification will appear again
> +  tabs.closeAllTabs();
> +  controller.open(TEST_DATA);

waitForPageLoad()?

@@ +85,5 @@
> +  var alwaysAllowButton = toolBar.locationBar.getNotificationElement("popup-blocked",
> +                          {type: "label", value: alwaysAllowLabel});
> +  assert.ok(alwaysAllowButton.getNode(),
> +            "'" + alwaysAllowLabel + "' button has been found");
> +  alwaysAllowButton.click();

tap()

@@ +87,5 @@
> +  assert.ok(alwaysAllowButton.getNode(),
> +            "'" + alwaysAllowLabel + "' button has been found");
> +  alwaysAllowButton.click();
> +
> +  controller.open(TEST_DATA);

So this is not working, after tapping we should get 6 tabs and we don't for some reason. This is a workaround, we want to report the bug/fix it and make it right.

@@ +93,5 @@
> +    return (tabs.length - 1) === parseInt(POPUPS_COUNT);
> +  }, POPUPS_COUNT + " tabs has been opened.");
> +
> +  assert.ok(tabs.selectedIndex === parseInt(POPUPS_COUNT),
> +            "The last opened pop-up is selected");

With what is this helping? Instead we want to check that when we revisit the page, no notification appears.

@@ +109,5 @@
> +  controller.open(TEST_DATA);
> +  controller.waitForPageLoad();
> +
> +  expect.equal(tabsCount, tabs.length,
> +               "The window count has not changed");

nit: tabs

@@ +114,5 @@
> +  toolBar.locationBar.waitForNotification("popup-blocked");
> +
> +  var neverWarnLabel = utils.getProperty("chrome://browser/locale/browser.properties",
> +                                         "popupButtonNeverWarn3");
> +

Please remove the blank line

@@ +119,5 @@
> +  var neverWarnButton = toolBar.locationBar.getNotificationElement("popup-blocked",
> +                        {type: "label", value: neverWarnLabel});
> +  assert.ok(neverWarnButton.getNode(),
> +            "'" + neverWarnLabel + "' button has been found");
> +  neverWarnButton.click();

tap()

@@ +125,5 @@
> +  assert.waitFor(function () {
> +    return !toolBar.locationBar.getNotification("popup-blocked");
> +  }, "No notification is displayed");
> +
> +  //Second time we open the page no pop-ups will be given

"Revisiting the page will not trigger a notification."

Comment 7

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.0

Thanks Andreea for the input here, I also opened bug 968739 for the issue with "Always Show".

Comment 8

[Andrei Eftimie]

Comment on attachment 8371380 [details] [diff] [review]
patch_v3.0

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

::: metrofirefox/lib/ui/toolbars.js
@@ +440,5 @@
> +   *          The Notification element
> +   */
> +  getNotification : function locationBar_getNotification(aType) {
> +    var nodeCollector = new domUtils.nodeCollector(this._controller.window.document);
> +    nodeCollector.queryNodes('notification');

nit: please change the single quotes to double quotes for consistency.
All instances please.

@@ +495,5 @@
> +    assert.waitFor(function () {
> +      notification = self.getNotification(aType);
> +
> +      return !!notification;
> +    });

We should include a message here.

@@ +498,5 @@
> +      return !!notification;
> +    });
> +
> +    assert.waitFor(function () {
> +      return !!utils.isDisplayed(self.controller, notification);

This should already return a boolean.
Also please adda message here as well.

::: metrofirefox/tests/functional/testPopups/testPopupBlocker.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +

nit: remove extra newline

@@ +14,5 @@
> +var nsIPermissionManager = Cc["@mozilla.org/permissionmanager;1"].
> +                           getService(Ci.nsIPermissionManager);
> +
> +const BASE_URL = collector.addHttpResource("../../../../data/");
> +const POPUPS_COUNT = "6";

You can leave this as an integer, then you can remove the need to call parseInt throughout the test.

@@ +26,5 @@
> +
> +var teardownTest = function (aModule) {
> +  aModule.tabs.closeAllTabs();
> +
> +  // Remove all pup-ups permissions

"Remove all permissions" should be enough

@@ +34,5 @@
> +/**
> + * Test to make sure pop-ups are allowed once and blocked when revisiting the page
> + */
> +function testPopupsAllowOnce() {
> +  var tabsCount = tabs.length;

We should issue a closeAllTabs() in setupModule()
We'll be confident this will be 1 then.

@@ +55,5 @@
> +  allowOnceButton.tap();
> +
> +  //Wait for the six popups to be displayed
> +  assert.waitFor(function () {
> +    return (tabs.length - 1) === parseInt(POPUPS_COUNT);

I would change this in `tabs.length === POPUPS_COUNT + 1`

@@ +56,5 @@
> +
> +  //Wait for the six popups to be displayed
> +  assert.waitFor(function () {
> +    return (tabs.length - 1) === parseInt(POPUPS_COUNT);
> +  }, POPUPS_COUNT + " tabs has been opened.");

[...] have been [...]

@@ +70,5 @@
> + * we had hit the Always Allow button prior to that
> + *
> + */
> +function testPopupsAlwaysAllow() {
> +  var tabsCount = tabs.length;

Same as above. This should be 1.

@@ +82,5 @@
> +  toolBar.locationBar.waitForNotification("popup-blocked");
> +
> +  var alwaysAllowLabel = utils.getProperty("chrome://browser/locale/browser.properties",
> +                                           "popupButtonAlwaysAllow3");
> +

nit: remove this extra line to be in sync with the above test

@@ +90,5 @@
> +            "'" + alwaysAllowLabel + "' button has been found");
> +  alwaysAllowButton.tap();
> +
> +  // Bug 968739
> +  // TODO: After taping 'Always Allow' we should wait for 6 pop-ups to open

So this doesn't work until bug 968739 is fixed?

We should have the proper code in and skip only this particular test.
This test should fail right now, not pass.
When the dependency gets fixed we should unskip this.

@@ +95,5 @@
> +
> +  controller.open(TEST_DATA);
> +  assert.waitFor(function () {
> +    return (tabs.length - 1) === parseInt(POPUPS_COUNT);
> +  }, POPUPS_COUNT + " tabs has been opened.");

Same as in the above test please.

Comment 9

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.1

(In reply to Andrei Eftimie from comment #8)
> > +  // Bug 968739
> > +  // TODO: After taping 'Always Allow' we should wait for 6 pop-ups to open
> 
> So this doesn't work until bug 968739 is fixed?
> 
> We should have the proper code in and skip only this particular test.
> This test should fail right now, not pass.
> When the dependency gets fixed we should unskip this.
Thanks for review, I added the check as it should be fixed and I skipped the test.

Comment 10

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8381400 [details] [diff] [review]
patch_v3.1

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

::: metrofirefox/lib/ui/toolbars.js
@@ +443,5 @@
> +    var nodeCollector = new domUtils.nodeCollector(this._controller.window.document);
> +    nodeCollector.queryNodes("notification");
> +
> +    if (aType)
> +      nodeCollector.filterByDOMProperty("value", aType);

Please wrap this with brackets.

@@ +462,5 @@
> +   */
> +  getNotificationElement : function locationBar_getNotificationElement(aType, aChildElement) {
> +    var notification = this.getNotification(aType);
> +    if (!aChildElement)
> +      return notification;

Same here.

@@ +488,5 @@
> +   *
> +   * @param {string} aType
> +   *        Type of the notification bar to look for
> +   */
> +  waitForNotification : function (aType) {

We should keep in sync with the other methods, so would be:
waitForNotification: locationBar_waitForNotification(aType) {

::: metrofirefox/tests/functional/testPopups/testPopupBlocker.js
@@ +53,5 @@
> +  assert.ok(allowOnceButton.getNode(),
> +            "'" + allowOnceLabel + "' button has been found");
> +  allowOnceButton.tap();
> +
> +  //Wait for the six popups to be displayed

Space after //

@@ +70,5 @@
> + * we had hit the Always Allow button prior to that
> + *
> + */
> +function testPopupsAlwaysAllow() {
> +

Please remove this blank line.

@@ +87,5 @@
> +  assert.ok(alwaysAllowButton.getNode(),
> +            "'" + alwaysAllowLabel + "' button has been found");
> +  alwaysAllowButton.tap();
> +
> +  //Wait for the six popups to be displayed

Also a space here

@@ +100,5 @@
> +    return tabs.length === POPUPS_COUNT + 1;
> +  }, POPUPS_COUNT + " tabs have been opened.");
> +
> +  assert.equal(tabs.selectedIndex, POPUPS_COUNT,
> +            "The last opened pop-up is selected");

Indentation is off a bit.

@@ +105,5 @@
> +}
> +
> +/**
> + * Test that pop-ups are blocked and no notification is shown second time we
> + * revisit the page

[..] the second time

@@ +109,5 @@
> + * revisit the page
> + *
> + */
> +function testPopupsNeverWarn() {
> +

Please remove the blank line.

Comment 11

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.2

Comment 12

[Cosmin Malutan, [:cosmin-malutan]]

Comment on attachment 8386083 [details] [diff] [review]
patch_v3.2

Thanks for review Andreea, I fixed the nits!

Comment 13

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8386083 [details] [diff] [review]
patch_v3.2

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

Looks good. Passing review to Henrik.

Comment 14

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

Comment on attachment 8386083 [details] [diff] [review]
patch_v3.2

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

::: metrofirefox/lib/ui/toolbars.js
@@ +436,5 @@
> +   * @param {string} aType
> +   *        Type of the notification bar to look for
> +   *
> +   * @returns {MozElement}
> +   *          The Notification element

nit: one liner please.

@@ +440,5 @@
> +   *          The Notification element
> +   */
> +  getNotification : function locationBar_getNotification(aType) {
> +    var nodeCollector = new domUtils.nodeCollector(this._controller.window.document);
> +    nodeCollector.queryNodes("notification");

Why is that not part of getElements()?

@@ +444,5 @@
> +    nodeCollector.queryNodes("notification");
> +
> +    if (aType) {
> +      nodeCollector.filterByDOMProperty("value", aType);
> +    }

So aType is optional?

@@ +450,5 @@
> +    return nodeCollector.elements[0];
> +  },
> +
> +  /**
> +   * Retrieves the specified element of the door hanger notification bar

Same description as above.

@@ +461,5 @@
> +   * @returns {MozElement}
> +   *          The Notification child element
> +   */
> +  getNotificationElement : function locationBar_getNotificationElement(aType, aChildElement) {
> +    var notification = this.getNotification(aType);

Is aType optional or not?

@@ +467,5 @@
> +      return notification;
> +    }
> +    var nodeCollector = new domUtils.nodeCollector(notification.getNode());
> +    nodeCollector.queryNodes("button");
> +    nodeCollector.filterByDOMProperty(aChildElement.type, aChildElement.value);

Same here regarding getElements()

@@ +500,5 @@
> +    }, "Notification has been found");
> +
> +    assert.waitFor(function () {
> +      return utils.isDisplayed(self.controller, notification);
> +    }, "Notification popup has been displayed");

I assume there is no event we could wait for here?

::: metrofirefox/tests/functional/testPopups/testPopupBlocker.js
@@ +5,5 @@
> +"use strict";
> +
> +// Include the required modules
> +var { expect } = require("../../../../lib/assertions");
> +var tabBrowser = require("../../../lib/ui/tabs");

please name it tabs.

@@ +10,5 @@
> +var toolbars = require("../../../lib/ui/toolbars");
> +var utils = require("../../../../firefox/lib/utils");
> +
> +var nsIPermissionManager = Cc["@mozilla.org/permissionmanager;1"].
> +                           getService(Ci.nsIPermissionManager);

Please use Services.perms.

@@ +14,5 @@
> +                           getService(Ci.nsIPermissionManager);
> +
> +const BASE_URL = collector.addHttpResource("../../../../data/");
> +const POPUPS_COUNT = 6;
> +const TEST_DATA = BASE_URL + "popups/popup_trigger.html?count=" + POPUPS_COUNT;

POPUPS_COUNT doesnt' belong to this code block.

@@ +16,5 @@
> +const BASE_URL = collector.addHttpResource("../../../../data/");
> +const POPUPS_COUNT = 6;
> +const TEST_DATA = BASE_URL + "popups/popup_trigger.html?count=" + POPUPS_COUNT;
> +
> +var setupModule = function (aModule) {

no var declarations.

@@ +19,5 @@
> +
> +var setupModule = function (aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.toolBar = new toolbars.ToolBar(aModule.controller);
> +  aModule.tabs = new tabBrowser.TabBrowser(aModule.controller);

tabBrowser

@@ +28,5 @@
> +var teardownTest = function (aModule) {
> +  aModule.tabs.closeAllTabs();
> +
> +  // Remove all permissions
> +  nsIPermissionManager.removeAll();

You might also want to call this in setupModule().

@@ +32,5 @@
> +  nsIPermissionManager.removeAll();
> +}
> +
> +/**
> + * Test to make sure pop-ups are allowed once and blocked when revisiting the page

Please add the bug number.

@@ +36,5 @@
> + * Test to make sure pop-ups are allowed once and blocked when revisiting the page
> + */
> +function testPopupsAllowOnce() {
> +
> +  // Open the Pop-up test site

no empty line please.

@@ +41,5 @@
> +  controller.open(TEST_DATA)
> +  controller.waitForPageLoad();
> +
> +  expect.equal(tabs.length, 1,
> +               "No new tabs have been opened yet");

closeAllTabs() ensures that already. This can be removed.

@@ +48,5 @@
> +  // Get the Allow once Button and click on it
> +  var allowOnceLabel = utils.getProperty("chrome://browser/locale/browser.properties",
> +                                         "popupButtonAllowOnce2");
> +  var allowOnceButton = toolBar.locationBar.getNotificationElement("popup-blocked",
> +                                            {type: "label", value: allowOnceLabel});

Can we move this into the popups.js library and make it a method which gets the necessary parameters?

@@ +50,5 @@
> +                                         "popupButtonAllowOnce2");
> +  var allowOnceButton = toolBar.locationBar.getNotificationElement("popup-blocked",
> +                                            {type: "label", value: allowOnceLabel});
> +  assert.ok(allowOnceButton.getNode(),
> +            "'" + allowOnceLabel + "' button has been found");

not necessary. tap() will fail.

@@ +68,5 @@
> +/**
> + * Test to make sure pop-ups are allowed second time we visit the page if
> + * we had hit the Always Allow button prior to that
> + */
> +function testPopupsAlwaysAllow() {

Please make this a separate test module.

@@ +106,5 @@
> + * Test that pop-ups are blocked and no notification is shown the second time we
> + * revisit the page
> + *
> + */
> +function testPopupsNeverWarn() {

Please also a new test module.

Comment 15

[Daniel Gherasim]

Attachment: popups_tests_v1.patch

Updated patch according to Henrik's review.

Comment 16

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8394797 [details] [diff] [review]
popups_tests_v1.patch

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

::: metrofirefox/lib/ui/toolbars.js
@@ +466,2 @@
>        case "backButton":
> +        return [new findElement.ID(this._controller.window.document, "back-button")];

I'd prefer with elem and use nodeCollector since you need it in other cases. That will allow you to return in the end only.

::: metrofirefox/tests/functional/testPopups/manifest.ini
@@ +1,1 @@
> +[testAllowAlways.js]

testAlwaysAllow

@@ +1,4 @@
> +[testAllowAlways.js]
> +disabled = Bug 968739 - Pop-up blocker 'Always Show' should open the pop-ups right away
> +[testAllowOnce.js]
> +[testBlocked.js]

Please rename it to testPopupsBlocked.js

::: metrofirefox/tests/functional/testPopups/testAllowAlways.js
@@ +14,5 @@
> +
> +const BASE_URL = collector.addHttpResource("../../../../data/");
> +const POPUPS_COUNT = 6;
> +const TEST_PAGE = BASE_URL + "popups/popup_trigger.html";
> +const TEST_DATA = TEST_PAGE + "?count=" + POPUPS_COUNT;

We want the constants separated in blocks. Here you will have BASE_URL one, TEST_PAGE and TEST_DATA the second and the POPUPS_COUNT.

@@ +61,5 @@
> +               "The last opened pop-up is selected");
> +}
> +
> +setupModule.__force_skip__ = "Bug 968739 - After taping 'Always Allow' "
> +                             + "we should wait for 6 pop-ups to open";

"+" should be on the first line.

Comment 17

[Daniel Gherasim]

Attachment: popups_tests_v2.patch

Thanks for review Andreea.
I've updated the patch now.

Comment 18

[Andrei Eftimie]

Comment on attachment 8397730 [details] [diff] [review]
popups_tests_v2.patch

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

::: metrofirefox/lib/ui/toolbars.js
@@ +9,5 @@
>  var utils = require("../../../firefox/lib/utils");
>  var tabs = require("tabs");
>  
> +const NOTIFICATION_BUTTONS = {
> +  allowAlwaysButton:  utils.getProperty("chrome://browser/locale/browser.properties",

nit: extra spaces

@@ +482,5 @@
> +        var notification = this.getElement({type: "notification",
> +                                           subtype: spec.subtype});
> +        nodeCollector.root = notification.getNode();
> +        nodeCollector.queryNodes("button");
> +        nodeCollector.filterByDOMProperty("label", NOTIFICATION_BUTTONS[spec.value]);

I'm not sold that we need nodeCollector here as I don't see any Anonymous elements.
If this is the case `findElement.Selector` should be enough.

@@ +590,5 @@
> +   */
> +  waitForNotification : function locationBar_waitForNotification(aType, aCallback) {
> +    var self = { active: false };
> +    function onAlertActive() { self.active = true; }
> +    this.controller.window.document.addEventListener("AlertActive", onAlertActive, false);

`userCapture` default to false, so you can omit the last argument.

@@ +597,5 @@
> +      aCallback();
> +      assert.waitFor(() => self.active, "Notification has been opened");
> +    }
> +    finally {
> +      this.controller.window.document.removeEventListener("AlertActive", onAlertActive, false);

Same here.

::: metrofirefox/tests/functional/testPopups/testPopupsBlocked.js
@@ +47,5 @@
> +  neverWarnButton.tap();
> +
> +  assert.waitFor(() => !toolBar.locationBar.getElement({type: "notification",
> +                                                       subtype: "popup-blocked"}),
> +                 "No notification is displayed");

The alignment doesn't look good, please add braces.

@@ +54,5 @@
> +  controller.open(TEST_DATA + POPUPS_COUNT)
> +  controller.waitForPageLoad();
> +  assert.waitFor(() => !toolBar.locationBar.getElement({type: "notification",
> +                                                       subtype: "popup-blocked"}),
> +                 "No notification is displayed");

The alignment doesn't look good, please add braces.

Comment 19

[Daniel Gherasim]

Attachment: popups_tests_v3.patch

(In reply to Andrei Eftimie from comment #18)
> @@ +482,5 @@
> > +        var notification = this.getElement({type: "notification",
> > +                                           subtype: spec.subtype});
> > +        nodeCollector.root = notification.getNode();
> > +        nodeCollector.queryNodes("button");
> > +        nodeCollector.filterByDOMProperty("label", NOTIFICATION_BUTTONS[spec.value]);
> 
> I'm not sold that we need nodeCollector here as I don't see any Anonymous
> elements.
> If this is the case `findElement.Selector` should be enough.
> 

You were right, findElement.Selector works great in this and above case too.

Comment 20

[Andrei Eftimie]

Comment on attachment 8399894 [details] [diff] [review]
popups_tests_v3.patch

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

This looks good. Only some nits to be addressed. 

Please request a review from Henrik with the mentioned nits fixed.

::: metrofirefox/lib/ui/toolbars.js
@@ +10,5 @@
>  var tabs = require("tabs");
>  
> +const NOTIFICATION_BUTTONS = {
> +  allowAlwaysButton: utils.getProperty("chrome://browser/locale/browser.properties",
> +                                        "popupButtonAlwaysAllow3"),

nit: there's an extra space.

@@ +475,5 @@
>          break;
> +      case "notification":
> +        elem = new findElement.Selector(this._controller.window.document,
> +                                       "notification[value='" +
> +                                       spec.subtype + "']");

nit: alignment

@@ +480,5 @@
> +        break;
> +      case "notificationButton":
> +        elem = new findElement.Selector(this._controller.window.document,
> +                                       "button[label='" +
> +                                       NOTIFICATION_BUTTONS[spec.value] + "']");

nit: alignment

::: metrofirefox/tests/functional/testPopups/testAlwaysAllow.js
@@ +47,5 @@
> +
> +  alwaysAllowButton.tap();
> +
> +  // Wait for the six popups to be displayed
> +  assert.waitFor(() => (tabBrowser.length === POPUPS_COUNT + 1),

When the return is more complex, please use curly braces and move the return statement on a new line for improved readability. Please check this for all files.

::: metrofirefox/tests/functional/testPopups/testPopupsBlocked.js
@@ +41,5 @@
> +    controller.waitForPageLoad();
> +  });
> +
> +  var neverWarnButton = toolBar.locationBar.getElement({type: "notificationButton",
> +                                                       subtype: "popup-blocked",

I missed these before.
Please indent all objects that span multiple line with the first property, not the curly brace (so one extra space).

Please check these for all files.

@@ +52,5 @@
> +    return !notification.exists()
> +  }, "No notification is displayed");
> +
> +  // Revisiting the page will not trigger a notification
> +  controller.open(TEST_DATA + POPUPS_COUNT)

nit: missing semicolon

Comment 21

[Daniel Gherasim]

Attachment: popups_tests_v4.patch

Thanks, nits fixed.
Henrik, can you have a final look on this ?

Comment 22

[Daniel Gherasim]

Attachment: popups_tests_v5.patch

We don't use nodeCollector in getElements anymore so I removed it.

Comment 23

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

Comment on attachment 8400028 [details] [diff] [review]
popups_tests_v5.patch

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

::: metrofirefox/lib/ui/toolbars.js
@@ +16,5 @@
> +                                     "popupButtonAllowOnce2"),
> +
> +  allowNeverButton: utils.getProperty("chrome://browser/locale/browser.properties",
> +                                      "popupButtonNeverWarn3")
> +}

We haven't talked about where DTD and propertie values should live. But it should at least not be at the global scope. This is class specific.

@@ +472,5 @@
>          break;
> +      case "notification":
> +        elem = new findElement.Selector(this._controller.window.document,
> +                                        "notification[value='" +
> +                                        spec.subtype + "']");

Are those notifications really part of the location bar? If not, those should not be part of the LocationBar class.

@@ +477,5 @@
> +        break;
> +      case "notificationButton":
> +        elem = new findElement.Selector(this._controller.window.document,
> +                                        "button[label='" +
> +                                        NOTIFICATION_BUTTONS[spec.value] + "']");

Do we have something else as the label to use here? I would prefer that.

@@ +587,5 @@
> +    function onAlertActive() { self.active = true; }
> +    this.controller.window.document.addEventListener("AlertActive", onAlertActive);
> +
> +    try {
> +      aCallback();

Test if it is a function first.

::: metrofirefox/tests/functional/testPopups/testAllowOnce.js
@@ +49,5 @@
> +  allowOnceButton.tap();
> +
> +  // Wait for the six popups to be displayed
> +  assert.waitFor(() => {
> +    return tabBrowser.length === POPUPS_COUNT + 1;

() => (tabBrowser.length === POPUPS_COUNT + 1)

@@ +50,5 @@
> +
> +  // Wait for the six popups to be displayed
> +  assert.waitFor(() => {
> +    return tabBrowser.length === POPUPS_COUNT + 1;
> +  }, POPUPS_COUNT + " tabs have been opened.");

You are waiting for POPUPS_COUNT + 1 here.

::: metrofirefox/tests/functional/testPopups/testPopupsBlocked.js
@@ +47,5 @@
> +  neverWarnButton.tap();
> +
> +  assert.waitFor(() => {
> +    var notification = toolBar.locationBar.getElement({type: "notification",
> +                                                       subtype: "popup-blocked"});

There is no need to re-retrieve the element. Once fetched it can be used and members will always return the current state.

Comment 24

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #23)
> Comment on attachment 8400028 [details] [diff] [review]
> popups_tests_v5.patch
> 
> Review of attachment 8400028 [details] [diff] [review]:
> ----------------------------------------------------------------- 
> 
> We haven't talked about where DTD and propertie values should live. But it
> should at least not be at the global scope. This is class specific.
> 
> Are those notifications really part of the location bar? If not, those
> should not be part of the LocationBar class.
> 

I think we should have a notification class in toolbars.js as it's directly binded to the window, this is the stack for a notification: 

window > stack > vbox (id=page) > stack (id=content-viewport) > deck > notificationbox > notification.

> @@ +477,5 @@
> > +        break;
> > +      case "notificationButton":
> > +        elem = new findElement.Selector(this._controller.window.document,
> > +                                        "button[label='" +
> > +                                        NOTIFICATION_BUTTONS[spec.value] + "']");
> 
> Do we have something else as the label to use here? I would prefer that.
> 

Every button has this attributes: label, accesskey, class: "notification-button".
We can get all buttons using notification-button class and retrieve the needed one using an index, as we know the order they appear. Would this approach be better ?

Comment 25

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

(In reply to daniel.gherasim from comment #24)
> > We haven't talked about where DTD and propertie values should live. But it
> > should at least not be at the global scope. This is class specific.
> > 
> > Are those notifications really part of the location bar? If not, those
> > should not be part of the LocationBar class.
> 
> I think we should have a notification class in toolbars.js as it's directly
> binded to the window, this is the stack for a notification: 
> 
> window > stack > vbox (id=page) > stack (id=content-viewport) > deck >
> notificationbox > notification.

I think that's better yes.

> > > +      case "notificationButton":
> > > +        elem = new findElement.Selector(this._controller.window.document,
> > > +                                        "button[label='" +
> > > +                                        NOTIFICATION_BUTTONS[spec.value] + "']");
> > 
> > Do we have something else as the label to use here? I would prefer that.
> > 
> 
> Every button has this attributes: label, accesskey, class:
> "notification-button".
> We can get all buttons using notification-button class and retrieve the
> needed one using an index, as we know the order they appear. Would this
> approach be better ?

Not really. Lets keep it that way and use the label. It works as long as we have an id to base on.

Comment 26

[Daniel Gherasim]

Attachment: popups_tests_v6.patch

Thanks for the quick answer.
So I created a new NotificationBar class as part of the toolbars.js file.
We use it via the exported ToolBar class.

Also moved the 'get button labels' as part of the tests, given that a notification is a more generalized object that can contain different elements.

Comment 27

[Andrei Eftimie]

Comment on attachment 8401255 [details] [diff] [review]
popups_tests_v6.patch

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

Just a couple small changes.

::: metrofirefox/lib/ui/toolbars.js
@@ +641,5 @@
> +   * @param {function} aCallback
> +   *        Function that triggers the notification to open
> +   */
> +  waitForNotification : function locationBar_waitForNotification(aType, aCallback) {
> +    assert.ok(typeof aCallback === "function", "Callback is a function");

assert.equal

::: metrofirefox/tests/functional/testPopups/testAlwaysAllow.js
@@ +49,5 @@
> +
> +  // Wait for the six popups to be displayed
> +  assert.waitFor(() => {
> +    return tabBrowser.length === POPUPS_COUNT + 1
> +  }, POPUPS_COUNT + " tabs have been opened.");

Should be POPUPS_COUNT + 1

@@ +56,5 @@
> +  controller.open(TEST_DATA + POPUPS_COUNT);
> +
> +  assert.waitFor(() => {
> +    return tabBrowser.length === POPUPS_COUNT + 1;
> +  }, POPUPS_COUNT + " tabs have been opened.");

POPUPS_COUNT + 1

Comment 28

[Daniel Gherasim]

Attachment: popups_tests_v7.patch

Made the changes.

Comment 29

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

Comment on attachment 8403179 [details] [diff] [review]
popups_tests_v7.patch

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

::: metrofirefox/lib/ui/toolbars.js
@@ +450,3 @@
>      var elem = null;
>  
> +    switch(spec.type) {

nit: missing blank after 'switch'

@@ +616,5 @@
> +  getElements : function toolbar_getElements(aSpec) {
> +    var spec = aSpec || {};
> +    var elem = null;
> +
> +    switch(spec.type) {

missing blank after switch.

@@ +619,5 @@
> +
> +    switch(spec.type) {
> +      case "notification":
> +        elem = new findElement.Selector(this._controller.window.document,
> +                                        "notification[value='" + spec.value + "']");

Shouldn't we assert if a property is mandatory?

@@ +623,5 @@
> +                                        "notification[value='" + spec.value + "']");
> +        break;
> +      case "button":
> +        elem = new findElement.Selector(this._controller.window.document,
> +                                        "button[label='" + spec.value + "']");

Same here.

::: metrofirefox/tests/functional/testPopups/testAllowOnce.js
@@ +26,5 @@
> +  Services.perms.removeAll();
> +}
> +
> +function teardownModule(aModule) {
> +  aModule.tabBrowser.closeAllTabs();

We have seen that we can fail here if a popup stays open. We might want to make sure the opened popup notification gets closed too.

::: metrofirefox/tests/functional/testPopups/testAlwaysAllow.js
@@ +48,5 @@
> +  alwaysAllowButton.tap();
> +
> +  // Wait for the six popups to be displayed
> +  assert.waitFor(() => {
> +    return tabBrowser.length === POPUPS_COUNT + 1

() => (tabBrowser.length === POPUPS_COUNT + 1)

@@ +56,5 @@
> +  controller.open(TEST_DATA + POPUPS_COUNT);
> +
> +  assert.waitFor(() => {
> +    return tabBrowser.length === POPUPS_COUNT + 1;
> +  }, (POPUPS_COUNT + 1) + " tabs have been opened.");

Beside this waitfor make sure you also assert that no notification gets displayed!

::: metrofirefox/tests/functional/testPopups/testPopupsBlocked.js
@@ +51,5 @@
> +                                                         subtype: "popup-blocked"});
> +
> +  assert.waitFor(() => {
> +    return !notification.exists();
> +  }, "No notification is displayed");

Something you might also want to add to the other tests?

@@ +58,5 @@
> +  controller.open(TEST_DATA + POPUPS_COUNT);
> +  controller.waitForPageLoad();
> +  assert.waitFor(() => {
> +    return !notification.exists();
> +  }, "No notification is displayed");

This is not doing what you expect it would do. If the notification gets opened a bit delayed, it will also pass. I would suggest you make use of waitFor() as in all other cases, and catch the TimeoutError inside an assert.throws() call.

Comment 30

[Daniel Gherasim]

Attachment: popups_tests_v8.patch

(In reply to Henrik Skupin (:whimboo) from comment #29)
> Comment on attachment 8403179 [details] [diff] [review]
> ::: metrofirefox/tests/functional/testPopups/testAllowOnce.js
> @@ +26,5 @@
> > +  Services.perms.removeAll();
> > +}
> > +
> > +function teardownModule(aModule) {
> > +  aModule.tabBrowser.closeAllTabs();
> 
> We have seen that we can fail here if a popup stays open. We might want to
> make sure the opened popup notification gets closed too.
> 

Popups are tabs in metrofirefox and we are closing them using closeAllTabs.
I also added to each test a notification force closing if that's present.

> @@ +58,5 @@
> > +  controller.open(TEST_DATA + POPUPS_COUNT);
> > +  controller.waitForPageLoad();
> > +  assert.waitFor(() => {
> > +    return !notification.exists();
> > +  }, "No notification is displayed");
> 
> This is not doing what you expect it would do. If the notification gets
> opened a bit delayed, it will also pass. I would suggest you make use of
> waitFor() as in all other cases, and catch the TimeoutError inside an
> assert.throws() call.

Fixed by adding:

assert.throws(() => {
  assert.waitFor(() => notification.exists(), "Notification element exists");
}, "Testing if notification exists should return - TimeoutError");

Hope it's ok

Comment 31

[Andrei Eftimie]

Comment on attachment 8406722 [details] [diff] [review]
popups_tests_v8.patch

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

Looks good to me, just some nits regarding comments.
Request a review from Henrik with these fixed.

::: metrofirefox/lib/ui/toolbars.js
@@ +581,5 @@
> +   * Retrieve an UI element based on the given specification
> +   *
> +   * @param {object} aSpec
> +   *        Information of the UI elements which should be retrieved
> +   * @param {object} type

nit: this is an attribute of aSpec, and should be noted as `aSpec.type`
Applies to all attributes in this comment.

@@ +603,5 @@
> +   * Retrieve list of UI elements based on the given specification
> +   *
> +   * @param {Object} aSpec
> +   *        Information of the UI elements which should be retrieved
> +   * @param {Object} type

nit: same as above: `aSpec.type`

@@ +605,5 @@
> +   * @param {Object} aSpec
> +   *        Information of the UI elements which should be retrieved
> +   * @param {Object} type
> +   *        General type information
> +   * @param {Object} [subtype]

We're not using subtype at all right now.

@@ +607,5 @@
> +   * @param {Object} type
> +   *        General type information
> +   * @param {Object} [subtype]
> +   *        Specific element or property
> +   * @param {Object} [value]

This doesn't appear to be optional. We require it in both cases.

Comment 32

[Daniel Gherasim]

Attachment: popups_tests_v9.patch

Nits fixed.

Comment 33

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

Comment on attachment 8408019 [details] [diff] [review]
popups_tests_v9.patch

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

::: metrofirefox/lib/ui/toolbars.js
@@ +612,5 @@
> +    var elem = null;
> +
> +    switch (spec.type) {
> +      case "notification":
> +        assert.ok(spec.value, "Notification's value has been specified");

What is Notification's value? I assume you wanted to say: 'Type of notification has been specified'.

@@ +617,5 @@
> +        elem = new findElement.Selector(this._controller.window.document,
> +                                        "notification[value='" + spec.value + "']");
> +        break;
> +      case "button":
> +        assert.ok(spec.value, "Button's label has been specified");

Same here.. should be 'Type of button...'.

::: metrofirefox/tests/functional/testPopups/testAlwaysAllow.js
@@ +27,5 @@
> +}
> +
> +function teardownModule(aModule) {
> +  var notification = toolBar.notificationBar.getElement({type: "notification",
> +                                                         value: "popup-blocked"});

Wait... why the value property here and not subtype as it was before? Please revert that. We are looking for a given element type, even if the query would be based on a property value. For the abstraction layer here it should be irrelevant.

@@ +57,5 @@
> +  assert.waitFor(() => (tabBrowser.length === POPUPS_COUNT + 1),
> +                 (POPUPS_COUNT + 1) + " tabs have been opened.");
> +
> +  var notification = toolBar.notificationBar.getElement({type: "notification",
> +                                                         value: "popup-blocked"});

Define it when you need it. So before the assert call.

@@ +60,5 @@
> +  var notification = toolBar.notificationBar.getElement({type: "notification",
> +                                                         value: "popup-blocked"});
> +
> +  tabBrowser.closeAllTabs();
> +  controller.open(TEST_DATA + POPUPS_COUNT);

For traceability I would insert a waitForPageLoad() call, so that we know if something was wrong with loading the page. It's independent from the notification check.

@@ +64,5 @@
> +  controller.open(TEST_DATA + POPUPS_COUNT);
> +
> +  assert.throws(() => {
> +    assert.waitFor(() => notification.exists(), "Notification element exists");
> +  }, "Testing if notification exists should return - TimeoutError");

This call condition is wrong. The second parameter to throws() is the exception type to catch. You overwrite this with the message.

::: metrofirefox/tests/functional/testPopups/testPopupsBlocked.js
@@ +62,5 @@
> +  controller.open(TEST_DATA + POPUPS_COUNT);
> +  controller.waitForPageLoad();
> +  assert.throws(() => {
> +    assert.waitFor(() => notification.exists(), "Notification element exists");
> +  }, "Testing if notification exists should return - TimeoutError");

Same as for the other test.

Comment 34

[Daniel Gherasim]

Attachment: popups_tests_v10.patch

Used errors.TimeouError as the second argument for throws and added the appropriate message.

Comment 35

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

Comment on attachment 8408164 [details] [diff] [review]
popups_tests_v10.patch

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

I think that looks fine now. Thanks.

Comment 36

[Andreea Matei [:AndreeaMatei]]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/1ee31829688c