Bug 78627

Summary: Implement an adb-based driver for the ChromiumAndroidPort
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: dpranke, eric, jnd, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 78524    
Attachments:
Description Flags
Patch
none
Patch eric: review+

Description Adam Barth 2012-02-14 13:09:57 PST
Implement an adb-based driver for the ChromiumAndroidPort
Comment 1 Adam Barth 2012-02-14 13:14:06 PST
Created attachment 127025 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-02-14 13:20:30 PST
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 3 Adam Barth 2012-02-14 13:32:18 PST
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.
Comment 4 Adam Barth 2012-02-14 13:33:07 PST
Created attachment 127032 [details]
Patch
Comment 5 Dirk Pranke 2012-02-14 16:00:33 PST
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?
Comment 6 Adam Barth 2012-02-14 16:26:02 PST
(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!
Comment 7 Adam Barth 2012-02-14 16:57:05 PST
> > > 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.
Comment 8 Adam Barth 2012-02-14 16:57:18 PST
(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.
Comment 9 Dirk Pranke 2012-02-14 17:00:16 PST
(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.
Comment 10 Adam Barth 2012-02-14 17:00:51 PST
Committed r107756: <http://trac.webkit.org/changeset/107756>