Bug 78627 – Implement an adb-based driver for the ChromiumAndroidPort

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Implement an adb-based driver for the ChromiumAndroidPort

[Adam Barth]

Created attachment 127025 [details]
Patch

[Eric Seidel (no email)]

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. :)

[Adam Barth]

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.

[Adam Barth]

Created attachment 127032 [details]
Patch

[Dirk Pranke]

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?

[Adam Barth]

(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!

[Adam Barth]

> > > 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.

[Adam Barth]

(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.

[Dirk Pranke]

(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.

[Adam Barth]

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