[Webkit-unassigned] [Bug 99588] Use the new forwarder2

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=99588


Tony Chang <tony at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #170423|review?                     |review-
               Flag|                            |




--- Comment #7 from Tony Chang <tony at chromium.org>  2012-10-24 11:56:31 PST ---
(From update of attachment 170423)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.