[webkit-reviews] review denied: [Bug 79859] Customize layout test timeout value for different ports. : [Attachment 132945] patch v2

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Johnny(Jianning) Ding
<jnd at chromium.org>'s request for review:
Bug 79859: Customize layout test timeout value for different ports.
https://bugs.webkit.org/show_bug.cgi?id=79859

Attachment 132945: patch v2
https://bugs.webkit.org/attachment.cgi?id=132945&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=132945&action=review


> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:165
> +    else:
> +	   if int(options.time_out_ms) < port_default_timeout_ms:
> +	       options.time_out_ms = str(port_default_timeout_ms)

You should be able to set the timeout to less than the port default.  I don't
think we want this else statement. See my other coment below.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:86
> +    DEFAULT_TEST_TIMEOUT_MS = 6 * 1000

Nit: I don't think we need this variable. You can just move the 6*1000 into
default_test_timeout_ms

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:622
>	   if time_out_ms and not self._no_timeout:
> -	       # FIXME: Port object shouldn't be dependent on layout test
manager.
> -	       kill_timeout_seconds = 3.0 * int(time_out_ms) /
Manager.DEFAULT_TEST_TIMEOUT_MS
> +	       kill_timeout_seconds = 3.0 * int(time_out_ms) /
self._port.default_test_timeout_ms()
>	   else:
>	       kill_timeout_seconds = 3.0

If you want, you can make sure that kill_timeout_seconds is at least 3.0 here.
That would deal with the case of time_out_ms being less than
default_test_timeout_ms.

Really this chunk of code is very confusing. Seems like we should always just
set kill_timeout_seconds to 3.0.