[webkit-reviews] review denied: [Bug 53158] nrwt multiprocessing: add simplified message broker for new-style messaging : [Attachment 81021] add more comments describing the design

[bugzilla-daemon at webkit.org]

Tony Chang <tony 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 81021: add more comments describing the design
https://bugs.webkit.org/attachment.cgi?id=81021&action=review

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

r- because I still don't have a good sense of how these classes fit together.

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:31
> +"""Module for handling messaging for run-webkit-tests.
> +
> +This modules implements a simple message broker abstraction that will be

This docstring seems overly long and it seems like it could be done simply as a
ASCII art diagram with a couple sentences of text.  Having read the docstring a
few times, I still don't have a good idea of how these classes interact.

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:76
> +class ClientInterface(object):
> +    """Abstract base class / interface that all message broker clients must

Nit: I would maybe just name this BrokerClient.  That seems to be more
consistent with the naming WebKit/WebCore.

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:116
> +	   Args:
> +	       options: a runtime option class from optparse

Nit: Can you document queue_maker?

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:124
> +    def add_topic(self, topic_name):
> +	   if topic_name not in self._topics:
> +	       self._topics[topic_name] = self._queue_maker()

Do we change topics on the fly or is the list of topics something that can be
passed into the ctor?

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:127
> +    def _find_topic(self, topic_name):
> +	   return self._topics[topic_name]

Nit: Maybe _get_topic_queue or even _get_message_queue_for_topic?