Some of the methods of the MozMillController object don't wrap the waitFor call into a try/catch clause. The result of that are useless failure messages from the waitFor function. We have to update those methods to throw informative information for the actual element.

The fact that waitFor throws a generic Error makes me a bit uneasy about everything calling it just wrapping it in a try/catch. if waitFor is just used as a helper for the other assertion methods then maybe it should take in the error message to throw as an argument rather than making everything calling it wrap it in try/catch. Or at least throw a TimeoutError and everything catching it checks for that.

Sounds good. If a forth argument is specified use it for the message, otherwise we can throw the default message which will include the closure content.

This patch adds a 4th argument to waitFor, if the timeout is exceeded in waitFor, an Error with that message is thrown, otherwise a default message is used. Also adds this fourth arg for several util functions in controller.js

Comment on attachment 465418 [details] [diff] [review] add 4th error message argument to waitFor, use it in util functions Looks good. We should also add the message to utils.sleep and to all controller functions which make use of the assert() call. I will be off for today so please ask Andrew for next review.

adds error message argument to assert() too.

whoops, made patch wrong

Comment on attachment 465453 [details] [diff] [review] add error message argument to utils.waitFor and utils.assert Everything matches up to the old error messages and looks good r=ahalberstadt

1.4.2: http://github.com/mozautomation/mozmill/commit/d4a5953ca55d2d8387bf3dc880e702f8b79b3dd4 http://github.com/mozautomation/mozmill/commit/7bc884b218f7f7131e5f0d25afc145e02d1ea3f1 master: http://github.com/mozautomation/mozmill/commit/1c35a038f2b69254d936da9f414893949929c9c7 http://github.com/mozautomation/mozmill/commit/da4794242f29be729b177d6640f9774b8b9b6381

Thanks Heather for taking this. We will have to add documentation for that change and the waitFor and assert functions. I haven't had time for it yet. Lets track this in the whiteboard. I have to reopen because you missed one instance for waitForPageLoad.

use new arg in waitForPageLoad

Comment on attachment 465793 [details] [diff] [review] take out try/catch in waitForPageLoad >+ // Wait until the page has been loaded >+ this.waitFor(function() { return self.loaded; }, aTimeout, aInterval, >+ "controller.waitForPageLoad(): Timeout waiting for page loaded."); >+ this.sleep(1000, "controller.waitForPageLoad(): sleep failed"); Huh, why is that in here? Has this been accidentally checked in by the original patch? We shouldn't wait 1000ms after this event has been fired. >+ tab.removeEventListener("DOMContentLoaded", pageLoaded, true); So when waitFor throws an exception we do not remove the event listener and leak that memory. Looks like that we really need the try/catch here.

So, it seems to me like this try/catch in waitForPageLoad should remain. I don't like the hardcoded timeouts, but if they predate this patch, then they should probably stay. I really don't want to take large refactoring at this point for 1.4.2. So, we should leave the code as is and leave the waitForPageLoad method alone. We can file a bug to revisit this for future releases. And this bug will return to a fixed state. Sound good?

From Heather and others, it looks like we don't want to mess with the waitForPageLoad try/catch at this time. We'll leave the fix from this code that has already landed in the tree and close this as FIXED again. If there are other issues or further work needed on the specific waitForPageLoad case, let's open a new bug to track that specific issue. --> FIXED.

The 1000ms concern is now bug 590048. Otherwise it looks good. Marking as verified fixed.

Documentation added: https://developer.mozilla.org/en/Mozmill/Mozmill_Controller_Object#assert() https://developer.mozilla.org/en/Mozmill/Mozmill_Controller_Object#waitFor()