[webkit-reviews] review denied: [Bug 46225] new-run-webkit-tests: add support for chromium gpu port, accelerated switches : [Attachment 68298] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 46225: new-run-webkit-tests: add support for chromium gpu port, accelerated
switches
https://bugs.webkit.org/show_bug.cgi?id=46225

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

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=68298&action=review

> LayoutTests/platform/chromium-gpu/test_expectations.txt:10
> +WONTFIX SKIP : accessibility = PASS FAIL
> +WONTFIX SKIP : animations = PASS FAIL
> +WONTFIX SKIP : canvas = PASS FAIL

FWIW, I would not bother explicitly skipping these since developers can do that
if they want.  Also, if there is a test outside of the compositing
subdirectories below, it'll be hard to manually run it.

> WebKitTools/ChangeLog:10
> +	   - support for the '--accelerated-compositing' and
> +	     'accelerated-2d-canvas' flags to new-run-webkit-tests (and the
> +	   'no-' flags)

Nit: indention is off

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu.py:39
> +    """Some tests have slightly different results when compiled as Google
> +    Chrome vs Chromium.  In those cases, we prepend an additional directory
to
> +    to the baseline paths."""

This docstring seems incorrect.

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu.py:41
> +    if port_name == 'chromium-gpu':
> +	   if sys.platform == 'win32' or sys.platform == 'cygwin':

Nit: sys.platform in ('win32', 'cygwin')

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu.py:54
> +    elif port_name.startswith('chromium-gpu-mac'):
> +	   return ChromiumGpuMacPort('chromium-gpu-mac', options)

Why does mac pass in the port name but the other ports don't?  Also, I thought
webkit style was to just use 'if' in the case of an early return.

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu.py:71
> +    try:
> +	   overrides_path = port.path_from_chromium_base('webkit', 'tools',
> +	       'layout_tests', 'test_expectations_gpu.txt')

Do we plan on initially running with test_shell?  If we only support DRT, then
we don't need to have an override file.

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu.py:99
> +    def __init__(self, port_name=None, options=None, **kwargs):
> +	   _set_gpu_options(options)

Is it possible to make options a required param (and we wouldn't have to test
for it in _set_gpu_options)?  Do we need **kwargs (you don't use them)?

> WebKitTools/Scripts/webkitpy/layout_tests/port/factory_unittest.py:138
> +    def test_chromium_gpu_linux(self):
> +	   self.assert_port("chromium-gpu-linux",
chromium_gpu.ChromiumGpuLinuxPort)

Can you add some tests that verify that the fallback works as expected?  What
about testing to see if the test_expectations.txt override works?

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1515
> +	   optparse.make_option("--no-accelerated-compositing",
> +	       action="store_false",
> +	       help="Don't use hardware-accelated compositing for rendering"),

Don't you need dest='accelerated-compositing' for the no- options?

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1521
> +	   optparse.make_option("--no-accelerated-2d-canvas",
> +	       action="store_false",
> +	       help="Don't use hardware-accelated 2D Canvas calls"),

dest='accelerated-2d-canvas'