[webkit-reviews] review granted: [Bug 78545] chromium_android.py should implement "virtual" methods from ChromiumPort : [Attachment 126852] Patch

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 78545: chromium_android.py should implement "virtual" methods from
ChromiumPort
https://bugs.webkit.org/show_bug.cgi?id=78545

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

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=126852&action=review


There should probably be a chromium_unittest.py file as well; a skeleton that
just subclasses port_testcase should be sufficient to get coverage.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:71
>      port_name = 'chromium-android'

Are we going to be assuming that 'android' is a new "operating system" value
(like linux/win/mac)? If not, 'chromium_android' might be a better name (much
like we have google_chrome or chromium_gpu, to indicate that the implementation
differs in ways other than just by operating system).

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:116
> +	   # This method is called by dump_render_tree_thread.py before testing


Nit: there is no dump_render_tree_thread.py any more; presumably you mean
manager.py, but you could probably omit this sentence completely and still get
the point across, e.g., 'we use an http server that is always running during
tests'.

That said, I'm curious ... do chromium/android devs ever run these tests
locally and/or interactively, and if so, how is the http server run then?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:125
> +    def start_helper(self):

Nit: is this actually needed? if not, you could probably remove the FIXME and
just note that it isn't.