[webkit-reviews] review granted: [Bug 78627] Implement an adb-based driver for the ChromiumAndroidPort : [Attachment 127025] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 78627: Implement an adb-based driver for the ChromiumAndroidPort
https://bugs.webkit.org/show_bug.cgi?id=78627

Attachment 127025: Patch
https://bugs.webkit.org/attachment.cgi?id=127025&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=127025&action=review


This is going to need further lovin.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:371
> +	   if len(tombstones) > 0:

I would have probably early-returned instead of indenting this long block.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:436
> +	       self._proc.stdout.read(2)

Should we assert that it is '# '?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:442
> +	       # When DumpRenderTree crashes, the Android debuggerd will stop
the

Oh, the debuggerd!

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:480
> +	       driver_output.error += self._port._get_last_stacktrace()

private method on the port?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:494
> +		   # The pipe has already been closed, indicating abnormal
> +		   # situation occurred. Wait a while to allow the device to
> +		   # recover.
> +		   time.sleep(1)

And cross your fingers?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:497
> +    def _test_shell_command(self, uri, timeoutms, checksum):

nit: I might hav enamed it timeout_ms instead.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:498
> +	   file_uri_preamble = 'file:///'

I believe python has url library code for dealign with file-urls and this
funtion doens't need to write its own. :)