Bug 90126 – nrwt: clean up how arguments are passed to workers

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

nrwt: clean up how arguments are passed to workers

[Dirk Pranke]

Created attachment 149854 [details]
Patch

[Dirk Pranke]

this patch is probably one of the more subtle of the whole refactoring. let me know if it needs more explanation.

[Ojan Vafai]

Comment on attachment 149854 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:97
> +        worker_factory: factory method for creatin objects that implement the AbstractWorker interface.

typo: creatin

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:114
> +    """Raised when we receive an unexpected/unknown exception from a worker."""

Don't think this comment adds value.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:332
> +        host = self._host

Do you need this local variable?

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:354
> +        host = None
> +        if self._can_pickle_host():
> +            host = self._host

This seems odd to me. Should we just error out if the host can't be pickled?

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 149854 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149854&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:97
> > +        worker_factory: factory method for creatin objects that implement the AbstractWorker interface.
> 
> typo: creatin
> 

Will fix.

> > Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:114
> > +    """Raised when we receive an unexpected/unknown exception from a worker."""
> 
> Don't think this comment adds value.
>

Will delete.
 
> > Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:332
> > +        host = self._host
> 
> Do you need this local variable?
> 

Not in this case, will remove.

> > Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:354
> > +        host = None
> > +        if self._can_pickle_host():
> > +            host = self._host
> 
> This seems odd to me. Should we just error out if the host can't be pickled?

It is a bit odd :)

It's not a requirement that the host be picklable (and in normal operation, I don't expect it to be). The only reason we want to send the host to the worker at all is so in the test scenario it can know to use a mock host get the same test port configuration.

[Dirk Pranke]

(In reply to comment #4)
> > > Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:354
> > > +        host = None
> > > +        if self._can_pickle_host():
> > > +            host = self._host
> > 
> > This seems odd to me. Should we just error out if the host can't be pickled?
> 
> It is a bit odd :)
> 
> It's not a requirement that the host be picklable (and in normal operation, I don't expect it to be). The only reason we want to send the host to the worker at all is so in the test scenario it can know to use a mock host get the same test port configuration.

Oh, also, in a later patch I merge InlineManager and MultiProcessManager, and so we need to pass the actual host to the worker (in process) so that it can get the overridden host.port_factory.get() method that allows us to share a port between the test code in run_webkit_tests_integrationtest.

Note that requiring the code to be able to share the port is a bug 78168 and fixing that will simplify this slightly, but we'll still need some way to know to use a MockHost in the worker process and using this approach means that we don't have to actually import MockHost in the file.

[Dirk Pranke]

Committed r121509: <http://trac.webkit.org/changeset/121509>