994040 - Failures across several tests due to notification panel changes

Status: RESOLVED FIXED

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

Description

[Andrei Eftimie]

A new regression in Nightly.
http://mozmill-daily.blargon7.com/#/functional/report/7ab760e27012969ae02e3b9e417e214e

Comment 1

[Andreea Matei [:AndreeaMatei]]

I'll look for the regression here.

Comment 2

[Andrei Eftimie]

Attachment: skip.patch

Skip for default.

Comment 3

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8403947 [details] [diff] [review]
skip.patch

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

Disabled:
http://hg.mozilla.org/qa/mozmill-tests/rev/414666a18ad1 (default)

Comment 4

[Andrei Eftimie]

There's additional tests that need to be skipped.

Functional:
> testSecurity/testGreyLarry.js 

Remote:
> /testSecurity/testDVCertificate.js
> /testSecurity/testSecurityInfoViaMoreInformation.js
> /testSecurity/testSubmitUnencryptedInfoWarning.js

All are failing while working with the Notification Panel. The message is "Window has been found."

Comment 5

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

I would assume this has been caused by bug 610545.

Comment 6

[Andrei Eftimie]

Attachment: skip2.patch

The additional skip patch.

Comment 7

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8403972 [details] [diff] [review]
skip2.patch

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

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

Comment 8

[Andreea Matei [:AndreeaMatei]]

Yep, bug 610545 is the cause, I used inbound builds and found first bad build with rev:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2c661f15fcd which is part of that bug.

Comment 9

[Neil Deakin]

You would need to change any tests to wait for the popup transition to finish before sending mouse events to it.

Comment 10

[Andreea Matei [:AndreeaMatei]]

Attachment: skip_endurance.patch

Also one endurance test affected. Fix will be done tomorrow morning, after some more testing.

Comment 11

[Andrei Eftimie]

Comment on attachment 8404059 [details] [diff] [review]
skip_endurance.patch

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

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

Comment 12

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

Hm, would the patch on bug 987098 actually fix that?

Comment 13

[Daniel Gherasim]

Not really, but the same modifications may be needed though.
In tests that were skipped by this bug we have to wait for different panels like the identity panel or the bookmark page panel.

Not sure why 
> /testSecurity/testSubmitUnencryptedInfoWarning.js
fails cause I don't see that we open any panel there.

Comment 14

[Andreea Matei [:AndreeaMatei]]

That one fail intermittently and only in testrun, I was curious too because I have no fix change for it in my upcoming patch.

We have 2 new attributes when a panel opens up, until now we only checked "state" (open/closed). Now we have "animate" and "panelopen".

Comment 15

[Daniel Gherasim]

Great, I suggest that at least for the identity panel to make an workaround here and once bug 816084 is done, we'll have correct open() and close() methods for that.

Comment 16

[Andrei Eftimie]

It seems /testGeolocation/testShareLocation.js is also affected: 
http://mozmill-daily.blargon7.com/#/functional/report/7ab760e27012969ae02e3b9e418fcbf7

Comment 17

[Andreea Matei [:AndreeaMatei]]

It seems waiting for 'transitionend' and the right attributes for each case (open/close) won't fix it every time, I still have failures intermittently for the tests that use this method changed:
http://hg.mozilla.org/qa/mozmill-tests/file/233878149561/firefox/lib/toolbars.js#l665

Comment 18

[Andreea Matei [:AndreeaMatei]]

Attachment: skip more affected tests

Added skip patch for testSwitchToTabs.js and testShareLocation.js

Comment 19

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

Comment on attachment 8405240 [details] [diff] [review]
skip more affected tests

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

Why do we have to skip switchToTab when the failure occurs in suggestBookmarks as given on the other bug?

Comment 20

[Andreea Matei [:AndreeaMatei]]

Attachment: skip more affected tests

Right, updated that.

Comment 21

[Andrei Eftimie]

Comment on attachment 8405247 [details] [diff] [review]
skip more affected tests

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

Looks good.
I've updated the commit message to contain the correct skipped test.

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

Comment 22

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v1

Changed the waitForNotification() method as well as waitForPanel() for bookmarks to add a callback which triggers the notification/panel and to wait for transitionend and the appropriate attributes (animate/state).
The security tests will have that part of the code moved in the upcoming library.
Fixed all affected tests.

Reports:
functional: http://mozmill-crowd.blargon7.com/#/functional/report/7ab760e27012969ae02e3b9e41e5ecb2
remote: http://mozmill-crowd.blargon7.com/#/remote/report/7ab760e27012969ae02e3b9e41e702ba
endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/7ab760e27012969ae02e3b9e41e7fd33

Comment 23

[Neil Deakin]

(In reply to Andreea Matei [:AndreeaMatei] from comment #14)
> We have 2 new attributes when a panel opens up, until now we only checked
> "state" (open/closed). Now we have "animate" and "panelopen".

Please don't use these. For the former (animate), you should check the popup.state or use the events (popupshown, popuphidden) instead.  The latter (panelopen) is slated to be removed.

Comment 24

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

Comment on attachment 8405362 [details] [diff] [review]
patch v1

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

r- based on the feedback from Neil. Please fix it accordingly.

Comment 25

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v2

Updated to use "popupshown" and "popuphidden" and also removed the panelopen attribute, we're only checking state.

endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/3b05997c57f8cb2e7551cc2d7734700b
functional: http://mozmill-crowd.blargon7.com/#/functional/report/3b05997c57f8cb2e7551cc2d7733a865
remote: http://mozmill-crowd.blargon7.com/#/remote/report/3b05997c57f8cb2e7551cc2d7734a572

Comment 26

[Andrei Eftimie]

Comment on attachment 8406186 [details] [diff] [review]
patch v2

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

::: firefox/tests/remote/testSecurity/testSecurityInfoViaMoreInformation.js
@@ +47,5 @@
> +  }
> +
> +  finally {
> +    controller.window.removeEventListener("popupshown", onPopupsShown);
> +  }

We have this code in 4-5 places.
For DRY-ness can we include this code somewhere and call it from the tests?

Actually, I see this is very similar to what we do in waitForNotification from toolbars, can't we use that?

Comment 27

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v3

Used that method, also updated identity-box in getElements as it had a typo. Those will be moved in the new security class anyway some time soon.

Reports:
http://mozmill-crowd.blargon7.com/#/remote/report/3b05997c57f8cb2e7551cc2d7799e411
http://mozmill-crowd.blargon7.com/#/functional/report/3b05997c57f8cb2e7551cc2d779a2999
http://mozmill-crowd.blargon7.com/#/endurance/report/3b05997c57f8cb2e7551cc2d779a666f

Comment 28

[Andrei Eftimie]

Comment on attachment 8406904 [details] [diff] [review]
patch v3

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

Looks good overall.
There are some more changes to most of these tests, but that will be covered by the refactoring.

There is one issue. I still see a failure in testSwitchToTab.js
I've also seen 1 failure in testPasswordSavedAndDeleted.js with "Notification popup visibility state has been changed"[1] but I haven't been able to reproduce it:

I'm eager to land this fix since it affects lots of tests.
If we can't find an easy fix for the testSwitchToTab.js failure, I'd suggest to leave the testSuggestBookmarks.js skipped and land the fix as is for the rest of the tests, and fix that later.

[1]http://mozmill-crowd.blargon7.com/#/functional/report/3b05997c57f8cb2e7551cc2d77b3b39b

::: firefox/tests/functional/testAwesomeBar/testSuggestBookmarks.js
@@ +50,2 @@
>    var doneButton = locationBar.editBookmarksPanel.getElement({type: "doneButton"});
> +  controller.waitThenClick(doneButton);

This still fails intermittently for me. (50%)
It might be that we're still not waiting enough in waitForPanel?

I see the bookmark panel open, and we fail to close it.

