Bug 82033 – Make chromium android port use hardware gpu path default.

• Component: New Bugs
• Status: RESOLVED FIXED

[Hao Zheng]

Make chromium android port use hardware gpu path default.

[Hao Zheng]

Created attachment 133443 [details]
Patch

[Peter Beverloo]

Comment on attachment 133443 [details]
Patch

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

Thanks! Some drive-by nits.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:1
> +#!/usr/bin/env python

Do we invoke chromium_android.py directly? I don't think this is necessary if it's just being imported elsewhere.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:137
> +        'chromium-gpu',

These names correspond to directories in LayoutTests/platform/. All the chromium-gpu-* directories got removed in r110517.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:148
> +        # The chromium-android port always uses the hardware GPU code path,

textual nit: "The Chromium port for Android always uses the hardware GPU path."

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:237
> +        # All tests run in hardware gpu path, so don't need to run virtual gpu tests.

textual nit: "All tests are being run in the hardware GPU path, so there is no need to run virtual GPU tests."

[Adam Barth]

Comment on attachment 133443 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:149
> +        self._graphics_type = 'gpu'

Does graphics_type still exist?  It seems like it should be removed now that the GPU configuration no longer exists.

[Dirk Pranke]

Comment on attachment 133443 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:149
>> +        self._graphics_type = 'gpu'
> 
> Does graphics_type still exist?  It seems like it should be removed now that the GPU configuration no longer exists.

No, it doesn't still exist.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:238
> +        return []

To be more future-proof, you might want to call the base class and then remove the gpu suites, in case other suites get added down the road.

[Hao Zheng]

(In reply to comment #4)
> (From update of attachment 133443 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133443&action=review
> 
> >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:149
> >> +        self._graphics_type = 'gpu'
> > 
> > Does graphics_type still exist?  It seems like it should be removed now that the GPU configuration no longer exists.
> 
> No, it doesn't still exist.
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:238
> > +        return []
> 
> To be more future-proof, you might want to call the base class and then remove the gpu suites, in case other suites get added down the road.

I guess Adam is right. It's gone. We fall behind trunk some way, and apparently I need to rework this change. But anyway, I'd like to hear your opinion first on issues we come across.

As we always enable hardware gpu in our final build, we only need to run tests once in both real and virtual suites. 2 choices:
- Run these tests in real suites. Return [] for virtual suites. But we can not inherit baselines in platform/chromium-*/platform/chromium/virtual/gpu/, and can not reuse expectations like
BUGWK63933 : platform/chromium/virtual/gpu/fast/canvas/canvas-webkitLineDash.html = FAIL .

- Run these tests in virtual suites. This seems more natural to me. I can use skipped_tests() to skip these tests in real suites?

Which way looks better to you?

[Dirk Pranke]

(In reply to comment #5)
> As we always enable hardware gpu in our final build, we only need to run tests once in both real and virtual suites. 2 choices:
> - Run these tests in real suites. Return [] for virtual suites. But we can not inherit baselines in platform/chromium-*/platform/chromium/virtual/gpu/, and can not reuse expectations like
> BUGWK63933 : platform/chromium/virtual/gpu/fast/canvas/canvas-webkitLineDash.html = FAIL .
> 
> - Run these tests in virtual suites. This seems more natural to me. I can use skipped_tests() to skip these tests in real suites?
> 
> Which way looks better to you?

I think you are right that you should do whichever is likely to let you reuse more baselines and expectations. Running them as virtual suites -- which at least means they have the same "hardware accelerated" usage as the other ports -- seems perfectly reasonable to me.

Sorry for the inconvenience; it's unfortunate that you're trying to merge things at the same time we were reworking the gpu ports, but at least that's done and I don't foresee anything similar on the horizon that should affect you.

[Hao Zheng]

Created attachment 133989 [details]
Patch

[Hao Zheng]

(In reply to comment #2)
> (From update of attachment 133443 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133443&action=review
> 
> Thanks! Some drive-by nits.
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:1
> > +#!/usr/bin/env python
> 
> Do we invoke chromium_android.py directly? I don't think this is necessary if it's just being imported elsewhere.
> 

All scripts under webkitpy have such head. I just don't want android port to be an exception.

> > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:137
> > +        'chromium-gpu',
> 
> These names correspond to directories in LayoutTests/platform/. All the chromium-gpu-* directories got removed in r110517.
> 

Done.

> > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:148
> > +        # The chromium-android port always uses the hardware GPU code path,
> 

Done.

> textual nit: "The Chromium port for Android always uses the hardware GPU path."
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:237
> > +        # All tests run in hardware gpu path, so don't need to run virtual gpu tests.
> 
> textual nit: "All tests are being run in the hardware GPU path, so there is no need to run virtual GPU tests."

Removed.

[Hao Zheng]

(In reply to comment #7)
> Created an attachment (id=133989) [details]
> Patch

I use skipped_tests(), which is not used by chromium port, because I find adding a line like 'WONTFIX ANDROID SKIP : fast/canvas = PASS FAIL' is not enough. More specific rules like 'BUGWEBGL : fast/canvas/webgl/read-pixels-test.html = TIMEOUT' would override the general SKIP rule, so the tests are still run. I have to override again using specific platform rules like 'WONTFIX ANDROID SKIP : fast/canvas/webgl/read-pixels-test.html = TIMEOUT'. This is annoying because we have to add one SKIP rule for Android each time we add one rule for canvas tests.

[Dirk Pranke]

Comment on attachment 133989 [details]
Patch

I think using skipped_tests in this case is fine.

[WebKit Review Bot]

Comment on attachment 133989 [details]
Patch

Clearing flags on attachment: 133989

Committed r112368: <http://trac.webkit.org/changeset/112368>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Xianzhu Wang]

Comment on attachment 133989 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:1
> +#!/usr/bin/env python

Just FYI: I think abarth intentionally left this out in the first check-in. The magic is useless in a file that is not excutable itself.