Bug 49707 - new-run-webkit-tests: create first part of 'message_broker' class for multiprocessing fixes
Summary: new-run-webkit-tests: create first part of 'message_broker' class for multipr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-17 19:24 PST by Dirk Pranke
Modified: 2010-11-18 15:23 PST (History)
4 users (show)

See Also:

Attachments
Patch (19.01 KB, patch)
2010-11-17 19:27 PST, Dirk Pranke 		no flags Details | Formatted Diff | Diff
Patch (20.28 KB, patch)
2010-11-18 15:09 PST, Dirk Pranke 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2010-11-17 19:24:55 PST 
new-run-webkit-tests: create first part of 'message_broker' class for multiprocessing fixes
Comment 1 Dirk Pranke 2010-11-17 19:27:28 PST 
Created attachment 74195 [details]
Patch
Comment 2 Tony Chang 2010-11-18 14:46:45 PST 
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.
Comment 3 Dirk Pranke 2010-11-18 15:09:21 PST 
Created attachment 74297 [details]
Patch
Comment 4 Dirk Pranke 2010-11-18 15:11:50 PST 
(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.
Comment 5 Dirk Pranke 2010-11-18 15:23:44 PST 
Comment on attachment 74297 [details]
Patch

Clearing flags on attachment: 74297

Committed r72339: <http://trac.webkit.org/changeset/72339>
Comment 6 Dirk Pranke 2010-11-18 15:23:50 PST 
All reviewed patches have been landed.  Closing bug.