[webkit-reviews] review granted: [Bug 78565] webkitpy: create ports in Workers, not in manager_worker_broker : [Attachment 126889] add comment about the change in run_webkit_tests.py

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 78565: webkitpy: create ports in Workers, not in manager_worker_broker
https://bugs.webkit.org/show_bug.cgi?id=78565

Attachment 126889: add comment about the change in run_webkit_tests.py
https://bugs.webkit.org/attachment.cgi?id=126889&action=review

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


> Tools/ChangeLog:13
> +	   bug 78171.
> +
> +	   *
Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:

Is this covered by existing tests? Please name the tests that cover this in the
ChangeLog.

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:90
> +	       # we are running in a child process and need to create a new
Host.

Nit: Capitalize 'we'.

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:97
> +	       host._initialize_scm()

For my education, do we always have to call _initialize_scm() on host objects? 
I noticed that running layout tests always calls this, which runs svn and git
to determine the checkout type, even though we never use the information (e.g.,
if all the tests pass).  It's slow on my Windows machine.

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:107
> +	       # FIXME: this won't work if the calling process is logging

Nit: Capitalize 'this'.