[webkit-reviews] review denied: [Bug 80042] [WK2] run-perf-tests should be able to run with WTR : [Attachment 129741] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Zan Dobersek
<zandobersek at gmail.com>'s request for review:
Bug 80042: [WK2] run-perf-tests should be able to run with WTR
https://bugs.webkit.org/show_bug.cgi?id=80042

Attachment 129741: Patch
https://bugs.webkit.org/attachment.cgi?id=129741&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.