[webkit-reviews] review granted: [Bug 90513] nrwt: reimplement manager_worker_broker in a much simpler form : [Attachment 151830] merge to r122394

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 90513: nrwt: reimplement manager_worker_broker in a much simpler form
https://bugs.webkit.org/show_bug.cgi?id=90513

Attachment 151830: merge to r122394
https://bugs.webkit.org/attachment.cgi?id=151830&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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.