Findbar chrome tests should use SimpleTest.executeSoon rather than using nsIThreadManager directly

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Toolkit
Find Toolbar
--
minor
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: graememcc, Unassigned)

Tracking

Trunk
mozilla1.9.3a5
x86
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
Created attachment 445644 [details] [diff] [review]
v1

Per Gavin's IRC comment.

This patch also catches another couple of nits while I'm in the area.

I'm curious as to why the additional delay is necessary in the first place; I'll admit to being guilty in essentially copying the template of Mano and Gavin's existing tests when I started adding findbar tests of my own. I've pushed a patch to try with s/onload="executeSoon(startTest);"/onload="startTest();"/ which doesn't seem to have caused any ill effects. Unless there's a compelling reason otherwise, I'll change that next, along with the findbar tests with a setTimeout(_delayedonLoad).
(Reporter)

Updated

7 years ago
Attachment #445644 - Flags: review?(gavin.sharp)
Summary: Findbar crhome tests should use SimpleTest.executeSoon rather than using nsIThreadManager directly → Findbar chrome tests should use SimpleTest.executeSoon rather than using nsIThreadManager directly
Comment on attachment 445644 [details] [diff] [review]
v1

>diff --git a/toolkit/content/tests/chrome/bug263683_window.xul b/toolkit/content/tests/chrome/bug263683_window.xul

>-    function onLoad() {

>-        gBrowser.addEventListener("pageshow", onPageShow, false);

>     function onPageShow() {
>-      gBrowser.removeEventListener("load", onPageShow, true);
>+      gBrowser.removeEventListener("pageshow", onPageShow, true);

I guess this didn't have any negative effects because we only trigger the one load, and this window is closed at the end of the test anyways? Is there any point in removing the listener then? Doesn't hurt, I guess, but bug331215_window.xul doesn't remove its listener - probably best to be consistent.
Attachment #445644 - Flags: review?(gavin.sharp) → review+
(Reporter)

Comment 2

7 years ago
Pushed with review comment addressed.
http://hg.mozilla.org/mozilla-central/rev/2e351d9a80ca
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
You need to log in before you can comment on or make changes to this bug.