Comment 29

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

(In reply to Andrei Eftimie from comment #28)
> ::: firefox/tests/functional/testAwesomeBar/testSuggestBookmarks.js
> @@ +50,2 @@
> >    var doneButton = locationBar.editBookmarksPanel.getElement({type: "doneButton"});
> > +  controller.waitThenClick(doneButton);

FYI: Any event methods are no longer available in Mozmill 2.1, so please don't use controller.waitThenClick() and others anymore.

Comment 30

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #29)
> (In reply to Andrei Eftimie from comment #28)
> > ::: firefox/tests/functional/testAwesomeBar/testSuggestBookmarks.js
> > @@ +50,2 @@
> > >    var doneButton = locationBar.editBookmarksPanel.getElement({type: "doneButton"});
> > > +  controller.waitThenClick(doneButton);
> 
> FYI: Any event methods are no longer available in Mozmill 2.1, so please
> don't use controller.waitThenClick() and others anymore.

Yeah, we have bug 863139 where we already addressed these.
And it touches all this code.

I'm inclined to let these changes be made in a refactoring patch and for now have these tests re-enabled as soon as possible.

Comment 31

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

(In reply to Andrei Eftimie from comment #30)
> Yeah, we have bug 863139 where we already addressed these.
> And it touches all this code.

No, we have bug 992137 for that, but for code we touch or newly add, please do it right now. After the patch on the before mentioned bug landed, I don't want to see any of those instances in our repository anymore. Also I don't want to stress Gilles to have to update the patch due to changes like that. 

> I'm inclined to let these changes be made in a refactoring patch and for now
> have these tests re-enabled as soon as possible.

As said above. Do it only for code this patch touches.

Comment 32

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v4

We're waiting for the panel to be open/closed as well, we need both popupshown/popuphidden and transitionend in order to wait for the panel to be fully open and click on it's buttons. Also updated the controller.events we're using on the code we touch here.

http://mozmill-crowd.blargon7.com/#/remote/report/3b05997c57f8cb2e7551cc2d77de166b
http://mozmill-crowd.blargon7.com/#/functional/report/3b05997c57f8cb2e7551cc2d77e3163f
http://mozmill-crowd.blargon7.com/#/endurance/report/3b05997c57f8cb2e7551cc2d77e42655

Comment 33

[Andrei Eftimie]

Comment on attachment 8408233 [details] [diff] [review]
patch v4

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

::: firefox/lib/toolbars.js
@@ +287,4 @@
>  
> +    var bookmarkPanel = this.getElement({type: "bookmarkPanel"});
> +    bookmarkPanel.getNode().addEventListener("transitionend", onTransitionEnd);
> +    bookmarkPanel.getNode().addEventListener(eventType, onPanelState);

This fails with:
> "eventType is not defined"
In testSuggestBookmarks.js and testAddBookmarkToMenu.js

I think it should have been `state`

@@ +294,5 @@
> +
> +      assert.waitFor(() => {
> +        return transitionEnd && panelState &&
> +               bookmarkPanel.getNode().state === state;
> +      }, "Edit Bookmarks Panel has been " + state, timeout, undefined, this);

You can remove the last 2 arguments.

::: firefox/tests/functional/restartTests/testAddons_installMultipleExtensions/test1.js
@@ +76,4 @@
>  
>      // Wait for the notification to unload
> +    locationBar.waitForNotification("notification_popup", () => {
> +      controller.keypress(null , 'VK_ESCAPE', {});

As noted in comment 31 please also change this action.
You can probably trigger it on the `locationBar`.

::: firefox/tests/functional/testPasswordManager/testPasswordNotificationBar.js
@@ +55,5 @@
>                               );
>  
> +  // Close the notification and wait for it to unload
> +  locationBar.waitForNotification("notification_popup", () => {
> +    controller.keypress(passwordNotification, "VK_ESCAPE", {});

elem.keypress

Comment 34

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v5

Updated as requested.

http://mozmill-crowd.blargon7.com/#/endurance/report/c90eeeb0a5511695efc74625531e1ac7
http://mozmill-crowd.blargon7.com/#/functional/report/c90eeeb0a5511695efc74625531d9121
http://mozmill-crowd.blargon7.com/#/remote/report/c90eeeb0a5511695efc746255310fe8a

Comment 35

[Andrei Eftimie]

Comment on attachment 8410263 [details] [diff] [review]
patch v5

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

Looks good.

Comment 36

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

Comment on attachment 8410263 [details] [diff] [review]
patch v5

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

Wonderful changes which will make our tests way more stable! Please fix the issues I have pointed out.

::: firefox/lib/toolbars.js
@@ +271,5 @@
>     *        Object with parameters for customization
>     *        Elements: timeout - Duration to wait for the target state
>     *                            [optional - default: 5s]
> +   * @param {Boolean} [aState=false]
> +   *        True if the panel should be open

Why not making it part of aSpec? Also I would default to true given that we will mostly wait for panels to be shown. Also I don't think aState is a good name given that I would expect a state and not a boolean as value.

@@ +280,5 @@
>      var spec = aSpec || { };
>      var timeout = spec.timeout;
> +    var eventType = aState ? "popupshown" : "popuphidden";
> +    var state = aState ? "open" : "closed";
> +    var panelState = false;

I would call it panelStateChanged

@@ +281,5 @@
>      var timeout = spec.timeout;
> +    var eventType = aState ? "popupshown" : "popuphidden";
> +    var state = aState ? "open" : "closed";
> +    var panelState = false;
> +    var transitionEnd = false;

Also can we put both variables in an object, so we have all event related variables bundled?

@@ +554,3 @@
>        case "identityPopup":
> +        return [new elementslib.ID(root, "identity-popup")];
> +      case "identityPopupEncryption":

lets call this case identityPopup_encryption.

@@ +696,5 @@
>     */
> +  waitForNotification : function locationBar_waitForNotification(aElement,
> +                                                                 aCallback,
> +                                                                 aState,
> +                                                                 aIcon) {

waitForNotification should have the same API across different library classes. So also make use of aSpec here please.

@@ +705,5 @@
> +    var eventType = aState ? "popupshown" : "popuphidden";
> +    var state = aState ? "open" : "closed";
> +
> +    function onPopupState() { popups = true; }
> +    this._controller.window.addEventListener(eventType, onPopupState);

We don't need transitionend here?

::: firefox/tests/functional/restartTests/testAddons_installMultipleExtensions/test1.js
@@ +76,3 @@
>  
>      // Wait for the notification to unload
> +    locationBar.waitForNotification("notification_popup", () => {

Maybe we should get a new bug filed so we can rename 'notification_popup' to a better name. It's unclear what this is.

Comment 37

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v6

Updated to use aSpec.

http://mozmill-crowd.blargon7.com/#/functional/report/c90eeeb0a5511695efc74625538acc4d
http://mozmill-crowd.blargon7.com/#/endurance/report/c90eeeb0a5511695efc74625538c5a1b
http://mozmill-crowd.blargon7.com/#/remote/report/c90eeeb0a5511695efc74625538aebc5

Comment 38

[Andrei Eftimie]

Comment on attachment 8410998 [details] [diff] [review]
patch v6

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

Looks good to me.

::: firefox/lib/toolbars.js
@@ +317,5 @@
> +   * @param {object} aSpec
> +   *        Object with parameters for customization
> +   * @param {Boolean} [aSpec.open=true]
> +   *        True if the panel should be open
> +   * @param {Number} [aSpec.timeout=5000]

I don't see this defaulting here.
We're still sending `undefined` further to mozmill.
Not sure in this case if this comment should mention this or not.

Comment 39

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

Comment on attachment 8410998 [details] [diff] [review]
patch v6

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

Do you have a reply to my comment for:

> Maybe we should get a new bug filed so we can rename 'notification_popup' to a better name. It's unclear what this is.

Otherwise please see the inline comments.

::: firefox/lib/toolbars.js
@@ +311,5 @@
> +
> +  /**
> +   * Wait for Edit Bookmarks Panel to load
> +   *
> +   * @param {Function} aCallback

Please use 'function' here. Same for the types below, which should all start with a lower-case letter.

@@ +317,5 @@
> +   * @param {object} aSpec
> +   *        Object with parameters for customization
> +   * @param {Boolean} [aSpec.open=true]
> +   *        True if the panel should be open
> +   * @param {Number} [aSpec.timeout=5000]

Agreed. Just leave out the default value.

@@ +331,5 @@
> +    var state = open ? "open" : "closed";
> +    var self = {
> +      panelStateChanged: false,
> +      transitionEnd: false
> +    }

nit: missing semicolon.

@@ +344,5 @@
> +      aCallback();
> +
> +      assert.waitFor(() => {
> +        return self.transitionEnd && self.panelStateChanged &&
> +               bookmarkPanel.getNode().state === state;

Do we really need the last condition? We will never hit multiple ones given that we only call the callback once. So similar to other modules we should use an assert.equal afterward to check if it has the correct state.

@@ +698,3 @@
>     *        True if the notification should be open
> +   * @param {String} [aSpec.type]
> +   *        Notification type element

Is this really optional?

@@ +701,2 @@
>     */
> +  waitForNotification : function locationBar_waitForNotification(aSpec, aCallback) {

Ensure we keep the calling conventions between waitForNotification methods across classes. So callback should come first.

@@ +704,3 @@
>  
> +    var spec = aSpec || { };
> +    var notification = this.getElement({type: spec.type});

Ensure that spec.type is really available.

@@ +728,5 @@
> +    finally {
> +      this._controller.window.removeEventListener(eventType, onPopupState);
> +      this._controller.window.removeEventListener("transitionend", onTransitionEnd);
> +
> +    }

Lots of identical code. We might want to centralize this in the future.

@@ +736,1 @@
>        assert.ok(elem.getNode(), "The notification icon has been found");

Do we really need this here? Isn't that part of a test? Personally i don't see a relation to open/close a notification.

Comment 40

[Andreea Matei [:AndreeaMatei]]

(In reply to Henrik Skupin (:whimboo) from comment #39)
> Comment on attachment 8410998 [details] [diff] [review]
> patch v6
> 
> Review of attachment 8410998 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do you have a reply to my comment for:
> 
> > Maybe we should get a new bug filed so we can rename 'notification_popup' to a better name. It's unclear what this is.

Yeah, sorry I missed it. I can even rename it in here, it's not difficult. The popup is for notifications like after you install an addon, you get the small puzzle icon near the glob/lock icon and informs you about the action (the addon has been installed/needs restart).
Same notification popup for login information, to save the password (you get a key icon).

Comment 41

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

No, lets move it out to a separate bug please.

Comment 42

[Andreea Matei [:AndreeaMatei]]

I still have a question here before uploading a new version.

(In reply to Henrik Skupin (:whimboo) from comment #39)
> 
> @@ +736,1 @@
> >        assert.ok(elem.getNode(), "The notification icon has been found");
> 
> Do we really need this here? Isn't that part of a test? Personally i don't
> see a relation to open/close a notification.

This was introduced in bug 758187 when we created this method and figured we can also have icon checks along with the notification (as mentioned above, can be the globe, a key, a puzzle, the geolocation one). The geolocation test was checking it, should I move this back in there and remove it from here?
I think it makes sense to leave it as it is, we wait for a notification and for it's icon too, now depending of the test, some test icons too, some don't.
https://bugzilla.mozilla.org/show_bug.cgi?id=758187#c98

Comment 43

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

I would move it back. There will be most likely a single test for each area which will have to check the icon - if at all. Maybe browser chrome tests are already doing that, so this check might even be redundant.

Comment 44

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v6.1

Removed the icon check as established. Regarding the waitFor with the state check removed in a separate assert, I had testAddons_installMultipleExtensions failing, because even after the events are true, the state can still be unchanged or goes through different values (notice hiding):

WaitFor state to be open:

transitionend true
popupState true
state closed

transitionend true
popupState true
state open

WaitFor state to be closed:
transitionend false
popupState false
state hiding

transitionend false
popupState false
state hiding

transitionend true
popupState true
state closed

Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/c90eeeb0a5511695efc7462553b775c5
http://mozmill-crowd.blargon7.com/#/remote/report/c90eeeb0a5511695efc7462553b7bcd5
http://mozmill-crowd.blargon7.com/#/endurance/report/c90eeeb0a5511695efc7462553b871be

Comment 45

[Andrei Eftimie]

Comment on attachment 8411841 [details] [diff] [review]
patch v6.1

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

Looks good.

Just a note that I've ran several functional testruns with this patch applied, and I've seen a few instances where it failed in panel related tests.

Some examples:
http://mozmill-crowd.blargon7.com/#/functional/report/db3ef7ec039e4c6b255196b62805b302
http://mozmill-crowd.blargon7.com/#/functional/report/db3ef7ec039e4c6b255196b6280596eb
http://mozmill-crowd.blargon7.com/#/functional/report/c90eeeb0a5511695efc7462553b5fd7e
http://mozmill-crowd.blargon7.com/#/functional/report/c90eeeb0a5511695efc7462553b5e8ae
http://mozmill-crowd.blargon7.com/#/functional/report/c90eeeb0a5511695efc7462553adfee7

Is there maybe another event we should wait for?
I'd say to land it (and reenable all these skipped tests) and if we still see failures log them separately.

From my results mentioned above I only saw 2 tests that _might_ be affected...

Comment 46

[Andreea Matei [:AndreeaMatei]]

I ran multiple times on my mac but it didn't failed. 

Neil, I'm using now events popupshown and popuphidden and transitionend (for bookmark panel at lease I've seen it's necessary) and also the state of the popup (to be open/closed).
Do you think this should be enough?

Comment 47

[Andrei Eftimie]

Comment on attachment 8411841 [details] [diff] [review]
patch v6.1

Henrik, what do you say?

I've gotten _some_ failures as mentioned in comment 45 in 2 tests. But I don't have anything conclusive. Both intermittent. And only I have seen them.

I would go ahead with unskipping these tests and if we see any failures manage them separately (if they are localised in 2 tests).

Comment 48

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

Comment on attachment 8411841 [details] [diff] [review]
patch v6.1

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

Looks way better now. Inline you can find some remaining things to get fixed. Please do so. With those changes you get r=me, and you can carry it over.

::: firefox/lib/toolbars.js
@@ +344,5 @@
> +      aCallback();
> +
> +      assert.waitFor(() => {
> +        return self.transitionEnd && self.panelStateChanged &&
> +               bookmarkPanel.getNode().state === state;

I'm still not happy about this. If we don't get a response from Neal in time, you might also ask Marco Bonardo for feedback in regards of necessary events and states.

@@ +345,5 @@
> +
> +      assert.waitFor(() => {
> +        return self.transitionEnd && self.panelStateChanged &&
> +               bookmarkPanel.getNode().state === state;
> +      }, "Edit Bookmarks Panel has been " + state, timeout);

The wording looks awkward in combination with state 'open'. May be this is better: "Edit Bookmarks Panel state has been changed to 'open'".

@@ +702,3 @@
>  
> +    var spec = aSpec || { };
> +    assert.ok(spec.type, "Notification type element has been specified.");

I would keep the asserts for checking parameters side by side. So move the one for the callback right here.

::: firefox/tests/functional/testGeolocation/testShareLocation.js
@@ +46,5 @@
>  
> +  // Check the geolocation icon is visible in the location bar
> +  var geolocationIcon = locationBar.getElement({type: "notificationIcon",
> +                                                subtype: "geo"});
> +  expect.ok(geolocationIcon.getNode(), "The notification icon has been found");

It's enough to call it icon. There is no other icon variable around here. Also use icon.exists() instead for the check.

Comment 49

[Andreea Matei [:AndreeaMatei]]

Thanks Henrik, I'll look for Neil to see what he thinks of that. In comment 23 he mentioned to use state OR popup events, but it seems we need both, there's still a time issue otherwise. 

About the icon, there is another one in the test, the one in the notification itself. The one I added is in the location bar.
http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/tests/functional/testGeolocation/testShareLocation.js#l48

Comment 50

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

Then I would call the icon for the notification popup `notification_icon', and keep the addition as `icon`

Comment 51

[Neil Deakin]

For showing the popup, until bug 994117 is fixed, you should use transitionend if you know that there will be a transition, however it would probably be safer to do what you need to after the event has finished with a setTimeout(fn, 0) or something similar.

For hiding the popup, you can just use popuphidden as normal.

Comment 52

[Andreea Matei [:AndreeaMatei]]

Neil, my issue here is that I wait for popupshown and transitionend but then when I assert the state which should be open, it fails. So something else is taking time to change the state as well. Please see comment 48, first code snippet. If I remove the state line from that waitFor and just assert it later after the events are true, it still fails because it didn't change yet.

Comment 53

[Neil Deakin]

Do you mean the panel's state or the state checked by bookmarkPanel.getNode().state? For the latter, I don't know.

Comment 54

[Daniel Gherasim]

As far as I can see, we have 9 tests skipped here.
We are failing in waiting for 3 panels : 
* identity-popup (in 5 tests)
* bookmarksPanel (2 tests)
* notification-popup (2 tests)

All this 3 are different elements (even though the first and third appear in the same location) and we should have different waitForPanel methods (to wait for the proper events for each, as they were implemented differently). 

For the first panel (id=identity-popup) waiting for 'popuphidden'||'popupshown'&&'blur' events seems to be enough (I've tested all the skipped tests under linux/mac/windows with 20 runs on each & works great, no failure, patch from bug 816084). 

So what I suggest here, let's work & unskip all those 5 security tests (handling the identity popup) with the patch from bug 816084 which is close to be done as Henrik also said in bug 816084 comment 53 & finish here the the work for the other tests that were skipped (with the other 2 panels).

Henrik, what do you think of this?

Comment 55

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

Sounds reasonable to me. Lets not work on stuff which has been already (or nearly) fixed somewhere else.

Comment 57

[Daniel Gherasim]

Attachment: v7.patch

Changed to wait for 2 transitionend events given that we have 2 transitions on each popup (transform & opacity) & no transition on linux.

Also in testAddons_installMultipleExtension changed to wait for the proper notification to be showed.

Functional testrun reports, I'll run some more & test the other tests too.
http://mozmill-crowd.blargon7.com/#/functional/report/44e0169acb64b68a44c1ad880b7584bc
http://mozmill-crowd.blargon7.com/#/functional/report/44e0169acb64b68a44c1ad880b759c06
http://mozmill-crowd.blargon7.com/#/functional/report/44e0169acb64b68a44c1ad880b75a0ef

Comment 58

[Daniel Gherasim]

Attachment: v7.1.patch

Changed to use only one 'waitForArrowPanel' method to avoid code duplication.

Comment 59

[Daniel Gherasim]

Attachment: v7.2.patch

We don't need the identity-popup changes to be done in this patch.

Comment 60

[Daniel Gherasim]

Attachment: v7.3.patch

Replaced 'waitThenClick()' methods with simple 'click()'.

Comment 61

[Daniel Gherasim]

Tests that were modified:
> firefox/tests/endurance/testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/test1.js
> firefox/tests/functional/restartTests/testPreferences_masterPassword/test1.js
> firefox/tests/functional/restartTests/testAddons_installMultipleExtensions/test1.js
> firefox/tests/functional/testAwesomeBar/testSuggestBookmarks.js
> firefox/tests/functional/testBookmarks/testAddBookmarkToMenu.js
> firefox/tests/functional/testGeolocation/testShareLocation.js
> firefox/tests/functional/testPasswordManager/testPasswordNotificationBar.js
> firefox/tests/functional/testPreferences/testPasswordSavedAndDeleted.js
> firefox/tests/remote/restartTests/testAddons_InstallAddonWithoutEULA/test1.js

Testing results with the latest patch:
Linux 12.04: 
http://mozmill-crowd.blargon7.com/#/functional/report/44e0169acb64b68a44c1ad880b7c22f5
Mac 10.6: 
http://mozmill-crowd.blargon7.com/#/functional/report/44e0169acb64b68a44c1ad880b7bb4e8
Windows 8.1:
http://mozmill-crowd.blargon7.com/#/functional/report/44e0169acb64b68a44c1ad880b7ba4ba

I also ran all tests (except the last one) individually *50* times each, on *all 3* platforms I got *no failure*. 

I'll try to see what's not working with the last test as it fails about 50% of time with 'Add-on has been specified. -got 'undefined' in the second test and we get a promise exception in the first (AsyncShutdown.jsm).

>extractFilesAsync: failed to extract file /var/folders/cN/cNHCbIOzES0sdotWUgvmf++++TM/-Tmp-/tmprvZ_nb.mozrunner/extensions/staged/{8620c15f-30dc-4dba-a131-7c5d20cf4a29}/chrome/locale/zh-CN/customize.dtd
>1400157544746    addons.xpi    WARN    Failed to install /Users/danielgherasim/Library/Caches/TemporaryItems/tmp-nar.xpi from https://addons.mozilla.org/firefox/downloads/latest/6543/addon-6543-latest.xpi?src=dp-btn-primary: Error: OS.File has been shut down. Rejecting request to open (resource://gre/modules/osfile/osfile_async_front.jsm:425:1) JS Stack trace: post/<@osfile_async_front.jsm:425:2 < [e-mail]:282:13 < [e-mail]:247:3 < createAsyncFunction/[e-mail]:224:7 < [e-mail]:863:11 < [e-mail]:742:7 < [e-mail]:344:7
>*************************
>A coding exception was thrown in a Promise rejection callback.

>Full message: TypeError: this.installs is null
>See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
>Full stack: XPI_removeActiveInstall@resource://gre/modules/addons/XPIProvider.jsm:3562:9
>AI_startInstall/<@resource://gre/modules/addons/XPIProvider.jsm:5589:7
>Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:9
>this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:742:7
>Spinner.prototype.observe@resource://gre/modules/AsyncShutdown.jsm:344:7

I believe we are trying to close firefox too fast, so we should wait for the 'addons-complete-install' to popup and then let firefox to close. I'll test this.

Comment 62

[Daniel Gherasim]

On linux: 

*************************
A coding exception was thrown in a Promise rejection callback.

Full message: ReferenceError: logger is not defined
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
Full stack: saveStreamAsync/readFailed/<@resource://gre/modules/ZipUtils.jsm:68:7
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:9
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:742:7
Spinner.prototype.observe@resource://gre/modules/AsyncShutdown.jsm:446:7

*************************
1400158787621	addons.xpi	WARN	Attempted to remove {8620c15f-30dc-4dba-a131-7c5d20cf4a29} from app-profile but it was already gone

Waiting for "addon-install-complete" notification popup fixes the problem. We are trying to close firefox before this popup even pops out.

Comment 63

[Daniel Gherasim]

Attachment: v7.4.patch

Now all works fine.

Comment 64

[Andrei Eftimie]

Comment on attachment 8423028 [details] [diff] [review]
v7.3.patch

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

::: firefox/lib/toolbars.js
@@ +507,5 @@
>          nodeCollector.root = this.getElement({type: "urlbar"}).getNode();
>          nodeCollector.queryAnonymousNode("anonid", "historydropmarker");
>          break;
>        case "identityBox":
> +        return [new elementslib.ID(root, "identity-box")];

findElement please

@@ +652,3 @@
>     */
> +  waitForArrowPanel : function locationBar_waitForArrowPanel(aCallback, aSpec) {
> +    assert.equal(typeof aCallback, "function", "Callback function is defined");

nit: leave an empty line here

@@ +661,4 @@
>  
> +    switch (type) {
> +      case "notification_popup":
> +        eventNode = this._controller.window;

You already assign the same value to eventNode before the switch.

@@ +673,5 @@
> +
> +    var self = {
> +      panelStateChanged: false,
> +      transitions: 0
> +    };

nit: leave an empty line please

@@ +675,5 @@
> +      panelStateChanged: false,
> +      transitions: 0
> +    };
> +    function onTransitionEnd() {
> +      if (self.transitions < 2)

Always use curly braces.

@@ +681,5 @@
> +    }
> +    function onPanelState() { self.panelStateChanged = true; }
> +
> +    eventNode.addEventListener(eventType, onPanelState);
> +    // Bug 1001234

This already landed in Nightly and Aurora.
Please check if it is fixed.

We might only need to add this on Beta and older.

@@ +683,5 @@
> +
> +    eventNode.addEventListener(eventType, onPanelState);
> +    // Bug 1001234
> +    // Animation is disabled on linux
> +    if(!mozmill.isLinux) {

nit: missing space after `if`

@@ +692,5 @@
> +      aCallback();
> +
> +      assert.waitFor(() => {
> +        return self.panelStateChanged &&
> +               ((!mozmill.isLinux) ? (self.transitions === 2) : true);

You can remove the outermost parentheses.

::: firefox/tests/functional/testBookmarks/testAddBookmarkToMenu.js
@@ +33,1 @@
>    // editBookmarksPanel is loaded lazily. Wait until overlay for StarUI has been loaded

I don't think this comment is needed anymore. All panel are now "loaded lazily"

Comment 65

[Daniel Gherasim]

> @@ +681,5 @@
> > +    }
> > +    function onPanelState() { self.panelStateChanged = true; }
> > +
> > +    eventNode.addEventListener(eventType, onPanelState);
> > +    // Bug 1001234
> 
> This already landed in Nightly and Aurora.
> Please check if it is fixed.
> 
> We might only need to add this on Beta and older.
> 

That bug was intended to disable the animation on linux in nightly & aurora (only branches it's currently present). 
This patch it's also for this 2 branches only so we're fine.

Comment 66

[Daniel Gherasim]

Attachment: v7.5.patch

Thanks, I made the changes.

Comment 67

[Daniel Gherasim]

Attachment: v7.6.patch

Small nit.

Comment 68

[Andrei Eftimie]

Comment on attachment 8423133 [details] [diff] [review]
v7.6.patch

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

I have some failures we'll need to investigate.

::: firefox/tests/functional/restartTests/testPreferences_masterPassword/test2.js
@@ +112,5 @@
>  
>    var button = new elementslib.Lookup(controller.window.document,
>                                 '/id("commonDialog")/anon({"anonid":"buttons"})/{"dlgtype":"accept"}');
>    controller.click(button);
>  }

This test fails. Filed bug 1011426 for it, it's probably a different issue, but we didn't catch it because the test is skipped.

::: firefox/tests/functional/testGeolocation/testShareLocation.js
@@ +64,4 @@
>    // Wait for the notification to unload
> +  locationBar.waitForArrowPanel(() => {
> +    button.click();
> +  }, {type: "notification_popup", open: false});

This fails for me. The test fails to click on the button.
http://mozmill-crowd.blargon7.com/#/functional/report/7a69d3e8424ecf8642bb6566e401535e

Comment 69

[Daniel Gherasim]

Attachment: v7.7.patch

(In reply to Andrei Eftimie from comment #68)
> Comment on attachment 8423133 [details] [diff] [review]
> v7.6.patch
> 
> Review of attachment 8423133 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have some failures we'll need to investigate.
> 
> :::
> firefox/tests/functional/restartTests/testPreferences_masterPassword/test2.js
> @@ +112,5 @@
> >  
> >    var button = new elementslib.Lookup(controller.window.document,
> >                                 '/id("commonDialog")/anon({"anonid":"buttons"})/{"dlgtype":"accept"}');
> >    controller.click(button);
> >  }
> 
> This test fails. Filed bug 1011426 for it, it's probably a different issue,
> but we didn't catch it because the test is skipped.
> 
> ::: firefox/tests/functional/testGeolocation/testShareLocation.js
> @@ +64,4 @@
> >    // Wait for the notification to unload
> > +  locationBar.waitForArrowPanel(() => {
> > +    button.click();
> > +  }, {type: "notification_popup", open: false});
> 
> This fails for me. The test fails to click on the button.
> http://mozmill-crowd.blargon7.com/#/functional/report/
> 7a69d3e8424ecf8642bb6566e401535e


OUPS! Sorry for that! Both of the failures appeared after I removed the paranthesis from the terminary operathor (it has a lower precedence then '&&') in the last patch & it was evaluated bad. Fixed that now!

Comment 70

[Andrei Eftimie]

Comment on attachment 8423790 [details] [diff] [review]
v7.7.patch

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

Works fine now. Thanks for the quick update.
This looks good to me.

There are 5 test that remain skipped, and they will be fixed in bug 816084.

Comment 72

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

Comment on attachment 8423790 [details] [diff] [review]
v7.7.patch

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

What happened with a couple of the other tests which had been changed previously? Like security tests and other remote ones. I don't see them in this patch. Maybe I need a carificatino here:

> So what I suggest here, let's work & unskip all those 5 security tests
> (handling the identity popup) with the patch from bug 816084 which is close
> to be done as Henrik also said in bug 816084 comment 53 & finish here the
> the work for the other tests that were skipped (with the other 2 panels).

We cannot only unskip but also have to make changes as done by this test. How will this be handled? Looks like I misunderstood the initial request.

::: firefox/lib/toolbars.js
@@ +281,5 @@
>         * subtype: subtype to match
>         * value: value to match
>         */
> +      case "bookmarkPanel":
> +        elem = new findElement.ID(this._controller.window.document, "editBookmarkPanel");

findElement.ID is a method which already returns an instance. So no need for new here.

@@ +287,2 @@
>        case "doneButton":
> +        elem = new findElement.ID(this._controller.window.document, "editBookmarkPanelDoneButton");

Not sure why all those lines have been changed now. It should really have been done on another bug! All those changes are unrelated.

@@ +652,2 @@
>     */
> +  waitForArrowPanel : function locationBar_waitForArrowPanel(aCallback, aSpec) {

What is an arrow panel? I think 'PopupNotification' is a better name.

@@ +688,5 @@
> +    // Bug 1001234
> +    // Animation is disabled on linux
> +    if (!mozmill.isLinux) {
> +      eventNode.addEventListener("transitionend", onTransitionEnd);
> +    }

I feel that all those transition handling is awkward at the moment. Especially with Linux behaving completely different at the moment. I think Daniel already asked if we could disable the transitions for now. Meanwhile I would agree. We should wait until the dependent bug gets fixed for real events.

::: firefox/tests/functional/restartTests/testAddons_installMultipleExtensions/test1.js
@@ +76,5 @@
>  
> +    var notification = locationBar.getElement({type: "notification_popup"});
> +    assert.waitFor(() => {
> +      return notification.getNode().getAttribute("popupid") === "addon-install-complete";
> +    }, "Addon has been installed");

It's less likely that another popup will be opened here. I would leave this change out of the patch and for bug 1000832. Same for the other instances in this patch.

Comment 73

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #72)
> What happened with a couple of the other tests which had been changed
> previously? Like security tests and other remote ones. I don't see them in
> this patch. Maybe I need a carificatino here:
> 
> > So what I suggest here, let's work & unskip all those 5 security tests
> > (handling the identity popup) with the patch from bug 816084 which is close
> > to be done as Henrik also said in bug 816084 comment 53 & finish here the
> > the work for the other tests that were skipped (with the other 2 panels).
> 
> We cannot only unskip but also have to make changes as done by this test.
> How will this be handled? Looks like I misunderstood the initial request.
> 
I thought we could finish the current patch for this tests here & if this 'waitForPopupNotification' also applies on the identity popup we could use it in patch for bug 816084, otherwise we can create a new specific method there in the new class ( one was created before in that patch with just a single 'transitionend' event & works fine).

But if you want to also unskip & fix those within this patch I can easily do this.
I just hopped that splitting the work a bit will help us go faster with this.

> > +    // Bug 1001234
> > +    // Animation is disabled on linux
> > +    if (!mozmill.isLinux) {
> > +      eventNode.addEventListener("transitionend", onTransitionEnd);
> > +    }
> 
> I feel that all those transition handling is awkward at the moment.
> Especially with Linux behaving completely different at the moment. I think
> Daniel already asked if we could disable the transitions for now. Meanwhile
> I would agree. We should wait until the dependent bug gets fixed for real
> events.
> 

Wating for 2 transitionend events works fine, I also have seen a browser chrome test that does the same: https://hg.mozilla.org/mozilla-central/rev/77aff162aa0c#l3.18 
But I can go further with disabeling transitions until that bug get's fixed.

> > +    var notification = locationBar.getElement({type: "notification_popup"});
> > +    assert.waitFor(() => {
> > +      return notification.getNode().getAttribute("popupid") === "addon-install-complete";
> > +    }, "Addon has been installed");
> 
> It's less likely that another popup will be opened here. I would leave this
> change out of the patch and for bug 1000832. Same for the other instances in
> this patch.

I tested this & works great on all platforms, that notification changes it's id when the addon install is complete, oterwise the test fail about 50% of time as said in Comment 61 & Comment 62 . I can remember that we also see this failures in our testruns so this fix might be a good improvement.

Other option could be to fix this in bug 974930.

Comment 74

[Daniel Gherasim]

Attachment: v8.patch

Notes:
* disable animations for all notification popups;
* modified the addons test to wait for the propper instalation state;

Linux: 
http://mozmill-crowd.blargon7.com/#/functional/report/7a69d3e8424ecf8642bb6566e491dec9
http://mozmill-crowd.blargon7.com/#/remote/report/7a69d3e8424ecf8642bb6566e493f593
Windows: 
http://mozmill-crowd.blargon7.com/#/functional/report/7a69d3e8424ecf8642bb6566e493a4ed
http://mozmill-crowd.blargon7.com/#/remote/report/7a69d3e8424ecf8642bb6566e4940c41
Mac: 
http://mozmill-crowd.blargon7.com/#/functional/report/7a69d3e8424ecf8642bb6566e491e6d9
http://mozmill-crowd.blargon7.com/#/remote/report/7a69d3e8424ecf8642bb6566e493fbbc

Comment 75

[Daniel Gherasim]

/testPreferences/testPasswordSavedAndDeleted.js fails often with the same reason & will be fixed by this patch. We may consider also disabling it.

http://mozmill-daily.blargon7.com/#/functional/failure?app=All&branch=All&platform=All&from=2014-02-03&test=%2FtestPreferences%2FtestPasswordSavedAndDeleted.js&func=testSaveAndDeletePassword

Comment 76

[Andrei Eftimie]

Comment on attachment 8426189 [details] [diff] [review]
v8.patch

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

Looks good.

Just a few small items.
Theres a conflict in firefox/tests/remote/testSecurity/manifest.ini

::: firefox/lib/addons.js
@@ +1740,5 @@
> + *        Function that triggers the change
> + * @param {string} aState
> + *        The expected state
> + */
> +function waitForAddonInstallationState(aCallback, aState) {

Its not clear to me what we use aState for. We actually don't know and don't check what the state really is, so we use aState just for debugging purposes, right?

::: firefox/lib/toolbars.js
@@ +676,5 @@
> +
> +    // Bug 994117
> +    // Transitions are not handled correctly
> +    // Add waiting for transition events once they get fixed
> +    panel.getNode().setAttribute("animate", "false");

Is this a reliable way of turning off animations?

@@ +687,5 @@
> +      aCallback();
> +
> +      assert.waitFor(() => {
> +        return panelStateChanged;
> +      }, "Notification popup state has been " + (open ? "open" : "closed"));

nit: "opened"

Comment 77

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

Comment on attachment 8426189 [details] [diff] [review]
v8.patch

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

::: firefox/lib/toolbars.js
@@ +676,5 @@
> +
> +    // Bug 994117
> +    // Transitions are not handled correctly
> +    // Add waiting for transition events once they get fixed
> +    panel.getNode().setAttribute("animate", "false");

I don't think so. There should be a pref afaik.

Comment 78

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #77)
> Comment on attachment 8426189 [details] [diff] [review]
> v8.patch
> 
> Review of attachment 8426189 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/lib/toolbars.js
> @@ +676,5 @@
> > +
> > +    // Bug 994117
> > +    // Transitions are not handled correctly
> > +    // Add waiting for transition events once they get fixed
> > +    panel.getNode().setAttribute("animate", "false");
> 
> I don't think so. There should be a pref afaik.

I don't find any pref for this. As I can see in other tests there is exactly the same approach.
Another way would mean to use http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/PopupNotifications.jsm#163 but that does exactly the same thing.

Comment 79

[Daniel Gherasim]

(In reply to Andrei Eftimie from comment #76)
> Comment on attachment 8426189 [details] [diff] [review]
> v8.patch
> 
> Review of attachment 8426189 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: firefox/lib/addons.js
> @@ +1740,5 @@
> > + *        Function that triggers the change
> > + * @param {string} aState
> > + *        The expected state
> > + */
> > +function waitForAddonInstallationState(aCallback, aState) {
> 
> Its not clear to me what we use aState for. We actually don't know and don't
> check what the state really is, so we use aState just for debugging
> purposes, right?
> 

aState is the topic of the notified observer (that corresponds with a state in which the addon installation is). Maybe naming it aTopic would be better ?

Comment 80

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

(In reply to daniel.gherasim from comment #78)
> > > +    panel.getNode().setAttribute("animate", "false");
> > 
> > I don't think so. There should be a pref afaik.
> 
> I don't find any pref for this. As I can see in other tests there is exactly
> the same approach.
> Another way would mean to use
> http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/
> PopupNotifications.jsm#163 but that does exactly the same thing.

Ok, but then we should disable it for all popups and not only for that single one. Do that in setupModule/teardownModule.

Comment 81

[Daniel Gherasim]

Attachment: v9.patch

That's the patch for disabling transitions on all panels in each test in setupModule & then enable them back in teardownModule().

http://mozmill-crowd.blargon7.com/#/functional/report/4e9e123ff3e2bbcb949b21929202d492
http://mozmill-crowd.blargon7.com/#/functional/report/4e9e123ff3e2bbcb949b219292022990
http://mozmill-crowd.blargon7.com/#/functional/report/4e9e123ff3e2bbcb949b21929201c69f
http://mozmill-crowd.blargon7.com/#/remote/report/4e9e123ff3e2bbcb949b219292026953
http://mozmill-crowd.blargon7.com/#/remote/report/4e9e123ff3e2bbcb949b2192920274be
http://mozmill-crowd.blargon7.com/#/remote/report/6985c2d4927931765aff21d239670e03

Comment 82

[Andrei Eftimie]

Comment on attachment 8429219 [details] [diff] [review]
v9.patch

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

On OSX I see a failure on testAddons_installMultipleExtensions/test1.js which I don't see without this patch.
> ERROR | Test Failure | {
>   "exception": {
>     "message": "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]", 
>     "name": "NS_ERROR_FAILURE", 
>     "lineNumber": 1775
>   }
> }

::: firefox/lib/toolbars.js
@@ +411,5 @@
> +   */
> +  disablePopupAnimations : function locationBar_disablePopupAnimations() {
> +    var nodeCollector = new domUtils.nodeCollector(this._controller.window.document);
> +    nodeCollector.queryNodes("panel");
> +    for (var i in nodeCollector.elements) {

You could use nodeCollector.elements.forEach()

@@ +671,2 @@
>     *        True if the notification should be open
> +   * @param {string} aSpec.type

this is optional and has a default

@@ +677,3 @@
>  
> +    var spec = aSpec || { };
> +    var open = (spec.open == undefined) ? true : spec.open;

nit: might use ===

@@ +691,5 @@
> +        break;
> +      case "identity_popup":
> +        panel = this.getElement({type: "identityPopup"});
> +        break;
> +      default :

nit: there is an extra space

::: firefox/tests/endurance/testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/test1.js
@@ +24,5 @@
>  
> +  // Bug 994117
> +  // Transitions are not handled correctly
> +  // Remove this and add waiting for the correct transitions once this is fixed
> +  aModule.locationBar.disablePopupAnimations();

I can't say I'm a fan of disabling this on _each_ test. I like the initial workaround of disabling this in the library much better.
But if Henrik really wants it this way I guess we'll go ahead with it...

Comment 83

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

(In reply to Andrei Eftimie from comment #82)
> I can't say I'm a fan of disabling this on _each_ test. I like the initial
> workaround of disabling this in the library much better.
> But if Henrik really wants it this way I guess we'll go ahead with it...

I haven't thought it would be so many tests. So instead of implementing a rough idea that quickly, Daniel should have brought this up for discussion. Don't force my information always as true. :)

Comment 84

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #83)
> (In reply to Andrei Eftimie from comment #82)
> > I can't say I'm a fan of disabling this on _each_ test. I like the initial
> > workaround of disabling this in the library much better.
> > But if Henrik really wants it this way I guess we'll go ahead with it...
> 
> I haven't thought it would be so many tests. So instead of implementing a
> rough idea that quickly, Daniel should have brought this up for discussion.
> Don't force my information always as true. :)

I remember I specifically brought this into discussion on IRC because the request also looked for me a bit awkward.

Comment 85

[Daniel Gherasim]

(In reply to Andrei Eftimie from comment #82)
> Comment on attachment 8429219 [details] [diff] [review]
> v9.patch
> 
> Review of attachment 8429219 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> On OSX I see a failure on testAddons_installMultipleExtensions/test1.js
> which I don't see without this patch.
> > ERROR | Test Failure | {
> >   "exception": {
> >     "message": "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]", 
> >     "name": "NS_ERROR_FAILURE", 
> >     "lineNumber": 1775
> >   }
> > }
> 

Oups that happened because I was removing an observer with a bad topic :
> Services.obs.addObserver(observer, topic, false);
...
> Services.obs.removeObserver(observer, aState);

Comment 86

[Daniel Gherasim]

Attachment: v10.patch

Comment 87

[Andrei Eftimie]

Comment on attachment 8431417 [details] [diff] [review]
v10.patch

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

With the nits fixed, please ask Henrik for a review.
I'd really like to land this today before the merge.

::: firefox/tests/functional/restartTests/testAddons_installMultipleExtensions/test1.js
@@ +69,3 @@
>  
> +    addons.waitForAddonInstallationState(() => {
> +      var installLink = new elementslib.ID(controller.tabs.activeTab, "addon");

Why not findElement here?

@@ +77,2 @@
>  
> +     // Wait for the notification to unload

nit: indentation

Comment 88

[Daniel Gherasim]

Attachment: v10.1.patch

Done!

Comment 89

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

Comment on attachment 8432398 [details] [diff] [review]
v10.1.patch

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

Mostly wording issues. Please update and request review from myself, so we can get this done soon. Thanks.

::: firefox/lib/addons.js
@@ +1747,5 @@
> + * Wait for an addon to be in a specific state
> + *
> + * @param {function} aCallback
> + *        Function that triggers the change
> + * @param {string} aState="complete"|"started"|"blocked"|"failed"

You might want to put the possible states in brackets. The equal sign looks a bit strange here.

@@ +1750,5 @@
> + *        Function that triggers the change
> + * @param {string} aState="complete"|"started"|"blocked"|"failed"
> + *        The state change for addon installation that we are waiting for
> + */
> +function waitForAddonInstallationState(aCallback, aState) {

nit: Lets call it 'waitForAddonInstallState'.

::: firefox/lib/toolbars.js
@@ +281,5 @@
>         * subtype: subtype to match
>         * value: value to match
>         */
> +      case "bookmarkPanel":
> +        elem = findElement.ID(this._controller.window.document, "editBookmarkPanel");

Again, why this change? Please stay in sync with the rest of the file which is using elementslib.ID. We should change that all at once.

@@ +680,5 @@
>    /**
>     * Waits for the given notification popup
>     *
> +   * @param {function} aCallback
> +   *        Function that triggeres the panel to open/close

nit: triggers

@@ +686,1 @@
>     *        Notification element to wait for

nit: This description doesn't apply anymore. This is the type subtype now.

@@ +691,2 @@
>     */
> +  waitForNotificationPopup : function locationBar_waitForNotificationPopup(aCallback, aSpec) {

Everything related to the location bar is not a popup but a panel. Please visualize that in the method name.

@@ +700,5 @@
> +    var panel = null;
> +
> +    switch (type) {
> +      case "notification_popup":
> +        panel = this.getElement({type: "notification_popup"});

Lets call it notification_panel, or better could we strip off the _panel suffix from all those elements?

@@ +705,5 @@
> +        break;
> +      case "bookmark_panel":
> +        panel = this._editBookmarksPanel.getElement({type: "bookmarkPanel"});
> +        break;
> +      case "identity_popup":

This is not a popup but a panel. We should keep consistency. Otherwise similar to the above comment.

@@ +729,5 @@
> +        return panelStateChanged;
> +      }, "Notification popup state has been " + (open ? "opened" : "closed"));
> +    }
> +    finally {
> +      panel.getNode().removeEventListener(eventType, onPanelState);

And where do you reset the animation setting?

::: firefox/tests/functional/restartTests/testAddons_installMultipleExtensions/test1.js
@@ +69,3 @@
>  
> +    addons.waitForAddonInstallationState(() => {
> +      var installLink = findElement.ID(controller.tabs.activeTab, "addon");

This can be moved into the inner closure.

@@ +72,5 @@
> +      locationBar.waitForNotificationPopup(() => {
> +        installLink.click();
> +        md.waitForDialog(TIMEOUT_DOWNLOAD);
> +      }, {type: "notification_popup"});
> +    }, "complete")

Does it mean that when the notification is shown, the installation has not been finished yet? I see that we have two instances, one for downloading and another one for the installation itself.

Otherwise missing semi-colon at the end.

@@ +78,5 @@
>      // Wait for the notification to unload
> +    var notification = locationBar.getElement({type: "notification_popup"});
> +    locationBar.waitForNotificationPopup(() => {
> +      notification.keypress('VK_ESCAPE', {});
> +    }, {type: "notification_popup", open: false});

Can we make this a method on the location bar class please? Sounds like a helpful method to have for at least add-on installation related tests. I'm happy with a follow-up mentored bug for that.

::: firefox/tests/functional/testPreferences/testPasswordSavedAndDeleted.js
@@ +55,1 @@
>    var logInButton = new elementslib.ID(controller.tabs.activeTab, "LogIn");

This can be moved into the closure.

@@ +60,3 @@
>  
>    // After logging in, remember the login information
>    var button = locationBar.getNotificationElement(

Please move into the closure.

::: firefox/tests/functional/testSecurity/testGreyLarry.js
@@ +31,5 @@
>    var identityIconLabel = new elementslib.ID(controller.window.document,
>                                               "identity-icon-label");
>    expect.equal(identityIconLabel.getNode().value, "", "The favicon has no label");
>  
> +  var identityBox = locationBar.getElement({type: "identityBox"});

Same here and probably following ones too.

Comment 90

[Daniel Gherasim]

Attachment: v11.patch

I was a bit confused of how to wait for those both notifications panels triggered by external parts but I finally got it done more simply. 
Hope now it's fine.

Comment 91

[Andrei Eftimie]

Comment on attachment 8433146 [details] [diff] [review]
v11.patch

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

I would go with anonymous functions in the places I mentioned inline.
Other than that it looks ok to me.

::: firefox/tests/functional/restartTests/testAddons_installMultipleExtensions/test1.js
@@ +67,2 @@
>      var md = new modalDialog.modalDialog(addonsManager.controller.window);
> +    md.start(handleInstallAddonDialog);

It took me a bit to understand that we are waiting for 2 different notifications. Instead of naming this the same as the addons exported function, we should use an anonymous function here.

::: firefox/tests/functional/testPreferences/testPasswordSavedAndDeleted.js
@@ +65,3 @@
>                   "password-save-notification",
>                   {type: "anonid", value: "button"}
>                 );

nit: this is not indentet properly anymore.

::: firefox/tests/remote/restartTests/testAddons_InstallAddonWithoutEULA/test1.js
@@ +64,5 @@
>    var addButton = addonPage.getElement({type: "install-button"});
>    var md = new modalDialog.modalDialog(controller.window);
>  
>    // Install the add-on
> +  md.start(handleInstallAddonDialog);

Same as the other test. Use an anonymous function.

Comment 92

[Daniel Gherasim]

Attachment: v11.1.patch

Comment 93

[Andrei Eftimie]

Comment on attachment 8434055 [details] [diff] [review]
v11.1.patch

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

r+ from me

Comment 94

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

Comment on attachment 8434055 [details] [diff] [review]
v11.1.patch

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

We are close! Please update the code regarding my comments, and ask for review by myself again. Thanks.

::: firefox/lib/toolbars.js
@@ +695,5 @@
> +    var spec = aSpec || { };
> +    var open = (spec.open == undefined) ? true : spec.open;
> +    var eventType = open ? "popupshown" : "popuphidden";
> +    var state = open ? "open" : "closed";
> +    var type = aSpec.type || "notification_popup";

The type should not be optional. We would have lots of issues in the future with it. Please make it mandatory, as the jsdoc already says.

@@ +722,5 @@
> +    function onPanelState() { panelStateChanged = true; }
> +
> +    panel.getNode().addEventListener(eventType, onPanelState);
> +    try {
> +      aCallback();

I would suggest to pass in the panel element as parameter, given that we already have retrieve it above.

::: firefox/tests/functional/restartTests/testAddons_installMultipleExtensions/test1.js
@@ +67,2 @@
>      var md = new modalDialog.modalDialog(addonsManager.controller.window);
> +    md.start((aController) => {

nit: unnecessary brackets around aController.

@@ +82,2 @@
>      // Wait for the notification to unload
> +    var notification = locationBar.getElement({type: "notification_popup"});

This retrieval we will not need once we pass the panel as argument to the callback.

::: firefox/tests/remote/restartTests/testAddons_InstallAddonWithoutEULA/test1.js
@@ +64,5 @@
>    var addButton = addonPage.getElement({type: "install-button"});
>    var md = new modalDialog.modalDialog(controller.window);
>  
>    // Install the add-on
> +  md.start((aController) => {

nit: unnecessary brackets

Comment 95

[Daniel Gherasim]

Attachment: v11.2.patch

Thanks, I've done the changes base on your review.

Comment 96

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

Comment on attachment 8434876 [details] [diff] [review]
v11.2.patch

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

I would suggest that we really get more conversations if specific review comments are unclear. Clarifying it before uploading a new patch will save us a lot from doing reviews over reviews. This bug is extreme meanwhile with the amount of patches...

::: firefox/lib/toolbars.js
@@ +692,5 @@
> +  waitForNotificationPanel : function locationBar_waitForNotificationPanel(aCallback, aSpec) {
> +    var spec = aSpec || {};
> +
> +    assert.equal(typeof aCallback, "function", "Callback function is defined");
> +    assert.ok(spec.type, "Type of the notification is mandatory");

nit: 'notification panel'

@@ +710,5 @@
> +      case "identity":
> +        panel = this.getElement({type: "identityPopup"});
> +        break;
> +      default :
> +        assert.fail("Unknown notification to wait for: " + spec.type);

Same here as above.

::: firefox/tests/functional/testPasswordManager/testPasswordNotificationBar.js
@@ +55,5 @@
>                               );
>  
> +  // Close the notification and wait for it to unload
> +  locationBar.waitForNotificationPanel(() => {
> +    passwordNotification.keypress("VK_ESCAPE", {});

This cannot be updated for the new panel parameter?

::: firefox/tests/functional/testPreferences/testPasswordSavedAndDeleted.js
@@ +64,5 @@
> +    var button = locationBar.getNotificationElement(
> +                   "password-save-notification",
> +                   {type: "anonid", value: "button"}
> +                 );
> +    button.click();

Which button is that? Shouldn't we use the panel as coming with the parameter and issuing a ESC event? That should be the standard way. Anything else should become specific tests.

Comment 97

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v11.3

Updated the nits and the parameter for the escape line.
For the last item with the button, that's a requirement of the test, to click the Remember password button, which will trigger the unload of the event. 
The test checks then that the dates have been saved.

Comment 98

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

Comment on attachment 8435024 [details] [diff] [review]
patch v11.3

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

Just a nit left. With that fixed you have my r+. You can carry it over and get the patch landed. Thanks!

::: firefox/tests/functional/testPasswordManager/testPasswordNotificationBar.js
@@ +51,3 @@
>  
> +  // Close the notification and wait for it to unload
> +  locationBar.waitForNotificationPanel((aNotification) => {

Please name it aPanel. 'Notification' is just a closer description for it.

Comment 99

[Andreea Matei [:AndreeaMatei]]

Updated it and landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/a04dacbe79fc (default)

I'll check what other branches are affected.

Comment 100

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

In the future please also upload the final version of the patch to bugzilla, before pushing it.

Comment 101

[Andreea Matei [:AndreeaMatei]]

Attachment: patch aurora

Aurora is also affected, updated the conflict on one test and ran testruns, all works fine.

Comment 102

[Neil Deakin]

Did you test this with the patches in bug 994117 applied? I wouldn't want to make more work here since that can affect things here.

Comment 103

[Andreea Matei [:AndreeaMatei]]

I don't think it will affect, we have a to do tag to remove the animation code once bug 994117 lands. At the moment we disabled animations.

Comment 104

[Andrei Eftimie]

Comment on attachment 8435756 [details] [diff] [review]
patch aurora

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

Looks good.
Landed: https://hg.mozilla.org/qa/mozmill-tests/rev/5a8308ec52db (mozilla-aurora)

(In reply to Neil Deakin from comment #102)
> Did you test this with the patches in bug 994117 applied? I wouldn't want to
> make more work here since that can affect things here.

We currently have animations disabled until bug 994117 lands.
Either way we need to land this asap. Once bug 994117 lands we'll see if any other changes are needed (and if they are, we'll only need to update a the library, not all tests)