Last Comment Bug 566251 - Findbar chrome tests should use SimpleTest.executeSoon rather than using nsIThreadManager directly
: Findbar chrome tests should use SimpleTest.executeSoon rather than using nsIT...
Product: Toolkit
Classification: Components
Component: Find Toolbar (show other bugs)
: Trunk
: x86 Linux
-- minor (vote)
: mozilla1.9.3a5
Assigned To: Nobody; OK to take it and work on it
: Mike de Boer [:mikedeboer]
Depends on:
  Show dependency treegraph
Reported: 2010-05-16 22:38 PDT by Graeme McCutcheon [:graememcc]
Modified: 2010-06-02 04:47 PDT (History)
2 users (show)
graeme.mccutcheon: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v1 (10.34 KB, patch)
2010-05-16 22:38 PDT, Graeme McCutcheon [:graememcc] review+
Details | Diff | Splinter Review

Description User image Graeme McCutcheon [:graememcc] 2010-05-16 22:38:27 PDT
Created attachment 445644 [details] [diff] [review]

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).
Comment 1 User image :Gavin Sharp [email:] 2010-05-31 10:11:53 PDT
Comment on attachment 445644 [details] [diff] [review]

>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.
Comment 2 User image Graeme McCutcheon [:graememcc] 2010-06-02 04:47:11 PDT
Pushed with review comment addressed.

Note You need to log in before you can comment on or make changes to this bug.