[webkit-reviews] review denied: [Bug 47465] new-run-webkit-tests should bail out when all the threads are wedged : [Attachment 70388] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 47465: new-run-webkit-tests should bail out when all the threads are wedged
https://bugs.webkit.org/show_bug.cgi?id=47465

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=70388&action=review

I think this is generally a great patch, just I'd like to see one more round.
:)

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:653
> +	   return (thread_timings, test_timings, individual_test_timings)

Eventually this probably needs to be a class.  Tuples rarely stay tuples for
long. :)  (Obviously not related to this patch, just noting.)

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:661
> +	   last_t = time.time()

It's slightly unfrotunate this variable doesn't have units in its name.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:666
> +	       living_threads = 1
> +	       while living_threads:
> +		   living_threads = 0

This feels like maybe a do while?  Does python even have such?

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:668
> +		   min_timeout = last_t + 600

Lack of units in the variable name makes "600" harder to read.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:681
> +			   if next_timeout and next_timeout < min_timeout:
> +			       min_timeout = next_timeout

Isn't this just a min function?  Or does min() not handle None as you'd want it
to?

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:683
> +			       if not thread in wedged_threads:

"foo not in bar" reads slightly nicer to my eyes than "not foo in bar"

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:691
> +		   if t > last_t + 10:

I'm confused by what this does.  Possibly due to variable naming.  Not sure.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:700
> +	       if len(wedged_threads):

if wedged_threads: should do the same with 4 fewer chars.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:709
> +	   except Exception, e:
> +	       self._exception = e

Do we not want to cancel threads in the non-keyboard case?  Maybe we should
wrap this whole thing (possibly by using a helper function) in a separate try
block to do the self._exception setting and the inner one would handle the
keyboard stuff?  Maybe that's a separate patch?