Bug 46225 – new-run-webkit-tests: add support for chromium gpu port, accelerated switches

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

new-run-webkit-tests: add support for chromium gpu port, accelerated switches

[Dirk Pranke]

Created attachment 68295 [details]
Patch

[Dirk Pranke]

(Details in the ChangeLogs for what is entailed here ...)

[Dirk Pranke]

Created attachment 68298 [details]
Patch

[Dirk Pranke]

update with what is hopefully the correct set of tests to skip and to run in LayoutTests/chromium-gpu/test_expectations.txt.

[Dirk Pranke]

Note that I have not yet modified the buildbots to run the tests in an additional step yet. I'd like to land this patch and have people get at least a little experience running this and getting correct expectations in before we modify the bots.

[Tony Chang]

Comment on attachment 68298 [details]
Patch

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'

[Stephen White]

Comment on attachment 68298 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68298&action=review

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1512
> +            help="Use hardware-accelated compositing for rendering"),

Nit:  accelated -> accelerated.

>> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1515
>> +            help="Don't use hardware-accelated compositing for rendering"),
> 
> Don't you need dest='accelerated-compositing' for the no- options?

And here.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1518
> +            help="Use hardware-accelated 2D Canvas calls"),

And here.

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

And here.

[Dirk Pranke]

Thanks for the review ... you found some good things.

(In reply to comment #6)
> (From update of attachment 68298 [details])
> 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.
> 

You can add specific files outside of the directories below to the expectations file, or you can specify them on the command line and add --force.

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

fixed.

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

fixed

> > 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')
> 

fixed.

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

Whoops. Fixed (they shouldn't pass the port names).

>  Also, I thought webkit style was to just use 'if' in the case of an early return.
>

It is, but I'll never get used to it :) Fixed.
 
> > 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.

We want to be able to test both test_shell and DRT.

> 
> > 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)?

Yes, but I don't want to do this in this CL (there's a bunch of other CLs in flight cleaning this stuff up).

>  Do we need **kwargs (you don't use them)?

Depending on when this patch lands, yes, we'll need them and they'll get passed to the base class. This patch was in a weird halfway-in-between state. I'll remove them for now and update the patch when the other patch lands, making them necessary.

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

Will do. My initial thinking was that the tests wouldn't be testing anything interesting (I'd be just as likely to get the test wrong as get the code wrong), but I"ll add them since you asked.

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

Yes. Good catch.

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

Ditto.

[Dirk Pranke]

Created attachment 68323 [details]
Patch

[Dirk Pranke]

Created attachment 68331 [details]
Patch

[Dirk Pranke]

Created attachment 68335 [details]
Patch

[Tony Chang]

Comment on attachment 68335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68335&action=review

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu.py:45
> +        if sys.platform in ('cygwin', 'win32'):
> +            port_name = 'chromium-gpu-win'
> +        elif sys.platform == 'linux2':
> +            port_name = 'chromium-gpu-linux'

python pro-tip: this could be rewritten as:
plaforms = {
  'cygwin': '-win',
  'win32': '-win',
  'linux2': '-linux',
  'darwin': '-mac',
}
if sys.platform not in platforms:
    raise NotImplementedError('...')
port_name += platforms[sys.platform]

[Dirk Pranke]

(In reply to comment #12)
> (From update of attachment 68335 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68335&action=review
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu.py:45
> > +        if sys.platform in ('cygwin', 'win32'):
> > +            port_name = 'chromium-gpu-win'
> > +        elif sys.platform == 'linux2':
> > +            port_name = 'chromium-gpu-linux'
> 
> python pro-tip: this could be rewritten as:
> plaforms = {
>   'cygwin': '-win',
>   'win32': '-win',
>   'linux2': '-linux',
>   'darwin': '-mac',
> }
> if sys.platform not in platforms:
>     raise NotImplementedError('...')
> port_name += platforms[sys.platform]

Thanks! Yeah, I was going to add something like this as part of the next patch.

[Dirk Pranke]

Comment on attachment 68335 [details]
Patch

Clearing flags on attachment: 68335

Committed r68063: <http://trac.webkit.org/changeset/68063>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.

[Ojan Vafai]

Comment on attachment 68335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68335&action=review

Belated drive-by comment...

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

Based off the comment above, I'd expect we would run these tests. Is the comment wrong?

[Dirk Pranke]

(In reply to comment #16)
> (From update of attachment 68335 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68335&action=review
> 
> Belated drive-by comment...
> 
> > LayoutTests/platform/chromium-gpu/test_expectations.txt:10
> > +WONTFIX SKIP : canvas = PASS FAIL
> 
> Based off the comment above, I'd expect we would run these tests. Is the comment wrong?

Yeah, I wrote the comment before I realized we only want to run some of the canvas bugs. I'll update it; good catch!