[webkit-reviews] review granted: [Bug 50374] nrwt multiprocessing - simpify message_broker and move logic back into run_webkit_tests : [Attachment 75357] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Dec 2 12:15:46 PST 2010
Tony Chang <tony at chromium.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 50374: nrwt multiprocessing - simpify message_broker and move logic back
into run_webkit_tests
https://bugs.webkit.org/show_bug.cgi?id=50374
Attachment 75357: Patch
https://bugs.webkit.org/attachment.cgi?id=75357&action=review
------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=75357&action=review
Seems like this is mostly just moving stuff around.
>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:117
> + thread = super(_MultiThreadedBroker, self).start_worker(test_runner,
Nit: I think the preferred calling convention is
_WorkerMessageBroker.start_worker(self, test_runner, worker_number) because
super can be ambiguous in the case of multiple inheritance. I think this is
consistent to how we use the super class name in __init__.
> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:233
> +# FIXME: This name is somewhat misleading because the actual workers have
> +# their own internal state in TestShellThread. However, a better name for
> +# this has yet to be found.
Nit: I think this name is fine for now and the docstring describes it. I would
just remove this comment.
> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:269
> + # or processes. This is a bit awkward but a better name has yet to
Nit: Missing closing paren.
> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:687
> + for thread in [w.thread for w in workers.values()]:
> + thread_timings.append({'name': thread.name(),
This is a bit convoluted. Why not just:
for worker in workers.values():
thread = worker.thread
thread_timings.append(....
More information about the webkit-reviews
mailing list