Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2201033003:
Check more GLES versions when creating context (Closed)
DescriptionCheck more GLES versions when creating context
Unifies the context creation for GL and GLES into a single loop.
BUG=skia:5403
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2201033003
Committed: https://skia.googlesource.com/skia/+/fc3ea41cebb8272c3f683f9cf585ff780b18f09b
Patch Set 1 #
Total comments: 21
Patch Set 2 : fixed issues found in review #
Total comments: 4
Patch Set 3 : fixed issues found in review, #2 #
Total comments: 4
Patch Set 4 : fixed issues found in review, #3 #Messages
Total messages: 19 (10 generated)
Description was changed from ========== Check more GLES versions when creating context Unifies the context creation for GL and GLES into a single loop. BUG=skia:5403 ========== to ========== Check more GLES versions when creating context Unifies the context creation for GL and GLES into a single loop. BUG=skia:5403 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2201033003 ==========
The CQ bit was checked by martina.kollarova@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
martina.kollarova@intel.com changed reviewers: + bsalomon@google.com, reed@google.com
robertphillips@google.com changed reviewers: + robertphillips@google.com
https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... File tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp (right): https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:58: void destroyGLContext(); is_es should be isES the "f" prefix is reserved for member variables so "fDisplay" should just be "display" https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:59: GLXContext createBestContext(bool is_es, Display* fDisplay, GLXFBConfig bestFbc, Line this last parameter up with the opening '(' ? https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:174: reinterpret_cast<const GLubyte*>(glxExts))) { line the last parameter up with the opening '(' ? Can it all fit on one line ? Member function calls are prefixed with "this->" https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:178: } else { Can this fit on one line? https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:257: */ is_es & fDisplay again Can we make this a static method? It is odd to be assigning to the fContext member variable inside this method and then return it so that the caller can assigned it to fContext. https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:261: glXGetProcAddressARB((GrGLubyte*)"glXCreateContextAttribsARB"); either: nullptr == glXCreateContextAttribsARB or !glXCreateContexAttribsARB https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:268: ctxErrorOccurred = false; Can this fit on one line ? https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:271: The 'f' prefix is reserved for member variables. This should be "context" https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:276: for (int i = versions.size() - 1; i >= 0 ; i--) { I believe the nVidia specific comment should be moved down to where we add the COMPATIBILITY_PROFILE_BIT https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:301: if (!ctxErrorOccurred && fContext) { Remove this commented out code
https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... File tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp (right): https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:58: void destroyGLContext(); On 2016/08/03 17:31:55, robertphillips wrote: > is_es should be isES > the "f" prefix is reserved for member variables so "fDisplay" should just be > "display" Done. https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:59: GLXContext createBestContext(bool is_es, Display* fDisplay, GLXFBConfig bestFbc, On 2016/08/03 17:31:55, robertphillips wrote: > Line this last parameter up with the opening '(' ? Done. https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:174: reinterpret_cast<const GLubyte*>(glxExts))) { On 2016/08/03 17:31:55, robertphillips wrote: > line the last parameter up with the opening '(' ? > Can it all fit on one line ? > Member function calls are prefixed with "this->" Done. https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:178: } else { On 2016/08/03 17:31:56, robertphillips wrote: > Can this fit on one line? Done. https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:257: */ On 2016/08/03 17:31:55, robertphillips wrote: > is_es & fDisplay again > > Can we make this a static method? It is odd to be assigning to the fContext > member variable inside this method and then return it so that the caller can > assigned it to fContext. I've made it a normal member and directly set fContext. Is that ok or would a static member be preferable? https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:261: glXGetProcAddressARB((GrGLubyte*)"glXCreateContextAttribsARB"); On 2016/08/03 17:31:56, robertphillips wrote: > either: > > nullptr == glXCreateContextAttribsARB > > or > > !glXCreateContexAttribsARB Done. https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:268: ctxErrorOccurred = false; On 2016/08/03 17:31:56, robertphillips wrote: > Can this fit on one line ? Done. https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:271: On 2016/08/03 17:31:56, robertphillips wrote: > The 'f' prefix is reserved for member variables. This should be "context" Done. https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:276: for (int i = versions.size() - 1; i >= 0 ; i--) { On 2016/08/03 17:31:55, robertphillips wrote: > I believe the nVidia specific comment should be moved down to where we add the > COMPATIBILITY_PROFILE_BIT Done. https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:301: if (!ctxErrorOccurred && fContext) { On 2016/08/03 17:31:55, robertphillips wrote: > Remove this commented out code Done.
https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... File tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp (right): https://codereview.chromium.org/2201033003/diff/1/tools/gpu/gl/glx/CreatePlat... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:257: */ On 2016/08/04 13:26:25, mkollaro wrote: > On 2016/08/03 17:31:55, robertphillips wrote: > > is_es & fDisplay again > > > > Can we make this a static method? It is odd to be assigning to the fContext > > member variable inside this method and then return it so that the caller can > > assigned it to fContext. > > I've made it a normal member and directly set fContext. Is that ok or would a > static member be preferable? I think having it be a static method would be clearer b.c. it would centralize the setting of fContext to the GLXGLTestContext ctor. https://codereview.chromium.org/2201033003/diff/20001/tools/gpu/gl/glx/Create... File tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp (right): https://codereview.chromium.org/2201033003/diff/20001/tools/gpu/gl/glx/Create... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:58: void destroyGLContext(); rename is_es to isES https://codereview.chromium.org/2201033003/diff/20001/tools/gpu/gl/glx/Create... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:254: */ rename is_es to isES
I made it into a static method now. Thanks for the review! Changing between different project styles can be difficult, sorry for not reading the style guide properly. https://codereview.chromium.org/2201033003/diff/20001/tools/gpu/gl/glx/Create... File tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp (right): https://codereview.chromium.org/2201033003/diff/20001/tools/gpu/gl/glx/Create... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:58: void destroyGLContext(); On 2016/08/04 19:42:13, robertphillips wrote: > rename is_es to isES Done. https://codereview.chromium.org/2201033003/diff/20001/tools/gpu/gl/glx/Create... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:254: */ On 2016/08/04 19:42:13, robertphillips wrote: > rename is_es to isES Done.
lgtm - please fix the static function's name & the comment before landing https://codereview.chromium.org/2201033003/diff/40001/tools/gpu/gl/glx/Create... File tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp (right): https://codereview.chromium.org/2201033003/diff/40001/tools/gpu/gl/glx/Create... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:58: void destroyGLContext(); Please make this CreateBestContext. Static class-scoped methods have their first letter capitalized. https://codereview.chromium.org/2201033003/diff/40001/tools/gpu/gl/glx/Create... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:253: * * Returns the created context or NULL on failure.
https://codereview.chromium.org/2201033003/diff/40001/tools/gpu/gl/glx/Create... File tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp (right): https://codereview.chromium.org/2201033003/diff/40001/tools/gpu/gl/glx/Create... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:58: void destroyGLContext(); On 2016/08/08 12:22:51, robertphillips wrote: > Please make this CreateBestContext. Static class-scoped methods have their first > letter capitalized. Done. https://codereview.chromium.org/2201033003/diff/40001/tools/gpu/gl/glx/Create... tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp:253: * On 2016/08/08 12:22:52, robertphillips wrote: > * Returns the created context or NULL on failure. Done.
The CQ bit was checked by martina.kollarova@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from robertphillips@google.com Link to the patchset: https://codereview.chromium.org/2201033003/#ps60001 (title: "fixed issues found in review, #3")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Check more GLES versions when creating context Unifies the context creation for GL and GLES into a single loop. BUG=skia:5403 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2201033003 ========== to ========== Check more GLES versions when creating context Unifies the context creation for GL and GLES into a single loop. BUG=skia:5403 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2201033003 Committed: https://skia.googlesource.com/skia/+/fc3ea41cebb8272c3f683f9cf585ff780b18f09b ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/fc3ea41cebb8272c3f683f9cf585ff780b18f09b |
