[webkit-reviews] review granted: [Bug 99588] Use the new forwarder2 : [Attachment 171871] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted felipe <felipeg at chromium.org>'s
request for review:
Bug 99588: Use the new forwarder2
https://bugs.webkit.org/show_bug.cgi?id=99588

Attachment 171871: Patch
https://bugs.webkit.org/attachment.cgi?id=171871&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=171871&action=review


Sorry, just got back from vacation.  Feel free to ask someone else for a review
if I don't respond in a day.

Overall this seems fine, but I suspect the code will break if you don't add
unit tests.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:228
>	   result = super(ChromiumAndroidPort, self).check_build(needs_http)
>	   result = self._check_file_exists(self._path_to_md5sum(), 'md5sum
utility') and result
> -	   result = self._check_file_exists(self._path_to_forwarder(),
'forwarder utility') and result
> +	   result = self._check_file_exists(self._path_to_device_forwarder(),
'device_forwarder utility') and result
> +	   result = self._check_file_exists(self._path_to_host_forwarder(),
'host_forwarder utility') and result

Nit: I would probably merge these lines.
result = (super(...)
    and self._check_file_exists(...)
    and self._check_file_exists(...)
    ...)

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:487
> +	       failure_m = failure_re.match(line)

Nit: failure_m -> failure_match

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:491
> +	       success_m = success_re.match(line)

Nit: success_m -> success_match

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:501
> +    def close(self):
> +	   self._process.stop(0.0)

This is OK, but it would be cleaner to use __enter__ and __exit__ to start/stop
the process and use the 'with' statement in the calling code.  Then you
wouldn't need to manually remember to call close() everywhere.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:629
> +	   output = self._android_cmd.run_host_command(['lsof', '-nPi:%d' %
host_port], ignore_error=True, return_exit_code=False)

I guess this is OK, but it requires that lsof is installed.  I assume you
verified this was the case on our bots?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:639
> +	   self._android_cmd.run_host_command(['pkill', '-f', host_pattern])

Same comment about pkill and pgrep (procps package) as for lsof.  Maybe these
are installed with the other android specific dependencies?