| Summary: | Make chromium android port use hardware gpu path default. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Hao Zheng <zhenghao> | ||||||
| Component: | New Bugs | Assignee: | Hao Zheng <zhenghao> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | abarth, dpranke, ojan, peter, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Hao Zheng
2012-03-23 01:31:22 PDT
Created attachment 133443 [details]
Patch
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." 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. 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. (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? (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. Created attachment 133989 [details]
Patch
(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. (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. Comment on attachment 133989 [details]
Patch
I think using skipped_tests in this case is fine.
Comment on attachment 133989 [details] Patch Clearing flags on attachment: 133989 Committed r112368: <http://trac.webkit.org/changeset/112368> All reviewed patches have been landed. Closing bug. 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. |