Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(129)

Issue 2737983002: WebGL feature will only enabled when accelerated (Closed)

Created:
3 years, 9 months ago by sugoi1
Modified:
3 years, 9 months ago
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, jam, darin-cc_chromium.org, piman+watch_chromium.org, extensions-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

This is necessary for tests that wish to disable WebGL A few different tests are blacklisting the WebGL GPU feature. This will no longer work when SwiftShader is fully integrated in chromium, as it will become a fallback for GPU accelerated WebGL, and the WebGL feature will stay enabled. In order to fix this issue, the WebGL feature will now only be enabled when accelerated. When SwiftShader is used, these features will now be disabled. A new utility function, IsWebGLEnabled(), which checks for both swiftshader and the WebGL feature, was added in order to verify if WebGL can actually be used by chromium. BUG=630728 R=rdevlin.cronin@chromium.org, jbauman@chromium.org, zmo@chromium.org, piman@chromium.org TBR=laforge@chromium.org TBR_REASON=Trivial change to swiftshader_component_installer.cc, which will soon be deleted entirely CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2737983002 Cr-Commit-Position: refs/heads/master@{#456405} Committed: https://chromium.googlesource.com/chromium/src/+/9e3f185360c78d129af6a5fbc4e0515c3b2f6793

Patch Set 1 #

Patch Set 2 : Added SwiftShader as a GPU feature #

Total comments: 7

Patch Set 3 : Removed kDisableSoftwareRasterizer from disabling SwiftShader #

Patch Set 4 : Restored kDisableSoftwareRasterizer for disabling SwiftShader #

Total comments: 2

Patch Set 5 : WebGL and WebGL2 features will only enabled when accelerated #

Total comments: 4

Patch Set 6 : Reverted WebGL2 change and renamed wegl -> accelerated_webgl #

Total comments: 1

Patch Set 7 : Removed change to requirements_info.cc #

Total comments: 2

Patch Set 8 : Fixed SwiftShader tests #

Patch Set 9 : Formatting fix #

Unified diffs Side-by-side diffs Delta from patch set Stats (+92 lines, -73 lines) Patch
M chrome/browser/component_updater/swiftshader_component_installer.cc View 1 2 3 4 5 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/extensions/api/webstore_private/webstore_private_api.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/extensions/chrome_requirements_checker.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/extensions/extension_service_unittest.cc View 1 2 3 4 5 1 chunk +10 lines, -10 lines 0 comments Download
M chrome/browser/extensions/requirements_checker_browsertest.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M content/browser/gpu/compositor_util.cc View 1 2 3 4 5 3 chunks +12 lines, -10 lines 0 comments Download
M content/browser/gpu/gpu_data_manager_impl.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/gpu/gpu_data_manager_impl.cc View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M content/browser/gpu/gpu_data_manager_impl_private.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/gpu/gpu_data_manager_impl_private.cc View 1 2 3 4 5 8 chunks +17 lines, -18 lines 0 comments Download
M content/browser/gpu/gpu_data_manager_impl_private_unittest.cc View 1 2 3 4 5 6 7 8 16 chunks +25 lines, -16 lines 0 comments Download
M content/public/browser/gpu_data_manager.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M gpu/config/gpu_blacklist.cc View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M gpu/config/gpu_blacklist_unittest.cc View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M gpu/config/gpu_feature_type.h View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M gpu/config/software_rendering_list_json.cc View 1 2 3 4 5 8 chunks +8 lines, -8 lines 0 comments Download

Messages

Total messages: 75 (29 generated)
sugoi1
Hi Devlin, I looked into the solution we talked about yesterday, but it looked a ...
3 years, 9 months ago (2017-03-08 19:06:48 UTC) #1
sugoi1
Adding zmo@ to get his opinion on the fix.
3 years, 9 months ago (2017-03-08 19:34:51 UTC) #4
Zhenyao Mo
On 2017/03/08 19:34:51, sugoi1 wrote: > Adding zmo@ to get his opinion on the fix. ...
3 years, 9 months ago (2017-03-08 21:09:07 UTC) #5
Zhenyao Mo
On 2017/03/08 21:09:07, Zhenyao Mo wrote: > On 2017/03/08 19:34:51, sugoi1 wrote: > > Adding ...
3 years, 9 months ago (2017-03-08 21:10:36 UTC) #6
sugoi1
On 2017/03/08 21:10:36, Zhenyao Mo wrote: > On 2017/03/08 21:09:07, Zhenyao Mo wrote: > > ...
3 years, 9 months ago (2017-03-08 21:19:02 UTC) #7
Devlin
This is a little different than I had originally envisioned, but I'm fine with this ...
3 years, 9 months ago (2017-03-08 21:23:38 UTC) #8
sugoi1
On 2017/03/08 21:09:07, Zhenyao Mo wrote: > On 2017/03/08 19:34:51, sugoi1 wrote: > > Adding ...
3 years, 9 months ago (2017-03-09 16:16:00 UTC) #11
sugoi1
https://codereview.chromium.org/2737983002/diff/20001/chrome/browser/extensions/extension_service_unittest.cc File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/2737983002/diff/20001/chrome/browser/extensions/extension_service_unittest.cc#newcode643 chrome/browser/extensions/extension_service_unittest.cc:643: "{\n" Note: Those whitespace changes are from running 'git ...
3 years, 9 months ago (2017-03-09 16:17:34 UTC) #12
piman
Should we rename "webgl" as "accelerated_webgl" (similar to accelerated_2d_canvas, etc.)? It'd be super confusing if ...
3 years, 9 months ago (2017-03-09 18:31:34 UTC) #16
sugoi1
On 2017/03/09 18:31:34, piman - On leave - No reviews wrote: > Should we rename ...
3 years, 9 months ago (2017-03-09 18:49:32 UTC) #17
Zhenyao Mo
On 2017/03/09 18:49:32, sugoi1 wrote: > On 2017/03/09 18:31:34, piman - On leave - No ...
3 years, 9 months ago (2017-03-09 18:53:30 UTC) #18
Zhenyao Mo
https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu_data_manager_impl_private.cc File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu_data_manager_impl_private.cc#newcode939 content/browser/gpu/gpu_data_manager_impl_private.cc:939: switches::kDisableSoftwareRasterizer)) { Consider this situation: on a system where ...
3 years, 9 months ago (2017-03-09 19:18:03 UTC) #19
sugoi1
https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu_data_manager_impl_private.cc File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu_data_manager_impl_private.cc#newcode939 content/browser/gpu/gpu_data_manager_impl_private.cc:939: switches::kDisableSoftwareRasterizer)) { On 2017/03/09 19:18:03, Zhenyao Mo wrote: > ...
3 years, 9 months ago (2017-03-09 19:24:33 UTC) #20
sugoi1
I looked into adding gpu::GPU_FEATURE_TYPE_WEBGL_ACCELERATED, but that adds some complexity to regular gpu::GPU_FEATURE_TYPE_WEBGL checks. Also, ...
3 years, 9 months ago (2017-03-09 20:07:12 UTC) #21
sugoi1
https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu_data_manager_impl_private.cc File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu_data_manager_impl_private.cc#newcode939 content/browser/gpu/gpu_data_manager_impl_private.cc:939: switches::kDisableSoftwareRasterizer)) { On 2017/03/09 20:07:12, sugoi1 wrote: > On ...
3 years, 9 months ago (2017-03-09 20:17:07 UTC) #24
Zhenyao Mo
On 2017/03/09 20:07:12, sugoi1 wrote: > I looked into adding gpu::GPU_FEATURE_TYPE_WEBGL_ACCELERATED, but that adds some ...
3 years, 9 months ago (2017-03-09 21:06:56 UTC) #25
Zhenyao Mo
On 2017/03/09 21:06:56, Zhenyao Mo wrote: > On 2017/03/09 20:07:12, sugoi1 wrote: > > I ...
3 years, 9 months ago (2017-03-09 21:10:45 UTC) #26
sugoi1
On 2017/03/09 21:06:56, Zhenyao Mo wrote: > On 2017/03/09 20:07:12, sugoi1 wrote: > > I ...
3 years, 9 months ago (2017-03-09 21:12:05 UTC) #27
Zhenyao Mo
On 2017/03/09 21:10:45, Zhenyao Mo wrote: > On 2017/03/09 21:06:56, Zhenyao Mo wrote: > > ...
3 years, 9 months ago (2017-03-09 21:12:54 UTC) #28
Zhenyao Mo
On Thu, Mar 9, 2017 at 1:12 PM, <sugoi@chromium.org> wrote: > On 2017/03/09 21:06:56, Zhenyao ...
3 years, 9 months ago (2017-03-09 21:15:16 UTC) #29
sugoi1
On 2017/03/09 21:15:16, Zhenyao Mo wrote: > On Thu, Mar 9, 2017 at 1:12 PM, ...
3 years, 9 months ago (2017-03-09 21:18:05 UTC) #30
Zhenyao Mo
On 2017/03/09 21:18:05, sugoi1 wrote: > On 2017/03/09 21:15:16, Zhenyao Mo wrote: > > On ...
3 years, 9 months ago (2017-03-09 21:31:05 UTC) #31
sugoi1
On 2017/03/09 21:31:05, Zhenyao Mo wrote: > On 2017/03/09 21:18:05, sugoi1 wrote: > > On ...
3 years, 9 months ago (2017-03-09 21:47:25 UTC) #32
Devlin
extensions lgtm
3 years, 9 months ago (2017-03-09 22:04:44 UTC) #33
jbauman
https://codereview.chromium.org/2737983002/diff/60001/gpu/config/gpu_blacklist.cc File gpu/config/gpu_blacklist.cc (right): https://codereview.chromium.org/2737983002/diff/60001/gpu/config/gpu_blacklist.cc#newcode45 gpu/config/gpu_blacklist.cc:45: list->AddSupportedFeature("swiftshader", GPU_FEATURE_TYPE_SWIFTSHADER); Won't this cause swiftshader to be blacklisted ...
3 years, 9 months ago (2017-03-09 22:08:40 UTC) #34
piman
On 2017/03/09 21:31:05, Zhenyao Mo wrote: > On 2017/03/09 21:18:05, sugoi1 wrote: > > On ...
3 years, 9 months ago (2017-03-09 22:21:00 UTC) #35
Zhenyao Mo
Per discussion with ericrk, he agrees kDisableSoftwareRasterization is not actually related to rasterizer and should ...
3 years, 9 months ago (2017-03-09 22:33:15 UTC) #37
jbauman
On 2017/03/09 22:33:15, Zhenyao Mo wrote: > Per discussion with ericrk, he agrees kDisableSoftwareRasterization is ...
3 years, 9 months ago (2017-03-09 22:36:59 UTC) #38
Zhenyao Mo
On 2017/03/09 22:36:59, jbauman wrote: > On 2017/03/09 22:33:15, Zhenyao Mo wrote: > > Per ...
3 years, 9 months ago (2017-03-09 22:57:32 UTC) #39
ericrk
On 2017/03/09 22:57:32, Zhenyao Mo wrote: > On 2017/03/09 22:36:59, jbauman wrote: > > On ...
3 years, 9 months ago (2017-03-09 23:06:58 UTC) #40
jbauman
On 2017/03/09 23:06:58, ericrk wrote: > On 2017/03/09 22:57:32, Zhenyao Mo wrote: > > On ...
3 years, 9 months ago (2017-03-09 23:11:48 UTC) #41
sugoi1
On 2017/03/09 23:11:48, jbauman wrote: > On 2017/03/09 23:06:58, ericrk wrote: > > On 2017/03/09 ...
3 years, 9 months ago (2017-03-10 01:42:52 UTC) #42
sugoi1
On 2017/03/09 22:21:00, piman - On leave - No reviews wrote: > On 2017/03/09 21:31:05, ...
3 years, 9 months ago (2017-03-10 01:52:53 UTC) #43
sugoi1
https://codereview.chromium.org/2737983002/diff/60001/gpu/config/gpu_blacklist.cc File gpu/config/gpu_blacklist.cc (right): https://codereview.chromium.org/2737983002/diff/60001/gpu/config/gpu_blacklist.cc#newcode45 gpu/config/gpu_blacklist.cc:45: list->AddSupportedFeature("swiftshader", GPU_FEATURE_TYPE_SWIFTSHADER); On 2017/03/09 22:08:39, jbauman wrote: > Won't ...
3 years, 9 months ago (2017-03-10 02:09:10 UTC) #46
sugoi1
On 2017/03/10 01:52:53, sugoi1 wrote: > On 2017/03/09 22:21:00, piman - On leave - No ...
3 years, 9 months ago (2017-03-10 16:28:34 UTC) #49
sugoi1
Implemented Antoine's suggestion. I added a new function, IsWebGLEnabled(), which needs to be checked, instead ...
3 years, 9 months ago (2017-03-10 20:17:58 UTC) #51
piman
https://codereview.chromium.org/2737983002/diff/80001/content/browser/gpu/gpu_data_manager_impl_private.cc File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2737983002/diff/80001/content/browser/gpu/gpu_data_manager_impl_private.cc#newcode836 content/browser/gpu/gpu_data_manager_impl_private.cc:836: gpu_preferences->enable_es3_apis = (enabled || !blacklisted) && !disabled; Mmh, I ...
3 years, 9 months ago (2017-03-10 20:31:40 UTC) #52
sugoi1
https://codereview.chromium.org/2737983002/diff/80001/content/browser/gpu/gpu_data_manager_impl_private.cc File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2737983002/diff/80001/content/browser/gpu/gpu_data_manager_impl_private.cc#newcode836 content/browser/gpu/gpu_data_manager_impl_private.cc:836: gpu_preferences->enable_es3_apis = (enabled || !blacklisted) && !disabled; On 2017/03/10 ...
3 years, 9 months ago (2017-03-10 20:42:38 UTC) #53
sugoi1
Done. PTAL
3 years, 9 months ago (2017-03-10 21:17:56 UTC) #54
sugoi1
https://codereview.chromium.org/2737983002/diff/100001/extensions/common/manifest_handlers/requirements_info.cc File extensions/common/manifest_handlers/requirements_info.cc (right): https://codereview.chromium.org/2737983002/diff/100001/extensions/common/manifest_handlers/requirements_info.cc#newcode121 extensions/common/manifest_handlers/requirements_info.cc:121: if (feature == "accelerated_webgl") { I think this one ...
3 years, 9 months ago (2017-03-10 21:22:03 UTC) #56
piman
LGTM with the revert of that change in extensions code.
3 years, 9 months ago (2017-03-10 21:24:29 UTC) #57
Zhenyao Mo
lgtm with nits https://codereview.chromium.org/2737983002/diff/120001/content/browser/gpu/gpu_data_manager_impl_private_unittest.cc File content/browser/gpu/gpu_data_manager_impl_private_unittest.cc (right): https://codereview.chromium.org/2737983002/diff/120001/content/browser/gpu/gpu_data_manager_impl_private_unittest.cc#newcode157 content/browser/gpu/gpu_data_manager_impl_private_unittest.cc:157: {"id" : 1, "features" : ["accelerated_webgl"]}, ...
3 years, 9 months ago (2017-03-10 22:01:07 UTC) #64
jbauman
lgtm, though I agree with zmo on using "clang-format off" on those strings in gpu_data_manager_impl_private_unittest.cc
3 years, 9 months ago (2017-03-10 23:31:11 UTC) #68
sugoi1
On 2017/03/10 23:31:11, jbauman wrote: > lgtm, though I agree with zmo on using "clang-format ...
3 years, 9 months ago (2017-03-11 01:42:05 UTC) #69
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2737983002/160001
3 years, 9 months ago (2017-03-13 15:32:54 UTC) #72
commit-bot: I haz the power
3 years, 9 months ago (2017-03-13 17:06:06 UTC) #75
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/9e3f185360c78d129af6a5fbc4e0...

Powered by Google App Engine
This is Rietveld 408576698