Bug 49768

Created attachment 74316 [details] Patch

Created attachment 74321 [details] minor cleanup

Comment on attachment 74321 [details] minor cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=74321&action=review > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:55 > +from webkitpy.layout_tests.test_types import test_type_base > +from webkitpy.layout_tests.test_types import text_diff > +from webkitpy.layout_tests.test_types import image_diff Nit: Sort alphabetically > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:486 > + self._worker_number, > + self._name, It seems redundant to have to pass worker number and worker name. Are there cases where you can't derive the name from the number? Maybe there should just be a static function somewhere that converts number to name?

Created attachment 74335 [details] review feedback - sort imports alphabetically

(In reply to comment #3) > (From update of attachment 74321 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=74321&action=review > > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:55 > > +from webkitpy.layout_tests.test_types import test_type_base > > +from webkitpy.layout_tests.test_types import text_diff > > +from webkitpy.layout_tests.test_types import image_diff > > Nit: Sort alphabetically > Done. > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:486 > > + self._worker_number, > > + self._name, > > It seems redundant to have to pass worker number and worker name. Are there cases where you can't derive the name from the number? Maybe there should just be a static function somewhere that converts number to name? In fact, if you look at older versions of the rollup patches that's what I used to have. Then I realized that there's no reason for SingleTestThread to know that you can derive worker_name from worker_number, and that this better encapsulate things. I'd prefer to leave it as it is.

Comment on attachment 74335 [details] review feedback - sort imports alphabetically View in context: https://bugs.webkit.org/attachment.cgi?id=74335&action=review > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:171 > + def __init__(self, port, options, worker_number, worker_name, > + test_input, test_types, test_args): My main concern with having a separate name and number is that it makes functions with lots of parameters fragile. Since there's no compile time checking of parameters, it's easy to miss a caller and break things when refactoring. In this case, it's relatively isolated (only this call takes both), so I don't feel that strongly about it. This patch doesn't use worker_number, do you need it in the future? Maybe it should be added when it's used?

Another option for encapsulating is to make a small class that combines worker name and number. You could have a getter on the class that derives the name from the number.

(In reply to comment #6) > (From update of attachment 74335 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=74335&action=review > > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:171 > > + def __init__(self, port, options, worker_number, worker_name, > > + test_input, test_types, test_args): > > My main concern with having a separate name and number is that it makes functions with lots of parameters fragile. Since there's no compile time checking of parameters, it's easy to miss a caller and break things when refactoring. > It's a fair concern. A bunch of these arguments go away in subsequent patches.

Comment on attachment 74335 [details] review feedback - sort imports alphabetically Clearing flags on attachment: 74335 Committed r72458: <http://trac.webkit.org/changeset/72458>

All reviewed patches have been landed. Closing bug.