As described here: http://dvcs.w3.org/hg/webdriver/raw-file/tip/webdriver-spec.html#timeouts-1 and http://code.google.com/p/selenium/wiki/JsonWireProtocol#/session/:sessionId/timeouts, we need a command called 'timeouts' that takes in 'type' and 'ms' arguments so we can set either the script, pageload or implicit wait timeouts in one command. We already have support for setting the timeout of script and implicit wait timeouts in the server, so now we just need to add support for page load and use the 'timeouts' function to call either of these methods. I'm setting this bug aside as a ramping up project.

Comment on attachment 700552 [details] [diff] [review] timeout functionality update Review of attachment 700552 [details] [diff] [review]: ----------------------------------------------------------------- Great patch. I think you just need to undo the change you did to test_findelement.py, and fix up some naming/whitespace issues, and then I'll be ready to give you an r+ :) You just need to upload the new patch and r? me again. ::: testing/marionette/client/marionette/marionette.py @@ +388,5 @@ > > def navigate(self, url): > response = self._send_message('goUrl', 'ok', value=url) > return response > + There's some extra whitespace here (4 spaces). Can you remove it? ::: testing/marionette/client/marionette/tests/unit/test_findelement.py @@ +116,5 @@ > def test_not_found(self): > test_html = self.marionette.absolute_url("test.html") > self.marionette.navigate(test_html) > self.marionette.set_search_timeout(1000) > + self.assertRaises(NoSuchElementException, self.marionette.findeseelement, "id", "I'm not on the page") there's a typo here, "self.marionette.findeseelement". Are you able to run this test without errors? I don't think you want to change this file, either. I'd revert it and resubmit the patch. ::: testing/marionette/client/marionette/tests/unit/test_timeouts.py @@ +6,5 @@ > +from marionette_test import MarionetteTestCase > +from marionette import HTMLElement > +from errors import NoSuchElementException > +from errors import MarionetteException > +from errors import JavascriptException, MarionetteException, ScriptTimeoutException You can merge lines 8-10, so you can import everything in the same line. @@ +30,5 @@ > + self.marionette.timeouts("implicit", 1000) > + self.assertRaises(NoSuchElementException, self.marionette.find_element, "id", "I'm not on the page") > + self.marionette.timeouts("implicit", 0) > + self.assertRaises(NoSuchElementException, self.marionette.find_element, "id", "I'm not on the page") > + Trailing whitespace at the end of line 33 and the beginning of line 34 @@ +37,5 @@ > + self.marionette.navigate(test_html) > + self.marionette.timeouts("implicit", 4000) > + self.assertEqual(HTMLElement, type(self.marionette.find_element("id", "newDiv"))) > + > + Trailing whitespace @@ +42,5 @@ > + def test_searchtimeout_found(self): > + test_html = self.marionette.absolute_url("test.html") > + self.marionette.navigate(test_html) > + self.assertRaises(NoSuchElementException, self.marionette.find_element, "id", "newDiv") > + Trailing whitespace ::: testing/marionette/marionette-actors.js @@ +953,5 @@ > function checkLoad() { > + end = new Date().getTime(); > + let elapse = end - start; > + if (this.pageTimeout == null || elapse <= this.pageTimeout){ > + if (curWindow.document.readyState == "complete") { Trailing whitespace @@ +957,5 @@ > + if (curWindow.document.readyState == "complete") { > + sendOk(command_id); > + return; > + } > + else{ Trailing whitespace @@ +1235,5 @@ > + if (isNaN(timeout)){ > + this.sendError("Not a Number", 500, null, this.command_id); > + } > + else{ > + if (timeout_type == "implicit"){ There's extra whitespace trailing the { @@ +1238,5 @@ > + else{ > + if (timeout_type == "implicit"){ > + aRequest.value = aRequest.ms; > + this.setSearchTimeout(aRequest); > + Some more extra whitespace @@ +1247,5 @@ > + } > + else{ > + this.pageTimeout = timeout; > + this.sendOk(this.command_id); > + } More whitespace trailing the } ::: testing/marionette/marionette-listener.js @@ +553,5 @@ > + } > + if (!event.originalTarget.defaultView.frameElement || > + event.originalTarget.defaultView.frameElement == curWindow.frameElement) { > + removeEventListener("DOMContentLoaded", onDOMContentLoaded, false); > + More extra whitespace @@ +565,3 @@ > } > + }; > + function Timerfunc(){ Timerfunc should be timerFunc. We reserve capitalizing the first letter for "class" type objects.

Comment on attachment 701219 [details] [diff] [review] timeout functionality error/format fix Review of attachment 701219 [details] [diff] [review]: ----------------------------------------------------------------- Great! I'll land this on m-i for you.

landed on m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/ef384134776b

Comment on attachment 700115 [details] [diff] [review] Adding timeout functionality Review of attachment 700115 [details] [diff] [review]: ----------------------------------------------------------------- Reviewed a following patch.