Bug 49707 – new-run-webkit-tests: create first part of 'message_broker' class for multiprocessing fixes

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

new-run-webkit-tests: create first part of 'message_broker' class for multiprocessing fixes

[Dirk Pranke]

Created attachment 74195 [details]
Patch

[Tony Chang]

Comment on attachment 74195 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74195&action=review

Just some small nits

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread_unittest.py:35
> +    pass

Should we just delete this file for now?  It's easy to bring back.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:34
> +

Nit: Kill blank line?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:38
> +# FIXME: This is a small file for now to handle breaking the uber-patch into
> +# smaller, more manageable patches.

I don't understand what needs to be fixed here.  I think it's fine to have small .py files.  I would just remove this comment.

[Dirk Pranke]

Created attachment 74297 [details]
Patch

[Dirk Pranke]

(In reply to comment #2)
> (From update of attachment 74195 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74195&action=review
> 
> Just some small nits
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread_unittest.py:35
> > +    pass
> 
> Should we just delete this file for now?  It's easy to bring back.
> 

I figured the time in which it didn't exist was limited, so there wasn't much point in deleting it, but I dont feel that strongly about it, so I removed it.

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:34
> > +
> 
> Nit: Kill blank line?
>

Technically, the blank line at the end of a docstring is recommended by PEP 8, but I waffle on them myself. Deleted.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:38
> > +# FIXME: This is a small file for now to handle breaking the uber-patch into
> > +# smaller, more manageable patches.
> 
> I don't understand what needs to be fixed here.  I think it's fine to have small .py files.  I would just remove this comment.

The comment was more intended to indicate to the review that this file would be getting much bigger. Otherwise the reviewer might've wondered why this was being split out into a separate file at all (I would've, anyway). Deleted.

[Dirk Pranke]

Comment on attachment 74297 [details]
Patch

Clearing flags on attachment: 74297

Committed r72339: <http://trac.webkit.org/changeset/72339>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.