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

[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 132778: patch v1
https://bugs.webkit.org/attachment.cgi?id=132778&action=review

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


> Tools/Scripts/webkitpy/layout_tests/port/base.py:88
> +    # The per-test timeout in milliseconds, if no --time-out-ms option was
> +    # given to run_webkit_tests. Subclass can override it in
default_test_timeout_ms
> +    # according to the computing power on its platform.

This comment doesn't really add much value. It's pretty clear what
DEFAULT_TEST_TIMEOUT_MS refers to.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:93
> +    # The multiplier for per-test slow timeout. Subclass can override it in
> +    # default_slowtest_timeout_multiplier according to the computing power
on its platform.
> +    DEFAULT_SLOWTEST_TIMEOUT_MULTIPLIER = 5

I don't really see the need for ports to make this configurable since it's a
multiplier of the test timeout, which is configurable. Lets cut this out and
add it in when we find a port actually needs it.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:157
> +    def default_slowtest_timeout_multiplier(self):
> +	   return self.DEFAULT_SLOWTEST_TIMEOUT_MULTIPLIER

Ditto: above, lets remove this.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:620
> +	       kill_timeout_seconds = 3.0 * int(time_out_ms) /
Port.DEFAULT_TEST_TIMEOUT_MS

Shouldn't this be using default_test_timeout_ms?