nrwt: reimplement manager_worker_broker in a much simpler form
Created attachment 150711 [details] Patch
Created attachment 151830 [details] merge to r122394
Comment on attachment 151830 [details] merge to r122394 View in context: https://bugs.webkit.org/attachment.cgi?id=151830&action=review That's a lot of deleted code! > Tools/ChangeLog:19 > + I'm removing manager_worker_broker_unittest.py as well; we get > + nearly complete coverage from the integration tests, and will > + get more coverage when test-webkitpy moves to use this as well, > + so having unit tests seems like unnecessary overhead. (running > + coverage numbers with test-webkitpy shows that pretty much the only > + uncovered lines are lines that are only run in the child processes, > + which coverage doesn't handle at the moment). I'm a little torn about this. I think it makes it harder to make changes to this code in the future and have confidence. On the other hand, I believe you that integration tests cover most of the code paths. abarth, wdyt? > Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:98 > + worker.running_inline = self._running_inline Should running_inline and manager just be arguments to the constructor? There doesn't seem to be benefit to setting those after the constructor.
> abarth, wdyt? IMHO, it's fine. We run the integration tests by default, so we're not missing out on too much test coverage.
(In reply to comment #3) > > Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:98 > > + worker.running_inline = self._running_inline > > Should running_inline and manager just be arguments to the constructor? There doesn't seem to be benefit to setting those after the constructor. Sure, I can make that change.
Committed r122497: <http://trac.webkit.org/changeset/122497>