Created attachment 127025 [details] Patch

Comment on attachment 127025 [details] Patch 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. :)

Comment on attachment 127025 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127025&action=review >> 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. Fixenated. >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:436 >> + self._proc.stdout.read(2) > > Should we assert that it is '# '? Done. >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:480 >> + driver_output.error += self._port._get_last_stacktrace() > > private method on the port? Fixed. >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:494 >> + time.sleep(1) > > And cross your fingers? Done >> 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. Done. >> 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. :) Do you have a particular one in mind? We could use http://docs.python.org/library/urlparse.html, but I'm not sure it would really buy us much. We're just doing a translation from between host and device URIs.

Created attachment 127032 [details] Patch

Comment on attachment 127032 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127032&action=review > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:504 > + uri = FILE_TEST_URI_PREFIX + uri[len(file_uri_preamble) + len(self._port.layout_tests_dir()):] Does uri = FILE_TEST_URI_PREFIX + self.uri_to_test(uri) work here (I think it should ...)? > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:507 > + def _write_command_and_read_line(self, input=None): Does it make sense to get rid of this function and add an _expected_uri() method to Driver that is used in ChromiumDriver.py on line 547 instead?

(In reply to comment #5) > (From update of attachment 127032 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127032&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:504 > > + uri = FILE_TEST_URI_PREFIX + uri[len(file_uri_preamble) + len(self._port.layout_tests_dir()):] > > Does uri = FILE_TEST_URI_PREFIX + self.uri_to_test(uri) work here (I think it should ...)? That seems likely. The only strange case would be HTTP tests, but those should be filtered out already. > > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:507 > > + def _write_command_and_read_line(self, input=None): > > Does it make sense to get rid of this function and add an _expected_uri() method to Driver that is used in ChromiumDriver.py on line 547 instead? I'll check. Thanks!

> > > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:507 > > > + def _write_command_and_read_line(self, input=None): > > > > Does it make sense to get rid of this function and add an _expected_uri() method to Driver that is used in ChromiumDriver.py on line 547 instead? > > I'll check. This definitely looks worthwhile, but I'm going to do it in a followup patch if that's ok with you because it requires some refactoring.

(In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 127032 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=127032&action=review > > > > > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:504 > > > + uri = FILE_TEST_URI_PREFIX + uri[len(file_uri_preamble) + len(self._port.layout_tests_dir()):] > > > > Does uri = FILE_TEST_URI_PREFIX + self.uri_to_test(uri) work here (I think it should ...)? > > That seems likely. The only strange case would be HTTP tests, but those should be filtered out already. Done.

(In reply to comment #7) > This definitely looks worthwhile, but I'm going to do it in a followup patch if that's ok with you because it requires some refactoring. Sure.

Committed r107756: <http://trac.webkit.org/changeset/107756>