[webkit-reviews] review denied: [Bug 47465] new-run-webkit-tests should bail out when all the threads are wedged : [Attachment 71382] 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 71382: Patch
https://bugs.webkit.org/attachment.cgi?id=71382&action=review

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

I'm failing to understand what this is trying to do (perhaps it's trying to do
too much?)  Perhaps I should come down and read this in person sitting next to
you.

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_threa
d.py:247
> +    def __init__(self, worker_number, runner, port, options,
> +		    filename_list_queue, result_queue, test_types, test_args):

If we're going to have a back-pointer to runner, should more of this list just
be pulled off of runner?

Even if we just pull it off in the __init__ and cache it locally, that seems
cleaner than passing each value to the constructor.  donno.

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_threa
d.py:253
> +	     runner: handle to TestRunner (master thread) used to notify of
> +		 thread startup/shutdown

Does this use a specific delegate protocol?  Often we use such for easier
testing/api-containment.  You can see examples in CommitQueueTask, QueueEngine,
etc.

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_threa
d.py:274
> +	   self.setName("Worker-%d" % (worker_number + 1))

Seems the owner of this thread could just do this?  Why does the thread set its
own name?

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_threa
d.py:325
> +	       self._runner.notify_thread_start(self)

I wouldnt' have used notify in the name.  But it's OK.	I probably would have
just called it thread_started/thread_stopped.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:647
> +	   self._cv = threading.Condition()

What does _cv stand for?  It's not clear from that name what the variable is
doing.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:648
> +	   self._cv.acquire()

with_statement?

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:688
> +	   self._cv.acquire()

If this is a lock, then with_statement would be safer here.  I'm not sure what
_cv is.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1461
> +def _run_with_printer(port, options, printer, num_workers, args):

The changes in this function are all just changes in indent, right?

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:36
> +import pdb

leftover.