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

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Peter Beverloo <peter at chromium.org>  2012-10-24 04:34:20 PST ---
(From update of attachment 170105)
View in context: https://bugs.webkit.org/attachment.cgi?id=170105&action=review

Hi Felipe, thank you for the patch! Here are some comments.

Dirk, Adam, I proposed moving the AndroidCommands and Forwarder classes to separate files, as these will be shared for running w_u_t and TWA as well, and this file is becoming way too bloated. But I'm not sure where it should be.

> Tools/ChangeLog:1
> +2012-10-23  Felipe Goldstein  <felipeg at google.com>

nit: Your Bugzilla account says @chromium.org.

> Tools/ChangeLog:3
> +        Use the new forwarder2

I'm afraid that the rest of the WebKit community won't know what "forwarder2" is: it's a highly Android-specific term. In general, for platform specific code such as this, we'd use a prefix such as [Chromium] to tell other vendors that this doesn't concern their ports. Maybe a title such as "[Chromium] Use the new forwarder2 for running layout tests on Android" would work better?

> Tools/ChangeLog:5
> +

The "Reviewed by NOBODY (OOPS!)." line needs to be here.
Furthermore, commit messages in WebKit tend to be quite verbose. Could you add one or two paragraphs explaining *why* we are switching to forwarder2?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:33
> +import pexpect

I don't think we can rely on the pexpect library to be available on all ports, as the cr-linux failure indicates. Chromium has the dependency in third_party/pexpect for that reason, and loads it through this file: src/build/android/pylib/pexpect.py. Since it's MIT licensed and WebKit only accepts LGPL and BST, maybe it's an option to pull it in through Tools/Scripts/webkitpy/thirdparty/__init__.py?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:166
> +        'mac',

This seems unintentional, Chromium no longer falls back to mac baselines.

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

nit: inherit from object? Other classes seem to do that.

I think it'd be worthwhile to split AndroidCommands (and forwarder?) to their own files, removing all code related to them from this file. We'll also need to use them in order to be able to run webkit_unit_tests and TestWebKitAPI. I'm not entirely sure about the ideal directory, maybe Tools/Scripts/webkitpy/common/system/? One worry I have here is that the forwarder is only available for Chromium, which kind of would be a layering violation there...

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:354
> +    def push_md5sum(self):

Should we maybe have a setup_device() method which also executes _setup_performance (and, maybe, a teardown_device() method for _teardown_performance)? Removing all device-logic from the ChromiumAndroidDriver seems preferable. We may be able to address this in later patches..

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:398
> +        return self._port

The Forwarder class only needs the path to the forwarders, exposing the whole Port seems a bit excessive.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:414
> +    _TIMEOUT_SECS = 30

The mixture between global constants and class-local ones is a bit strange. Is there a reason we have both?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:419
> +        Works like adb forward, but in reverse.

I think it'd be fine to remove this line, as 417 already covers what it does.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:422
> +            adb: Instance of AndroidCommands for talking to the device.

nit: the argument is named android_cmd.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:423
> +            port_pairs: A list of tuples (device_port, host_port) to forward. Note

nit: worker_number is missing. Maybe we should remove the argument explanation altogether?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:448
> +        forward_string = ['%d:%d:%s' %

nit: this would fit on a single line.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:450
> +        logging.info('Forwarding ports: %s', forward_string)

Should this be str(forward_string)?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:454
> +        # TODO(felipeg): Rather than using a blocking kill() here, the device

nit: WebKit uses unattributed FIXMEs.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:518
> +                logging.info("Forwarding device port: %d to host port: %d." %

nit: this could fit on one line (no line length limits).

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:525
> +                raise Exception('Failed to forward port %d to %d' % (device_port,

nit: dito.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:530
> +                raise Exception('Unexpected EOF while trying to forward ports %s' %

nit: dito.

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

pkill and pgrep are not available on Mac systems.. Ideally we'd be able to run layout tests on any platform, not just Linux. Not a huge issue now, though.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:545
> +        while not self._android_cmd.run_host_command(['pgrep', '-f', host_pattern]) and (

Not splitting the line would make this more readable.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:550
> +                raise Exception('Timed out while killing ' + host_pattern)

nit: indenting.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:560
> +        while self._android_cmd.run_adb_command(['shell', 'ps']).find(

Not splitting the line would make this more readable.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:565
> +                raise Exception('Timed out while killing device_forwarder.')

nit: indenting.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:579
> +        """Get the device port that corresponds to a given host port."""

We don't need this comment, the name already describes it :-).

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:583
> +        """Terminate the forwarder process."""

We don't need this comment, the name already describes it :-).

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:602
> +        self._android = AndroidCommands(port, worker_number)

The AndroidCommands instance is named "_android_cmd" in the Forwarder class, but "_android" here. Being more verbose is a good thing, so I'd prefer using "_android_cmd" (or maybe even "_android_commands") for both.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:780
> +        self._forwarder = Forwarder(self._android, self._worker_number, port_pairs, '127.0.0.1')

Should there be a try/catch here in case the Forwarder.__init__ throws?

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