[webkit-reviews] review denied: [Bug 170186] webkitpy: Add target host concept : [Attachment 305640] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Mar 28 16:53:24 PDT 2017

Alexey Proskuryakov <ap at webkit.org> has denied Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 170186: webkitpy: Add target host concept

Attachment 305640: Patch


--- Comment #4 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 305640
  --> https://bugs.webkit.org/attachment.cgi?id=305640

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

I'd like to better understand the difference between "host", "device" and
"port". Perhaps the code could be more clear about these.

Looks good overall, please address the comments about the Port class member
functions with unused arguments.

> Tools/Scripts/webkitpy/port/base.py:809
> -    def abspath_for_test(self, test_name):
> +    def abspath_for_test(self, test_name, target_host=None):

It would be cleaner to implement the functionality for non-None target host
here, not in subclasses. As it is, the signature misleads the reader into
believing that passing a target_host is always supported.

> Tools/Scripts/webkitpy/port/base.py:1277
> +    def _driver_tempdir(self, target_host=None):


> Tools/Scripts/webkitpy/port/base.py:1349
> +    def sample_process(self, name, pid, target_host=None):


> Tools/Scripts/webkitpy/port/darwin.py:125
> +    def sample_process(self, name, pid, target_host=None):


> Tools/Scripts/webkitpy/port/ios.py:79
> +    # Devices are the target host

Please add the sentence with a period.

Is it right that multiple "devices" are a singular "host"?

More information about the webkit-reviews mailing list