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

Wed Dec 5 05:07:51 PST 2012

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

--- Comment #31 from Philippe Liard <pliard at chromium.org>  2012-12-05 05:10:17 PST ---
(From update of attachment 177474)
View in context: https://bugs.webkit.org/attachment.cgi?id=177474&action=review

>> Tools/ChangeLog:3
>> +        Use forwarder2 in Chrome for Android layout tests.
> 
> Please also update the bug's title to reflect this change.

I will see if Felipe can do it. I don't think I can given that he created the bug.

>> Tools/ChangeLog:5
>> +
> 
> Please add a description to the ChangeLog. Why is this change necessary? What kind of impact on functionality does it have, etcetera.

Done.

>> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:114
>> +            self._port.pre_sharding_init()
> 
> Can we stop the host forwarder as part of Port.setup_test_run()? The consequence would be that it won't be run separately for the failed tests' second run.

Nice, thanks. I wasn't aware of this method. My only constraint was to kill the host daemon only once before sharding happens.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:377
>> +        self._adb_command = ['adb', '-s', device_serial]
> 
> Did you rebase before uploading this change? We no longer require 'adb' to be in the path, and a utility method was added to get the path to use (ChromiumAndroidDriver.path_to_adb()).

Done. Good catch.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:384
>> +                raise AssertionError('Could not push md5sum to device')
> 
> nit: to THE device.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:389
>> +    def run_command(self, cmd):
> 
> This is only used in the AndroidCommands class, you should prefix it with _ to indicate visibility.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:391
>> +        process = subprocess.Popen(cmd, stderr=subprocess.STDOUT, stdout=subprocess.PIPE)
> 
> You should use the host's executive here.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:396
>> +        _log.debug('Run adb result: ' + str(output[:80]))
> 
> The worry I have here is that the current logging output includes the device's serial for the output. We'd loose that coverage here.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:414
>> +                self.run_command(['%s_host' % self._md5sum_path, host_file]))
> 
> Would it be worth to assert for making sure that _md5sum_path has been setup (i.e. setup_md5sum() must have been invoked)?

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:427
>> +    DEVICE_FORWARDER_PATH = '/data/local/tmp/device_forwarder'
> 
> Could we just append "device_forwarder" to DEVICE_SOURCE_ROOT_DIR?

Done. Good point.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:432
>> +        self._device_forwarder_path = path_builder('device_forwarder')
> 
> Since there also is Forwarder.DEVICE_FORWARDER_PATH, I'd prefer to clarify here that it's the host path.

Done. I renamed path_builder to host_path_builder.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:433
>> +        self._host_forwarder_path = path_builder('host_forwarder')
> 
> For consistency's sake, maybe invoke Forwarder.path_to_{device,host}_forwarder()?

Done. Good point.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:439
>> +        _log.debug('Killing host daemon ')
> 
> nit: ends with a space.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:447
>> +        _log.debug('Killing device daemon ')
> 
> nit: ends with a space.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:451
>> +                '%s shell %s kill-server' % (Forwarder._get_adb_command_string(android_cmd), Forwarder.DEVICE_FORWARDER_PATH))
> 
> It wouldn't be hard to convert either of the calls to _run_shell_command_and_get_exit_code() to use a list of arguments instead of a string, while removing the need for Forwarder._get_adb_command_string().

Done. Good point.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:453
>> +            raise AssertionError('Could not kill device_forwarder:\n%s' % output)
> 
> nit: You print output on a new line here, but not in the other error messages (lines 474, 479, 485).

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:456
>> +        assert(FORWARDER_CONTROL_PORT_BEGIN + worker_number < Forwarder.ADB_CONTROL_PORT)
> 
> nit: no need for the parenthesis.

Done. I removed this assert.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:458
>> +        adb_control_port = self._worker_number_to_adb_control_port_map.get(worker_number)
> 
> adb_control_port is something entirely different than Forwarder.ADB_CONTROL_PORT. Could you please use a clearer name?

Done. Indeed.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:465
>> +        self._android_cmd.push_file_if_needed(self._device_forwarder_path, Forwarder.DEVICE_FORWARDER_PATH)
> 
> Again: distinguishing between the names for host and device paths would make this significantly clearer.

I noticed that I forgot to address this point. I will come back to it in a next patch set.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:466
>> +        # Command format: <ADB port>:<Device port>:<Host port>:<Host address>
> 
> nit: could you add a blank line before this command and lowercase the parts (/s/Device/device etc)? That'd make it a lot easier to read.
> "ADB port" should probably read "ADB control port".

Done. I also moved this part to a more appropriate place (right before it's used).

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:471
>> +                        Forwarder._get_adb_command_string(self._android_cmd), adb_control_port, Forwarder.ADB_CONTROL_PORT)
> 
> You seem to indent continuations with either four, eight or an arbitrary amount of spaces. There is no line length limit in WebKit, if you do decide to wrap a line (for example, to improve readability), please indent with four spaces.

Indeed, thanks.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:489
>> +        while port < FORWARDER_CONTROL_PORT_END and self._is_port_used(port):
> 
> Theoretically, with a thousand control ports, we'd open the /proc/net/tcp an equal amount of times. Since _is_port_used() is only used for this purpose, wouldn't it be easier to read /proc/net/tcp once and then find the first available port between [FORWARDER_CONTROL_PORT_BEGIN + worker_number, FORWARDER_CONTROL_PORT_END]?

Good point. Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:497
>> +        for line in open('/proc/net/tcp').xreadlines():
> 
> Instead of just calling open(), could we go through webkitpy.common.system.filesystem?

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:506
>> +                _log.debug('  Port %d is used.' % host_port)
> 
> nit: Why are these outputs indented?

No obvious reason. This was taken from Chromium.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:516
>> +    def _run_shell_command_and_get_exit_code(cmd):
> 
> Since Executive.run_command() knows both the output and exit code, it would be a lot easier to add an return_exit_code_and_output argument. That'd be three lines of code, as opposed to bypassing the entire executive.

Good point. Note that I still need the '; echo *$?' trick used below since adb shell doesn't return the exit code from the command which was run on the device.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:749
>> +        forwarder = Forwarder(self._port._build_path, self._android_cmd)
> 
> Is forwarder intentionally not being stored?

Yes we don't need to store it. I can make setup_redirections a static method if you prefer. What do you think?

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:750
>> +        forwarder.setup_redirections(self._worker_number, [(port, port) for port in FORWARD_PORTS], '127.0.0.1')
> 
> If host_address is always 127.0.0.1 we shouldn't pass it as an argument.

Done.

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