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

Wed Oct 24 10:25:58 PDT 2012

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

--- Comment #4 from felipe <felipeg at chromium.org>  2012-10-24 10:27:04 PST ---
(From update of attachment 170105)
View in context: https://bugs.webkit.org/attachment.cgi?id=170105&action=review

>> Tools/ChangeLog:1
>> +2012-10-23  Felipe Goldstein  <felipeg at google.com>
> 
> nit: Your Bugzilla account says @chromium.org.

done

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

done

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

done

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

dpne

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:166
>> +        'mac',
> 
> This seems unintentional, Chromium no longer falls back to mac baselines.

done

>> 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...

done

>> 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..

done

>> 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.

done

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

I dont like the global constants. It has no reason be global, but I didn't want to change that, and since this is the pattern I found in the file, I will keep it and move my local constants to the global namespace.

>> 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.

done

>> 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.

done

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

The argument explanation came from Chromium code style requirement. Should I remove ?

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:448
>> +        forward_string = ['%d:%d:%s' %
> 
> nit: this would fit on a single line.

done

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

done

>> 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.

done

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

done

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

done

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

done

>> 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.

But I thought this was meant to be used with android.
The file is called chromium_android.py and everything is related to running the tests on android devices, which should happen on linux.
If we try to run it from a Mac or Windows we have a much bigger problem. For example, the forwarder binary only works on linux.

>> 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.

done

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

done

>> 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.

done

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

done

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

done

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

done

>> 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.

done

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

The exceptions from the __init__ in the Forwarder are quite clear and it is preferred to fail and show the exception from there instead of catching it here.

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