[Webkit-unassigned] [Bug 99588] Use forwarder2 in Chrome for Android layout tests.

[bugzilla-daemon at webkit.org]

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





--- Comment #46 from Peter Beverloo <peter at chromium.org>  2012-12-19 03:57:26 PST ---
(From update of attachment 179921)
View in context: https://bugs.webkit.org/attachment.cgi?id=179921&action=review

Hi Philippe, a few more comments. I tested your patch and verified that it works both for layout tests and for performance tests. Thanks!

For reference's sake, when are you going on holiday?

> Tools/ChangeLog:5
> +

This needs the "Reviewed by NOBODY (OOPS!)." line, otherwise it won't be able to pass through the commit queue.

> Tools/ChangeLog:-397
> -

Accidental change?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:679
> +            self._port.host.filesystem, self._port._executive.run_command, self._adb_command(), self._port._build_path, self._log_debug)

This means that it'll be required for the user to either have "adb" installed or have a Chromium WebKit checkout if they want to instantiate ChromiumAndroidDriver. This is why the _adb_command_base property exists, so it can be determined lazily.

Can we remove ChromiumAndroidDriver._adb_command() and ChromiumAndroidDriver._adb_command_base altogether and move this logic to AndroidCommands?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:730
> +        self._android_commands.setup_md5sum()

Regarding Eric's plans to have a more generic DeviceConnection class, it may make sense to have an intermediate class to encapsulate md5sum/push_if_needed... Not sure, something to address later, though I agree Eric's abstraction is cleaner.

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

Can we perhaps just call these in _setup_test()? It doesn't really add much to have a separate method.

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