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

Issue 2094513002: Move static GL binding initialization to //ui/gl/init. (Closed)

Created:
4 years, 6 months ago by kylechar
Modified:
4 years, 5 months ago
Reviewers:
Dirk Pranke, piman
CC:
chromium-reviews, piman+watch_chromium.org, ozone-reviews_chromium.org, kalyank
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Move static GL binding initialization to //ui/gl/init. Static GL binding initialization needs to call out to the Ozone platform. Move this initialization code from //ui/gl to //ui/gl/init as part of larger effort to break //ui/gl dep on //ui/ozone. Also move InitializationDebugGLBindings() and ClearGLBindings() in a similar fashion as they are closely linked to static initialization. Unfortunately, dynamic GL binding initialization can't be moved //ui/gl/init. It would be nice to have all the initialization code in one target but dynamic GL binding initialization is used by GLContext. The existing InitializationStaticGLBindings() functions had grown to be very large for some platforms. Where appropriate the code for each implementation has been extracted into it's own method to improve readability. The PRESUBMIT.py script is modified slightly here. The existing static GL initialization uses ScopedAllowIO on some platforms. This function is banned so moving the code triggers presubmit errors. The GPU main thread can't continue until initialization is finished anyways so moving blocking code to a different thread isn't helpful. Add new exemptions to PRESUBMIT.py. BUG=611142 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/507532e60d484704354edd17a7b51904e7a97d88 Cr-Commit-Position: refs/heads/master@{#402259}

Patch Set 1 #

Patch Set 2 : Fix compile. #

Patch Set 3 : Add export. #

Patch Set 4 : Non-exported base. #

Patch Set 5 : Rebase + windows fix again. #

Patch Set 6 : Working and cleaned up. #

Total comments: 2

Patch Set 7 : Fix for comment. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+806 lines, -795 lines) Patch
M PRESUBMIT.py View 1 2 3 4 5 1 chunk +5 lines, -2 lines 0 comments Download
M gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc View 2 chunks +2 lines, -1 line 0 comments Download
M gpu/command_buffer/service/gpu_service_test.cc View 2 chunks +2 lines, -1 line 0 comments Download
M gpu/config/gpu_info_collector_unittest.cc View 2 chunks +2 lines, -1 line 0 comments Download
M gpu/ipc/service/gpu_channel_unittest.cc View 2 chunks +2 lines, -1 line 0 comments Download
M ui/gl/angle_platform_impl.h View 1 2 3 4 1 chunk +3 lines, -1 line 0 comments Download
M ui/gl/gl_egl_api_implementation.h View 1 chunk +3 lines, -3 lines 0 comments Download
M ui/gl/gl_egl_api_implementation.cc View 1 2 3 4 5 6 2 chunks +18 lines, -0 lines 0 comments Download
M ui/gl/gl_gl_api_implementation.h View 1 chunk +3 lines, -3 lines 0 comments Download
M ui/gl/gl_glx_api_implementation.h View 1 chunk +3 lines, -3 lines 0 comments Download
M ui/gl/gl_implementation.h View 5 chunks +6 lines, -13 lines 0 comments Download
M ui/gl/gl_implementation_android.cc View 3 chunks +4 lines, -94 lines 0 comments Download
M ui/gl/gl_implementation_mac.cc View 4 chunks +4 lines, -108 lines 0 comments Download
M ui/gl/gl_implementation_osmesa.h View 1 chunk +5 lines, -4 lines 0 comments Download
M ui/gl/gl_implementation_ozone.cc View 2 chunks +2 lines, -71 lines 0 comments Download
M ui/gl/gl_implementation_win.cc View 3 chunks +3 lines, -301 lines 0 comments Download
M ui/gl/gl_implementation_x11.cc View 4 chunks +4 lines, -166 lines 0 comments Download
M ui/gl/gl_osmesa_api_implementation.h View 1 chunk +3 lines, -3 lines 0 comments Download
M ui/gl/gl_wgl_api_implementation.h View 1 1 chunk +3 lines, -3 lines 0 comments Download
M ui/gl/gpu_timing_unittest.cc View 2 chunks +2 lines, -1 line 0 comments Download
M ui/gl/init/gl_factory.h View 1 2 3 4 5 2 chunks +11 lines, -8 lines 0 comments Download
M ui/gl/init/gl_factory.cc View 1 chunk +7 lines, -0 lines 0 comments Download
M ui/gl/init/gl_initializer.h View 1 2 3 4 5 6 1 chunk +12 lines, -1 line 0 comments Download
M ui/gl/init/gl_initializer_android.cc View 1 2 3 4 5 6 2 chunks +79 lines, -1 line 0 comments Download
M ui/gl/init/gl_initializer_mac.cc View 1 2 3 4 5 4 chunks +109 lines, -0 lines 0 comments Download
M ui/gl/init/gl_initializer_ozone.cc View 1 2 3 4 5 6 2 chunks +63 lines, -1 line 0 comments Download
M ui/gl/init/gl_initializer_win.cc View 1 2 3 4 5 6 2 chunks +289 lines, -1 line 0 comments Download
M ui/gl/init/gl_initializer_x11.cc View 1 2 3 4 5 6 2 chunks +154 lines, -1 line 0 comments Download
M ui/gl/test/gl_image_test_support.cc View 2 chunks +2 lines, -1 line 0 comments Download
M ui/gl/test/gl_surface_test_support.cc View 1 chunk +1 line, -1 line 0 comments Download

