610849 - Need a waitForNotificationBar() function in tabBrowserAPI

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

After some extensive research, some help from Gavin, and much trial and error, I've finally come up with some code which reliably checks that a notification bar has stopped animating...

  // Wait for the notification bar to finish animating
  controller.waitFor(function() {
    var panel = tabBrowser.getTabPanelElement(tabBrowser.selectedIndex,
                                              '/{"value":"password-save"}');
    var style = controller.window.getComputedStyle(panel.getNode(), null);      
    return style.marginTop == '0px';
  }, "Save password notification bar is not visible", null, 100);

All notification bars are created with a negative margin-top px value and that increments to 0px (the animation).

Instead of having this code in any particular test, we should really wrap this in a waitForNotificationBar() function which takes the notificationbar.value as a parameter.

This should make all notification bar handling code much more reliable, globally.

Comment 1

[u279076]

I'll come up with a patch which implements this method and refactors our existing tests to use it.

I recommend landing this on all branches (default included) as notification bars are not completely phased out in Firefox 4.0.

Comment 2

[u279076]

Attachment: Patch v1

Comment 3

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

Comment on attachment 489965 [details] [diff] [review]
Patch v1

>+   * @param {string} aValue
>+   *        The value property of the required notification bar

Actually we don't need the name of the tab panel at this point. The code is getting it automatically inside the getElements function. But something we would need is the tab index. See the getTabPanelElement function. 

>+  waitForNotificationBar : function tabBrowser_waitForNotificationBar(aValue) {

The function should be named based to the getTabPanelElement function.

>+    var notificationBar = (aValue == undefined) ? this.getTabPanelElement(this.selectedIndex)
>+                                                : this.getTabPanelElement(this.selectedIndex,
>+                                                                          '/{"value":"' + aValue + '"}');

Simply call getTabPanelElement by forwarding the index.
 
>+    // Wait for the top margin to be 0px - ie. has stopped animating

It would be nice if you could put in some more text which explains why we have to do this ugly workaround.

>+    this._controller.waitFor(function () {
>+      return style.marginTop == '0px';
>+    }, aValue + " notification bar is not visible", null, 100);

The message which get passed in here should be positive. Also given the defaults for waitFor, you can leave out the timeout and delay.

Comment 4

[u279076]

Attachment: Patch v2

Most of the changes addressed, including what we talked about offline.

Comment 5

[u279076]

Attachment: Patch v2.1

Comment 6

[u279076]

Attachment: Patch v2.2

Comment 7

[u279076]

Attachment: Patch v2.3

Comment 8

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

Comment on attachment 490032 [details] [diff] [review]
Patch v2.3

Looks good. Lets get this landed so we can fix the broken tests. Please also land the patch on older branches.

Comment 9

[u279076]

(In reply to comment #7)
> Created attachment 490032 [details] [diff] [review]
> Patch v2.3

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/51f229adccb2 [default]

Comment 10

[u279076]

Attachment: Patch v2.3 (1.9.2)

Backport for 1.9.2

Comment 11

[u279076]

(In reply to comment #10)
> Created attachment 490140 [details] [diff] [review]
> Patch v2.3 (1.9.2)

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/a423b626492d [mozilla1.9.2]
http://hg.mozilla.org/qa/mozmill-tests/rev/d596e3930b1f [mozilla1.9.1]

Comment 12

[u279076]

Now that this has been landed, I will follow up with a patch to use it in all notification bar tests.

Comment 13

[u279076]

Attachment: Patch v3 (default)

Comment 14

[Aaron Train [:aaronmt]]

Some clarity would help here with the use of this function now. Looking at the patch and it's use, we now have a pattern of double-waits (i.e., wait for the bar, wait for the button on the bar). Henrik or Geo, will waiting for the notification in it's entirety negate the necessity to wait for elements on the notification bar, or is this pattern still needed. Is this something that the shared modules refactoring can clear up?

It looks like right now, we're safety-netting any immediate follow up code through waits, just in case

Comment 15

[u279076]

Attachment: Patch v3 (backport)

Comment 16

[u279076]

(In reply to comment #14)
> Some clarity would help here with the use of this function now. Looking at the
> patch and it's use, we now have a pattern of double-waits (i.e., wait for the
> bar, wait for the button on the bar). Henrik or Geo, will waiting for the
> notification in it's entirety negate the necessity to wait for elements on the
> notification bar, or is this pattern still needed. Is this something that the
> shared modules refactoring can clear up?
> 
> It looks like right now, we're safety-netting any immediate follow up code
> through waits, just in case

That's more of a case for the refactoring work.  The purpose of this bug (and it's patches) is to use waitForTabPanel().  This can be evaluated on a test by test basis, the same as with sleep().

Comment 17

[u279076]

Documentation added:
https://developer.mozilla.org/en/Mozmill_Tests/Shared_Modules/TabbedBrowsingAPI

Comment 18

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 490159 [details] [diff] [review]
Patch v3 (default)

Looks fine, please land.

Comment 19

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 490179 [details] [diff] [review]
Patch v3 (backport)

Also looks fine, please land.

Comment 20

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

(In reply to comment #14)
> It looks like right now, we're safety-netting any immediate follow up code
> through waits, just in case

It doesn't matter if we are waiting a second time for the element itself. Personally I would even propose to do so all the time, which will make the test more robust. The API refactoring will will take care of that.

Comment 21

[u279076]

> Created attachment 490159 [details] [diff] [review]
> Patch v3 (default)

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/cba7a13b252b [default]

Comment 22

[u279076]

(In reply to comment #15)
> Created attachment 490179 [details] [diff] [review]
> Patch v3 (backport)

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/792b9584e35c [mozilla1.9.2]

Comment 23

[u279076]

Attachment: Patch v3 (1.9.1)

Backport patch did not apply cleanly to 1.9.1.  Here is a patch for this branch. Please review in case I missed something.

On a side note, I noticed that I missed the Geolocation test in testPrivateBrowsing.  I'll submit a follow-up patch once this one is landed.

Comment 24

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 491031 [details] [diff] [review]
Patch v3 (1.9.1)

Looks fine, please land.

Comment 25

[u279076]

(In reply to comment #24)
> Comment on attachment 491031 [details] [diff] [review]
> Patch v3 (1.9.1)
> 
> Looks fine, please land.

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/38f874f9444f [mozilla.1.9.1]

Will follow up with Geolocation patches soon...then we can call this fixed I think.

Comment 26

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

Move of Mozmill Test related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.

Comment 27

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

Anthony, any update on that bug? Seems like we haven't finished everything yet.

Comment 28

[u279076]

(In reply to comment #27)
> Anthony, any update on that bug? Seems like we haven't finished everything
> yet.

I'll have to look into this and see what else we wanted. This is a 1.9.1/1.9.2 change only however. Do we want to exert more effort on this?

Comment 29

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

Definitely not for 1.9.1 but we should check what's necessary to get it finished up for 1.9.2.

Comment 30

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

Is that still a problem, Anthony?

Comment 31

[u279076]

This bug is getting a bit old. All that remained was adding something for the geolocation notification bar. Since this notification bar only exists on 1.9.2 (2.0 and above use a doorhanger) I'm inclined to say WONTFIX on that part -- making this a RESOLVED FIXED.

What do you think Henrik?

Comment 32

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

No new features on the older branches. So if that applies here, yes mark it as fixed please.

Comment 33

[u279076]

(In reply to Henrik Skupin (:whimboo) from comment #32)
> No new features on the older branches. So if that applies here, yes mark it
> as fixed please.

Indeed it does -- marking FIXED.