446212 - Random failure on Linux mozilla-central qm-centos5-03 / browser_bug410900.js

Status: RESOLVED FIXED

(Firefox :: File Handling, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/preferences/tests/browser_bug410900.js | handlersView is present
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/preferences/tests/browser_bug410900.js | App handler list populated

Using timeout to see if window is opened and the document for it is loaded is
error prone. Would be better if load event could be used.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: something like this

Comment 2

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Bulletproofing

The problem with listening for load on the window is that the content of the richlistbox we're testing is built on a timeout after http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/applications.xul#57 is fired, so it still leaves us somewhat susceptible to starting the tests before things are actually ready.

This patch changes the test to observe a topic we fire off once we're sure the contents of the pane are ready for poking. I've ifdef'd the observer bits in applications.js since I don't think this topic would be genuinely useful outside of a testing context; especially when there are much better ways to go about manipulating the handler list.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

I think you might as well unconditionally fire the notification, it could potentially be useful for extensions wanting to extend this prefpane, it's pretty cheap, and test-only code in general is to be avoided.

I would also leave "runTest" separate from observe() for legibility (and have observe just pass the window in).

Comment 4

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: v2

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 330437 [details] [diff] [review]
v2

>diff --git a/browser/components/preferences/applications.js b/browser/components/preferences/applications.js

>+      // Notify observers the the UI is now ready

"that the"

Comment 6

[Ryan Flint [:rflint] (ping via IRC for reviews)]

http://hg.mozilla.org/index.cgi/mozilla-central/rev/f91119d7f946

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 330437 [details] [diff] [review]
v2

I'd like to get this test fix onto 1.9.0, but it touches non-test code, so asking for approval. The actual impact of the non-test code is pretty small.

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 330437 [details] [diff] [review]
v2

Approved for 1.9.0.4, a=dveditz for release-drivers

Comment 9

[Ryan Flint [:rflint] (ping via IRC for reviews)]

mozilla/browser/components/preferences/applications.js 1.46
mozilla/browser/components/preferences/tests/browser_bug410900.js 1.6