Make lost context and error message callbacks on GpuControl go to client

Make lost context and error message callbacks on GpuControl go to client

In this we introduce a GpuControlClient, that implementations of
GpuControl will call to notify about the context becoming lost, or of
error messages.

Users of GpuControl implement the client to hear about these states.
The most important users of GpuControl here are:
- GLES2Implementation, which now has 2 base::Callbacks which it forwards
  these events to, if they have been set by the user of the
  GLES2Implementation.
- PPB_Graphics3D_Impl, which acts on these two events.
- PepperVideoEncoderHost, which turns lost context events into a call
  to NotifyPepperError(PP_ERROR_RESOURCE_FAILED).
- mojo::GLES2Context, which forwards the lost context event to a
  registered callback.

Other owners of GpuControl (if they exist) never set callbacks on
CommandBufferProxyImpl or equivalent before, so they don't care to
listen to these events and just opt out of setting the client
altogether.

The main implementations of GpuControl here are:
- CommandBufferProxyImpl, which will now call the client instead of
  owning base::Callbacks for these two events.
- mojo::CommandBufferClientImpl, which notifies its client of lost
  contexts, but has no concept of error messages.
- InProcessCommandBuffer, which will now call the client (on the client
  thread) instead of owning and wrapping base::Callbacks for the lost
  context event. It also has no error messages.

For other implementations of GpuControl there is no chance to lose the
context or generate error messages, and they don't even bother to store
the client since it would go unused.

WebGraphicsContext3D{CommandBuffer}Impl continues to be used for these
callbacks, but it now goes directly to the GLES2Implementation to set
callbacks, instead of bypassing it to the CommandBufferProxyImpl.

The next step will be to have Blink or the ContextProvider directly set
these callbacks on the GLES2Implementation, bypassing the
WebGraphicsContext3DImpls entirely. This isn't possible today as the
two versions of WebGraphicsContext3DImpl (which are
WebGraphicsContext3DCommandBufferImpl and
WebGraphicsContext3DInProcessCommandBufferImpl) each have different
versions of the GpuControl, while both having a GLES2Implementation,
forcing ContextProviderCommandBuffer to route through this fork
when setting the callbacks.

