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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=170186

Attachment 305640: Patch

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




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

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):

Ditto.

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

Ditto.

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

Ditto.

> 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"?