[webkit-reviews] review denied: [Bug 49779] nrwt multiprocessing - implement first part of broker object : [Attachment 74351] yet more cleanup

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 49779: nrwt multiprocessing - implement first part of broker object
https://bugs.webkit.org/show_bug.cgi?id=49779

Attachment 74351: yet more cleanup
https://bugs.webkit.org/attachment.cgi?id=74351&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=74351&action=review

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:48

> +    if options.workers_are == 'inline':
> +	   return SingleThreadedBroker(port, options)

Where is the workers_are flag declared?  I think workers_are is a poor name.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:56

> +class _WorkerState(object):
> +    def __init__(self):

Maybe this class should be declared in AbstractBroker?	Do other classes need
to instantiate it?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:62

> +class AbstractBroker(object):
> +    def __init__(self, port, options):

Can you add a docstring describing AbstractBroker?  The name is very vague
(could be named better?).

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:76

> +	       worker = _WorkerState()
> +	       worker.thread = self._start_worker(worker_number)
> +	       worker.name = 'worker-%d' % worker_number

Wouldn't it be better to put this in __init__?	Also, isn't this name
generation a duplicate with code from bug 49768?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:81

> +    def _start_worker(self, worker_number):
> +	   raise NotImplementedError

Please document what implementations should go here and what the return type
should be.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:85

> +    def loop(self):
> +	   """Loop waiting for threads to finish."""
> +	   raise NotImplementedError

Based on the docstring, it's not clear to me what the implementation is
supposed to do.  Looking at SingleThreadedBroker, it looks like this is the
run_loop() command.  What should be returned?

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:101
> +	   # Note: Don't start() the thread!

Fill this comment out and say why.  It's not obvious to me why this method
doesn't start the thread and MultiThreadedBroker does.

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:141
> +		       if (next_timeout and t > next_timeout):

Nit: remove the ()

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker_unittes
t.py:109
> +	   # FIXME: implement
> +	   return

Why do these tests no longer work?  Can we make it work in this patch?	It's
nice to have each step of the code landing be in a working state.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:253
> +	   self._broker = broker

I think this variable should be named more specifically than broker.  The name
broker doesn't tell you anything about what it does.  A broker for what?

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:629
> +	   (thread_timings, test_timings, individual_test_timings) = \

Nit: Not () on the left side.