R=piman@chomium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/10057772128ab5e67452dc736bf15752dc434ed3
Cr-Commit-Position: refs/heads/master@{#386761}

[danakj, 2016-04-06 01:28:07]

Description was changed from

==========
Make lost context and error message callbacks on GpuControl go to client

In this we introduce a GpuControlClient, that implementations of
GpuControl will call to notify about the context becoming lost, or of
error messages.

Owners of GpuControl implement the client to hear about these states.
The most important owners of GpuControl here are:
- GLES2Implementation, which now has 2 base::Callbacks which it forwards
  these events to, if they have been set by the user of the
  GLES2Implementation.
- PPB_Graphics3D_Impl, which acts on these two events.
- PepperVideoEncoderHost, which turns lost context events into a call
  to NotifyPepperError(PP_ERROR_RESOURCE_FAILED).
- mojo::GLES2Context, which forwards the lost context event to a
  registered callback.

Other owners of GpuControl (if they exist) never set callbacks on
CommandBufferProxyImpl or equivalent before, so they don't care to
listen to these events and just opt out of setting the client
altogether.

The main implementations of GpuControl here are:
- CommandBufferProxyImpl, which will now call the client instead of
  owning base::Callbacks for these two events.
- mojo::CommandBufferClientImpl, which notifies its client of lost
  contexts, but has no concept of error messages.

For other implementations of GpuControl there is no chance to lose the
context or generate error messages, and they don't even bother to store
the client since it would go unused.

WebGraphicsContext3D{CommandBuffer}Impl continues to be used for these
callbacks, but it now goes directly to the GLES2Implementation to set
callbacks, instead of bypassing it to the CommandBufferProxyImpl.

The next step will be to have Blink or the ContextProvider directly set
these callbacks on the GLES2Implementation, bypassing the
WebGraphicsContext3DImpls entirely. This isn't possible today as the
two versions of WebGraphicsContext3DImpl (which are
WebGraphicsContext3DCommandBufferImpl and
WebGraphicsContext3DInProcessCommandBufferImpl) each have different
versions of the GpuControl, while both having a GLES2Implementation,
forcing ContextProviderCommandBuffer to route through this fork
when setting the callbacks.

R=piman@chomium.org
BUG=584497
==========

to

==========
Make lost context and error message callbacks on GpuControl go to client

In this we introduce a GpuControlClient, that implementations of
GpuControl will call to notify about the context becoming lost, or of
error messages.

Owners of GpuControl implement the client to hear about these states.
The most important owners of GpuControl here are:
- GLES2Implementation, which now has 2 base::Callbacks which it forwards
  these events to, if they have been set by the user of the
  GLES2Implementation.
- PPB_Graphics3D_Impl, which acts on these two events.
- PepperVideoEncoderHost, which turns lost context events into a call
  to NotifyPepperError(PP_ERROR_RESOURCE_FAILED).
- mojo::GLES2Context, which forwards the lost context event to a
  registered callback.

Other owners of GpuControl (if they exist) never set callbacks on
CommandBufferProxyImpl or equivalent before, so they don't care to
listen to these events and just opt out of setting the client
altogether.

The main implementations of GpuControl here are:
- CommandBufferProxyImpl, which will now call the client instead of
  owning base::Callbacks for these two events.
- mojo::CommandBufferClientImpl, which notifies its client of lost
  contexts, but has no concept of error messages.

For other implementations of GpuControl there is no chance to lose the
context or generate error messages, and they don't even bother to store
the client since it would go unused.

WebGraphicsContext3D{CommandBuffer}Impl continues to be used for these
callbacks, but it now goes directly to the GLES2Implementation to set
callbacks, instead of bypassing it to the CommandBufferProxyImpl.

The next step will be to have Blink or the ContextProvider directly set
these callbacks on the GLES2Implementation, bypassing the
WebGraphicsContext3DImpls entirely. This isn't possible today as the
two versions of WebGraphicsContext3DImpl (which are
WebGraphicsContext3DCommandBufferImpl and
WebGraphicsContext3DInProcessCommandBufferImpl) each have different
versions of the GpuControl, while both having a GLES2Implementation,
forcing ContextProviderCommandBuffer to route through this fork
when setting the callbacks.

R=piman@chomium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[danakj, 2016-04-06 02:03:17]

Patchset #1 (id:1) has been deleted

[danakj, 2016-04-06 02:14:14]

danakj@chromium.org changed reviewers:
+ sievers@chromium.org

[danakj, 2016-04-06 02:14:15]

https://codereview.chromium.org/1864723003/diff/10001/gpu/blink/webgraphicsco...
File gpu/blink/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/1864723003/diff/10001/gpu/blink/webgraphicsco...
gpu/blink/webgraphicscontext3d_in_process_command_buffer_impl.cc:114:
&WebGraphicsContext3DInProcessCommandBufferImpl::OnErrorMessage,
I started setting this here, but I don't think this can actually be called, as
the InProcessCommandBuffer doesn't call its GpuControlClient at all for errors.
So.. idk I could leave this out.

https://codereview.chromium.org/1864723003/diff/10001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gl_in_process_context.cc (right):

https://codereview.chromium.org/1864723003/diff/10001/gpu/command_buffer/clie...
gpu/command_buffer/client/gl_in_process_context.cc:76: // GpuControlClient
implementation. These methods are called on the GPU thread
It all gets quite hairy in here. The InProcessCommandBuffer runs on the GPU
thread, so these methods get called on the GPU thread. Which is ok I guess in
theory.

Except, in this patchset I have GLInProcessContextImpl as the GpuControlClient,
but the correct relationship is to have GLES2Implementation as the client. So I
should be calling

command_buffer_->SetGpuControlClient(gles2_implementation_.get());

But.. GLES2Implementation is not expecting those methods to be called on a GPU
thread.

And it turns out we want to intercept the lost context event in here somehow and
store lost_context_ = true on the context...

Alternatively we need multiple lost context callbacks on GLES2Implementation I
guess. But still we're calling into GLES2Implementation on the wrong thread
then..

https://codereview.chromium.org/1864723003/diff/10001/gpu/command_buffer/clie...
gpu/command_buffer/client/gl_in_process_context.cc:98: mutable base::Lock
context_lost_lock_;
We used to wrap callbacks inside callbacks to get to the right thread, but all
that just to set a context_lost_ bool.. so, I just added a lock instead.

Where I start to get lost is why we had this other |lock_|, which we'd grab when
setting the context_lost_ bool. The lock_ is set via SetLock in the
GLInProcessContext API, and passed to the InProcessCommandBuffer, but never
used!

The lock_ is actually owned out in (for example) AwRenderThreadContextProvider,
which calls SetLock in its own public SetupLock which is part of the
ContextProvider API.

So.. I don't get the point of this lock_ at all basically, and I removed it
(replaced with this new locally-owned lock). But then.. why have a lock on
AwRenderThreadContextProvider? Everything is confusing, halp.

[danakj, 2016-04-06 02:19:59]

https://codereview.chromium.org/1864723003/diff/10001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gl_in_process_context.cc (right):

https://codereview.chromium.org/1864723003/diff/10001/gpu/command_buffer/clie...
gpu/command_buffer/client/gl_in_process_context.cc:76: // GpuControlClient
implementation. These methods are called on the GPU thread
On 2016/04/06 02:14:15, danakj wrote:
> It all gets quite hairy in here. The InProcessCommandBuffer runs on the GPU
> thread, so these methods get called on the GPU thread. Which is ok I guess in
> theory.
> 
> Except, in this patchset I have GLInProcessContextImpl as the
GpuControlClient,
> but the correct relationship is to have GLES2Implementation as the client. So
I
> should be calling
> 
> command_buffer_->SetGpuControlClient(gles2_implementation_.get());
> 
> But.. GLES2Implementation is not expecting those methods to be called on a GPU
> thread.
> 
> And it turns out we want to intercept the lost context event in here somehow
and
> store lost_context_ = true on the context...
> 
> Alternatively we need multiple lost context callbacks on GLES2Implementation I
> guess. But still we're calling into GLES2Implementation on the wrong thread
> then..

Oh no I realized this class should not be calling SetGpuControlClient() on the
InProcessCommandBuffer at all. GLES2Implementation will already do that.

But GLES2Implementation is going to get called on the wrong thread then.

So.. do we have to do this with callbacks (wrapped in callbacks inside
callbacks) to thread hop here? :/

[danakj, 2016-04-06 02:25:44]

https://codereview.chromium.org/1864723003/diff/10001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gl_in_process_context.cc (right):

https://codereview.chromium.org/1864723003/diff/10001/gpu/command_buffer/clie...
gpu/command_buffer/client/gl_in_process_context.cc:76: // GpuControlClient
implementation. These methods are called on the GPU thread
On 2016/04/06 02:19:58, danakj wrote:
> On 2016/04/06 02:14:15, danakj wrote:
> > It all gets quite hairy in here. The InProcessCommandBuffer runs on the GPU
> > thread, so these methods get called on the GPU thread. Which is ok I guess
in
> > theory.
> > 
> > Except, in this patchset I have GLInProcessContextImpl as the
> GpuControlClient,
> > but the correct relationship is to have GLES2Implementation as the client.
So
> I
> > should be calling
> > 
> > command_buffer_->SetGpuControlClient(gles2_implementation_.get());
> > 
> > But.. GLES2Implementation is not expecting those methods to be called on a
GPU
> > thread.
> > 
> > And it turns out we want to intercept the lost context event in here somehow
> and
> > store lost_context_ = true on the context...
> > 
> > Alternatively we need multiple lost context callbacks on GLES2Implementation
I
> > guess. But still we're calling into GLES2Implementation on the wrong thread
> > then..
> 
> Oh no I realized this class should not be calling SetGpuControlClient() on the
> InProcessCommandBuffer at all. GLES2Implementation will already do that.
> 
> But GLES2Implementation is going to get called on the wrong thread then.
> 
> So.. do we have to do this with callbacks (wrapped in callbacks inside
> callbacks) to thread hop here? :/

It seems like what's weird here (or different) is that InProcessCommandBuffer is
entirely living on the GPU thread (aka server side), whereas in the OOP version
we have CommandBufferProxyImpl on client side.

Perhaps we should have a thread split on InProcessCommandBuffer, and the
GpuControl parts should be on the main thread, and the rest of it should be on
the GPU thread? Then calling back to the GpuControlClient would be a thread hop
in the InProcessCommandBuffer (but wouldn't need nested callback dolls yay)
since it would be going to the "GpuControl" thread.

[danakj, 2016-04-07 20:14:31]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-07 20:15:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1864723003/30001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1864723003/30001

[commit-bot: I haz the power, 2016-04-07 20:18:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-07 20:18:37]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  mac_optional_gpu_tests_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[danakj, 2016-04-07 20:19:49]

Description was changed from

==========
Make lost context and error message callbacks on GpuControl go to client

In this we introduce a GpuControlClient, that implementations of
GpuControl will call to notify about the context becoming lost, or of
error messages.

Owners of GpuControl implement the client to hear about these states.
The most important owners of GpuControl here are:
- GLES2Implementation, which now has 2 base::Callbacks which it forwards
  these events to, if they have been set by the user of the
  GLES2Implementation.
- PPB_Graphics3D_Impl, which acts on these two events.
- PepperVideoEncoderHost, which turns lost context events into a call
  to NotifyPepperError(PP_ERROR_RESOURCE_FAILED).
- mojo::GLES2Context, which forwards the lost context event to a
  registered callback.

Other owners of GpuControl (if they exist) never set callbacks on
CommandBufferProxyImpl or equivalent before, so they don't care to
listen to these events and just opt out of setting the client
altogether.

The main implementations of GpuControl here are:
- CommandBufferProxyImpl, which will now call the client instead of
  owning base::Callbacks for these two events.
- mojo::CommandBufferClientImpl, which notifies its client of lost
  contexts, but has no concept of error messages.

For other implementations of GpuControl there is no chance to lose the
context or generate error messages, and they don't even bother to store
the client since it would go unused.

WebGraphicsContext3D{CommandBuffer}Impl continues to be used for these
callbacks, but it now goes directly to the GLES2Implementation to set
callbacks, instead of bypassing it to the CommandBufferProxyImpl.

The next step will be to have Blink or the ContextProvider directly set
these callbacks on the GLES2Implementation, bypassing the
WebGraphicsContext3DImpls entirely. This isn't possible today as the
two versions of WebGraphicsContext3DImpl (which are
WebGraphicsContext3DCommandBufferImpl and
WebGraphicsContext3DInProcessCommandBufferImpl) each have different
versions of the GpuControl, while both having a GLES2Implementation,
forcing ContextProviderCommandBuffer to route through this fork
when setting the callbacks.

R=piman@chomium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Make lost context and error message callbacks on GpuControl go to client

In this we introduce a GpuControlClient, that implementations of
GpuControl will call to notify about the context becoming lost, or of
error messages.

Owners of GpuControl implement the client to hear about these states.
The most important owners of GpuControl here are:
- GLES2Implementation, which now has 2 base::Callbacks which it forwards
  these events to, if they have been set by the user of the
  GLES2Implementation.
- PPB_Graphics3D_Impl, which acts on these two events.
- PepperVideoEncoderHost, which turns lost context events into a call
  to NotifyPepperError(PP_ERROR_RESOURCE_FAILED).
- mojo::GLES2Context, which forwards the lost context event to a
  registered callback.

Other owners of GpuControl (if they exist) never set callbacks on
CommandBufferProxyImpl or equivalent before, so they don't care to
listen to these events and just opt out of setting the client
altogether.

The main implementations of GpuControl here are:
- CommandBufferProxyImpl, which will now call the client instead of
  owning base::Callbacks for these two events.
- mojo::CommandBufferClientImpl, which notifies its client of lost
  contexts, but has no concept of error messages.
- InProcessCommandBuffer, which will now call the client (on the client
  thread) instead of owning and wrapping base::Callbacks for the lost
  context event. It also has no error messages.

For other implementations of GpuControl there is no chance to lose the
context or generate error messages, and they don't even bother to store
the client since it would go unused.

WebGraphicsContext3D{CommandBuffer}Impl continues to be used for these
callbacks, but it now goes directly to the GLES2Implementation to set
callbacks, instead of bypassing it to the CommandBufferProxyImpl.

The next step will be to have Blink or the ContextProvider directly set
these callbacks on the GLES2Implementation, bypassing the
WebGraphicsContext3DImpls entirely. This isn't possible today as the
two versions of WebGraphicsContext3DImpl (which are
WebGraphicsContext3DCommandBufferImpl and
WebGraphicsContext3DInProcessCommandBufferImpl) each have different
versions of the GpuControl, while both having a GLES2Implementation,
forcing ContextProviderCommandBuffer to route through this fork
when setting the callbacks.

R=piman@chomium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[danakj, 2016-04-07 20:26:35]

danakj@chromium.org changed reviewers:
+ boliu@chromium.org, fsamuel@google.com

[danakj, 2016-04-07 20:26:38]

Updated CL after https://codereview.chromium.org/1864723003/# and fixed the
android/inprocess stuff.

https://codereview.chromium.org/1864723003/diff/50001/components/mus/gles2/co...
File components/mus/gles2/command_buffer_local.cc (right):

https://codereview.chromium.org/1864723003/diff/50001/components/mus/gles2/co...
components/mus/gles2/command_buffer_local.cc:194: last_state_.error =
gpu::error::kLostContext;
I mean, this never had callbacks before, but maybe this should actually call the
client here?

[danakj, 2016-04-07 20:32:06]

danakj@chromium.org changed reviewers:
+ fsamuel@chromium.org
- fsamuel@google.com

[danakj, 2016-04-07 20:44:08]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-07 20:45:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1864723003/70001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1864723003/70001

[commit-bot: I haz the power, 2016-04-07 20:58:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-07 20:58:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[danakj, 2016-04-07 21:01:19]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-07 21:03:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1864723003/90001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1864723003/90001

[no sievers, 2016-04-07 21:07:59]

https://codereview.chromium.org/1864723003/diff/50001/components/mus/gles2/co...
File components/mus/gles2/command_buffer_local.cc (right):

https://codereview.chromium.org/1864723003/diff/50001/components/mus/gles2/co...
components/mus/gles2/command_buffer_local.cc:222: // the client.
It does handle lost contexts and trigger the callback to the client though. It's
just that it's forwarded from CommandBufferDriver to
CommandBufferLocal::DidLoseContext() separately.

I guess this class is something like GLInProcessContext/InProcessCommandBuffer
which blurs the notion of client/service a bit.

https://codereview.chromium.org/1864723003/diff/70001/gpu/blink/webgraphicsco...
File gpu/blink/webgraphicscontext3d_impl.cc (right):

https://codereview.chromium.org/1864723003/diff/70001/gpu/blink/webgraphicsco...
gpu/blink/webgraphicscontext3d_impl.cc:50: void
WebGraphicsContext3DImpl::OnErrorMessage(const char* message, int id) {
So only one of the two subclasses uses this? Should we move it there?

https://codereview.chromium.org/1864723003/diff/70001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1864723003/diff/70001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:203:
gpu_control_->SetGpuControlClient(this);
It's probably safer to unset this somewhere (in the destructor?), since it's
GpuControl outlives |this|.

https://codereview.chromium.org/1864723003/diff/70001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.h (right):

https://codereview.chromium.org/1864723003/diff/70001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.h:116: // NOTE: The
GpuControlClient will be called on the GPU thread.
on the client thread, right?

https://codereview.chromium.org/1864723003/diff/70001/gpu/ipc/client/command_...
File gpu/ipc/client/command_buffer_proxy_impl.cc (left):

https://codereview.chromium.org/1864723003/diff/70001/gpu/ipc/client/command_...
gpu/ipc/client/command_buffer_proxy_impl.cc:125: // Avoid calling the error
callback more than once.
OnDestroyed() might be triggered more than once I think because of the call site
from OnChannelError(). Should we reset |gpu_control_client_| when the context is
first lost?

https://codereview.chromium.org/1864723003/diff/90001/mojo/gles2/gles2_contex...
File mojo/gles2/gles2_context.cc (right):

https://codereview.chromium.org/1864723003/diff/90001/mojo/gles2/gles2_contex...
mojo/gles2/gles2_context.cc:38: command_buffer_.SetGpuControlClient(this);
Won't GLES2Impl below clobber this and set itself as the client?

You probably want to set |lost_callback| as the callback on gles2 impl instead.

[commit-bot: I haz the power, 2016-04-07 21:25:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-07 21:25:36]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[danakj, 2016-04-07 21:45:19]

PTAL

https://codereview.chromium.org/1864723003/diff/50001/components/mus/gles2/co...
File components/mus/gles2/command_buffer_local.cc (right):

https://codereview.chromium.org/1864723003/diff/50001/components/mus/gles2/co...
components/mus/gles2/command_buffer_local.cc:222: // the client.
On 2016/04/07 21:07:58, sievers wrote:
> It does handle lost contexts and trigger the callback to the client though.
It's
> just that it's forwarded from CommandBufferDriver to
> CommandBufferLocal::DidLoseContext() separately.
> 
> I guess this class is something like GLInProcessContext/InProcessCommandBuffer
> which blurs the notion of client/service a bit.

Ooops thanks, I was looking for "Lost" not "Lose".

So the SurfacesContextProvider should just pass the lost context callback given
to it thru to the GLES2Impl, and this will call the GLES2Impl on loss.

Removing DidLoseContext from the CommandBufferLocalClient.

https://codereview.chromium.org/1864723003/diff/70001/gpu/blink/webgraphicsco...
File gpu/blink/webgraphicscontext3d_impl.cc (right):

https://codereview.chromium.org/1864723003/diff/70001/gpu/blink/webgraphicsco...
gpu/blink/webgraphicscontext3d_impl.cc:50: void
WebGraphicsContext3DImpl::OnErrorMessage(const char* message, int id) {
On 2016/04/07 21:07:58, sievers wrote:
> So only one of the two subclasses uses this? Should we move it there?

Ya I think that works. Done.

https://codereview.chromium.org/1864723003/diff/70001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1864723003/diff/70001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:203:
gpu_control_->SetGpuControlClient(this);
On 2016/04/07 21:07:58, sievers wrote:
> It's probably safer to unset this somewhere (in the destructor?), since it's
> GpuControl outlives |this|.

Ya definitely.

https://codereview.chromium.org/1864723003/diff/70001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.h (right):

https://codereview.chromium.org/1864723003/diff/70001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.h:116: // NOTE: The
GpuControlClient will be called on the GPU thread.
On 2016/04/07 21:07:58, sievers wrote:
> on the client thread, right?

Yep that was fixed. Comment fixed too now.

https://codereview.chromium.org/1864723003/diff/70001/gpu/ipc/client/command_...
File gpu/ipc/client/command_buffer_proxy_impl.cc (left):

https://codereview.chromium.org/1864723003/diff/70001/gpu/ipc/client/command_...
gpu/ipc/client/command_buffer_proxy_impl.cc:125: // Avoid calling the error
callback more than once.
On 2016/04/07 21:07:58, sievers wrote:
> OnDestroyed() might be triggered more than once I think because of the call
site
> from OnChannelError(). Should we reset |gpu_control_client_| when the context
is
> first lost?

Ah.. I had done this but in content/ and the file moved, ported those changes.

https://codereview.chromium.org/1864723003/diff/90001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1864723003/diff/90001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:502: void
InProcessCommandBuffer::OnContextLost() {
Also earlying out here if context_lost_ is true.

https://codereview.chromium.org/1864723003/diff/90001/mojo/gles2/gles2_contex...
File mojo/gles2/gles2_context.cc (right):

https://codereview.chromium.org/1864723003/diff/90001/mojo/gles2/gles2_contex...
mojo/gles2/gles2_context.cc:38: command_buffer_.SetGpuControlClient(this);
On 2016/04/07 21:07:58, sievers wrote:
> Won't GLES2Impl below clobber this and set itself as the client?
> 
> You probably want to set |lost_callback| as the callback on gles2 impl
instead.

Yes I was doing this wrong like the GLInProcessContext before, fixed.

[danakj, 2016-04-07 22:09:47]

OK After talking with sievers, what we have is:

- GpuControls (ie command buffers..) will ensure they only tell the client about
context loss once.
- GpuControlClients (ie gles2impl and some pepper stuff) will DCHECK that they
are told only once.
- Everything else (gles2impl, context providers) stops trying to protect against
multiple calls

PTAL

[danakj, 2016-04-07 22:10:25]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-07 22:10:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1864723003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1864723003/140001

[danakj, 2016-04-07 22:13:04]

Description was changed from

==========
Make lost context and error message callbacks on GpuControl go to client

In this we introduce a GpuControlClient, that implementations of
GpuControl will call to notify about the context becoming lost, or of
error messages.

Owners of GpuControl implement the client to hear about these states.
The most important owners of GpuControl here are:
- GLES2Implementation, which now has 2 base::Callbacks which it forwards
  these events to, if they have been set by the user of the
  GLES2Implementation.
- PPB_Graphics3D_Impl, which acts on these two events.
- PepperVideoEncoderHost, which turns lost context events into a call
  to NotifyPepperError(PP_ERROR_RESOURCE_FAILED).
- mojo::GLES2Context, which forwards the lost context event to a
  registered callback.

Other owners of GpuControl (if they exist) never set callbacks on
CommandBufferProxyImpl or equivalent before, so they don't care to
listen to these events and just opt out of setting the client
altogether.

The main implementations of GpuControl here are:
- CommandBufferProxyImpl, which will now call the client instead of
  owning base::Callbacks for these two events.
- mojo::CommandBufferClientImpl, which notifies its client of lost
  contexts, but has no concept of error messages.
- InProcessCommandBuffer, which will now call the client (on the client
  thread) instead of owning and wrapping base::Callbacks for the lost
  context event. It also has no error messages.

For other implementations of GpuControl there is no chance to lose the
context or generate error messages, and they don't even bother to store
the client since it would go unused.

WebGraphicsContext3D{CommandBuffer}Impl continues to be used for these
callbacks, but it now goes directly to the GLES2Implementation to set
callbacks, instead of bypassing it to the CommandBufferProxyImpl.

The next step will be to have Blink or the ContextProvider directly set
these callbacks on the GLES2Implementation, bypassing the
WebGraphicsContext3DImpls entirely. This isn't possible today as the
two versions of WebGraphicsContext3DImpl (which are
WebGraphicsContext3DCommandBufferImpl and
WebGraphicsContext3DInProcessCommandBufferImpl) each have different
versions of the GpuControl, while both having a GLES2Implementation,
forcing ContextProviderCommandBuffer to route through this fork
when setting the callbacks.

R=piman@chomium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Make lost context and error message callbacks on GpuControl go to client

In this we introduce a GpuControlClient, that implementations of
GpuControl will call to notify about the context becoming lost, or of
error messages.

Users of GpuControl implement the client to hear about these states.
The most important owners of GpuControl here are:
- GLES2Implementation, which now has 2 base::Callbacks which it forwards
  these events to, if they have been set by the user of the
  GLES2Implementation.
- PPB_Graphics3D_Impl, which acts on these two events.
- PepperVideoEncoderHost, which turns lost context events into a call
  to NotifyPepperError(PP_ERROR_RESOURCE_FAILED).
- mojo::GLES2Context, which forwards the lost context event to a
  registered callback.

Other owners of GpuControl (if they exist) never set callbacks on
CommandBufferProxyImpl or equivalent before, so they don't care to
listen to these events and just opt out of setting the client
altogether.

The main implementations of GpuControl here are:
- CommandBufferProxyImpl, which will now call the client instead of
  owning base::Callbacks for these two events.
- mojo::CommandBufferClientImpl, which notifies its client of lost
  contexts, but has no concept of error messages.
- InProcessCommandBuffer, which will now call the client (on the client
  thread) instead of owning and wrapping base::Callbacks for the lost
  context event. It also has no error messages.

For other implementations of GpuControl there is no chance to lose the
context or generate error messages, and they don't even bother to store
the client since it would go unused.

WebGraphicsContext3D{CommandBuffer}Impl continues to be used for these
callbacks, but it now goes directly to the GLES2Implementation to set
callbacks, instead of bypassing it to the CommandBufferProxyImpl.

The next step will be to have Blink or the ContextProvider directly set
these callbacks on the GLES2Implementation, bypassing the
WebGraphicsContext3DImpls entirely. This isn't possible today as the
two versions of WebGraphicsContext3DImpl (which are
WebGraphicsContext3DCommandBufferImpl and
WebGraphicsContext3DInProcessCommandBufferImpl) each have different
versions of the GpuControl, while both having a GLES2Implementation,
forcing ContextProviderCommandBuffer to route through this fork
when setting the callbacks.

R=piman@chomium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[danakj, 2016-04-07 22:13:24]

Description was changed from

==========
Make lost context and error message callbacks on GpuControl go to client

In this we introduce a GpuControlClient, that implementations of
GpuControl will call to notify about the context becoming lost, or of
error messages.

Users of GpuControl implement the client to hear about these states.
The most important owners of GpuControl here are:
- GLES2Implementation, which now has 2 base::Callbacks which it forwards
  these events to, if they have been set by the user of the
  GLES2Implementation.
- PPB_Graphics3D_Impl, which acts on these two events.
- PepperVideoEncoderHost, which turns lost context events into a call
  to NotifyPepperError(PP_ERROR_RESOURCE_FAILED).
- mojo::GLES2Context, which forwards the lost context event to a
  registered callback.

Other owners of GpuControl (if they exist) never set callbacks on
CommandBufferProxyImpl or equivalent before, so they don't care to
listen to these events and just opt out of setting the client
altogether.

The main implementations of GpuControl here are:
- CommandBufferProxyImpl, which will now call the client instead of
  owning base::Callbacks for these two events.
- mojo::CommandBufferClientImpl, which notifies its client of lost
  contexts, but has no concept of error messages.
- InProcessCommandBuffer, which will now call the client (on the client
  thread) instead of owning and wrapping base::Callbacks for the lost
  context event. It also has no error messages.

For other implementations of GpuControl there is no chance to lose the
context or generate error messages, and they don't even bother to store
the client since it would go unused.

WebGraphicsContext3D{CommandBuffer}Impl continues to be used for these
callbacks, but it now goes directly to the GLES2Implementation to set
callbacks, instead of bypassing it to the CommandBufferProxyImpl.

The next step will be to have Blink or the ContextProvider directly set
these callbacks on the GLES2Implementation, bypassing the
WebGraphicsContext3DImpls entirely. This isn't possible today as the
two versions of WebGraphicsContext3DImpl (which are
WebGraphicsContext3DCommandBufferImpl and
WebGraphicsContext3DInProcessCommandBufferImpl) each have different
versions of the GpuControl, while both having a GLES2Implementation,
forcing ContextProviderCommandBuffer to route through this fork
when setting the callbacks.

R=piman@chomium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Make lost context and error message callbacks on GpuControl go to client

In this we introduce a GpuControlClient, that implementations of
GpuControl will call to notify about the context becoming lost, or of
error messages.

Users of GpuControl implement the client to hear about these states.
The most important users of GpuControl here are:
- GLES2Implementation, which now has 2 base::Callbacks which it forwards
  these events to, if they have been set by the user of the
  GLES2Implementation.
- PPB_Graphics3D_Impl, which acts on these two events.
- PepperVideoEncoderHost, which turns lost context events into a call
  to NotifyPepperError(PP_ERROR_RESOURCE_FAILED).
- mojo::GLES2Context, which forwards the lost context event to a
  registered callback.

Other owners of GpuControl (if they exist) never set callbacks on
CommandBufferProxyImpl or equivalent before, so they don't care to
listen to these events and just opt out of setting the client
altogether.

The main implementations of GpuControl here are:
- CommandBufferProxyImpl, which will now call the client instead of
  owning base::Callbacks for these two events.
- mojo::CommandBufferClientImpl, which notifies its client of lost
  contexts, but has no concept of error messages.
- InProcessCommandBuffer, which will now call the client (on the client
  thread) instead of owning and wrapping base::Callbacks for the lost
  context event. It also has no error messages.

For other implementations of GpuControl there is no chance to lose the
context or generate error messages, and they don't even bother to store
the client since it would go unused.

WebGraphicsContext3D{CommandBuffer}Impl continues to be used for these
callbacks, but it now goes directly to the GLES2Implementation to set
callbacks, instead of bypassing it to the CommandBufferProxyImpl.

The next step will be to have Blink or the ContextProvider directly set
these callbacks on the GLES2Implementation, bypassing the
WebGraphicsContext3DImpls entirely. This isn't possible today as the
two versions of WebGraphicsContext3DImpl (which are
WebGraphicsContext3DCommandBufferImpl and
WebGraphicsContext3DInProcessCommandBufferImpl) each have different
versions of the GpuControl, while both having a GLES2Implementation,
forcing ContextProviderCommandBuffer to route through this fork
when setting the callbacks.

R=piman@chomium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-04-07 22:20:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-07 22:20:19]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[danakj, 2016-04-07 23:17:30]

Patchset #11 (id:200001) has been deleted

[danakj, 2016-04-08 00:59:07]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-08 00:59:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1864723003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1864723003/240001

[commit-bot: I haz the power, 2016-04-08 02:39:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-08 02:39:52]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[kristinllewellyn88, 2016-04-08 02:57:56]

kristinllewellyn88@gmail.com changed reviewers:
+ kristinllewellyn88@gmail.com

[boliu, 2016-04-08 16:53:48]

boliu@chromium.org changed reviewers:
+ piman@chromium.org
- kristinllewellyn88@gmail.com, piman@chomium.org

[boliu, 2016-04-08 16:53:49]

piman's email was misspelled, and fixed

[no sievers, 2016-04-08 18:11:46]

lgtm

https://codereview.chromium.org/1864723003/diff/240001/content/renderer/peppe...
File content/renderer/pepper/ppb_graphics_3d_impl.cc (left):

https://codereview.chromium.org/1864723003/diff/240001/content/renderer/peppe...
content/renderer/pepper/ppb_graphics_3d_impl.cc:263:
&PPB_Graphics3D_Impl::OnConsoleMessage, weak_ptr_factory_.GetWeakPtr()));
Unlike PepeprVideoEncoderHost, here the WeakPtr factory invalidates before the
command_buffer_ is destroyed. But as long as there is no reentrancy from the
callback in the destructor, which looks like it should never happen, we're good.
But maybe SetGpuControlClient(null) in the destructor to be paranoid.

https://codereview.chromium.org/1864723003/diff/240001/gpu/command_buffer/cli...
File gpu/command_buffer/client/gles2_implementation.h (right):

https://codereview.chromium.org/1864723003/diff/240001/gpu/command_buffer/cli...
gpu/command_buffer/client/gles2_implementation.h:212: // GpuControlClient
implementation.
nit: these two can be private, I think?

[no sievers, 2016-04-08 18:11:47]

lgtm

[Fady Samuel, 2016-04-08 18:13:33]

mus lgtm

[danakj, 2016-04-08 18:38:58]

https://codereview.chromium.org/1864723003/diff/240001/content/renderer/peppe...
File content/renderer/pepper/ppb_graphics_3d_impl.cc (left):

https://codereview.chromium.org/1864723003/diff/240001/content/renderer/peppe...
content/renderer/pepper/ppb_graphics_3d_impl.cc:263:
&PPB_Graphics3D_Impl::OnConsoleMessage, weak_ptr_factory_.GetWeakPtr()));
On 2016/04/08 18:11:45, sievers wrote:
> Unlike PepeprVideoEncoderHost, here the WeakPtr factory invalidates before the
> command_buffer_ is destroyed. But as long as there is no reentrancy from the
> callback in the destructor, which looks like it should never happen, we're
good.
> But maybe SetGpuControlClient(null) in the destructor to be paranoid.

Sure, done.

https://codereview.chromium.org/1864723003/diff/240001/gpu/command_buffer/cli...
File gpu/command_buffer/client/gles2_implementation.h (right):

https://codereview.chromium.org/1864723003/diff/240001/gpu/command_buffer/cli...
gpu/command_buffer/client/gles2_implementation.h:212: // GpuControlClient
implementation.
On 2016/04/08 18:11:45, sievers wrote:
> nit: these two can be private, I think?

Done. And final too.

[danakj, 2016-04-08 18:39:26]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-08 18:40:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1864723003/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1864723003/260001

[danakj, 2016-04-08 18:40:41]

danakj@chromium.org changed reviewers:
+ sky@chromium.org, wez@chromium.org

[danakj, 2016-04-08 18:40:43]

piman: PTAL at ppapi/ and cc/ (and the rest if u like)
sky: PTAL at mojo/
wez: PTAL at blimp/

[boliu, 2016-04-08 19:02:08]

android_webview lgtm

[boliu, 2016-04-08 19:51:17]

https://codereview.chromium.org/1864723003/diff/260001/gpu/command_buffer/ser...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1864723003/diff/260001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.cc:281: origin_task_runner_
= base::ThreadTaskRunnerHandle::Get();
Oh! Need to deal with the case that this is null. Happens on android webview on
android's render thread.

In that case, the client and server threads are the same, so a direct call is
ok. Guess need to compare PlatformThreadHandle to get that case.

[danakj, 2016-04-08 19:55:41]

https://codereview.chromium.org/1864723003/diff/260001/gpu/command_buffer/ser...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1864723003/diff/260001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.cc:281: origin_task_runner_
= base::ThreadTaskRunnerHandle::Get();
On 2016/04/08 19:51:17, boliu wrote:
> Oh! Need to deal with the case that this is null. Happens on android webview
on
> android's render thread.
> 
> In that case, the client and server threads are the same, so a direct call is
> ok. Guess need to compare PlatformThreadHandle to get that case.

The old code dealt with null in WrapCallback and PostCallback by just calling
the function directly. I should be doing the same thing in
OnContextLostOnGpuThread().

I guess it would call IsSet tho before calling Get, I can restore that.

[danakj, 2016-04-08 19:56:53]

https://codereview.chromium.org/1864723003/diff/260001/gpu/command_buffer/ser...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1864723003/diff/260001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.cc:281: origin_task_runner_
= base::ThreadTaskRunnerHandle::Get();
On 2016/04/08 19:55:41, danakj wrote:
> On 2016/04/08 19:51:17, boliu wrote:
> > Oh! Need to deal with the case that this is null. Happens on android webview
> on
> > android's render thread.
> > 
> > In that case, the client and server threads are the same, so a direct call
is
> > ok. Guess need to compare PlatformThreadHandle to get that case.
> 
> The old code dealt with null in WrapCallback and PostCallback by just calling
> the function directly. I should be doing the same thing in
> OnContextLostOnGpuThread().
> 
> I guess it would call IsSet tho before calling Get, I can restore that.

Are there no tests where IsSet is false?

[boliu, 2016-04-08 20:02:34]

https://codereview.chromium.org/1864723003/diff/260001/gpu/command_buffer/ser...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1864723003/diff/260001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.cc:281: origin_task_runner_
= base::ThreadTaskRunnerHandle::Get();
On 2016/04/08 19:56:53, danakj wrote:
> On 2016/04/08 19:55:41, danakj wrote:
> > On 2016/04/08 19:51:17, boliu wrote:
> > > Oh! Need to deal with the case that this is null. Happens on android
webview
> > on
> > > android's render thread.
> > > 
> > > In that case, the client and server threads are the same, so a direct call
> is
> > > ok. Guess need to compare PlatformThreadHandle to get that case.
> > 
> > The old code dealt with null in WrapCallback and PostCallback by just
calling
> > the function directly. I should be doing the same thing in
> > OnContextLostOnGpuThread().
> > 
> > I guess it would call IsSet tho before calling Get, I can restore that.
> 
> Are there no tests where IsSet is false?

Most of the webview instrumentation tests won't have a message loop here I
believe. But they don't explicitly exercise the context loss path here.

OnContextLostOnGpuThread already checks for null origin_task_runner_, so maybe
this should be ok. Not sure why those tests are blowing up then.. Need to wait
for the bot to finish to get symbolized stack, hopefully.

[danakj, 2016-04-08 20:05:26]

On Fri, Apr 8, 2016 at 1:02 PM, <boliu@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1864723003/diff/260001/gpu/command_buffer/ser...
> File gpu/command_buffer/service/in_process_command_buffer.cc (right):
>
>
>
https://codereview.chromium.org/1864723003/diff/260001/gpu/command_buffer/ser...
> gpu/command_buffer/service/in_process_command_buffer.cc:281:
> origin_task_runner_ = base::ThreadTaskRunnerHandle::Get();
> On 2016/04/08 19:56:53, danakj wrote:
> > On 2016/04/08 19:55:41, danakj wrote:
> > > On 2016/04/08 19:51:17, boliu wrote:
> > > > Oh! Need to deal with the case that this is null. Happens on
> android webview
> > > on
> > > > android's render thread.
> > > >
> > > > In that case, the client and server threads are the same, so a
> direct call
> > is
> > > > ok. Guess need to compare PlatformThreadHandle to get that case.
> > >
> > > The old code dealt with null in WrapCallback and PostCallback by
> just calling
> > > the function directly. I should be doing the same thing in
> > > OnContextLostOnGpuThread().
> > >
> > > I guess it would call IsSet tho before calling Get, I can restore
> that.
> >
> > Are there no tests where IsSet is false?
>
> Most of the webview instrumentation tests won't have a message loop here
> I believe. But they don't explicitly exercise the context loss path
> here.
>

I think that if IsSet() is false, that Get() would crash. So just
constructing a context should crash if there's no ThreadTaskRunnerHandle.

https://code.google.com/p/chromium/codesearch#chromium/src/base/thread_task_r...

IsSet checks lazy_tls_ptr.Pointer()->Get() is not null.
Get returns lazy_tls_ptr.Pointer()->Get()->task_runner_.


>
> OnContextLostOnGpuThread already checks for null origin_task_runner_, so
> maybe this should be ok. Not sure why those tests are blowing up then..
> Need to wait for the bot to finish to get symbolized stack, hopefully.
>

Oh I didn't see a red bot, maybe they are hitting this.


>
> https://codereview.chromium.org/1864723003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2016-04-08 20:14:04]

https://codereview.chromium.org/1864723003/diff/260001/gpu/command_buffer/ser...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1864723003/diff/260001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.cc:281: origin_task_runner_
= base::ThreadTaskRunnerHandle::Get();
On 2016/04/08 20:02:34, boliu wrote:
> On 2016/04/08 19:56:53, danakj wrote:
> > On 2016/04/08 19:55:41, danakj wrote:
> > > On 2016/04/08 19:51:17, boliu wrote:
> > > > Oh! Need to deal with the case that this is null. Happens on android
> webview
> > > on
> > > > android's render thread.
> > > > 
> > > > In that case, the client and server threads are the same, so a direct
call
> > is
> > > > ok. Guess need to compare PlatformThreadHandle to get that case.
> > > 
> > > The old code dealt with null in WrapCallback and PostCallback by just
> calling
> > > the function directly. I should be doing the same thing in
> > > OnContextLostOnGpuThread().
> > > 
> > > I guess it would call IsSet tho before calling Get, I can restore that.
> > 
> > Are there no tests where IsSet is false?
> 
> Most of the webview instrumentation tests won't have a message loop here I
> believe. But they don't explicitly exercise the context loss path here.
> 
> OnContextLostOnGpuThread already checks for null origin_task_runner_, so maybe
> this should be ok. Not sure why those tests are blowing up then.. Need to wait
> for the bot to finish to get symbolized stack, hopefully.

I updated the CL to only set these when surface is null (which is when we don't
set the sequence_checker_ also).

[danakj, 2016-04-08 20:15:10]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-08 20:15:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1864723003/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1864723003/280001

[danakj, 2016-04-08 20:32:31]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-08 20:32:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1864723003/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1864723003/300001

[boliu, 2016-04-08 21:01:07]

https://codereview.chromium.org/1864723003/diff/260001/gpu/command_buffer/ser...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1864723003/diff/260001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.cc:281: origin_task_runner_
= base::ThreadTaskRunnerHandle::Get();
On 2016/04/08 20:14:04, danakj wrote:
> On 2016/04/08 20:02:34, boliu wrote:
> > On 2016/04/08 19:56:53, danakj wrote:
> > > On 2016/04/08 19:55:41, danakj wrote:
> > > > On 2016/04/08 19:51:17, boliu wrote:
> > > > > Oh! Need to deal with the case that this is null. Happens on android
> > webview
> > > > on
> > > > > android's render thread.
> > > > > 
> > > > > In that case, the client and server threads are the same, so a direct
> call
> > > is
> > > > > ok. Guess need to compare PlatformThreadHandle to get that case.
> > > > 
> > > > The old code dealt with null in WrapCallback and PostCallback by just
> > calling
> > > > the function directly. I should be doing the same thing in
> > > > OnContextLostOnGpuThread().
> > > > 
> > > > I guess it would call IsSet tho before calling Get, I can restore that.
> > > 
> > > Are there no tests where IsSet is false?
> > 
> > Most of the webview instrumentation tests won't have a message loop here I
> > believe. But they don't explicitly exercise the context loss path here.
> > 
> > OnContextLostOnGpuThread already checks for null origin_task_runner_, so
maybe
> > this should be ok. Not sure why those tests are blowing up then.. Need to
wait
> > for the bot to finish to get symbolized stack, hopefully.
> 
> I updated the CL to only set these when surface is null (which is when we
don't
> set the sequence_checker_ also).

And webview tests green:
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...

\o/

[commit-bot: I haz the power, 2016-04-08 21:24:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-08 21:24:55]

Dry run: This issue passed the CQ dry run.

[sky, 2016-04-08 23:05:24]

LGTM

[Wez, 2016-04-09 01:00:26]

https://codereview.chromium.org/1864723003/diff/300001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_context_provider.cc (right):

https://codereview.chromium.org/1864723003/diff/300001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_context_provider.cc:131:
lost_context_callback_.Run();
Was this previously broken in clearing the callback, or do we now specifically
want to allow multiple OnLostContext() w/out SetLostContextCallback() calls
in-between?

[danakj, 2016-04-09 01:02:17]

https://codereview.chromium.org/1864723003/diff/300001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_context_provider.cc (right):

https://codereview.chromium.org/1864723003/diff/300001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_context_provider.cc:131:
lost_context_callback_.Run();
On 2016/04/09 01:00:25, Wez wrote:
> Was this previously broken in clearing the callback, or do we now specifically
> want to allow multiple OnLostContext() w/out SetLostContextCallback() calls
> in-between?

The GpuControl will not call out more than once, so there's no need to guard
here. GLES2Impl DCHECKs this now.

[Wez, 2016-04-09 01:15:24]

lgtm

https://codereview.chromium.org/1864723003/diff/300001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_context_provider.cc (right):

https://codereview.chromium.org/1864723003/diff/300001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_context_provider.cc:131:
lost_context_callback_.Run();
On 2016/04/09 01:02:17, danakj wrote:
> On 2016/04/09 01:00:25, Wez wrote:
> > Was this previously broken in clearing the callback, or do we now
specifically
> > want to allow multiple OnLostContext() w/out SetLostContextCallback() calls
> > in-between?
> 
> The GpuControl will not call out more than once, so there's no need to guard
> here. GLES2Impl DCHECKs this now.

Ah, so this way if that contract gets broken then higher levels will see
multiple lost context callbacks and barf accordingly. SGTM!

[Ken Russell (switch to Gerrit), 2016-04-09 06:02:44]

On 2016/04/09 01:15:24, Wez wrote:
> lgtm
> 
>
https://codereview.chromium.org/1864723003/diff/300001/blimp/client/feature/c...
> File blimp/client/feature/compositor/blimp_context_provider.cc (right):
> 
>
https://codereview.chromium.org/1864723003/diff/300001/blimp/client/feature/c...
> blimp/client/feature/compositor/blimp_context_provider.cc:131:
> lost_context_callback_.Run();
> On 2016/04/09 01:02:17, danakj wrote:
> > On 2016/04/09 01:00:25, Wez wrote:
> > > Was this previously broken in clearing the callback, or do we now
> specifically
> > > want to allow multiple OnLostContext() w/out SetLostContextCallback()
calls
> > > in-between?
> > 
> > The GpuControl will not call out more than once, so there's no need to guard
> > here. GLES2Impl DCHECKs this now.
> 
> Ah, so this way if that contract gets broken then higher levels will see
> multiple lost context callbacks and barf accordingly. SGTM!

Note: hopefully the context_lost tests which run on the GPU bots should catch
these sorts of errors. If not, they should be expanded.

[piman, 2016-04-11 21:54:26]

https://codereview.chromium.org/1864723003/diff/300001/gpu/command_buffer/ser...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1864723003/diff/300001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.cc:456:
client_thread_weak_ptr_factory_.InvalidateWeakPtrs();
nit: reset  gpu_control_client_

https://codereview.chromium.org/1864723003/diff/300001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.cc:497: if (context_lost_)
Data race here - context_lost_ is modified on the main thread. Maybe this should
only be checked on l.499 after we know we're on the client thread?

Also, it could be a DCHECK. The only thing that will call this is
CommandBufferService::SetParseError, which only happens in 2 ways:
- in CommandExecutor, it only happens after executing commands, which only
happens if the error was kNoError.
- in InProcessCommandBuffer::MakeCurrent, if the makecurrent fails, but only
after we checked we didn't have an error.
So either way, it won't be called more than once.

https://codereview.chromium.org/1864723003/diff/300001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.cc:508:
DCHECK(gpu_control_client_);
Does the client *have* to provide one?
Also, GLES2Implementation resets it, so I think we want to handle the case where
gpu_control_client_ is null.

https://codereview.chromium.org/1864723003/diff/300001/gpu/command_buffer/ser...
File gpu/command_buffer/service/in_process_command_buffer.h (right):

https://codereview.chromium.org/1864723003/diff/300001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.h:256: GpuControlClient*
gpu_control_client_;
nit: this is accessed on the client thread. Move below?

https://codereview.chromium.org/1864723003/diff/300001/gpu/ipc/client/command...
File gpu/ipc/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1864723003/diff/300001/gpu/ipc/client/command...
gpu/ipc/client/command_buffer_proxy_impl.cc:114: DCHECK(gpu_control_client_);
Same as InProcessCommandBuffer, I think we want to allow for this to be null.

https://codereview.chromium.org/1864723003/diff/300001/gpu/ipc/client/command...
gpu/ipc/client/command_buffer_proxy_impl.cc:132: DCHECK(gpu_control_client_);
(ditto)

https://codereview.chromium.org/1864723003/diff/300001/mojo/gles2/command_buf...
File mojo/gles2/command_buffer_client_impl.cc (right):

https://codereview.chromium.org/1864723003/diff/300001/mojo/gles2/command_buf...
mojo/gles2/command_buffer_client_impl.cc:294: DCHECK(gpu_control_client_);
Here too, allow it to be null?

https://codereview.chromium.org/1864723003/diff/300001/ppapi/proxy/ppapi_comm...
File ppapi/proxy/ppapi_command_buffer_proxy.cc (right):

https://codereview.chromium.org/1864723003/diff/300001/ppapi/proxy/ppapi_comm...
ppapi/proxy/ppapi_command_buffer_proxy.cc:246: // of GpuControl, so we don't
bother to store the client.
Actually, there is a lost context callback, but right now it doesn't flow here
(it flows directly to the plugin instance), so this is fine. I'll try to wire it
up properly as a follow-up. You can TODO(piman).

[danakj, 2016-04-11 22:27:32]

Thanks! PTAL

https://codereview.chromium.org/1864723003/diff/300001/gpu/command_buffer/ser...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1864723003/diff/300001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.cc:456:
client_thread_weak_ptr_factory_.InvalidateWeakPtrs();
On 2016/04/11 21:54:26, piman wrote:
> nit: reset  gpu_control_client_

Done.

https://codereview.chromium.org/1864723003/diff/300001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.cc:497: if (context_lost_)
On 2016/04/11 21:54:26, piman wrote:
> Data race here - context_lost_ is modified on the main thread. Maybe this
should
> only be checked on l.499 after we know we're on the client thread?
> 
> Also, it could be a DCHECK. The only thing that will call this is
> CommandBufferService::SetParseError, which only happens in 2 ways:
> - in CommandExecutor, it only happens after executing commands, which only
> happens if the error was kNoError.
> - in InProcessCommandBuffer::MakeCurrent, if the makecurrent fails, but only
> after we checked we didn't have an error.
> So either way, it won't be called more than once.

OK I made it a DCHECK in OnContextLost().

https://codereview.chromium.org/1864723003/diff/300001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.cc:508:
DCHECK(gpu_control_client_);
On 2016/04/11 21:54:26, piman wrote:
> Does the client *have* to provide one?
> Also, GLES2Implementation resets it, so I think we want to handle the case
where
> gpu_control_client_ is null.

Hm... okay. Done for CommandBufferProxyImpl and mojo::CommandBufferClientImpl
too.

https://codereview.chromium.org/1864723003/diff/300001/ppapi/proxy/ppapi_comm...
File ppapi/proxy/ppapi_command_buffer_proxy.cc (right):

https://codereview.chromium.org/1864723003/diff/300001/ppapi/proxy/ppapi_comm...
ppapi/proxy/ppapi_command_buffer_proxy.cc:246: // of GpuControl, so we don't
bother to store the client.
On 2016/04/11 21:54:26, piman wrote:
> Actually, there is a lost context callback, but right now it doesn't flow here
> (it flows directly to the plugin instance), so this is fine. I'll try to wire
it
> up properly as a follow-up. You can TODO(piman).

Done.

[piman, 2016-04-11 22:35:54]

lgtm

https://codereview.chromium.org/1864723003/diff/300001/gpu/command_buffer/ser...
File gpu/command_buffer/service/in_process_command_buffer.h (right):

https://codereview.chromium.org/1864723003/diff/300001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.h:256: GpuControlClient*
gpu_control_client_;
On 2016/04/11 21:54:26, piman wrote:
> nit: this is accessed on the client thread. Move below?

ping (both  gpu_control_client_ and context_lost_, actually)

[danakj, 2016-04-11 22:42:46]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-04-11 22:42:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, sievers@chromium.org, boliu@chromium.org,
sky@chromium.org, wez@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1864723003/#ps360001
(title: "errorcallback: nitmissed")

[danakj, 2016-04-11 22:42:56]

https://codereview.chromium.org/1864723003/diff/300001/gpu/command_buffer/ser...
File gpu/command_buffer/service/in_process_command_buffer.h (right):

https://codereview.chromium.org/1864723003/diff/300001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.h:256: GpuControlClient*
gpu_control_client_;
On 2016/04/11 22:35:54, piman wrote:
> On 2016/04/11 21:54:26, piman wrote:
> > nit: this is accessed on the client thread. Move below?
> 
> ping (both  gpu_control_client_ and context_lost_, actually)

Oops thanks, missed this comment.

[commit-bot: I haz the power, 2016-04-11 22:43:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1864723003/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1864723003/360001

[commit-bot: I haz the power, 2016-04-12 02:02:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-12 02:02:37]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[danakj, 2016-04-12 18:38:24]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2016-04-12 18:38:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1864723003/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1864723003/360001

[commit-bot: I haz the power, 2016-04-12 19:35:52]

Description was changed from

==========
Make lost context and error message callbacks on GpuControl go to client

In this we introduce a GpuControlClient, that implementations of
GpuControl will call to notify about the context becoming lost, or of
error messages.

Users of GpuControl implement the client to hear about these states.
The most important users of GpuControl here are:
- GLES2Implementation, which now has 2 base::Callbacks which it forwards
  these events to, if they have been set by the user of the
  GLES2Implementation.
- PPB_Graphics3D_Impl, which acts on these two events.
- PepperVideoEncoderHost, which turns lost context events into a call
  to NotifyPepperError(PP_ERROR_RESOURCE_FAILED).
- mojo::GLES2Context, which forwards the lost context event to a
  registered callback.

Other owners of GpuControl (if they exist) never set callbacks on
CommandBufferProxyImpl or equivalent before, so they don't care to
listen to these events and just opt out of setting the client
altogether.

The main implementations of GpuControl here are:
- CommandBufferProxyImpl, which will now call the client instead of
  owning base::Callbacks for these two events.
- mojo::CommandBufferClientImpl, which notifies its client of lost
  contexts, but has no concept of error messages.
- InProcessCommandBuffer, which will now call the client (on the client
  thread) instead of owning and wrapping base::Callbacks for the lost
  context event. It also has no error messages.

For other implementations of GpuControl there is no chance to lose the
context or generate error messages, and they don't even bother to store
the client since it would go unused.

WebGraphicsContext3D{CommandBuffer}Impl continues to be used for these
callbacks, but it now goes directly to the GLES2Implementation to set
callbacks, instead of bypassing it to the CommandBufferProxyImpl.

The next step will be to have Blink or the ContextProvider directly set
these callbacks on the GLES2Implementation, bypassing the
WebGraphicsContext3DImpls entirely. This isn't possible today as the
two versions of WebGraphicsContext3DImpl (which are
WebGraphicsContext3DCommandBufferImpl and
WebGraphicsContext3DInProcessCommandBufferImpl) each have different
versions of the GpuControl, while both having a GLES2Implementation,
forcing ContextProviderCommandBuffer to route through this fork
when setting the callbacks.

R=piman@chomium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Make lost context and error message callbacks on GpuControl go to client

In this we introduce a GpuControlClient, that implementations of
GpuControl will call to notify about the context becoming lost, or of
error messages.

Users of GpuControl implement the client to hear about these states.
The most important users of GpuControl here are:
- GLES2Implementation, which now has 2 base::Callbacks which it forwards
  these events to, if they have been set by the user of the
  GLES2Implementation.
- PPB_Graphics3D_Impl, which acts on these two events.
- PepperVideoEncoderHost, which turns lost context events into a call
  to NotifyPepperError(PP_ERROR_RESOURCE_FAILED).
- mojo::GLES2Context, which forwards the lost context event to a
  registered callback.

Other owners of GpuControl (if they exist) never set callbacks on
CommandBufferProxyImpl or equivalent before, so they don't care to
listen to these events and just opt out of setting the client
altogether.

The main implementations of GpuControl here are:
- CommandBufferProxyImpl, which will now call the client instead of
  owning base::Callbacks for these two events.
- mojo::CommandBufferClientImpl, which notifies its client of lost
  contexts, but has no concept of error messages.
- InProcessCommandBuffer, which will now call the client (on the client
  thread) instead of owning and wrapping base::Callbacks for the lost
  context event. It also has no error messages.

For other implementations of GpuControl there is no chance to lose the
context or generate error messages, and they don't even bother to store
the client since it would go unused.

WebGraphicsContext3D{CommandBuffer}Impl continues to be used for these
callbacks, but it now goes directly to the GLES2Implementation to set
callbacks, instead of bypassing it to the CommandBufferProxyImpl.

The next step will be to have Blink or the ContextProvider directly set
these callbacks on the GLES2Implementation, bypassing the
WebGraphicsContext3DImpls entirely. This isn't possible today as the
two versions of WebGraphicsContext3DImpl (which are
WebGraphicsContext3DCommandBufferImpl and
WebGraphicsContext3DInProcessCommandBufferImpl) each have different
versions of the GpuControl, while both having a GLES2Implementation,
forcing ContextProviderCommandBuffer to route through this fork
when setting the callbacks.

R=piman@chomium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-04-12 19:35:55]

Committed patchset #18 (id:360001)

[commit-bot: I haz the power, 2016-04-12 19:37:10]

Description was changed from

==========
Make lost context and error message callbacks on GpuControl go to client

In this we introduce a GpuControlClient, that implementations of
GpuControl will call to notify about the context becoming lost, or of
error messages.

Users of GpuControl implement the client to hear about these states.
The most important users of GpuControl here are:
- GLES2Implementation, which now has 2 base::Callbacks which it forwards
  these events to, if they have been set by the user of the
  GLES2Implementation.
- PPB_Graphics3D_Impl, which acts on these two events.
- PepperVideoEncoderHost, which turns lost context events into a call
  to NotifyPepperError(PP_ERROR_RESOURCE_FAILED).
- mojo::GLES2Context, which forwards the lost context event to a
  registered callback.

Other owners of GpuControl (if they exist) never set callbacks on
CommandBufferProxyImpl or equivalent before, so they don't care to
listen to these events and just opt out of setting the client
altogether.

The main implementations of GpuControl here are:
- CommandBufferProxyImpl, which will now call the client instead of
  owning base::Callbacks for these two events.
- mojo::CommandBufferClientImpl, which notifies its client of lost
  contexts, but has no concept of error messages.
- InProcessCommandBuffer, which will now call the client (on the client
  thread) instead of owning and wrapping base::Callbacks for the lost
  context event. It also has no error messages.

For other implementations of GpuControl there is no chance to lose the
context or generate error messages, and they don't even bother to store
the client since it would go unused.

WebGraphicsContext3D{CommandBuffer}Impl continues to be used for these
callbacks, but it now goes directly to the GLES2Implementation to set
callbacks, instead of bypassing it to the CommandBufferProxyImpl.

The next step will be to have Blink or the ContextProvider directly set
these callbacks on the GLES2Implementation, bypassing the
WebGraphicsContext3DImpls entirely. This isn't possible today as the
two versions of WebGraphicsContext3DImpl (which are
WebGraphicsContext3DCommandBufferImpl and
WebGraphicsContext3DInProcessCommandBufferImpl) each have different
versions of the GpuControl, while both having a GLES2Implementation,
forcing ContextProviderCommandBuffer to route through this fork
when setting the callbacks.

R=piman@chomium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Make lost context and error message callbacks on GpuControl go to client

In this we introduce a GpuControlClient, that implementations of
GpuControl will call to notify about the context becoming lost, or of
error messages.

Users of GpuControl implement the client to hear about these states.
The most important users of GpuControl here are:
- GLES2Implementation, which now has 2 base::Callbacks which it forwards
  these events to, if they have been set by the user of the
  GLES2Implementation.
- PPB_Graphics3D_Impl, which acts on these two events.
- PepperVideoEncoderHost, which turns lost context events into a call
  to NotifyPepperError(PP_ERROR_RESOURCE_FAILED).
- mojo::GLES2Context, which forwards the lost context event to a
  registered callback.

Other owners of GpuControl (if they exist) never set callbacks on
CommandBufferProxyImpl or equivalent before, so they don't care to
listen to these events and just opt out of setting the client
altogether.

The main implementations of GpuControl here are:
- CommandBufferProxyImpl, which will now call the client instead of
  owning base::Callbacks for these two events.
- mojo::CommandBufferClientImpl, which notifies its client of lost
  contexts, but has no concept of error messages.
- InProcessCommandBuffer, which will now call the client (on the client
  thread) instead of owning and wrapping base::Callbacks for the lost
  context event. It also has no error messages.

For other implementations of GpuControl there is no chance to lose the
context or generate error messages, and they don't even bother to store
the client since it would go unused.

WebGraphicsContext3D{CommandBuffer}Impl continues to be used for these
callbacks, but it now goes directly to the GLES2Implementation to set
callbacks, instead of bypassing it to the CommandBufferProxyImpl.

The next step will be to have Blink or the ContextProvider directly set
these callbacks on the GLES2Implementation, bypassing the
WebGraphicsContext3DImpls entirely. This isn't possible today as the
two versions of WebGraphicsContext3DImpl (which are
WebGraphicsContext3DCommandBufferImpl and
WebGraphicsContext3DInProcessCommandBufferImpl) each have different
versions of the GpuControl, while both having a GLES2Implementation,
forcing ContextProviderCommandBuffer to route through this fork
when setting the callbacks.

R=piman@chomium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/10057772128ab5e67452dc736bf15752dc434ed3
Cr-Commit-Position: refs/heads/master@{#386761}
==========

[commit-bot: I haz the power, 2016-04-12 19:37:11]

Patchset 18 (id:??) landed as
https://crrev.com/10057772128ab5e67452dc736bf15752dc434ed3
Cr-Commit-Position: refs/heads/master@{#386761}

[rnephew (Reviews Here), 2016-04-13 16:49:45]

A revert of this CL (patchset #18 id:360001) has been created in
https://codereview.chromium.org/1885903002/ by rnephew@chromium.org.

The reason for reverting is: This possibly causes page_cycler.intl_hi_ru and
page_cycler.intl_es_fr_pt-BR to fail.
crbug.com/602801.

[kristinllewellyn88, 2016-05-18 06:58:57]

kristinllewellyn88@gmail.com changed reviewers:
+ kristinllewellyn88@gmail.com

[kristinllewellyn88, 2016-05-18 07:00:05]

lgtm