Bug 80042

Created attachment 129741 [details] Patch

(In reply to comment #1) > Created an attachment (id=129741) [details] > Patch This patch adds a necessary option to PerfTestRunner and does some small refactoring in WebKitTestRunner to avoid timing out, as required by perf tests. I've tested these changes on Gtk port with success. I've applied the similar logic to the Qt port - it should work, but I haven't tested it. I'm unfamiliar with the ways Mac and Win ports set up the loops, so I've left a FIXME note in those places. The code should build fine, though.

(In reply to comment #2) > I've tested these changes on Gtk port with success. I've applied the similar logic to the Qt port - it should work, but I haven't tested it. I'm testing your patch with the Qt port and it works just fine. Thanks!

Comment on attachment 129741 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129741&action=review Looks fine, in general, but I have a few nits. > Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:129 > + bool m_useTimeoutWatchdog; nit: m_useWaitToDumpWatchdogTimer? > Tools/WebKitTestRunner/TestController.cpp:546 > -void TestController::runUntil(bool& done, TimeoutDuration timeoutDuration) > +void TestController::runUntil(bool& done, TimeoutDuration timeoutDuration, bool shouldTimeout) > { > - platformRunUntil(done, timeoutDuration == ShortTimeout ? m_shortTimeout : m_longTimeout); > + platformRunUntil(done, timeoutDuration == ShortTimeout ? m_shortTimeout : m_longTimeout, shouldTimeout); Instead of adding an extra parameter, perhaps it would be better to add another value to the enum so there is ShortTimeout, LongTimeout and NoTimeout. > Tools/WebKitTestRunner/TestController.h:74 > + void platformRunUntil(bool& done, double timeout, bool shouldTimeout); Instead of adding another parameter, perhaps you could just have a constant == -1 that signifies no timeout. Another option would be to add another calld platformRun that didn't timeout. > Tools/WebKitTestRunner/TestInvocation.cpp:153 > + WKRetainPtr<WKStringRef> useTimeoutWatchdogKey = adoptWK(WKStringCreateWithUTF8CString("UseTimeoutWatchdog")); > + WKRetainPtr<WKBooleanRef> useTimeoutWatchdogValue = adoptWK(WKBooleanCreate(TestController::shared().shouldTimeout())); > + WKDictionaryAddItem(beginTestMessageBody.get(), useTimeoutWatchdogKey.get(), useTimeoutWatchdogValue.get()); Probably better to refer to this thing consistently ala useWaitToDumpWatchdogTimer. > Tools/WebKitTestRunner/mac/TestControllerMac.mm:56 > + // FIXME: No timeout should occur if shouldTimeout is false (necessary when running performance tests). It seems quite possible to add support here. > Tools/WebKitTestRunner/win/TestControllerWin.cpp:175 > + // FIXME: No timeout should occur if shouldTimeout is false (necessary when running performance tests). This one seems a bit more complicated, so if you don't add Windows support, be sure to open a bug for it and CC the appropriate people.

Comment on attachment 129741 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129741&action=review >> Tools/WebKitTestRunner/TestController.cpp:546 >> + platformRunUntil(done, timeoutDuration == ShortTimeout ? m_shortTimeout : m_longTimeout, shouldTimeout); > > Instead of adding an extra parameter, perhaps it would be better to add another value to the enum so there is ShortTimeout, LongTimeout and NoTimeout. Noted. >> Tools/WebKitTestRunner/TestController.h:74 >> + void platformRunUntil(bool& done, double timeout, bool shouldTimeout); > > Instead of adding another parameter, perhaps you could just have a constant == -1 that signifies no timeout. Another option would be to add another calld platformRun that didn't timeout. platformRun would probably introduce some code duplication, so I'll just use a negative constant to indicate no timeout and check for that. >> Tools/WebKitTestRunner/TestInvocation.cpp:153 >> + WKDictionaryAddItem(beginTestMessageBody.get(), useTimeoutWatchdogKey.get(), useTimeoutWatchdogValue.get()); > > Probably better to refer to this thing consistently ala useWaitToDumpWatchdogTimer. I'll just put (u/U)seWaitToDumpWatchdogTimer everywhere - consistency at its best. >> Tools/WebKitTestRunner/mac/TestControllerMac.mm:56 >> + // FIXME: No timeout should occur if shouldTimeout is false (necessary when running performance tests). > > It seems quite possible to add support here. I'm not familiar enough with Objective C or Cocoa to be comfortable making these changes. I can give it a shot, but I would rather open a bug for this problem, as you suggested doing for the Windows' TestController.

Created attachment 130183 [details] Patch

Comment on attachment 130183 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130183&action=review > Tools/WebKitTestRunner/TestController.cpp:558 > + timeout = -1.0; > + break; > + } Sorry, I should have been a bit clearer in my review comment. I think it makes sense to have a constant, perhaps a static member variable like: TestController:s_noTimeoutDuration = -1; I can change this and land it.

Committed r110382: <http://trac.webkit.org/changeset/110382>

(In reply to comment #8) > Committed r110382: <http://trac.webkit.org/changeset/110382> Thanks for the small changes and landing. Posted two new bugs for Mac and Win ports, #80780 and #80781.