Need a waitForNotificationBar() function in tabBrowserAPI

RESOLVED FIXED

Status

RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: ashughes, Assigned: ashughes)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 4 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
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.
Blocks: 609071
(Assignee)

Updated

8 years ago
Assignee: nobody → anthony.s.hughes
Status: NEW → ASSIGNED
(Assignee)

Comment 2

8 years ago
Created attachment 489965 [details] [diff] [review]
Patch v1
Attachment #489965 - Flags: review?(hskupin)

Updated

8 years ago
Blocks: 611544
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.
Attachment #489965 - Flags: review?(hskupin) → review-
(Assignee)

Comment 4

8 years ago
Created attachment 490014 [details] [diff] [review]
Patch v2

Most of the changes addressed, including what we talked about offline.
Attachment #489965 - Attachment is obsolete: true
Attachment #490014 - Flags: review?(hskupin)
(Assignee)

Comment 5

8 years ago
Created attachment 490015 [details] [diff] [review]
Patch v2.1
Attachment #490014 - Attachment is obsolete: true
Attachment #490015 - Flags: review?(hskupin)
Attachment #490014 - Flags: review?(hskupin)
(Assignee)

Comment 6

8 years ago
Created attachment 490018 [details] [diff] [review]
Patch v2.2
Attachment #490015 - Attachment is obsolete: true
Attachment #490018 - Flags: review?(hskupin)
Attachment #490015 - Flags: review?(hskupin)
(Assignee)

Comment 7

8 years ago
Created attachment 490032 [details] [diff] [review]
Patch v2.3
Attachment #490018 - Attachment is obsolete: true
Attachment #490032 - Flags: review?(hskupin)
Attachment #490018 - Flags: review?(hskupin)
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.
Attachment #490032 - Flags: review?(hskupin) → review+
(Assignee)

Comment 10

8 years ago
Created attachment 490140 [details] [diff] [review]
Patch v2.3 (1.9.2)

Backport for 1.9.2
(Assignee)

Comment 12

8 years ago
Now that this has been landed, I will follow up with a patch to use it in all notification bar tests.
(Assignee)

Comment 13

8 years ago
Created attachment 490159 [details] [diff] [review]
Patch v3 (default)
Attachment #490159 - Flags: review?(gmealer)
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
(Assignee)

Comment 15

8 years ago
Created attachment 490179 [details] [diff] [review]
Patch v3 (backport)
Attachment #490179 - Flags: review?(gmealer)
(Assignee)

Comment 16

8 years ago
(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 on attachment 490159 [details] [diff] [review]
Patch v3 (default)

Looks fine, please land.
Attachment #490159 - Flags: review?(gmealer) → review+
Comment on attachment 490179 [details] [diff] [review]
Patch v3 (backport)

Also looks fine, please land.
Attachment #490179 - Flags: review?(gmealer) → review+
(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.
(Assignee)

Comment 22

8 years ago
(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]
(Assignee)

Comment 23

8 years ago
Created attachment 491031 [details] [diff] [review]
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.
Attachment #491031 - Flags: review?(gmealer)
Comment on attachment 491031 [details] [diff] [review]
Patch v3 (1.9.1)

Looks fine, please land.
Attachment #491031 - Flags: review?(gmealer) → review+
(Assignee)

Comment 25

8 years ago
(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.
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.
Component: Mozmill Tests → Mozmill Tests
Product: Testing → Mozilla QA
Anthony, any update on that bug? Seems like we haven't finished everything yet.
(Assignee)

Comment 28

8 years ago
(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?
Definitely not for 1.9.1 but we should check what's necessary to get it finished up for 1.9.2.
Is that still a problem, Anthony?
(Assignee)

Comment 31

7 years ago
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?
No new features on the older branches. So if that applies here, yes mark it as fixed please.
(Assignee)

Comment 33

7 years ago
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.