Closed Bug 610849 Opened 11 years ago Closed 10 years ago

Need a waitForNotificationBar() function in tabBrowserAPI

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: u279076, Assigned: u279076)

References

Details

Attachments

(5 files, 4 obsolete files)

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.
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.
Assignee: nobody → anthony.s.hughes
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #489965 - Flags: review?(hskupin)
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-
Attached patch Patch v2 (obsolete) — Splinter Review
Most of the changes addressed, including what we talked about offline.
Attachment #489965 - Attachment is obsolete: true
Attachment #490014 - Flags: review?(hskupin)
Attached patch Patch v2.1 (obsolete) — Splinter Review
Attachment #490014 - Attachment is obsolete: true
Attachment #490015 - Flags: review?(hskupin)
Attachment #490014 - Flags: review?(hskupin)
Attached patch Patch v2.2 (obsolete) — Splinter Review
Attachment #490015 - Attachment is obsolete: true
Attachment #490018 - Flags: review?(hskupin)
Attachment #490015 - Flags: review?(hskupin)
Attached patch Patch v2.3Splinter Review
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+
(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]
Backport for 1.9.2
Now that this has been landed, I will follow up with a patch to use it in all notification bar tests.
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
Attachment #490179 - Flags: review?(gmealer)
(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.
> Created attachment 490159 [details] [diff] [review]
> Patch v3 (default)

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/cba7a13b252b [default]
(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]
Attached patch Patch v3 (1.9.1)Splinter Review
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+
(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.
Product: Testing → Mozilla QA
Anthony, any update on that bug? Seems like we haven't finished everything yet.
(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?
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.
(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
Closed: 10 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.