Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1220883008:
Add command_buffer_gles2 (Closed)
DescriptionAdd command_buffer_gles2
This is a very simple library that exposes some egl-like functions that
can be used by skia to use the command buffer as a backend for their
tests
Committed: https://crrev.com/604b615373e4e9f873db37cef9c6416176fc35f3
Cr-Commit-Position: refs/heads/master@{#346251}
Committed: https://crrev.com/efa3a446183fbbff2034a03989f12543d0713e39
Cr-Commit-Position: refs/heads/master@{#346840}
Committed: https://crrev.com/d65c55dac3f47e9a237579eb6f5fe31e19ca4704
Cr-Commit-Position: refs/heads/master@{#347013}
Committed: https://crrev.com/a89ffff5113b3feb0c905f7ddc50da84a4ed577b
Cr-Commit-Position: refs/heads/master@{#347103}
Committed: https://crrev.com/68d2c333812a14fbb81136f401bfbfe7cf915d7d
Cr-Commit-Position: refs/heads/master@{#347277}
Patch Set 1 #Patch Set 2 : make eglInitialize not call InitializeOneOff more than once. #Patch Set 3 : needed an additional function for skia #
Total comments: 25
Patch Set 4 : review comments #Patch Set 5 : remove comformance deps #Patch Set 6 : comments to build files #
Total comments: 2
Patch Set 7 : add missing file #Patch Set 8 : Simplify by exporting the egl functions #
Total comments: 1
Patch Set 9 : remove test code #Patch Set 10 : GN needs deps of deps #Patch Set 11 : GN didn't like that, let's just include base #Patch Set 12 : gn_check.py, why couldn't you tell me about these the first time? #Patch Set 13 : gn_check.py, I'm really not liking you atm #Patch Set 14 : remove exit manager from confromance tests #Patch Set 15 : uh, remove gpu helpers from the gn file? #Patch Set 16 : move some deps into !is_component #Patch Set 17 : make a new define to enable the exit manager only for this lib #Patch Set 18 : fixed comment #Patch Set 19 : fixed another comment #
Total comments: 1
Depends on Patchset: Messages
Total messages: 94 (32 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
hendrikw@chromium.org changed reviewers: + piman@chromium.org
PTAL, thanks
hendrikw@chromium.org changed reviewers: + dpranke@chromium.org, sievers@chromium.org
+ dpranke for base gyp/gn changes + sievers for gpu changes (piman's out until monday)
build changes lgtm
https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... File gpu/command_buffer_gles2/BUILD.gn (right): https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... gpu/command_buffer_gles2/BUILD.gn:24: configs += [ "//build/config/compiler:no_chromium_code" ] Is this not the opposite of "'chromium_code': 1" in the corresponding gyp file? https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... File gpu/command_buffer_gles2/command_buffer_egl.cc (right): https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... gpu/command_buffer_gles2/command_buffer_egl.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: 'Copyright 2015' https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... gpu/command_buffer_gles2/command_buffer_egl.cc:12: base::LazyInstance<base::AtExitManager> exit_manager = That's interesting. Doesn't LazyInstance register with AtExitManager to delete the instance? :) https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... gpu/command_buffer_gles2/command_buffer_egl.cc:17: EGLDisplay GPU_COMMAND_BUFER_EGL_EXPORT nit: GPU_COMMAND_BUFER_EGL_EXPORT -> GPU_COMMAND_BUFFER_EGL_EXPORT https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... File gpu/command_buffer_gles2/command_buffer_gles2.gyp (right): https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... gpu/command_buffer_gles2/command_buffer_gles2.gyp:1: # Copyright (c) 2015 The Chromium Authors. All rights reserved. nit: drop '(c)' https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... gpu/command_buffer_gles2/command_buffer_gles2.gyp:16: '../../gpu/gpu.gyp:gles2_c_lib', So how are you planning on using this overall? This doesn't seem to be pulling in all the cmdbuffer client code. And where will the service be? https://codereview.chromium.org/1220883008/diff/100001/gpu/gles2_conform_supp... File gpu/gles2_conform_support/egl/egl.cc (right): https://codereview.chromium.org/1220883008/diff/100001/gpu/gles2_conform_supp... gpu/gles2_conform_support/egl/egl.cc:105: // eglInitialze can be called multiple times, prevent InitializeOneOff from nit: s/eglInitialze/eglInitialize https://codereview.chromium.org/1220883008/diff/100001/gpu/gles2_conform_supp... gpu/gles2_conform_support/egl/egl.cc:106: // being called multiple times. Actually InitializeOneOff() already seems to handle being called multiple times, see gl_surface_glx/egl/wgl.cc
PTAL, thanks! https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... File gpu/command_buffer_gles2/BUILD.gn (right): https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... gpu/command_buffer_gles2/BUILD.gn:24: configs += [ "//build/config/compiler:no_chromium_code" ] On 2015/08/27 19:21:28, sievers wrote: > Is this not the opposite of "'chromium_code': 1" in the corresponding gyp file? Good catch. https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... File gpu/command_buffer_gles2/command_buffer_egl.cc (right): https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... gpu/command_buffer_gles2/command_buffer_egl.cc:12: base::LazyInstance<base::AtExitManager> exit_manager = On 2015/08/27 19:21:28, sievers wrote: > That's interesting. Doesn't LazyInstance register with AtExitManager to delete > the instance? :) Huh, yeah, don't know why I didn't think of that :D I did this because we have a policy to make everything lazy, usually the exit_manager is created on the stack in main, but as I'm a shared library, I have no main. I'm not sure what's the right approach, maybe I should declare the exit_manager without the LazyInstance? Probably... If I change it, it will create the exit manager when the library is loaded rather than when CommandBuffer_Initialize. Or maybe "static base::AtExitManager exit_manager" inside of CommandBuffer_Initialize is the right approach? That approach may not be thread safe (depending on compiler flags). What do you think? https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... gpu/command_buffer_gles2/command_buffer_egl.cc:17: EGLDisplay GPU_COMMAND_BUFER_EGL_EXPORT On 2015/08/27 19:21:28, sievers wrote: > nit: GPU_COMMAND_BUFER_EGL_EXPORT -> GPU_COMMAND_BUFFER_EGL_EXPORT ew, bad. Done, Thanks! https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... File gpu/command_buffer_gles2/command_buffer_gles2.gyp (right): https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... gpu/command_buffer_gles2/command_buffer_gles2.gyp:1: # Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/08/27 19:21:28, sievers wrote: > nit: drop '(c)' Done. https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... gpu/command_buffer_gles2/command_buffer_gles2.gyp:16: '../../gpu/gpu.gyp:gles2_c_lib', On 2015/08/27 19:21:28, sievers wrote: > So how are you planning on using this overall? > This doesn't seem to be pulling in all the cmdbuffer client code. > And where will the service be? I started out with more dependencies, and pulled the ones I didn't need out. I don't think this needs the client code. BTW, Skia's DM, nanobench, SampleApp, and maybe Visual dm will be the clients. You can see the changes made to skia to allow this here: https://codereview.chromium.org/1306823003/ https://codereview.chromium.org/1220883008/diff/100001/gpu/gles2_conform_supp... File gpu/gles2_conform_support/egl/egl.cc (right): https://codereview.chromium.org/1220883008/diff/100001/gpu/gles2_conform_supp... gpu/gles2_conform_support/egl/egl.cc:105: // eglInitialze can be called multiple times, prevent InitializeOneOff from On 2015/08/27 19:21:28, sievers wrote: > nit: s/eglInitialze/eglInitialize Done. https://codereview.chromium.org/1220883008/diff/100001/gpu/gles2_conform_supp... gpu/gles2_conform_support/egl/egl.cc:106: // being called multiple times. On 2015/08/27 19:21:28, sievers wrote: > Actually InitializeOneOff() already seems to handle being called multiple times, > see gl_surface_glx/egl/wgl.cc It DCHECKS if called more than once because of DCHECK_EQ(kGLImplementationNone, GetGLImplementation()), and in InitializeStaticGLBindings, it dchecks a second time. The DCHECK is there to prevent the initialization from changing GL implementations in tests, but in my case, it prevents InitializeOneOff from being called multiple times. For my use case, wiring this off should be fine.
https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... File gpu/command_buffer_gles2/command_buffer_egl.cc (right): https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... gpu/command_buffer_gles2/command_buffer_egl.cc:12: base::LazyInstance<base::AtExitManager> exit_manager = On 2015/08/27 20:34:44, hendrikw wrote: > On 2015/08/27 19:21:28, sievers wrote: > > That's interesting. Doesn't LazyInstance register with AtExitManager to delete > > the instance? :) > > Huh, yeah, don't know why I didn't think of that :D > > I did this because we have a policy to make everything lazy, usually the > exit_manager is created on the stack in main, but as I'm a shared library, I > have no main. > > I'm not sure what's the right approach, maybe I should declare the exit_manager > without the LazyInstance? > > Probably... > > If I change it, it will create the exit manager when the library is loaded > rather than when CommandBuffer_Initialize. Or maybe "static base::AtExitManager > exit_manager" inside of CommandBuffer_Initialize is the right approach? That > approach may not be thread safe (depending on compiler flags). > > What do you think? Does it work if you just allocate and free it from Initialize/Terminate respectively? https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... File gpu/command_buffer_gles2/command_buffer_gles2.gyp (right): https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... gpu/command_buffer_gles2/command_buffer_gles2.gyp:15: '../../gpu/gles2_conform_support/gles2_conform_support.gyp:egl_native', This actually also depends on all the GPU stuff. But it pulls in gles2_implementation_no_check, while below depends on gles2_implementation. Doesn't that lead to duplicate symbols for everything? It also feels like an abuse to use the conformance test stuff here. Does it have the same effect to pull in the needed stuff from the 'egl_native' dependency list here directly? https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... gpu/command_buffer_gles2/command_buffer_gles2.gyp:16: '../../gpu/gpu.gyp:gles2_c_lib', On 2015/08/27 20:34:44, hendrikw wrote: > On 2015/08/27 19:21:28, sievers wrote: > > So how are you planning on using this overall? > > This doesn't seem to be pulling in all the cmdbuffer client code. > > And where will the service be? > > I started out with more dependencies, and pulled the ones I didn't need out. > > I don't think this needs the client code. > > BTW, Skia's DM, nanobench, SampleApp, and maybe Visual dm will be the clients. > > You can see the changes made to skia to allow this here: > https://codereview.chromium.org/1306823003/ Ah because gles2_c_lib already depends on command_buffer_client. I'd double-check that that works for GN though. So once skia writes stuff in the command buffer, what's supposed to happen with the commands in the buffer?
PTAL, thanks! https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... File gpu/command_buffer_gles2/command_buffer_egl.cc (right): https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... gpu/command_buffer_gles2/command_buffer_egl.cc:12: base::LazyInstance<base::AtExitManager> exit_manager = On 2015/08/27 22:02:25, sievers wrote: > On 2015/08/27 20:34:44, hendrikw wrote: > > On 2015/08/27 19:21:28, sievers wrote: > > > That's interesting. Doesn't LazyInstance register with AtExitManager to > delete > > > the instance? :) > > > > Huh, yeah, don't know why I didn't think of that :D > > > > I did this because we have a policy to make everything lazy, usually the > > exit_manager is created on the stack in main, but as I'm a shared library, I > > have no main. > > > > I'm not sure what's the right approach, maybe I should declare the > exit_manager > > without the LazyInstance? > > > > Probably... > > > > If I change it, it will create the exit manager when the library is loaded > > rather than when CommandBuffer_Initialize. Or maybe "static > base::AtExitManager > > exit_manager" inside of CommandBuffer_Initialize is the right approach? That > > approach may not be thread safe (depending on compiler flags). > > > > What do you think? > > Does it work if you just allocate and free it from Initialize/Terminate > respectively? Not really, the pair of functions are called once for every nanobench test. https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... File gpu/command_buffer_gles2/command_buffer_gles2.gyp (right): https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... gpu/command_buffer_gles2/command_buffer_gles2.gyp:15: '../../gpu/gles2_conform_support/gles2_conform_support.gyp:egl_native', On 2015/08/27 22:02:25, sievers wrote: > This actually also depends on all the GPU stuff. But it pulls in > gles2_implementation_no_check, while below depends on gles2_implementation. > Doesn't that lead to duplicate symbols for everything? > > It also feels like an abuse to use the conformance test stuff here. > Does it have the same effect to pull in the needed stuff from the 'egl_native' > dependency list here directly? Hmm, gles2_c_lib didn't have a dependency on gles2_implementation, I guess that's why I didn't have duplicate symbols. That said, it's still a bit weird to have a mix of _no_check. Including the conformance test source files directly is a great idea (though still seems like a bit of abuse). Done. Maybe we can later move the egl stuff out of the conform_support folder. https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... gpu/command_buffer_gles2/command_buffer_gles2.gyp:16: '../../gpu/gpu.gyp:gles2_c_lib', On 2015/08/27 22:02:25, sievers wrote: > On 2015/08/27 20:34:44, hendrikw wrote: > > On 2015/08/27 19:21:28, sievers wrote: > > > So how are you planning on using this overall? > > > This doesn't seem to be pulling in all the cmdbuffer client code. > > > And where will the service be? > > > > I started out with more dependencies, and pulled the ones I didn't need out. > > > > I don't think this needs the client code. > > > > BTW, Skia's DM, nanobench, SampleApp, and maybe Visual dm will be the clients. > > > > You can see the changes made to skia to allow this here: > > https://codereview.chromium.org/1306823003/ > > Ah because gles2_c_lib already depends on command_buffer_client. I'd > double-check that that works for GN though. > > So once skia writes stuff in the command buffer, what's supposed to happen with > the commands in the buffer? They get played :D (Flush->ProcessCommands->Decode->ANGLE)
https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... File gpu/command_buffer_gles2/command_buffer_egl.cc (right): https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... gpu/command_buffer_gles2/command_buffer_egl.cc:12: base::LazyInstance<base::AtExitManager> exit_manager = On 2015/08/28 16:01:51, hendrikw wrote: > On 2015/08/27 22:02:25, sievers wrote: > > On 2015/08/27 20:34:44, hendrikw wrote: > > > On 2015/08/27 19:21:28, sievers wrote: > > > > That's interesting. Doesn't LazyInstance register with AtExitManager to > > delete > > > > the instance? :) > > > > > > Huh, yeah, don't know why I didn't think of that :D > > > > > > I did this because we have a policy to make everything lazy, usually the > > > exit_manager is created on the stack in main, but as I'm a shared library, I > > > have no main. > > > > > > I'm not sure what's the right approach, maybe I should declare the > > exit_manager > > > without the LazyInstance? > > > > > > Probably... > > > > > > If I change it, it will create the exit manager when the library is loaded > > > rather than when CommandBuffer_Initialize. Or maybe "static > > base::AtExitManager > > > exit_manager" inside of CommandBuffer_Initialize is the right approach? > That > > > approach may not be thread safe (depending on compiler flags). > > > > > > What do you think? > > > > Does it work if you just allocate and free it from Initialize/Terminate > > respectively? > > Not really, the pair of functions are called once for every nanobench test. How about making your main shell/test setup function responsible for creating the AtExitManager on the stack then, and just assume it's here in this code? I think that's what we'd usually do. https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... File gpu/command_buffer_gles2/command_buffer_gles2.gyp (right): https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... gpu/command_buffer_gles2/command_buffer_gles2.gyp:15: '../../gpu/gles2_conform_support/gles2_conform_support.gyp:egl_native', On 2015/08/28 16:01:51, hendrikw wrote: > Maybe we can later move the egl stuff out of the conform_support folder. Sounds good. You should probably put a TODO for that next to the included conform_support sources in here and GN.
https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... File gpu/command_buffer_gles2/command_buffer_egl.cc (right): https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... gpu/command_buffer_gles2/command_buffer_egl.cc:12: base::LazyInstance<base::AtExitManager> exit_manager = On 2015/08/28 17:59:31, sievers wrote: > On 2015/08/28 16:01:51, hendrikw wrote: > > On 2015/08/27 22:02:25, sievers wrote: > > > On 2015/08/27 20:34:44, hendrikw wrote: > > > > On 2015/08/27 19:21:28, sievers wrote: > > > > > That's interesting. Doesn't LazyInstance register with AtExitManager to > > > delete > > > > > the instance? :) > > > > > > > > Huh, yeah, don't know why I didn't think of that :D > > > > > > > > I did this because we have a policy to make everything lazy, usually the > > > > exit_manager is created on the stack in main, but as I'm a shared library, > I > > > > have no main. > > > > > > > > I'm not sure what's the right approach, maybe I should declare the > > > exit_manager > > > > without the LazyInstance? > > > > > > > > Probably... > > > > > > > > If I change it, it will create the exit manager when the library is loaded > > > > rather than when CommandBuffer_Initialize. Or maybe "static > > > base::AtExitManager > > > > exit_manager" inside of CommandBuffer_Initialize is the right approach? > > That > > > > approach may not be thread safe (depending on compiler flags). > > > > > > > > What do you think? > > > > > > Does it work if you just allocate and free it from Initialize/Terminate > > > respectively? > > > > Not really, the pair of functions are called once for every nanobench test. > > How about making your main shell/test setup function responsible for creating > the AtExitManager on the stack then, and just assume it's here in this code? > I think that's what we'd usually do. You mean Skia's testing framework? It doesn't have any knowledge of this (and doesn't link to base). https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... File gpu/command_buffer_gles2/command_buffer_gles2.gyp (right): https://codereview.chromium.org/1220883008/diff/100001/gpu/command_buffer_gle... gpu/command_buffer_gles2/command_buffer_gles2.gyp:15: '../../gpu/gles2_conform_support/gles2_conform_support.gyp:egl_native', On 2015/08/28 17:59:31, sievers wrote: > On 2015/08/28 16:01:51, hendrikw wrote: > > Maybe we can later move the egl stuff out of the conform_support folder. > > Sounds good. You should probably put a TODO for that next to the included > conform_support sources in here and GN. Done.
lgtm https://codereview.chromium.org/1220883008/diff/160001/gpu/command_buffer_gle... File gpu/command_buffer_gles2/command_buffer_egl.cc (right): https://codereview.chromium.org/1220883008/diff/160001/gpu/command_buffer_gle... gpu/command_buffer_gles2/command_buffer_egl.cc:11: base::AtExitManager exit_manager; Ok, so you don't want to expose this detail to skia for it to put the instance on the main stack frame. You could still try to create it on the heap from eglInitialize() and clean it up in Terminate(). Maybe it's a bit closer to what happens in other places where it's on the main stackframe. (Note that the AtExitManager is annotated with comments that say it should be used that way.) Maybe it doesn't matter here - you could try like this and revisit if you run into problems too. https://codereview.chromium.org/1220883008/diff/160001/gpu/gles2_conform_supp... File gpu/gles2_conform_support/egl/egl.cc (right): https://codereview.chromium.org/1220883008/diff/160001/gpu/gles2_conform_supp... gpu/gles2_conform_support/egl/egl.cc:107: if (gfx::GetGLImplementation() == gfx::kGLImplementationNone) { I think you can just run this code every time if you call gfx::ClearGLBindings() form eglTerminate().
On 2015/08/28 18:39:33, sievers wrote: > lgtm > > https://codereview.chromium.org/1220883008/diff/160001/gpu/command_buffer_gle... > File gpu/command_buffer_gles2/command_buffer_egl.cc (right): > > https://codereview.chromium.org/1220883008/diff/160001/gpu/command_buffer_gle... > gpu/command_buffer_gles2/command_buffer_egl.cc:11: base::AtExitManager > exit_manager; > Ok, so you don't want to expose this detail to skia for it to put the instance > on the main stack frame. > > You could still try to create it on the heap from eglInitialize() and clean it > up in Terminate(). Maybe it's a bit closer to what happens in other places where > it's on the main stackframe. (Note that the AtExitManager is annotated with > comments that say it should be used that way.) Maybe it doesn't matter here - > you could try like this and revisit if you run into problems too. > > https://codereview.chromium.org/1220883008/diff/160001/gpu/gles2_conform_supp... > File gpu/gles2_conform_support/egl/egl.cc (right): > > https://codereview.chromium.org/1220883008/diff/160001/gpu/gles2_conform_supp... > gpu/gles2_conform_support/egl/egl.cc:107: if (gfx::GetGLImplementation() == > gfx::kGLImplementationNone) { > I think you can just run this code every time if you call gfx::ClearGLBindings() > form eglTerminate(). I think it's a good idea to clear the bindings in terminate, but in GLContext::InitializeDynamicBindings() we have "static bool initialized = false". So we never recreate them. I'll land what I have, and revisit this when I have some time.
On 2015/08/28 21:01:49, hendrikw wrote: > On 2015/08/28 18:39:33, sievers wrote: > > lgtm > > > > > https://codereview.chromium.org/1220883008/diff/160001/gpu/command_buffer_gle... > > File gpu/command_buffer_gles2/command_buffer_egl.cc (right): > > > > > https://codereview.chromium.org/1220883008/diff/160001/gpu/command_buffer_gle... > > gpu/command_buffer_gles2/command_buffer_egl.cc:11: base::AtExitManager > > exit_manager; > > Ok, so you don't want to expose this detail to skia for it to put the instance > > on the main stack frame. > > > > You could still try to create it on the heap from eglInitialize() and clean it > > up in Terminate(). Maybe it's a bit closer to what happens in other places > where > > it's on the main stackframe. (Note that the AtExitManager is annotated with > > comments that say it should be used that way.) Maybe it doesn't matter here - > > you could try like this and revisit if you run into problems too. > > > > > https://codereview.chromium.org/1220883008/diff/160001/gpu/gles2_conform_supp... > > File gpu/gles2_conform_support/egl/egl.cc (right): > > > > > https://codereview.chromium.org/1220883008/diff/160001/gpu/gles2_conform_supp... > > gpu/gles2_conform_support/egl/egl.cc:107: if (gfx::GetGLImplementation() == > > gfx::kGLImplementationNone) { > > I think you can just run this code every time if you call > gfx::ClearGLBindings() > > form eglTerminate(). > > I think it's a good idea to clear the bindings in terminate, but in > GLContext::InitializeDynamicBindings() we have "static bool initialized = > false". So we never recreate them. I'll land what I have, and revisit this > when I have some time. Logged crbug.com/526281 to track it.
The CQ bit was checked by hendrikw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/1220883008/#ps160001 (title: "comments to build files")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220883008/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1220883008/160001
Message was sent while issue was closed.
Committed patchset #6 (id:160001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/604b615373e4e9f873db37cef9c6416176fc35f3 Cr-Commit-Position: refs/heads/master@{#346251}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:160001) has been created in https://codereview.chromium.org/1323643002/ by hendrikw@chromium.org. The reason for reverting is: Broken build.
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:160001) has been created in https://codereview.chromium.org/1320963003/ by dewittj@chromium.org. The reason for reverting is: Compile failure: FAILED: /b/build/goma/gomacc /b/build/slave/Android_Debug__Nexus_6_/build/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-g++ -MMD -MF obj/gpu/command_buffer_gles2/command_buffer_gles2.command_buffer_egl.o.d -DV8_DEPRECATION_WARNINGS -DCLD_VERSION=1 -D_FILE_OFFSET_BITS=64 -DNO_TCMALLOC -DDISABLE_NACL -DCHROMIUM_BUILD -DCR_CLANG_REVISION=245965-1 -DUSE_LIBJPEG_TURBO=1 -DENABLE_WEBRTC=1 -DENABLE_MEDIA_ROUTER=1 -DUSE_PROPRIETARY_CODECS -DENABLE_BROWSER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE -DDONT_EMBED_BUILD_METADATA -DFIELDTRIAL_TESTING_ENABLED -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1 -DENABLE_SPELLCHECK=1 -DUSE_BROWSER_SPELLCHECKER=1 -DENABLE_SUPERVISED_USERS=1 -DVIDEO_HOLE=1 -DV8_USE_EXTERNAL_STARTUP_DATA -DENABLE_WEBVR -DMOBILE_SAFE_BROWSING -DSAFE_BROWSING_DB_REMOTE -DSAFE_BROWSING_SERVICE -DEGLAPI= -DEGLAPIENTRY= -DMESA_EGL_NO_X11_HEADERS -DUSE_LIBPCI=1 -DUSE_OPENSSL=1 -DUSE_OPENSSL_CERTS=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DANDROID -D__GNU_SOURCE=1 '-DCHROME_BUILD_ID=""' -DHAVE_SYS_UIO_H -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -D_DEBUG -Igen -I../../third_party/khronos -I../../gpu -I../.. -I../../skia/config -Igen/angle -I../../third_party/mesa/src/include -fstack-protector --param=ssp-buffer-size=4 -Werror -fno-strict-aliasing -Wall -Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden -pipe -fPIC -Wno-unused-local-typedefs -march=armv7-a -mtune=generic-armv7-a -mfpu=vfpv3-d16 -mfloat-abi=softfp -mthumb -fno-tree-sra -fno-caller-saves -Wno-psabi -mthumb-interwork -ffunction-sections -funwind-tables -g -fstack-protector -fno-short-enums -finline-limit=64 --sysroot=../../third_party/android_tools/ndk//platforms/android-16/arch-arm -isystem../../third_party/android_tools/ndk//sources/cxx-stl/llvm-libc++/libcxx/include -isystem../../third_party/android_tools/ndk//sources/cxx-stl/llvm-libc++abi/libcxxabi/include -isystem../../third_party/android_tools/ndk//sources/android/support/include -Os -g -fdata-sections -ffunction-sections -fomit-frame-pointer -funwind-tables -g0 -fno-exceptions -fno-rtti -fno-threadsafe-statics -fvisibility-inlines-hidden -Wsign-compare -Wno-abi -std=gnu++11 -Wno-narrowing -Wno-literal-suffix -c ../../gpu/command_buffer_gles2/command_buffer_egl.cc -o obj/gpu/command_buffer_gles2/command_buffer_gles2.command_buffer_egl.o ../../gpu/command_buffer_gles2/command_buffer_egl.cc:8:66: fatal error: gpu/command_buffer_gles2/command_buffer_gles2_export.h: No such file or directory #include "gpu/command_buffer_gles2/command_buffer_gles2_export.h" ^ https://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus....
On 2015/08/28 22:54:20, dewittj wrote: > A revert of this CL (patchset #6 id:160001) has been created in > https://codereview.chromium.org/1320963003/ by mailto:dewittj@chromium.org. > > The reason for reverting is: Compile failure: > > FAILED: /b/build/goma/gomacc > /b/build/slave/Android_Debug__Nexus_6_/build/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-g++ > -MMD -MF > obj/gpu/command_buffer_gles2/command_buffer_gles2.command_buffer_egl.o.d > -DV8_DEPRECATION_WARNINGS -DCLD_VERSION=1 -D_FILE_OFFSET_BITS=64 -DNO_TCMALLOC > -DDISABLE_NACL -DCHROMIUM_BUILD -DCR_CLANG_REVISION=245965-1 > -DUSE_LIBJPEG_TURBO=1 -DENABLE_WEBRTC=1 -DENABLE_MEDIA_ROUTER=1 > -DUSE_PROPRIETARY_CODECS -DENABLE_BROWSER_CDMS -DENABLE_CONFIGURATION_POLICY > -DENABLE_NOTIFICATIONS -DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE > -DDONT_EMBED_BUILD_METADATA -DFIELDTRIAL_TESTING_ENABLED > -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1 > -DENABLE_SPELLCHECK=1 -DUSE_BROWSER_SPELLCHECKER=1 -DENABLE_SUPERVISED_USERS=1 > -DVIDEO_HOLE=1 -DV8_USE_EXTERNAL_STARTUP_DATA -DENABLE_WEBVR > -DMOBILE_SAFE_BROWSING -DSAFE_BROWSING_DB_REMOTE -DSAFE_BROWSING_SERVICE > -DEGLAPI= -DEGLAPIENTRY= -DMESA_EGL_NO_X11_HEADERS -DUSE_LIBPCI=1 > -DUSE_OPENSSL=1 -DUSE_OPENSSL_CERTS=1 -D__STDC_CONSTANT_MACROS > -D__STDC_FORMAT_MACROS -DANDROID -D__GNU_SOURCE=1 '-DCHROME_BUILD_ID=""' > -DHAVE_SYS_UIO_H -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 > -D_DEBUG -Igen -I../../third_party/khronos -I../../gpu -I../.. > -I../../skia/config -Igen/angle -I../../third_party/mesa/src/include > -fstack-protector --param=ssp-buffer-size=4 -Werror -fno-strict-aliasing -Wall > -Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden -pipe > -fPIC -Wno-unused-local-typedefs -march=armv7-a -mtune=generic-armv7-a > -mfpu=vfpv3-d16 -mfloat-abi=softfp -mthumb -fno-tree-sra -fno-caller-saves > -Wno-psabi -mthumb-interwork -ffunction-sections -funwind-tables -g > -fstack-protector -fno-short-enums -finline-limit=64 > --sysroot=../../third_party/android_tools/ndk//platforms/android-16/arch-arm > -isystem../../third_party/android_tools/ndk//sources/cxx-stl/llvm-libc++/libcxx/include > -isystem../../third_party/android_tools/ndk//sources/cxx-stl/llvm-libc++abi/libcxxabi/include > -isystem../../third_party/android_tools/ndk//sources/android/support/include -Os > -g -fdata-sections -ffunction-sections -fomit-frame-pointer -funwind-tables -g0 > -fno-exceptions -fno-rtti -fno-threadsafe-statics -fvisibility-inlines-hidden > -Wsign-compare -Wno-abi -std=gnu++11 -Wno-narrowing -Wno-literal-suffix -c > ../../gpu/command_buffer_gles2/command_buffer_egl.cc -o > obj/gpu/command_buffer_gles2/command_buffer_gles2.command_buffer_egl.o > ../../gpu/command_buffer_gles2/command_buffer_egl.cc:8:66: fatal error: > gpu/command_buffer_gles2/command_buffer_gles2_export.h: No such file or > directory > #include "gpu/command_buffer_gles2/command_buffer_gles2_export.h" > ^ > > https://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus.... Somehow a non-compiling build slipped through the commit queue. See https://code.google.com/p/chromium/issues/detail?id=526310. I've uploaded the missing file, and I'll try to reland. Hopefully nothing else breaks.
@sievers, piman, PTAL After speaking with piman, he suggested that I instead export the egl functions, originally I couldn't do this because I had additional crap in the init/terminate, but since that's all now gone, this is possible. There's a couple of questionable things with this change: 1. The build file define the EGLAPI stuff based on the os. I tried leaving it as the default, but it's not set as an export (and EGLAPIENTRY defaults to __stdcall, and I wanted to avoid the name mangling). 2. I put the exit_manager as a member of the display. The display is called before the initialize, and destroyed as the last step of terminate, so the lifetime extends slightly before the initialize/terminate pair. I think this is ok, let me know what you think. Thanks!
https://codereview.chromium.org/1220883008/diff/200001/gpu/gles2_conform_supp... File gpu/gles2_conform_support/egl/egl.cc (right): https://codereview.chromium.org/1220883008/diff/200001/gpu/gles2_conform_supp... gpu/gles2_conform_support/egl/egl.cc:435: __declspec(dllexport) void f(); Leftover from something ?
On 2015/09/01 03:57:17, piman (OOO back 2015-08-31) wrote: > https://codereview.chromium.org/1220883008/diff/200001/gpu/gles2_conform_supp... > File gpu/gles2_conform_support/egl/egl.cc (right): > > https://codereview.chromium.org/1220883008/diff/200001/gpu/gles2_conform_supp... > gpu/gles2_conform_support/egl/egl.cc:435: __declspec(dllexport) void f(); > Leftover from something ? haha, yeah, test to code to figure out what the macros expand to, gone!
lgtm
The CQ bit was checked by hendrikw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1220883008/#ps210006 (title: "remove test code")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220883008/210006 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1220883008/210006
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hendrikw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220883008/210006 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1220883008/210006
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/09/01 18:31:03, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Hmm, awesome, these tests fail on my linux machine without my changes :/
On 2015/09/01 21:20:40, hendrikw wrote: > On 2015/09/01 18:31:03, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > Hmm, awesome, these tests fail on my linux machine without my changes :/ Looks like a problem with mojo, appropriate people have been notified.
The CQ bit was checked by hendrikw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220883008/210006 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1220883008/210006
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hendrikw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, sievers@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1220883008/#ps230001 (title: "GN needs deps of deps")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220883008/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1220883008/230001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hendrikw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, sievers@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1220883008/#ps250001 (title: "GN didn't like that, let's just include base")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220883008/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1220883008/250001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hendrikw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, sievers@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1220883008/#ps270001 (title: "gn_check.py, why couldn't you tell me about these the first time?")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220883008/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1220883008/270001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hendrikw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, sievers@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1220883008/#ps290001 (title: "gn_check.py, I'm really not liking you atm")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220883008/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1220883008/290001
Message was sent while issue was closed.
Committed patchset #13 (id:290001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/efa3a446183fbbff2034a03989f12543d0713e39 Cr-Commit-Position: refs/heads/master@{#346840}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:290001) has been created in https://codereview.chromium.org/1319453005/ by jmadill@chromium.org. The reason for reverting is: Broke the gles2_conform_tests, visible on the GPU FYI waterfall..
This failed because of the exit manager that I added to display. There's a DCHECK that triggers if you create multiple exit managers on a non-component build.
On 2015/09/02 16:48:30, hendrikw wrote: > This failed because of the exit manager that I added to display. There's a > DCHECK that triggers if you create multiple exit managers on a non-component > build. ugh. There's a few things I could do, none of them nice. 1. Remove the DCHECK, I assume that this is here to notify us when we try to create more than one and we don't need to. 2. Move the exit manager to a separate file that creates an instance once globally for command_buffer_gles2 only 3. Add the ability to ask the exit manager if you exist, and if so, don't create another once. That would break if someone called init, init, terminate, terminate (which is likely not legal anyway, but still) 4. #ifndef GLES2_CONFORM_SUPPORT_ONLY the exit manager out in display. 5. Rewrite a bunch of code to not require the exit manager for anything command buffer related (may not be possible) 6. something I haven't thought of yet? For the time being I'm going with 4.
PTAL, :(
LGTM
The CQ bit was checked by hendrikw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1220883008/#ps310001 (title: "remove exit manager from confromance tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220883008/310001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1220883008/310001
Message was sent while issue was closed.
Committed patchset #14 (id:310001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/d65c55dac3f47e9a237579eb6f5fe31e19ca4704 Cr-Commit-Position: refs/heads/master@{#347013}
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:310001) has been created in https://codereview.chromium.org/1318933006/ by thakis@chromium.org. The reason for reverting is: Doesn't build on 64-bit Windows gn: http://build.chromium.org/p/chromium.fyi/builders/CrWinClang64%28dbg%29/build... FAILED: C:/b/depot_tools/python276_bin/python.exe gyp-win-tool link-wrapper environment.x64 False link.exe /nologo /IMPLIB:./command_buffer_gles2.dll.lib /DLL /OUT:./command_buffer_gles2.dll /PDB:./command_buffer_gles2.dll.pdb @./command_buffer_gles2.dll.rsp gpu.dll.lib(gpu.dll) :error LNK2005: "public: __cdecl gpu::gles2::GLES2CmdHelper::GLES2CmdHelper(class gpu::CommandBuffer *)" (??0GLES2CmdHelper@gles2@gpu@@QEAA@PEAVCommandBuffer@2@@Z) already defined in gles2_cmd_helper.obj gpu.dll.lib(gpu.dll) :error LNK2005: "public: bool __cdecl gpu::CommandBufferHelper::Initialize(int)" (?Initialize@CommandBufferHelper@gpu@@QEAA_NH@Z) already defined in cmd_buffer_helper.obj gpu.dll.lib(gpu.dll) :error LNK2005: "public: __cdecl gpu::TransferBuffer::TransferBuffer(class gpu::CommandBufferHelper *)" (??0TransferBuffer@gpu@@QEAA@PEAVCommandBufferHelper@1@@Z) already defined in transfer_buffer.obj gpu.dll.lib(gpu.dll) :error LNK2005: "public: void __cdecl base::RefCounted<class gpu::ValueStateMap>::Release(void)const " (?Release@?$RefCounted@VValueStateMap@gpu@@@base@@QEBAXXZ) already defined in value_state.obj gpu.dll.lib(gpu.dll) :error LNK2005: "public: void __cdecl base::RefCounted<class gpu::ValueStateMap>::AddRef(void)const " (?AddRef@?$RefCounted@VValueStateMap@gpu@@@base@@QEBAXXZ) already defined in value_state.obj .
Wow, ok, so, I've tried gn and gyp with both component and shared, all compile now. Hopefully, NO MORE REVERTS!
The CQ bit was checked by hendrikw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, sievers@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1220883008/#ps350001 (title: "move some deps into !is_component")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220883008/350001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1220883008/350001
Message was sent while issue was closed.
Committed patchset #16 (id:350001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/a89ffff5113b3feb0c905f7ddc50da84a4ed577b Cr-Commit-Position: refs/heads/master@{#347103}
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:350001) has been created in https://codereview.chromium.org/1311583006/ by jmadill@chromium.org. The reason for reverting is: This breaks the gles2_conform_test in Linux Release, Mac Release and some Windows Release configurations..
Message was sent while issue was closed.
sigh. gles2_conform_support/egl doesn't have GLES2_CONFORM_SUPPORT_ONLY defined, but since it's closed source, I couldn't know this. I've added yet another define, and this is now how I'm enabling the exit manager only for this code. Really need to find a better solution.
This is getting a lot of reverts for such a simple change. I should probably figure out how to get the conformance tests on my machine, anyway, this should fix it. PTAL, thanks
lgtm
The CQ bit was checked by hendrikw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1220883008/#ps400001 (title: "fixed another comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220883008/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1220883008/400001
Message was sent while issue was closed.
Committed patchset #19 (id:400001)
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/68d2c333812a14fbb81136f401bfbfe7cf915d7d Cr-Commit-Position: refs/heads/master@{#347277}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1220883008/diff/400001/gpu/BUILD.gn File gpu/BUILD.gn (right): https://codereview.chromium.org/1220883008/diff/400001/gpu/BUILD.gn#newcode73 gpu/BUILD.gn:73: if (!is_component_build) { Why is this conditional here? This seems wrong at first blush. Why are these component-build deps only? If you need to do something like this, please add a comment that explains why.
Message was sent while issue was closed.
On 2015/12/21 16:48:47, Nico (office move Wed Thu Fri) wrote: > https://codereview.chromium.org/1220883008/diff/400001/gpu/BUILD.gn > File gpu/BUILD.gn (right): > > https://codereview.chromium.org/1220883008/diff/400001/gpu/BUILD.gn#newcode73 > gpu/BUILD.gn:73: if (!is_component_build) { > Why is this conditional here? This seems wrong at first blush. Why are these > component-build deps only? If you need to do something like this, please add a > comment that explains why. I have a fix for this here: https://codereview.chromium.org/1542683002 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
