[Webkit-unassigned] [Bug 80042] [WK2] run-perf-tests should be able to run with WTR

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=80042


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #129741|review?                     |review-
               Flag|                            |




--- Comment #4 from Martin Robinson <mrobinson at webkit.org>  2012-03-02 10:26:34 PST ---
(From update of attachment 129741)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.