[webkit-reviews] review denied: [Bug 54070] nrwt multiprocessing: implement basic support for threads and processes : [Attachment 81758] Patch

[bugzilla-daemon at webkit.org]

Mihai Parparita <mihaip at chromium.org> has denied Dirk Pranke
<dpranke at chromium.org>'s request for review:
Bug 54070: nrwt multiprocessing: implement basic support for threads and
processes
https://bugs.webkit.org/show_bug.cgi?id=54070

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

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

>
Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:195

> +	   return _ThreadedWorker(self._broker, self._port)

There seems to be naming mismatch here (worker connection vs. worker). I
realize that may be because connections are supposed to be facades, but perhaps
we should reconsider the "connection" name? In the Chromium multi-process
scheme, facades for an object in another process are known as *Host (e.g.
RenderView and RenderViewHost). Perhaps we should use the same naming here?

>
Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker_unitte
st.py:122
> +    # Methods to implement the Manager side of the ClientInterface

ClientInterface is now BrokerClient. Also, can you make _TestsMixin actually
subclass BrokerClient, so that the relationship is more obvious?

Also, these header-style comments take up a bit too much room (not sure if we
have a convention for them in webkitpy)

>
Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker_unitte
st.py:201
> +    def queue(self):

Is this (or the other queue() implementation below) necessary anymore? The
queue class is now determined in based on the passed in worker model in
manager_worker_broker.get.