[webkit-reviews] review denied: [Bug 99588] Use the new forwarder2 : [Attachment 170423] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied 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 170423: Patch
https://bugs.webkit.org/attachment.cgi?id=170423&action=review

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


> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:42
> +import webkitpy.thirdparty.autoinstalled.pexepct as pexpect

Why not 'from webkitpy.thirdparty.autoinstalled import pexpect' like the other
import statements?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:78
> +FORWARD_PORTS = [8000, 8080, 8443, 8880, 9323]

Use a tuple instead of a list since this is a constant.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:348
> +class AndroidCommands(Object):

Object? I think you mean object.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:353
> +	   self._original_governors = {}

Maybe _original_governors_file_contents?  It wasn't obvious to me what this
dictionary is for.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:368
> +	   # Disable CPU scaling and drop ram cache to reduce noise in tests

Nit: Please end the sentence with a period.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:416
> +	   result = self._port._executive.run_command(cmd,
error_handler=error_handler, return_exit_code=return_exit_code)
> +	   return result

Nit: Maybe return directly and remove the temp variable?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:450
> +	       port_pairs: A list of tuples (device_port, host_port) to
forward. Note
> +			   that you can specify 0 as a device_port, in which
case a
> +			   port will by dynamically assigned on the device. You
can
> +			   get the number of the assigned port using the
> +			   device_port_for_host_port method.

Rather than passing in a list of tuples, can you make a small class for device
port and host port and pass in a list of the class? E.g.,

class Ports(object):
    def __init__(device_port, host_port):
	self.device_port = device_port
	self.host_port = host_port

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:458
> +	   self._host_to_device_port_map = dict()

Nit: Why dict() rather than {}?  You use {} above.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:473
> +	   timeout_sec = 5

This looks like a constant. Maybe name it TIMEOUT_SECONDS or
KILL_TIMEOUT_SECONDS?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:505
> +	       # Failure

I would remove this comment.  It seems obvious from the exception being raised.


> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:522
> +	   self._host_process = pexpect.spawn(self._host_forwarder_path,
> +					      ['--adb_port=%s' % (
> +						  self._host_adb_control_port)]
+
> +					      forward_string)

The wrapping makes this hard for me to read. Maybe put it on a single line
(WebKit doesn't have an 80 col limit) or just do a 4 space indent rather than
trying to align with the open paren.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:536
> +		   # Success

I would remove this comment. It seems obvious from the lack of exception being
raised.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:542
> +		   # Failure

I would remove this comment.  It seems obvious from the exception being raised.


> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:563
> +	   elapsed = 0
> +	   wait_period = 0.1

Nit: elapsed_seconds and wait_period_seconds.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:577
> +	   elapsed = 0
> +	   wait_period = 0.1

Nit: elapsed_seconds and wait_period_seconds.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:599
> +    def close(self):
> +	   self._close_process()

Do we need both close and _close_process?  I would just have a single
close_process.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:96
> +    def _install_pexpect(self):
> +	   return
self._install("http://pexpect.sourceforge.net/pexpect-2.3.tar.gz",
> +				"pexpect-2.3")

It looks like pexpect is MIT licensed: http://www.noah.org/wiki/Pexpect#License

That means we can just check in the files rather than using autoinstall.  It'll
be much less flaky to check the files in.