new-run-webkit-tests: minor cleanup for multiprocessing work
Created attachment 75354 [details] Patch
Created attachment 75356 [details] yet more cleanup
Comment on attachment 75356 [details] yet more cleanup I don't feel like line-wrapping patches are very useful. But then again, I'm in the anti-80c camp. :)
Comment on attachment 75356 [details] yet more cleanup I agree with Eric. We've decided not to do 80c in webkit python code, so changing a bunch of style to 80c just adds code churn. Can you upload this patch without the line-wrapping changes?
Created attachment 75404 [details] remove whitespace diffs / reformatting
(In reply to comment #4) > (From update of attachment 75356 [details]) > I agree with Eric. We've decided not to do 80c in webkit python code, so changing a bunch of style to 80c just adds code churn. Can you upload this patch without the line-wrapping changes? Okay. Although I'm obviously in the 80c camp, I only was reformatting this stuff because the rest of the file had already been formatted to 80c, so the mix of < 80 and > 80 made things awkward (I figured consistency one way or another was better). I've removed the line-wrapping, but I'm curious to your thoughts on the above?
I think you didn't mean to upload a squashed diff.
Created attachment 75415 [details] upload the incremental diff, not the whole diff
(In reply to comment #7) > I think you didn't mean to upload a squashed diff. Whoops, you're right. Sorry! I was rushing out to meet someone for lunch. Fixed now.
Comment on attachment 75415 [details] upload the incremental diff, not the whole diff View in context: https://bugs.webkit.org/attachment.cgi?id=75415&action=review > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:117 > + def start_worker(self, worker_number): > + thread = super(_MultiThreadedBroker, self).start_worker(worker_number) thread = _MultiThreadedBroker.start_worker(self, worker_number) for explicitness
Created attachment 75434 [details] add port.default_worker_model, merge in fix from bug 50420, other review feedback
(In reply to comment #10) > (From update of attachment 75415 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75415&action=review > > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:117 > > + def start_worker(self, worker_number): > > + thread = super(_MultiThreadedBroker, self).start_worker(worker_number) > > thread = _MultiThreadedBroker.start_worker(self, worker_number) for explicitness Actually, that would cause an infinite loop :) Correct version added.
(In reply to comment #11) > Created an attachment (id=75434) [details] > add port.default_worker_model, merge in fix from bug 50420, other review feedback This patch adds a way for us to do port-specific overrides to --worker-model, which I suspect we'll want as we stabilize this stuff and in case multiprocessing ends up not working that well on windows (or under cygwin).
http://trac.webkit.org/changeset/73222 has very strange commit log. (same as 73220) I think it might be a webkit-patch bug. Could you check it?
It's a problem when using git rebase. It sometimes merges your changelog not to the top of the file. We should have a SVN pre-commit hook, or webkit-patch step to validate that you're only adding to the top of the file (which is where we pull the commit message from).
(In reply to comment #15) > It's a problem when using git rebase. It sometimes merges your changelog not to the top of the file. > > We should have a SVN pre-commit hook, or webkit-patch step to validate that you're only adding to the top of the file (which is where we pull the commit message from). In this case I think I was creating the patch using the -g foo..bar option, since I had a bunch of work on branches I was attempting to merge in. So I would bet this was user error, and not a bug in webkit-patch. That said, I agree a presubmit check might be a good idea.
Marking as WONTFIX - will re-split up the patches and re-land a different way.