Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
429863002:
Remove GLInProcessContext::CreateContext() (Closed)
|
Created:
6 years, 4 months ago by hj.r.chung Modified:
6 years, 4 months ago Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionRemove GLInProcessContext::CreateContext()
Change all CreateContext() calls to Context() and remove unnecessary code.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286952
Patch Set 1 #
Total comments: 12
Patch Set 2 : addressed comments #
Messages
Total messages: 19 (0 generated)
PTAL!
https://codereview.chromium.org/429863002/diff/1/gpu/command_buffer/client/gl... File gpu/command_buffer/client/gl_in_process_context.cc (left): https://codereview.chromium.org/429863002/diff/1/gpu/command_buffer/client/gl... gpu/command_buffer/client/gl_in_process_context.cc:127: gfx::AcceleratedWidget window, I don't think you compiled this for android. synchronous_compositor_factory_impl.cc needs this
https://codereview.chromium.org/429863002/diff/1/gpu/command_buffer/client/gl... File gpu/command_buffer/client/gl_in_process_context.cc (left): https://codereview.chromium.org/429863002/diff/1/gpu/command_buffer/client/gl... gpu/command_buffer/client/gl_in_process_context.cc:127: gfx::AcceleratedWidget window, On 2014/07/31 02:27:22, boliu wrote: > I don't think you compiled this for android. > synchronous_compositor_factory_impl.cc needs this I removed it because synchronous_compositor_factory_impl was just passing kNullAcceleratedWidget but if you think we should keep this I'll add it back
Oh oops, my bad. Misse that bit. https://codereview.chromium.org/429863002/diff/1/gpu/command_buffer/client/gl... File gpu/command_buffer/client/gl_in_process_context.cc (right): https://codereview.chromium.org/429863002/diff/1/gpu/command_buffer/client/gl... gpu/command_buffer/client/gl_in_process_context.cc:227: gfx::kNullAcceleratedWidget, You can clean up in in_process_command_buffer as well.
https://codereview.chromium.org/429863002/diff/1/cc/test/test_in_process_cont... File cc/test/test_in_process_context_provider.cc (right): https://codereview.chromium.org/429863002/diff/1/cc/test/test_in_process_cont... cc/test/test_in_process_context_provider.cc:39: scoped_refptr<gpu::InProcessCommandBuffer::Service>(), nit: NULL should work too. https://codereview.chromium.org/429863002/diff/1/webkit/common/gpu/webgraphic... File webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (left): https://codereview.chromium.org/429863002/diff/1/webkit/common/gpu/webgraphic... webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:153: window_, aren't there callers that pass a non-NULL window? ui::InProcessContextFactory::CreateOutputSurface seems to be one. https://codereview.chromium.org/429863002/diff/1/webkit/common/gpu/webgraphic... File webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right): https://codereview.chromium.org/429863002/diff/1/webkit/common/gpu/webgraphic... webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:151: scoped_refptr< ::gpu::InProcessCommandBuffer::Service>(), nit: NULL
https://codereview.chromium.org/429863002/diff/1/webkit/common/gpu/webgraphic... File webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (left): https://codereview.chromium.org/429863002/diff/1/webkit/common/gpu/webgraphic... webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:153: window_, On 2014/07/31 02:42:29, piman (slow to review) wrote: > aren't there callers that pass a non-NULL window? > ui::InProcessContextFactory::CreateOutputSurface seems to be one. yes, it's called with a non-NULL window but kNullAcceleratedWidget is used instead of the passed window when initializing the context
https://codereview.chromium.org/429863002/diff/1/webkit/common/gpu/webgraphic... File webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (left): https://codereview.chromium.org/429863002/diff/1/webkit/common/gpu/webgraphic... webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:153: window_, On 2014/07/31 03:05:32, hj.r.chung wrote: > On 2014/07/31 02:42:29, piman (slow to review) wrote: > > aren't there callers that pass a non-NULL window? > > ui::InProcessContextFactory::CreateOutputSurface seems to be one. > > yes, it's called with a non-NULL window but kNullAcceleratedWidget is used > instead of the passed window when initializing the context I don't follow. ui::InProcessContextFactory::CreateOutputSurface calls WebGraphicsContext3DInProcessCommandBufferImpl::CreateViewContext with a non-NULL window. That calls WebGraphicsContext3DInProcessCommandBufferImpl::WebGraphicsContext3DInProcessCommandBufferImpl that keeps window into the field window_. When we get to MaybeInitializeGL, window_ is non-NULL. What am I missing? Taken another way, some of the tests/demos that use InProcessContextFactory display their results into the window. How else would it be possible, if not passing the window to the GL context?
Sorry for the confusion. I added 'window' back. PTAL https://codereview.chromium.org/429863002/diff/1/cc/test/test_in_process_cont... File cc/test/test_in_process_context_provider.cc (right): https://codereview.chromium.org/429863002/diff/1/cc/test/test_in_process_cont... cc/test/test_in_process_context_provider.cc:39: scoped_refptr<gpu::InProcessCommandBuffer::Service>(), On 2014/07/31 02:42:29, piman (slow to review) wrote: > nit: NULL should work too. Done. https://codereview.chromium.org/429863002/diff/1/gpu/command_buffer/client/gl... File gpu/command_buffer/client/gl_in_process_context.cc (right): https://codereview.chromium.org/429863002/diff/1/gpu/command_buffer/client/gl... gpu/command_buffer/client/gl_in_process_context.cc:227: gfx::kNullAcceleratedWidget, On 2014/07/31 02:35:32, boliu wrote: > You can clean up in in_process_command_buffer as well. sorry for the confusion, window is still needed https://codereview.chromium.org/429863002/diff/1/webkit/common/gpu/webgraphic... File webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (left): https://codereview.chromium.org/429863002/diff/1/webkit/common/gpu/webgraphic... webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:153: window_, On 2014/07/31 03:41:11, piman (slow to review) wrote: > On 2014/07/31 03:05:32, hj.r.chung wrote: > > On 2014/07/31 02:42:29, piman (slow to review) wrote: > > > aren't there callers that pass a non-NULL window? > > > ui::InProcessContextFactory::CreateOutputSurface seems to be one. > > > > yes, it's called with a non-NULL window but kNullAcceleratedWidget is used > > instead of the passed window when initializing the context > > I don't follow. > ui::InProcessContextFactory::CreateOutputSurface calls > WebGraphicsContext3DInProcessCommandBufferImpl::CreateViewContext with a > non-NULL window. That calls > WebGraphicsContext3DInProcessCommandBufferImpl::WebGraphicsContext3DInProcessCommandBufferImpl > that keeps window into the field window_. When we get to MaybeInitializeGL, > window_ is non-NULL. > > What am I missing? > > Taken another way, some of the tests/demos that use InProcessContextFactory > display their results into the window. How else would it be possible, if not > passing the window to the GL context? oops, I was looking at GLInProcessContext::Create instead of ::CreateContext. Sorry about the confusion. adding 'window' back https://codereview.chromium.org/429863002/diff/1/webkit/common/gpu/webgraphic... File webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right): https://codereview.chromium.org/429863002/diff/1/webkit/common/gpu/webgraphic... webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:151: scoped_refptr< ::gpu::InProcessCommandBuffer::Service>(), On 2014/07/31 02:42:29, piman (slow to review) wrote: > nit: NULL Done.
lgtm
On 2014/07/31 11:07:46, sievers wrote: > lgtm thank you all for the reviews!
The CQ bit was checked by heejin.r.chung@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/heejin.r.chung@samsung.com/429863002/2...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm
The CQ bit was checked by heejin.r.chung@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/heejin.r.chung@samsung.com/429863002/2...
Message was sent while issue was closed.
Change committed as 286952 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
