1067411 - Enable waiting for popup animations as bug 994117 is fixed and expose the method as a more general one

Status: RESOLVED FIXED

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

Description

[Daniel Gherasim]

As we were fixing bug 994040, we did a workaround for waiting for panels/popups as the animations were broken due to bug 994117. That was to disable them.

But now that was fixed, the old animations are back in place, so we can update our code to wait for a popup using the correct events.

We really need this especially for bug 908649, where the panel is not initialized at firefox start-up for memory saving, so in this case we don't actually have the element loaded before.

Comment 1

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

Well, the dependency has been already fixed for Firefox 33.0, so the code is in the tree for about 2 months now. For stability reasons I would consider to backport it down to beta, as where the patch originally landed.

Comment 2

[Daniel Gherasim]

Attachment: v1.patch

Waiting for "popupshown" & "popuphidden" is now enough to wait for the panels.
I've moved the function outside the locationBar scope so we can call it for every popup panel (e.g.: the downloads panel)

Also ran our all testruns on all platforms and I got no failure. 
(https://pastebin.mozilla.org/6526459)

Comment 3

[Andrei Eftimie]

Comment on attachment 8490689 [details] [diff] [review]
v1.patch

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

In general this looks good, a few notes inline.

::: firefox/lib/toolbars.js
@@ +868,5 @@
> + * Waits for a notification popup panel
> + *
> + * @param {function} aCallback
> + *        Function that triggers the panel to open/close
> + * @param {object} aSpec

This is also optional.

@@ +877,5 @@
> + *        Element to use for waiting for the events (usually same as the panel)
> + * @param {boolean} [aSpec.open=true]
> + *        True if the notification should be open
> + */
> +function waitForNotification(aCallback, aSpec) {

If you want to be able to use this outside of the locationbar you will also need to export it, otherwise it will only be available in this file. I guess this will be fine if the download panel (or any other consumers) will also live here.

@@ +882,5 @@
> +  var spec = aSpec || {};
> +
> +  assert.equal(typeof aCallback, "function", "Callback function is defined");
> +
> +  var open = (spec.open == undefined) ? true : spec.open;

Since we have a binary state (open|closed) we should assert that we are in the opposite state of the requested state. Otherwise it might get hard to track down a failure.

Comment 4

[Daniel Gherasim]

(In reply to Andrei Eftimie from comment #3)
> ::: firefox/lib/toolbars.js
> @@ +868,5 @@
> > + * Waits for a notification popup panel
> > + *
> > + * @param {function} aCallback
> > + *        Function that triggers the panel to open/close
> > + * @param {object} aSpec
> 
> This is also optional.
> 

Actually it's not optional. I made spec.panel optional by mistake. 
I will also make an assert for it.

I did it this way so we can handle the case where panel is not initialized & we can add a parent element on which the event propagates. But we can make panel mandatory so we pass a MozMillElement even before it is initialized.

> @@ +882,5 @@
> > +  var spec = aSpec || {};
> > +
> > +  assert.equal(typeof aCallback, "function", "Callback function is defined");
> > +
> > +  var open = (spec.open == undefined) ? true : spec.open;
> 
> Since we have a binary state (open|closed) we should assert that we are in
> the opposite state of the requested state. Otherwise it might get hard to
> track down a failure.

We have Bug 1044987 to handle this. 
What we actually need to do is to first check that the element is initialized and if it is, we have to check that is open (in the case we want to close it) or if it's closed (in case we want to open it).

Otherwise, it's surely closed.

Comment 5

[Daniel Gherasim]

Attachment: v1.1.patch

Comment 6

[Andrei Eftimie]

Comment on attachment 8490747 [details] [diff] [review]
v1.1.patch

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

I would like to get a couple improvements here. With those fixed I'll get this landed.
I'm thinking whether to also get bug 1044987 included here - we don't seem to make much progress over there... but this is not a blocker, so either way is fine for me.

::: firefox/lib/toolbars.js
@@ +883,5 @@
> +
> +  assert.equal(typeof aCallback, "function", "Callback function is defined");
> +  assert.ok(spec.panel, "Panel has to be specified");
> +
> +  var open = (spec.open == undefined) ? true : spec.open;

If we're already here we should improve this a bit.
- to check for undefined we should use typeof
- we can replace the ternary operator with the or operator since the first evaluation already returns `true`
- we should cast the received value as a boolean

> var open = typeof spec.open === "undefined" || !!spec.open;

@@ +885,5 @@
> +  assert.ok(spec.panel, "Panel has to be specified");
> +
> +  var open = (spec.open == undefined) ? true : spec.open;
> +  var eventType = open ? "popupshown" : "popuphidden";
> +  var parent = spec.parent ? spec.parent : spec.panel.getNode();

Can be simplified:
> var parent = spec.parent || spec.panel.getNode();

Comment 7

[Daniel Gherasim]

Attachment: v1.2.patch

Updated as requested.

Comment 8

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

Comment on attachment 8491421 [details] [diff] [review]
v1.2.patch

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

::: firefox/lib/toolbars.js
@@ +822,5 @@
>      switch (spec.type) {
>        case "notification":
>          panel = this.getElement({type: "notification_popup"});
>          break;
>        case "bookmark":

This should also be moved out here. This doesn't belong to the location bar. Please file a separate bug for it.

@@ +883,5 @@
> +
> +  assert.equal(typeof aCallback, "function", "Callback function is defined");
> +  assert.ok(spec.panel, "Panel has to be specified");
> +
> +  var open = typeof spec.open === "undefined" || !!spec.open;

Something I mention nearly each time, please put () brackes around the comparison.

I do not understand why we have to do the !!spec.open here, given that we already require a boolean to be passed in.

@@ +890,5 @@
> +  if (open) {
> +    if (spec.panel.getNode()) {
> +      expect.equal(spec.panel.getNode().state, "closed",
> +                   "Panel is in the correct state");
> +    }

Why the if condition here? If it can be that the panel doesn't exist, line 902 will also fail in case no parent is given.

Comment 9

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #8) 
> @@ +883,5 @@
> > +
> > +  assert.equal(typeof aCallback, "function", "Callback function is defined");
> > +  assert.ok(spec.panel, "Panel has to be specified");
> > +
> > +  var open = typeof spec.open === "undefined" || !!spec.open;
> 
> Something I mention nearly each time, please put () brackes around the
> comparison.

This was my suggestion.
I still can't really see the use of braces that don't serve any purpose... One might argue that they provide more readability, but that's not really the case. It's pretty clear what is happening here.

I won't fight it though.

> I do not understand why we have to do the !!spec.open here, given that we
> already require a boolean to be passed in.

We don't actually check or enforce that anywhere else...

Comment 10

[Daniel Gherasim]

Attachment: v1.3.patch

Patch updated based on review so I guess we can get it landed.

Comment 11

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> Comment on attachment 8491421 [details] [diff] [review]
> v1.2.patch
> 
> Review of attachment 8491421 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +890,5 @@
> > +  if (open) {
> > +    if (spec.panel.getNode()) {
> > +      expect.equal(spec.panel.getNode().state, "closed",
> > +                   "Panel is in the correct state");
> > +    }
> 
> Why the if condition here? If it can be that the panel doesn't exist, line
> 902 will also fail in case no parent is given.

For that case when panel node is not already loaded in the DOM, like the DownloadsPanel.
So in case "spec.panel.getNode()" is null, parent should be defined!
I added this test.

Comment 12

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

(In reply to Andrei Eftimie from comment #9)
> I still can't really see the use of braces that don't serve any purpose...
> One might argue that they provide more readability, but that's not really
> the case. It's pretty clear what is happening here.

The thing here is that you have 3 operands in this single expression. It's indeed not necessary for evaluating it correctly, but what we always agreed on in the past, is that it improves the readability a lot.

> > I do not understand why we have to do the !!spec.open here, given that we
> > already require a boolean to be passed in.
> 
> We don't actually check or enforce that anywhere else...

That's only the problem of Javascript, because it has no strong type definitions. It should already raise when the consumer uses a wrong calling convention and e.g. uses a string here. If we would try to start testing for the right type, we would have to change a lot of files in our repository.

I would say as long as the method declares the type of the parameter correctly, the consumer has to use the right type.

Comment 13

[Andrei Eftimie]

Comment on attachment 8491491 [details] [diff] [review]
v1.3.patch

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

Looks and works fine, fixed what Henrik requested prior.

Comment 14

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

Comment on attachment 8491491 [details] [diff] [review]
v1.3.patch

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

Looks good. If all runs fine please get it landed.

Comment 15

[Daniel Gherasim]

Attachment: v1.4.patch

Updated the commit message.

Testruns with Nightly on Mac, Linux & Windows runs fine.
I suggest we first land this on nightly, wait for 1-2 days to see how it's working and then backport on Aurora & Beta.

Comment 16

[Andrei Eftimie]

Comment on attachment 8493142 [details] [diff] [review]
v1.4.patch

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

Landed:
https://hg.mozilla.org/qa/mozmill-tests/rev/326852e464c1 (default)

Comment 17

[Andrei Eftimie]

All works fine on Aurora as well.

Transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/006824b75a28 (mozilla-aurora)

Comment 18

[Andrei Eftimie]

All tests work fine against Beta as well, lets close this issue.

Transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/d6ff18ef0ecf (mozilla-beta)

Thanks Daniel for the work here.