Dependent Patchsets:

Messages

Total messages: 23 (11 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2094513002/100001
4 years, 6 months ago (2016-06-24 14:33:21 UTC) #5
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_optional_gpu_tests_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_tests_rel/builds/1508)
4 years, 6 months ago (2016-06-24 16:10:27 UTC) #7
kylechar
This is a bit of big one. Sorry about that, it's essentially just moving all ...
4 years, 6 months ago (2016-06-24 18:43:26 UTC) #9
piman
https://codereview.chromium.org/2094513002/diff/100001/ui/gl/init/gl_initializer_android.cc File ui/gl/init/gl_initializer_android.cc (right): https://codereview.chromium.org/2094513002/diff/100001/ui/gl/init/gl_initializer_android.cc#newcode65 ui/gl/init/gl_initializer_android.cc:65: ::gl::g_driver_gl.fn.glDepthRangeFn = MarshalDepthRangeToDepthRangef; nit: all the EGL platforms have ...
4 years, 6 months ago (2016-06-24 19:13:21 UTC) #10
piman
lgtm
4 years, 6 months ago (2016-06-24 19:13:35 UTC) #11
kylechar
Thanks for the quick review! https://codereview.chromium.org/2094513002/diff/100001/ui/gl/init/gl_initializer_android.cc File ui/gl/init/gl_initializer_android.cc (right): https://codereview.chromium.org/2094513002/diff/100001/ui/gl/init/gl_initializer_android.cc#newcode65 ui/gl/init/gl_initializer_android.cc:65: ::gl::g_driver_gl.fn.glDepthRangeFn = MarshalDepthRangeToDepthRangef; On ...
4 years, 5 months ago (2016-06-27 13:35:22 UTC) #13
kylechar
+dpranke for PRESUBMIT.py. I moved some code that used a banned function, which makes presubmit ...
4 years, 5 months ago (2016-06-27 13:41:44 UTC) #15
Dirk Pranke
lgtm based on piman@'s approval.
4 years, 5 months ago (2016-06-27 16:29:48 UTC) #16
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/2094513002/120001
4 years, 5 months ago (2016-06-27 17:11:27 UTC) #19
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 5 months ago (2016-06-27 19:47:37 UTC) #20
commit-bot: I haz the power
Patchset 7 (id:??) landed as https://crrev.com/507532e60d484704354edd17a7b51904e7a97d88 Cr-Commit-Position: refs/heads/master@{#402259}
4 years, 5 months ago (2016-06-27 19:49:44 UTC) #22
Lambros
4 years, 5 months ago (2016-06-27 21:19:22 UTC) #23
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2099163003/ by lambroslambrou@chromium.org.

The reason for reverting is: Suspected of breaking Official "win trunk" builder:
c:\b\build\slave\win_trunk\build\src\ui\gl\init\gl_initializer_win.cc(32): fatal
error C1083: Cannot open include file: 'software_renderer.h': No such file or
directory
Please see crbug.com/623657 for details.
.

Powered by Google App Engine
This is Rietveld 408576698