[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 #42 from Eric Seidel <eric at webkit.org>  2012-12-17 14:01:44 PST ---
(From update of attachment 179740)
View in context: https://bugs.webkit.org/attachment.cgi?id=179740&action=review

I think you and I should compare notes.  Are you reachable on #webkit?  I'm eseidel there.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:39
> +from webkitpy.common.system.filesystem import FileSystem

Including this is rarely correct. :)

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:529
> +        assert os.path.exists(host_file)

Please try to use filesystem.exists instead of os.path.exists so you can mock/unittest your funtions more easily.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:547
> +class Forwarder:

All classes should inherit from object in modern python. :)  (At least that's my understanding.)

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:570
> +    @staticmethod
> +    def kill_host_daemon(command_executor, host_path_builder):

Do you reall want these static?  I find @staticmethod to be deceptive in python.  Intially I think "oh, this doesn't need any members" so I make something static... and then I realize it's not easily mockable and cry.  Even @classmethods are more mockable than statics. :)

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:624
> +        for line in FileSystem().open_text_file_for_reading('/proc/net/tcp').xreadlines():

Bad.  This makes this method unmockable.  Are you sure you can't pass in a filesystem or Host?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:651
> +    @staticmethod

Generally I've found a single shared instance works out better than a bunch of static methods.  You could split these off into a singleton if that would work better?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:774
> +        install_result = self._android_commands.run_adb_command(['install', drt_host_path])

In my version of this I named this DeviceConnection with the idea that we might some day make it generic not just for android.  That may have been over-design on my part.  But I do think the idea of teaching a "hosted" driver how to talk to the target is generic and not necessarily specific to ADB or android.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:914
> +        forwarder = Forwarder(self._port._executive.run_command, self._port._build_path, self._android_commands)

Why not just pass it an executive instead of a run_command method pointer?

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