Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1110923003:
Added switch to disable specified GL extensions. (Closed)
|
Created:
5 years, 7 months ago by David Yen Modified:
5 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, piman+watch_chromium.org, Zhenyao Mo Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded switch to disable specified GL extensions.
It is now possible to remove extensions from the GL_EXTENSIONS string
by specifying "--disable-gl-extensions=extension1,extension2".
R=sievers@chromium.org
BUG=482067
Committed: https://crrev.com/5a6bcfe5cc034e908c310f3405b62bd35f6a8793
Cr-Commit-Position: refs/heads/master@{#329752}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Skip filtering if no extensions disabled #Patch Set 3 : Build filtered extensions in constructor #Patch Set 4 : Simplify Removal #
Total comments: 7
Patch Set 5 : Added unit test by swapping in another GLApi which disables the extensions #Patch Set 6 : Accidentally removed necessary class declaration #
Total comments: 14
Patch Set 7 : Added gl_api_unittest under gfx_unittests #
Total comments: 12
Patch Set 8 : Added gn rule and reinterpret cast for function pointers #
Total comments: 6
Patch Set 9 : Reverted gn rule #Patch Set 10 : reinstantiate api instead of reinitialize #Patch Set 11 : Renamed InitializeWithCommandLine to just Initialize with CommandLine param #Patch Set 12 : Change back initialize function name, Export RealGLApi #Patch Set 13 : Added gl tests in gfx for non-ios only #Patch Set 14 : Also export GLApiBase #
Total comments: 1
Patch Set 15 : Moved gl_api_unittest from gfx_unittests to gl_unittest #Patch Set 16 : Remove gl_unittests for ios #Patch Set 17 : Fix gyp syntax #Patch Set 18 : Fix gl_unittests for windows #
Messages
Total messages: 41 (6 generated)
Very cool. Can you add a small test? Unfortunately we don't have unittests for ui/gl so it'd have to go into gpu_unittests. It would need to call InitBindings() (and ClearBindings() when tearing down). Since you don't want to mess with the CmdLine::ForCurrentProcess() in the tests I think this suggests passing the list of disabled extensions into InitializeDynamicGLBindingsGL(). https://codereview.chromium.org/1110923003/diff/1/ui/gl/gl_gl_api_implementat... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/1110923003/diff/1/ui/gl/gl_gl_api_implementat... ui/gl/gl_gl_api_implementation.cc:413: BuildFilteredExtensions(); Can we do this once from initialization instead of lazily? https://codereview.chromium.org/1110923003/diff/1/ui/gl/gl_gl_api_implementat... ui/gl/gl_gl_api_implementation.cc:420: const GLubyte* RealGLApi::glGetStringFn(GLenum name) { You have to do the same thing for VirtualGLApi also or it won't work for virtual contexts (i.e. most Android devices and Mac). https://codereview.chromium.org/1110923003/diff/1/ui/gl/gl_gl_api_implementat... ui/gl/gl_gl_api_implementation.cc:476: bool filtered = false; nit: Is it maybe slightly simpler to iterate over the disabled extensions and for each std::find() it in the original vector, and erase if found?
I will see about adding a test, I did see that virtual_contexts had a unit test. Perhaps I could add one next to that for regular gl_contexts? https://codereview.chromium.org/1110923003/diff/1/ui/gl/gl_gl_api_implementat... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/1110923003/diff/1/ui/gl/gl_gl_api_implementat... ui/gl/gl_gl_api_implementation.cc:413: BuildFilteredExtensions(); On 2015/04/29 18:48:10, sievers wrote: > Can we do this once from initialization instead of lazily? Done. https://codereview.chromium.org/1110923003/diff/1/ui/gl/gl_gl_api_implementat... ui/gl/gl_gl_api_implementation.cc:420: const GLubyte* RealGLApi::glGetStringFn(GLenum name) { On 2015/04/29 18:48:10, sievers wrote: > You have to do the same thing for VirtualGLApi also or it won't work for virtual > contexts (i.e. most Android devices and Mac). The VirtualGLApi obtains the list of extensions from its real GLContext in the constructor which gets it from the RealGLApi. It did this to filter out GL_EXT_occlusion_query_boolean I believe. The only thing I was not sure of was whether we should also override glGetIntegervFn() and glGetStringiFn(), but it seems like this would have been a problem with GL_EXT_occlusion_query_boolean if that were the case. https://codereview.chromium.org/1110923003/diff/1/ui/gl/gl_gl_api_implementat... ui/gl/gl_gl_api_implementation.cc:476: bool filtered = false; On 2015/04/29 18:48:10, sievers wrote: > nit: > Is it maybe slightly simpler to iterate over the disabled extensions and for > each std::find() it in the original vector, and erase if found? I've changed it to use std::remove instead.
zmo@chromium.org changed reviewers: + zmo@chromium.org
https://codereview.chromium.org/1110923003/diff/60001/ui/gl/gl_gl_api_impleme... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/1110923003/diff/60001/ui/gl/gl_gl_api_impleme... ui/gl/gl_gl_api_implementation.cc:451: *params = static_cast<GLint>(filtered_exts_str_.size()); This is incorrect. I think you mean filtered_ext_.size() instead.
https://codereview.chromium.org/1110923003/diff/1/ui/gl/gl_gl_api_implementat... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/1110923003/diff/1/ui/gl/gl_gl_api_implementat... ui/gl/gl_gl_api_implementation.cc:420: const GLubyte* RealGLApi::glGetStringFn(GLenum name) { On 2015/04/29 19:23:09, David Yen wrote: > On 2015/04/29 18:48:10, sievers wrote: > > You have to do the same thing for VirtualGLApi also or it won't work for > virtual > > contexts (i.e. most Android devices and Mac). > > The VirtualGLApi obtains the list of extensions from its real GLContext in the > constructor which gets it from the RealGLApi. It did this to filter out > GL_EXT_occlusion_query_boolean I believe. > > The only thing I was not sure of was whether we should also override > glGetIntegervFn() and glGetStringiFn(), but it seems like this would have been a > problem with GL_EXT_occlusion_query_boolean if that were the case. Yes, we should fix those functions too. But that should probably be a separate patch. https://codereview.chromium.org/1110923003/diff/60001/ui/gl/gl_gl_api_impleme... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/1110923003/diff/60001/ui/gl/gl_gl_api_impleme... ui/gl/gl_gl_api_implementation.cc:401: RealGLApi::RealGLApi() { That doesn't work. |driver_| will be NULL when RealGLApi is constructed and your GL calls below will crash. Can you simply move it to Initialize()? https://codereview.chromium.org/1110923003/diff/60001/ui/gl/gl_gl_api_impleme... ui/gl/gl_gl_api_implementation.cc:429: filtered_exts_.erase(std::remove(filtered_exts_.begin(), Actually can you just use std::set<> instead of vector<> then and do std::set_difference here? https://codereview.chromium.org/1110923003/diff/60001/ui/gl/gl_gl_api_impleme... ui/gl/gl_gl_api_implementation.cc:438: extensions_filtered_ = true; Why do we need this? It seems like it would just cause a bug if something below got the unfiltered string. And above you are calling GLApiBase directly.
Added a unit test. https://codereview.chromium.org/1110923003/diff/1/ui/gl/gl_gl_api_implementat... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/1110923003/diff/1/ui/gl/gl_gl_api_implementat... ui/gl/gl_gl_api_implementation.cc:420: const GLubyte* RealGLApi::glGetStringFn(GLenum name) { On 2015/04/30 20:45:02, sievers wrote: > On 2015/04/29 19:23:09, David Yen wrote: > > On 2015/04/29 18:48:10, sievers wrote: > > > You have to do the same thing for VirtualGLApi also or it won't work for > > virtual > > > contexts (i.e. most Android devices and Mac). > > > > The VirtualGLApi obtains the list of extensions from its real GLContext in the > > constructor which gets it from the RealGLApi. It did this to filter out > > GL_EXT_occlusion_query_boolean I believe. > > > > The only thing I was not sure of was whether we should also override > > glGetIntegervFn() and glGetStringiFn(), but it seems like this would have been > a > > problem with GL_EXT_occlusion_query_boolean if that were the case. > > Yes, we should fix those functions too. But that should probably be a separate > patch. Acknowledged. https://codereview.chromium.org/1110923003/diff/60001/ui/gl/gl_gl_api_impleme... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/1110923003/diff/60001/ui/gl/gl_gl_api_impleme... ui/gl/gl_gl_api_implementation.cc:401: RealGLApi::RealGLApi() { On 2015/04/30 20:45:02, sievers wrote: > That doesn't work. |driver_| will be NULL when RealGLApi is constructed and your > GL calls below will crash. > Can you simply move it to Initialize()? Done. https://codereview.chromium.org/1110923003/diff/60001/ui/gl/gl_gl_api_impleme... ui/gl/gl_gl_api_implementation.cc:438: extensions_filtered_ = true; On 2015/04/30 20:45:02, sievers wrote: > Why do we need this? > It seems like it would just cause a bug if something below got the unfiltered > string. And above you are calling GLApiBase directly. This was originally added to fix the mock interface expectations, it expects the GLApiBase::glGet...Fn() to be called so we cannot do our filtering for unit tests. It should only be false if we are not filtering anything. https://codereview.chromium.org/1110923003/diff/60001/ui/gl/gl_gl_api_impleme... ui/gl/gl_gl_api_implementation.cc:451: *params = static_cast<GLint>(filtered_exts_str_.size()); On 2015/04/29 20:04:23, Zhenyao Mo wrote: > This is incorrect. I think you mean filtered_ext_.size() instead. Ah good catch, Done.
https://codereview.chromium.org/1110923003/diff/100001/ui/gl/gl_gl_api_implem... File ui/gl/gl_gl_api_implementation.h (right): https://codereview.chromium.org/1110923003/diff/100001/ui/gl/gl_gl_api_implem... ui/gl/gl_gl_api_implementation.h:61: void Initialize(DriverGL* driver, base::CommandLine* command_line = nullptr); default values are against coding style. Otherwise the CL looks good.
https://codereview.chromium.org/1110923003/diff/100001/gpu/command_buffer/tes... File gpu/command_buffer/tests/gl_context_unittest.cc (right): https://codereview.chromium.org/1110923003/diff/100001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_context_unittest.cc:18: TEST(GLContextTests, DisabledExtensionsTest) { Maybe a better name for this file is gl_api_unittest.cc with a TODO in here that this should really be in ui/gl. https://codereview.chromium.org/1110923003/diff/100001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_context_unittest.cc:22: gl.Initialize(GLManager::Options()); Uh I wouldn't use GLManager here. That's for setting up a command decoder and client. https://codereview.chromium.org/1110923003/diff/100001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_context_unittest.cc:27: I think we can do something like this: const GLubyte* FakeGetString(GLenum name) { return reinterpret_cast<const GLubyte*>("GL_EXT_foo1 GL_EXT_foo2 GL_EXT_foo3"); } class GLApiTest : public testing::Test { public: void SetUp() override { SetGLImplementation(kGLImplementationEGLGLES2); driver_.fn.glGetStringFn = &FakeGetString; SetGLApi(&api_); } void TearDown() override { SetGLImplementation(kGLImplementationNone); SetGLApi(nullptr); } protected: DriverGL driver_; RealGLApi api_; }; TEST_F(GLApiTest, DisabledExtensionsTest) { base::CommandLine command_line(base::CommandLine::NO_PROGRAM); command_line.AppendSwitchASCII(switches::kDisableGLExtensions, "GL_EXT_foo2"); api_.Initialize(&driver_, &command_line); const std::string expected("GL_EXT_foo1 GL_EXT_foo3"); EXPECT_EQ(expected, reinterpret_cast<const char*>(glGetString(GL_EXTENSIONS))); } As usual, What's really unfortunate is that we have to GL_EXPORT symbols for this from gl_gl_api_implementation.cc/.h just for testing. https://codereview.chromium.org/1110923003/diff/100001/ui/gl/gl_gl_api_implem... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/1110923003/diff/100001/ui/gl/gl_gl_api_implem... ui/gl/gl_gl_api_implementation.cc:447: extensions_filtered_ = true; So this can go now? https://codereview.chromium.org/1110923003/diff/100001/ui/gl/gl_gl_api_implem... File ui/gl/gl_gl_api_implementation.h (right): https://codereview.chromium.org/1110923003/diff/100001/ui/gl/gl_gl_api_implem... ui/gl/gl_gl_api_implementation.h:15: class CommandLine; nit: no indent https://codereview.chromium.org/1110923003/diff/100001/ui/gl/gl_gl_api_implem... ui/gl/gl_gl_api_implementation.h:137: ScopedSetGLToRealGLApi(GLApi* set_api = nullptr); Instead of changing this (which is supposed to set the api to the internal real API, and not to a given API), can you just use SetGLApi() in your test?
Actually we do have gfx_unittests. Can you add it there?
On 2015/04/30 23:54:31, sievers wrote: > Actually we do have gfx_unittests. Can you add it there? Ok, I got it working, see here: https://codereview.chromium.org/1114263002/ This has nagged me for a while that we don't have ui/gl unittests, so it'd be nice to start adding stuff here.
https://codereview.chromium.org/1110923003/diff/100001/gpu/command_buffer/tes... File gpu/command_buffer/tests/gl_context_unittest.cc (right): https://codereview.chromium.org/1110923003/diff/100001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_context_unittest.cc:18: TEST(GLContextTests, DisabledExtensionsTest) { On 2015/04/30 23:49:05, sievers wrote: > Maybe a better name for this file is gl_api_unittest.cc with a TODO in here that > this should really be in ui/gl. done and moved to gfx_unittests https://codereview.chromium.org/1110923003/diff/100001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_context_unittest.cc:22: gl.Initialize(GLManager::Options()); On 2015/04/30 23:49:05, sievers wrote: > Uh I wouldn't use GLManager here. That's for setting up a command decoder and > client. No longer relevant in the new unit test. https://codereview.chromium.org/1110923003/diff/100001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_context_unittest.cc:27: On 2015/04/30 23:49:05, sievers wrote: > I think we can do something like this: > > > const GLubyte* FakeGetString(GLenum name) { > return reinterpret_cast<const GLubyte*>("GL_EXT_foo1 GL_EXT_foo2 > GL_EXT_foo3"); > } > > class GLApiTest : public testing::Test { > public: > void SetUp() override { > SetGLImplementation(kGLImplementationEGLGLES2); > driver_.fn.glGetStringFn = &FakeGetString; > SetGLApi(&api_); > } > > void TearDown() override { > SetGLImplementation(kGLImplementationNone); > SetGLApi(nullptr); > } > > protected: > DriverGL driver_; > RealGLApi api_; > }; > > TEST_F(GLApiTest, DisabledExtensionsTest) { > base::CommandLine command_line(base::CommandLine::NO_PROGRAM); > command_line.AppendSwitchASCII(switches::kDisableGLExtensions, "GL_EXT_foo2"); > api_.Initialize(&driver_, &command_line); > const std::string expected("GL_EXT_foo1 GL_EXT_foo3"); > EXPECT_EQ(expected, > reinterpret_cast<const char*>(glGetString(GL_EXTENSIONS))); > } > > As usual, What's really unfortunate is that we have to GL_EXPORT symbols for > this from gl_gl_api_implementation.cc/.h just for testing. Done, new unit test is more or less done this way. https://codereview.chromium.org/1110923003/diff/100001/ui/gl/gl_gl_api_implem... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/1110923003/diff/100001/ui/gl/gl_gl_api_implem... ui/gl/gl_gl_api_implementation.cc:447: extensions_filtered_ = true; On 2015/04/30 23:49:05, sievers wrote: > So this can go now? This is still necessary for mock functions to work (we cannot intercept the function calls or else the expect calls fail). I can change it to test the actual vector/string though if you are concerned about state maintenance. I have changed the below calls to check if the vector or string is empty instead. https://codereview.chromium.org/1110923003/diff/100001/ui/gl/gl_gl_api_implem... File ui/gl/gl_gl_api_implementation.h (right): https://codereview.chromium.org/1110923003/diff/100001/ui/gl/gl_gl_api_implem... ui/gl/gl_gl_api_implementation.h:15: class CommandLine; On 2015/04/30 23:49:05, sievers wrote: > nit: no indent Done. https://codereview.chromium.org/1110923003/diff/100001/ui/gl/gl_gl_api_implem... ui/gl/gl_gl_api_implementation.h:61: void Initialize(DriverGL* driver, base::CommandLine* command_line = nullptr); On 2015/04/30 22:40:51, Zhenyao Mo wrote: > default values are against coding style. Otherwise the CL looks good. Done. https://codereview.chromium.org/1110923003/diff/100001/ui/gl/gl_gl_api_implem... ui/gl/gl_gl_api_implementation.h:137: ScopedSetGLToRealGLApi(GLApi* set_api = nullptr); On 2015/04/30 23:49:05, sievers wrote: > Instead of changing this (which is supposed to set the api to the internal real > API, and not to a given API), can you just use SetGLApi() in your test? No longer relevant, so removed.
sievers@chromium.org changed reviewers: + danakj@chromium.org
This is great. +Dana to see if she lets us add ui/gl unittests in gfx_unittests https://codereview.chromium.org/1110923003/diff/120001/ui/gl/gl_api_unittest.cc File ui/gl/gl_api_unittest.cc (right): https://codereview.chromium.org/1110923003/diff/120001/ui/gl/gl_api_unittest.... ui/gl/gl_api_unittest.cc:10: #include "ui/gl/gl_surface.h" nit: gl_surface.h not needed. what about string_util/split above? https://codereview.chromium.org/1110923003/diff/120001/ui/gl/gl_api_unittest.... ui/gl/gl_api_unittest.cc:18: g_fake_extension_string = ""; nit: no "g_" prefix here and for other static variables since they are not in the global namespace https://codereview.chromium.org/1110923003/diff/120001/ui/gl/gl_api_unittest.... ui/gl/gl_api_unittest.cc:77: static uint32_t g_num_fake_extension_strings; Actually can you make them non-static since you are modifying them in the test? https://codereview.chromium.org/1110923003/diff/120001/ui/gl/gl_gl_api_implem... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/1110923003/diff/120001/ui/gl/gl_gl_api_implem... ui/gl/gl_gl_api_implementation.cc:417: filtered_exts_str_.clear(); Is this because of some unittests? Can we clear this from ClearGLBindings() instead and DCHECK() here that they are empty? https://codereview.chromium.org/1110923003/diff/120001/ui/gl/gl_gl_api_implem... ui/gl/gl_gl_api_implementation.cc:457: if (!filtered_exts_.empty() && pname == GL_NUM_EXTENSIONS) { nit: should it be DCHECK(!filtered_exts_.empty()) instead here and below? https://codereview.chromium.org/1110923003/diff/140001/ui/gfx/BUILD.gn File ui/gfx/BUILD.gn (right): https://codereview.chromium.org/1110923003/diff/140001/ui/gfx/BUILD.gn#newcod... ui/gfx/BUILD.gn:398: "//ui/gl/gl_api_unittest.cc", nit: maybe we can also treat these ui/gl unittest sources as a different set here? https://codereview.chromium.org/1110923003/diff/140001/ui/gl/gl_gl_api_implem... File ui/gl/gl_gl_api_implementation.h (right): https://codereview.chromium.org/1110923003/diff/140001/ui/gl/gl_gl_api_implem... ui/gl/gl_gl_api_implementation.h:63: base::CommandLine* command_line); So somebody has to call this in the production code for it to work. That being said, don't you rather want to just add this argument to Initialize()?
Are you sure we should be putting gl tests in gfx? I seem to be running into a lot of barriers that were put up so that gfx tests do not depend on gl. https://codereview.chromium.org/1110923003/diff/120001/ui/gl/gl_api_unittest.cc File ui/gl/gl_api_unittest.cc (right): https://codereview.chromium.org/1110923003/diff/120001/ui/gl/gl_api_unittest.... ui/gl/gl_api_unittest.cc:10: #include "ui/gl/gl_surface.h" On 2015/05/01 21:54:59, sievers wrote: > nit: gl_surface.h not needed. what about string_util/split above? Done. https://codereview.chromium.org/1110923003/diff/120001/ui/gl/gl_api_unittest.... ui/gl/gl_api_unittest.cc:18: g_fake_extension_string = ""; On 2015/05/01 21:55:00, sievers wrote: > nit: no "g_" prefix here and for other static variables since they are not in > the global namespace Done. https://codereview.chromium.org/1110923003/diff/120001/ui/gl/gl_api_unittest.... ui/gl/gl_api_unittest.cc:77: static uint32_t g_num_fake_extension_strings; On 2015/05/01 21:55:00, sievers wrote: > Actually can you make them non-static since you are modifying them in the test? They are being modified in static functions so they need to be static. I could put them all in another class that gets instantiated statically maybe, but that is more of a style difference I think. https://codereview.chromium.org/1110923003/diff/120001/ui/gl/gl_gl_api_implem... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/1110923003/diff/120001/ui/gl/gl_gl_api_implem... ui/gl/gl_gl_api_implementation.cc:417: filtered_exts_str_.clear(); On 2015/05/01 21:55:00, sievers wrote: > Is this because of some unittests? > Can we clear this from ClearGLBindings() instead and DCHECK() here that they are > empty? These are per instance of the class though, ClearGLBindings() just deletes the global instance of RealGLApi so it's not relevant. I suppose normally it would be cleared in a "Destroy" function of some sort. If it bothers you I can just re-instantiate the class in the unit tests. https://codereview.chromium.org/1110923003/diff/120001/ui/gl/gl_gl_api_implem... ui/gl/gl_gl_api_implementation.cc:457: if (!filtered_exts_.empty() && pname == GL_NUM_EXTENSIONS) { On 2015/05/01 21:55:00, sievers wrote: > nit: should it be DCHECK(!filtered_exts_.empty()) instead here and below? These are only filled if there are extensions disabled, otherwise it calls the driver. Additionally, not calling the driver breaks our mock api class. https://codereview.chromium.org/1110923003/diff/140001/ui/gfx/BUILD.gn File ui/gfx/BUILD.gn (right): https://codereview.chromium.org/1110923003/diff/140001/ui/gfx/BUILD.gn#newcod... ui/gfx/BUILD.gn:398: "//ui/gl/gl_api_unittest.cc", On 2015/05/01 21:55:00, sievers wrote: > nit: maybe we can also treat these ui/gl unittest sources as a different set > here? GN has much stricter gfx -> gl dependency rules so I am not sure if it is possible... I just removed this change. https://codereview.chromium.org/1110923003/diff/140001/ui/gl/gl_gl_api_implem... File ui/gl/gl_gl_api_implementation.h (right): https://codereview.chromium.org/1110923003/diff/140001/ui/gl/gl_gl_api_implem... ui/gl/gl_gl_api_implementation.h:63: base::CommandLine* command_line); On 2015/05/01 21:55:00, sievers wrote: > So somebody has to call this in the production code for it to work. > That being said, don't you rather want to just add this argument to > Initialize()? In Initialize() it defaults to using the process command line arguments. It was pointed out by zmo that adding default parameters is against the style guide, so I followed how gl_manager had added a command_line parameter.
https://codereview.chromium.org/1110923003/diff/120001/ui/gl/gl_api_unittest.cc File ui/gl/gl_api_unittest.cc (right): https://codereview.chromium.org/1110923003/diff/120001/ui/gl/gl_api_unittest.... ui/gl/gl_api_unittest.cc:77: static uint32_t g_num_fake_extension_strings; On 2015/05/01 22:34:38, David Yen wrote: > On 2015/05/01 21:55:00, sievers wrote: > > Actually can you make them non-static since you are modifying them in the > test? > > They are being modified in static functions so they need to be static. I could > put them all in another class that gets instantiated statically maybe, but that > is more of a style difference I think. Why do the functions have to be static? If they are protected the test will be able to access member functions and variables.
https://codereview.chromium.org/1110923003/diff/120001/ui/gl/gl_api_unittest.cc File ui/gl/gl_api_unittest.cc (right): https://codereview.chromium.org/1110923003/diff/120001/ui/gl/gl_api_unittest.... ui/gl/gl_api_unittest.cc:77: static uint32_t g_num_fake_extension_strings; On 2015/05/01 22:44:49, sievers wrote: > On 2015/05/01 22:34:38, David Yen wrote: > > On 2015/05/01 21:55:00, sievers wrote: > > > Actually can you make them non-static since you are modifying them in the > > test? > > > > They are being modified in static functions so they need to be static. I could > > put them all in another class that gets instantiated statically maybe, but > that > > is more of a style difference I think. > > Why do the functions have to be static? If they are protected the test will be > able to access member functions and variables. FakeGetString, FakeGetIntegervFn, FakeGetStringi are used as function pointers to set into the driver.
https://codereview.chromium.org/1110923003/diff/140001/ui/gl/gl_gl_api_implem... File ui/gl/gl_gl_api_implementation.h (right): https://codereview.chromium.org/1110923003/diff/140001/ui/gl/gl_gl_api_implem... ui/gl/gl_gl_api_implementation.h:63: base::CommandLine* command_line); On 2015/05/01 22:34:38, David Yen wrote: > On 2015/05/01 21:55:00, sievers wrote: > > So somebody has to call this in the production code for it to work. > > That being said, don't you rather want to just add this argument to > > Initialize()? > > In Initialize() it defaults to using the process command line arguments. It was > pointed out by zmo that adding default parameters is against the style guide, so > I followed how gl_manager had added a command_line parameter. My bad. I missed that Initialize() calls this with CommandLine::ForCurrentProcess(). So this method should be called InitializeForTesting().
https://codereview.chromium.org/1110923003/diff/140001/ui/gl/gl_gl_api_implem... File ui/gl/gl_gl_api_implementation.h (right): https://codereview.chromium.org/1110923003/diff/140001/ui/gl/gl_gl_api_implem... ui/gl/gl_gl_api_implementation.h:63: base::CommandLine* command_line); On 2015/05/01 22:48:35, sievers wrote: > On 2015/05/01 22:34:38, David Yen wrote: > > On 2015/05/01 21:55:00, sievers wrote: > > > So somebody has to call this in the production code for it to work. > > > That being said, don't you rather want to just add this argument to > > > Initialize()? > > > > In Initialize() it defaults to using the process command line arguments. It > was > > pointed out by zmo that adding default parameters is against the style guide, > so > > I followed how gl_manager had added a command_line parameter. > > My bad. I missed that Initialize() calls this with > CommandLine::ForCurrentProcess(). > So this method should be called InitializeForTesting(). I actually prefer the more descriptive name, but how about we meet in the middle and I just change the name to an overloaded Initialize with a CommandLine parameter.
On 2015/05/01 22:59:35, David Yen wrote: > https://codereview.chromium.org/1110923003/diff/140001/ui/gl/gl_gl_api_implem... > File ui/gl/gl_gl_api_implementation.h (right): > > https://codereview.chromium.org/1110923003/diff/140001/ui/gl/gl_gl_api_implem... > ui/gl/gl_gl_api_implementation.h:63: base::CommandLine* command_line); > On 2015/05/01 22:48:35, sievers wrote: > > On 2015/05/01 22:34:38, David Yen wrote: > > > On 2015/05/01 21:55:00, sievers wrote: > > > > So somebody has to call this in the production code for it to work. > > > > That being said, don't you rather want to just add this argument to > > > > Initialize()? > > > > > > In Initialize() it defaults to using the process command line arguments. It > > was > > > pointed out by zmo that adding default parameters is against the style > guide, > > so > > > I followed how gl_manager had added a command_line parameter. > > > > My bad. I missed that Initialize() calls this with > > CommandLine::ForCurrentProcess(). > > So this method should be called InitializeForTesting(). > > I actually prefer the more descriptive name, but how about we meet in the middle > and I just change the name to an overloaded Initialize with a CommandLine > parameter. I'd just stick with InitializeWithCommandLine() then. But LGTM otherwise. Can you file a bug to get this added to GN? (I'd prefer just have these tests as part of gfx_unittests somehow, rather than doing the whole dance of adding another suite.)
On 2015/05/01 23:12:24, sievers wrote: > On 2015/05/01 22:59:35, David Yen wrote: > > > https://codereview.chromium.org/1110923003/diff/140001/ui/gl/gl_gl_api_implem... > > File ui/gl/gl_gl_api_implementation.h (right): > > > > > https://codereview.chromium.org/1110923003/diff/140001/ui/gl/gl_gl_api_implem... > > ui/gl/gl_gl_api_implementation.h:63: base::CommandLine* command_line); > > On 2015/05/01 22:48:35, sievers wrote: > > > On 2015/05/01 22:34:38, David Yen wrote: > > > > On 2015/05/01 21:55:00, sievers wrote: > > > > > So somebody has to call this in the production code for it to work. > > > > > That being said, don't you rather want to just add this argument to > > > > > Initialize()? > > > > > > > > In Initialize() it defaults to using the process command line arguments. > It > > > was > > > > pointed out by zmo that adding default parameters is against the style > > guide, > > > so > > > > I followed how gl_manager had added a command_line parameter. > > > > > > My bad. I missed that Initialize() calls this with > > > CommandLine::ForCurrentProcess(). > > > So this method should be called InitializeForTesting(). > > > > I actually prefer the more descriptive name, but how about we meet in the > middle > > and I just change the name to an overloaded Initialize with a CommandLine > > parameter. > > I'd just stick with InitializeWithCommandLine() then. But LGTM otherwise. > Can you file a bug to get this added to GN? (I'd prefer just have these tests as > part of gfx_unittests somehow, rather than doing the whole dance of adding > another suite.) I've changed it back to InitializeWithCommandLine(). I don't think it's a GN bug, they intentionally do not allow circular dependencies (gl depends on gfx, so gfx cannot depend on gl). If we were going to add it to GN we would have to do it the proper way and have gl have its own tests. I also don't think it is sustainable to continue adding GL tests to gfx, you have to export the GL classes that are being tested just to write unit test.
https://codereview.chromium.org/1110923003/diff/240001/ui/gfx/gfx_tests.gyp File ui/gfx/gfx_tests.gyp (right): https://codereview.chromium.org/1110923003/diff/240001/ui/gfx/gfx_tests.gyp#n... ui/gfx/gfx_tests.gyp:29: '../gl/gl_api_unittest.cc', we shouldn't be including '../*' as sources in a gyp target
On 2015/05/04 17:29:55, David Yen wrote: > On 2015/05/01 23:12:24, sievers wrote: > > On 2015/05/01 22:59:35, David Yen wrote: > > > > > > https://codereview.chromium.org/1110923003/diff/140001/ui/gl/gl_gl_api_implem... > > > File ui/gl/gl_gl_api_implementation.h (right): > > > > > > > > > https://codereview.chromium.org/1110923003/diff/140001/ui/gl/gl_gl_api_implem... > > > ui/gl/gl_gl_api_implementation.h:63: base::CommandLine* command_line); > > > On 2015/05/01 22:48:35, sievers wrote: > > > > On 2015/05/01 22:34:38, David Yen wrote: > > > > > On 2015/05/01 21:55:00, sievers wrote: > > > > > > So somebody has to call this in the production code for it to work. > > > > > > That being said, don't you rather want to just add this argument to > > > > > > Initialize()? > > > > > > > > > > In Initialize() it defaults to using the process command line arguments. > > It > > > > was > > > > > pointed out by zmo that adding default parameters is against the style > > > guide, > > > > so > > > > > I followed how gl_manager had added a command_line parameter. > > > > > > > > My bad. I missed that Initialize() calls this with > > > > CommandLine::ForCurrentProcess(). > > > > So this method should be called InitializeForTesting(). > > > > > > I actually prefer the more descriptive name, but how about we meet in the > > middle > > > and I just change the name to an overloaded Initialize with a CommandLine > > > parameter. > > > > I'd just stick with InitializeWithCommandLine() then. But LGTM otherwise. > > Can you file a bug to get this added to GN? (I'd prefer just have these tests > as > > part of gfx_unittests somehow, rather than doing the whole dance of adding > > another suite.) > > I've changed it back to InitializeWithCommandLine(). > > I don't think it's a GN bug, they intentionally do not allow circular > dependencies (gl depends on gfx, so gfx cannot depend on gl). If we were going > to add it to GN we would have to do it the proper way and have gl have its own > tests. Feel free to add another unittest target and get it to run on the bots (incl. Android). I mainly wanted to save you some work, because I don't think it adds much benefit. The code in ui/gl even uses the 'gfx' namespace and there are no real serious dependency issues to be considered here (we are not changing any source file dependency rules or anything). This is mostly about build rule cosmetics. I think you just need to either define a separate source-file list for the tests and just inject it into the existing target, or you add a static target that you depend on. I also don't think it is sustainable to continue adding GL tests to gfx, > you have to export the GL classes that are being tested just to write unit test. That's always a problem and has nothing to do with the discussion of what the unittest executable target name should be. If ui_gl is a component, then in the component build *any* unittest executable target will need the tested functions to be exported. This is an issue for pretty much all unittests (such as content_unittests). Feel free to start a thread on chromium-dev if you think we should change how we do this. (We could for example define something LIKE FOO_EXPORTED_FOR_TESTING.) I don't think people have cared too much since we haven't been shipping anything in production based on the component build, so . Not sure if it's still true.
On Mon, May 4, 2015 at 11:34 AM, <sievers@chromium.org> wrote: > On 2015/05/04 17:29:55, David Yen wrote: > >> On 2015/05/01 23:12:24, sievers wrote: >> > On 2015/05/01 22:59:35, David Yen wrote: >> > > >> > >> > > > https://codereview.chromium.org/1110923003/diff/140001/ui/gl/gl_gl_api_implem... > >> > > File ui/gl/gl_gl_api_implementation.h (right): >> > > >> > > >> > >> > > > https://codereview.chromium.org/1110923003/diff/140001/ui/gl/gl_gl_api_implem... > >> > > ui/gl/gl_gl_api_implementation.h:63: base::CommandLine* command_line); >> > > On 2015/05/01 22:48:35, sievers wrote: >> > > > On 2015/05/01 22:34:38, David Yen wrote: >> > > > > On 2015/05/01 21:55:00, sievers wrote: >> > > > > > So somebody has to call this in the production code for it to >> work. >> > > > > > That being said, don't you rather want to just add this >> argument to >> > > > > > Initialize()? >> > > > > >> > > > > In Initialize() it defaults to using the process command line >> > arguments. > >> > It >> > > > was >> > > > > pointed out by zmo that adding default parameters is against the >> style >> > > guide, >> > > > so >> > > > > I followed how gl_manager had added a command_line parameter. >> > > > >> > > > My bad. I missed that Initialize() calls this with >> > > > CommandLine::ForCurrentProcess(). >> > > > So this method should be called InitializeForTesting(). >> > > >> > > I actually prefer the more descriptive name, but how about we meet in >> the >> > middle >> > > and I just change the name to an overloaded Initialize with a >> CommandLine >> > > parameter. >> > >> > I'd just stick with InitializeWithCommandLine() then. But LGTM >> otherwise. >> > Can you file a bug to get this added to GN? (I'd prefer just have these >> > tests > >> as >> > part of gfx_unittests somehow, rather than doing the whole dance of >> adding >> > another suite.) >> > > I've changed it back to InitializeWithCommandLine(). >> > > I don't think it's a GN bug, they intentionally do not allow circular >> dependencies (gl depends on gfx, so gfx cannot depend on gl). If we were >> going >> to add it to GN we would have to do it the proper way and have gl have >> its own >> tests. >> > > Feel free to add another unittest target and get it to run on the bots > (incl. > Android). I mainly wanted to save you some work, because I don't think it > adds > much benefit. The code in ui/gl even uses the 'gfx' namespace and there > are no > real serious dependency issues to be considered here (we are not changing > any > source file dependency rules or anything). This is mostly about build rule > cosmetics. I think you just need to either define a separate source-file > list > for the tests and just inject it into the existing target, or you add a > static > target that you depend on. Sorry, it sucks for the first person, but makes adding more tests much nicer and avoids building up things that don't belong in the wrong place. > > > I also don't think it is sustainable to continue adding GL tests to gfx, > >> you have to export the GL classes that are being tested just to write unit >> > test. > > That's always a problem and has nothing to do with the discussion of what > the > unittest executable target name should be. > If ui_gl is a component, then in the component build *any* unittest > executable > target will need the tested functions to be exported. > This is an issue for pretty much all unittests (such as > content_unittests). Feel > free to start a thread on chromium-dev if you think we should change how > we do > this. (We could for example define something LIKE > FOO_EXPORTED_FOR_TESTING.) I > don't think people have cared too much since we haven't been shipping > anything > in production based on the component build, so . Not sure if it's still > true. > I think it's still true. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/05/04 18:38:01, danakj wrote: > On Mon, May 4, 2015 at 11:34 AM, <mailto:sievers@chromium.org> wrote: > > > On 2015/05/04 17:29:55, David Yen wrote: > > > >> On 2015/05/01 23:12:24, sievers wrote: > >> > On 2015/05/01 22:59:35, David Yen wrote: > >> > > > >> > > >> > > > > > > > https://codereview.chromium.org/1110923003/diff/140001/ui/gl/gl_gl_api_implem... > > > >> > > File ui/gl/gl_gl_api_implementation.h (right): > >> > > > >> > > > >> > > >> > > > > > > > https://codereview.chromium.org/1110923003/diff/140001/ui/gl/gl_gl_api_implem... > > > >> > > ui/gl/gl_gl_api_implementation.h:63: base::CommandLine* command_line); > >> > > On 2015/05/01 22:48:35, sievers wrote: > >> > > > On 2015/05/01 22:34:38, David Yen wrote: > >> > > > > On 2015/05/01 21:55:00, sievers wrote: > >> > > > > > So somebody has to call this in the production code for it to > >> work. > >> > > > > > That being said, don't you rather want to just add this > >> argument to > >> > > > > > Initialize()? > >> > > > > > >> > > > > In Initialize() it defaults to using the process command line > >> > > arguments. > > > >> > It > >> > > > was > >> > > > > pointed out by zmo that adding default parameters is against the > >> style > >> > > guide, > >> > > > so > >> > > > > I followed how gl_manager had added a command_line parameter. > >> > > > > >> > > > My bad. I missed that Initialize() calls this with > >> > > > CommandLine::ForCurrentProcess(). > >> > > > So this method should be called InitializeForTesting(). > >> > > > >> > > I actually prefer the more descriptive name, but how about we meet in > >> the > >> > middle > >> > > and I just change the name to an overloaded Initialize with a > >> CommandLine > >> > > parameter. > >> > > >> > I'd just stick with InitializeWithCommandLine() then. But LGTM > >> otherwise. > >> > Can you file a bug to get this added to GN? (I'd prefer just have these > >> > > tests > > > >> as > >> > part of gfx_unittests somehow, rather than doing the whole dance of > >> adding > >> > another suite.) > >> > > > > I've changed it back to InitializeWithCommandLine(). > >> > > > > I don't think it's a GN bug, they intentionally do not allow circular > >> dependencies (gl depends on gfx, so gfx cannot depend on gl). If we were > >> going > >> to add it to GN we would have to do it the proper way and have gl have > >> its own > >> tests. > >> > > > > Feel free to add another unittest target and get it to run on the bots > > (incl. > > Android). I mainly wanted to save you some work, because I don't think it > > adds > > much benefit. The code in ui/gl even uses the 'gfx' namespace and there > > are no > > real serious dependency issues to be considered here (we are not changing > > any > > source file dependency rules or anything). This is mostly about build rule > > cosmetics. I think you just need to either define a separate source-file > > list > > for the tests and just inject it into the existing target, or you add a > > static > > target that you depend on. > > > Sorry, it sucks for the first person, but makes adding more tests much > nicer and avoids building up things that don't belong in the wrong place. Ok how about a 'ui_gl_unittests' target then?
On Mon, May 4, 2015 at 11:40 AM, <sievers@chromium.org> wrote: > On 2015/05/04 18:38:01, danakj wrote: > > On Mon, May 4, 2015 at 11:34 AM, <mailto:sievers@chromium.org> wrote: >> > > > On 2015/05/04 17:29:55, David Yen wrote: >> > >> >> On 2015/05/01 23:12:24, sievers wrote: >> >> > On 2015/05/01 22:59:35, David Yen wrote: >> >> > > >> >> > >> >> >> > >> > >> > >> > > > https://codereview.chromium.org/1110923003/diff/140001/ui/gl/gl_gl_api_implem... > >> > >> >> > > File ui/gl/gl_gl_api_implementation.h (right): >> >> > > >> >> > > >> >> > >> >> >> > >> > >> > >> > > > https://codereview.chromium.org/1110923003/diff/140001/ui/gl/gl_gl_api_implem... > >> > >> >> > > ui/gl/gl_gl_api_implementation.h:63: base::CommandLine* >> command_line); >> >> > > On 2015/05/01 22:48:35, sievers wrote: >> >> > > > On 2015/05/01 22:34:38, David Yen wrote: >> >> > > > > On 2015/05/01 21:55:00, sievers wrote: >> >> > > > > > So somebody has to call this in the production code for it to >> >> work. >> >> > > > > > That being said, don't you rather want to just add this >> >> argument to >> >> > > > > > Initialize()? >> >> > > > > >> >> > > > > In Initialize() it defaults to using the process command line >> >> >> > arguments. >> > >> >> > It >> >> > > > was >> >> > > > > pointed out by zmo that adding default parameters is against >> the >> >> style >> >> > > guide, >> >> > > > so >> >> > > > > I followed how gl_manager had added a command_line parameter. >> >> > > > >> >> > > > My bad. I missed that Initialize() calls this with >> >> > > > CommandLine::ForCurrentProcess(). >> >> > > > So this method should be called InitializeForTesting(). >> >> > > >> >> > > I actually prefer the more descriptive name, but how about we meet >> in >> >> the >> >> > middle >> >> > > and I just change the name to an overloaded Initialize with a >> >> CommandLine >> >> > > parameter. >> >> > >> >> > I'd just stick with InitializeWithCommandLine() then. But LGTM >> >> otherwise. >> >> > Can you file a bug to get this added to GN? (I'd prefer just have >> these >> >> >> > tests >> > >> >> as >> >> > part of gfx_unittests somehow, rather than doing the whole dance of >> >> adding >> >> > another suite.) >> >> >> > >> > I've changed it back to InitializeWithCommandLine(). >> >> >> > >> > I don't think it's a GN bug, they intentionally do not allow circular >> >> dependencies (gl depends on gfx, so gfx cannot depend on gl). If we >> were >> >> going >> >> to add it to GN we would have to do it the proper way and have gl have >> >> its own >> >> tests. >> >> >> > >> > Feel free to add another unittest target and get it to run on the bots >> > (incl. >> > Android). I mainly wanted to save you some work, because I don't think >> it >> > adds >> > much benefit. The code in ui/gl even uses the 'gfx' namespace and there >> > are no >> > real serious dependency issues to be considered here (we are not >> changing >> > any >> > source file dependency rules or anything). This is mostly about build >> rule >> > cosmetics. I think you just need to either define a separate source-file >> > list >> > for the tests and just inject it into the existing target, or you add a >> > static >> > target that you depend on. >> > > > Sorry, it sucks for the first person, but makes adding more tests much >> nicer and avoids building up things that don't belong in the wrong place. >> > > > Ok how about a 'ui_gl_unittests' target then? > Or gl_unittests? Similar to gfx_unittests. > > > > > https://codereview.chromium.org/1110923003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/05/04 18:44:56, danakj wrote:> > > > > Ok how about a 'ui_gl_unittests' target then? > > > > Or gl_unittests? Similar to gfx_unittests. > Yes, that works too. Note that there is also 'gl_tests' which are end-to-end command-buffer tests (gpu/command_buffer/tests), which use real GL, but I guess at the end of the day it's pretty easy to see what does what. Later, we should add GL mocking setup to gl_unittests, similar to gpu_unittests.
The bots have finally been setup to run gl_unittests including the android trybots. I just reran the android trybot and it ran the test correctly. PTAL!
dyen@chromium.org changed reviewers: + dpranke@chromium.org
+dpranke to review all.gyp, I just made it so gl_unittests does not build on iOS.
All.gyp change lgtm, though frankly I don't really understand where all chromium_builder_tests is used :).
lgtm thanks!
The CQ bit was checked by dyen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1110923003/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dyen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1110923003/320001
Message was sent while issue was closed.
Committed patchset #18 (id:320001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/5a6bcfe5cc034e908c310f3405b62bd35f6a8793 Cr-Commit-Position: refs/heads/master@{#329752} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
