[webkit-reviews] review granted: [Bug 50367] nrwt multiprocessing - clean up dump_render_tree_thread.py : [Attachment 75347] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 50367: nrwt multiprocessing - clean up dump_render_tree_thread.py
https://bugs.webkit.org/show_bug.cgi?id=50367

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

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

These refactorings all seem reasonable.  I would have been able to review this
faster if it were in smaller parts.  In its current form, there was lots of
scrolling up and down.	Please fix the comments before committing.

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_threa
d.py:71
> -	   self._canceled = False
> +	   self._cancelled = False

According to the internet, cancelled is the British spelling and canceled is
the American spelling.	Doesn't seem worthwhile to change it here.

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_threa
d.py:244
> +    def _cleanup(self):

Should __del__ call _cleanup?  Looks like self._tests_run_file won't get
cleaned up if you don't call run().

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_threa
d.py:270
> +	   self._test_list_timing_stats[list_name] = \
> +	      (num_tests, elapsed_time)

Nit: If you put the opening ( on the previous line, you don't need the line
continuation \.

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_threa
d.py:285
> +	   driver_timeout = 3.0 * float(test_input.timeout) / 1000.0
> +	   thread_padding = 1.0
> +	   thread_timeout = driver_timeout + thread_padding

Nit: It would be nice if the variables had time units in their name (e.g.,
driver_timeout_sec and thread_timeout_sec).

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_threa
d.py:333
> +	       def run(self):
> +		   result = worker._run_single_test(driver, test_input)

I think you can do:
def run(self, worker=self):

And avoid having to set worker = self above.

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_threa
d.py:471
> +	   if self._batch_size > 0 and self._batch_count >= self._batch_size:

Nit: self._batch_count >= self._batch_size > 0