[webkit-reviews] review denied: [Bug 53158] nrwt multiprocessing: add simplified message broker for new-style messaging : [Attachment 81308] fix typos

[bugzilla-daemon at webkit.org]

Mihai Parparita <mihaip at chromium.org> has denied Dirk Pranke
<dpranke at chromium.org>'s request for review:
Bug 53158: nrwt multiprocessing: add simplified message broker for new-style
messaging
https://bugs.webkit.org/show_bug.cgi?id=53158

Attachment 81308: fix typos
https://bugs.webkit.org/attachment.cgi?id=81308&action=review

------- Additional Comments from Mihai Parparita <mihaip at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81308&action=review

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:50
> +    TestRunner     ------>	 BrokerConnection

So TestRunner implements BrokerClient? It might be worthwhile to make that
explicit.

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:82
> +    def __init__(self, broker_connection, worker_number, options, **kwargs):


There's a bunch of worker references in the BrokerClient class that seem out of
place (i.e. at the wrong abstraction layer). If some (most?) BrokerClient
implementations are going to be workers, perhaps it makes sense to have a
WorkerBrokerClient abstract class (in a different file) that subclasses
BrokerClient has the cancel() and run() abstract methods.

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:108
> +    """Brokers provide the basic model of named queues. Clients can post a

There's some inconsistency in naming (topics vs. named queues), both in
comments and code. I think consistency would help (I have a slight preference
for "topic", since that's more common in publish-subscribe systems).

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:129
> +    def post_message(self, client, topic_name, message_name, *optargs):

Call optargs message_args for consistency with BrokerConnection.

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:140
> +	   self._run_loop(topic_name, client, True, delay_secs)

Use block=True so that it's easier to read the call.

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:201
> +    def client(self):

Is there a significant benefit from forcing users of this package to subclass
BrokerConnection to provide a client, instead of just making it a constructor
parameter?