cc: Make OutputSurface::BindToClient pure virtual and not return bool

cc: Make OutputSurface::BindToClient pure virtual and not return bool

The function does not need to return bool and we DCHECK it is always
true already, since the ContextProvider given to the OutputSurface can
be initialized before even creating the OutputSurface as it is not
used across threads anymore.

Since it is not cross-thread the base class code in BindToClient can
move to its constructor and the method can become pure virtual.

Drops the DidLoseOutputSurface from the OutputSurfaceClient interface
so that every implementation doesn't need to bind it - esp since only
OutputSurfaces with a context can even get lost. Instead have Display
listen to the context for loss.

And some random code cleanups in AndroidOutputSurface since it was
working around multiple threads that don't exist.

R=enne@chromium.org, piman@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/94486a41405a0a792a0aed2324b9a841d536d853
Cr-Commit-Position: refs/heads/master@{#427511}

[danakj, 2016-10-24 22:15:13]

Description was changed from

==========
cc: Make OutputSurface::BindToClient pure virtual and not return bool

The function does not need to return bool and we DCHECK it is always
true already, since the ContextProvider given to the OutputSurface can
be initialized before even creating the OutputSurface as it is not
used across threads anymore.

Since it is not cross-thread the base class code in BindToClient can
move to its constructor and the method can become pure virtual.

R=enne@chromium.org, piman@chromium.org
BUG=
==========

to

==========
cc: Make OutputSurface::BindToClient pure virtual and not return bool

The function does not need to return bool and we DCHECK it is always
true already, since the ContextProvider given to the OutputSurface can
be initialized before even creating the OutputSurface as it is not
used across threads anymore.

Since it is not cross-thread the base class code in BindToClient can
move to its constructor and the method can become pure virtual.

R=enne@chromium.org, piman@chromium.org
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[danakj, 2016-10-24 22:15:22]

Description was changed from

==========
cc: Make OutputSurface::BindToClient pure virtual and not return bool

The function does not need to return bool and we DCHECK it is always
true already, since the ContextProvider given to the OutputSurface can
be initialized before even creating the OutputSurface as it is not
used across threads anymore.

Since it is not cross-thread the base class code in BindToClient can
move to its constructor and the method can become pure virtual.

R=enne@chromium.org, piman@chromium.org
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Make OutputSurface::BindToClient pure virtual and not return bool

The function does not need to return bool and we DCHECK it is always
true already, since the ContextProvider given to the OutputSurface can
be initialized before even creating the OutputSurface as it is not
used across threads anymore.

Since it is not cross-thread the base class code in BindToClient can
move to its constructor and the method can become pure virtual.

R=enne@chromium.org, piman@chromium.org
BUG=606056
==========

[danakj, 2016-10-24 23:26:43]

Description was changed from

==========
cc: Make OutputSurface::BindToClient pure virtual and not return bool

The function does not need to return bool and we DCHECK it is always
true already, since the ContextProvider given to the OutputSurface can
be initialized before even creating the OutputSurface as it is not
used across threads anymore.

Since it is not cross-thread the base class code in BindToClient can
move to its constructor and the method can become pure virtual.

R=enne@chromium.org, piman@chromium.org
BUG=606056
==========

to

==========
cc: Make OutputSurface::BindToClient pure virtual and not return bool

The function does not need to return bool and we DCHECK it is always
true already, since the ContextProvider given to the OutputSurface can
be initialized before even creating the OutputSurface as it is not
used across threads anymore.

Since it is not cross-thread the base class code in BindToClient can
move to its constructor and the method can become pure virtual.

R=enne@chromium.org, piman@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[danakj, 2016-10-24 23:28:02]

Description was changed from

==========
cc: Make OutputSurface::BindToClient pure virtual and not return bool

The function does not need to return bool and we DCHECK it is always
true already, since the ContextProvider given to the OutputSurface can
be initialized before even creating the OutputSurface as it is not
used across threads anymore.

Since it is not cross-thread the base class code in BindToClient can
move to its constructor and the method can become pure virtual.

R=enne@chromium.org, piman@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Make OutputSurface::BindToClient pure virtual and not return bool

The function does not need to return bool and we DCHECK it is always
true already, since the ContextProvider given to the OutputSurface can
be initialized before even creating the OutputSurface as it is not
used across threads anymore.

Since it is not cross-thread the base class code in BindToClient can
move to its constructor and the method can become pure virtual.

R=enne@chromium.org, piman@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[danakj, 2016-10-24 23:28:02]

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

[danakj, 2016-10-24 23:29:18]

Description was changed from

==========
cc: Make OutputSurface::BindToClient pure virtual and not return bool

The function does not need to return bool and we DCHECK it is always
true already, since the ContextProvider given to the OutputSurface can
be initialized before even creating the OutputSurface as it is not
used across threads anymore.

Since it is not cross-thread the base class code in BindToClient can
move to its constructor and the method can become pure virtual.

R=enne@chromium.org, piman@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Make OutputSurface::BindToClient pure virtual and not return bool

The function does not need to return bool and we DCHECK it is always
true already, since the ContextProvider given to the OutputSurface can
be initialized before even creating the OutputSurface as it is not
used across threads anymore.

Since it is not cross-thread the base class code in BindToClient can
move to its constructor and the method can become pure virtual.

Drops the DidLoseOutputSurface from the OutputSurfaceClient interface
so that every implementation doesn't need to bind it - esp since only
OutputSurfaces with a context can even get lost. Instead have Display
listen to the context for loss.

And some random code cleanups in AndroidOutputSurface since it was
working around multiple threads that don't exist.

R=enne@chromium.org, piman@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[danakj, 2016-10-24 23:29:32]

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

[danakj, 2016-10-24 23:29:48]

enne: overall
piman: content
boliu: aw
fsamuel: services

[commit-bot: I haz the power, 2016-10-24 23:30:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-24 23:34:15]

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

[commit-bot: I haz the power, 2016-10-24 23:34:15]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[danakj, 2016-10-24 23:41:59]

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

[commit-bot: I haz the power, 2016-10-24 23:42:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2016-10-24 23:50:35]

lgtm

[commit-bot: I haz the power, 2016-10-24 23:58:10]

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

[commit-bot: I haz the power, 2016-10-24 23:58:11]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[boliu, 2016-10-25 00:07:35]

https://codereview.chromium.org/2443003004/diff/40001/content/browser/composi...
File content/browser/compositor/gpu_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2443003004/diff/40001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:36:
context_provider()->ContextCapabilities().flips_vertically;
is accessing the context safe here? or is the assumption here that browser
compositor is single threaded, so BindtoClient and all other context access also
happen on the UI thread?

basically the same question for all the content changes

[danakj, 2016-10-25 00:11:02]

https://codereview.chromium.org/2443003004/diff/40001/content/browser/composi...
File content/browser/compositor/gpu_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2443003004/diff/40001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:36:
context_provider()->ContextCapabilities().flips_vertically;
On 2016/10/25 00:07:35, boliu wrote:
> is accessing the context safe here? or is the assumption here that browser
> compositor is single threaded, so BindtoClient and all other context access
also
> happen on the UI thread?
> 
> basically the same question for all the content changes

Yes OutputSurface is now single-threaded. Display compositor does not use
multiple threads like LTHI (used to in the browser process sometimes)

The context is always bound before being handed to the OutputSurface now in GPTF
or CompositorImplAndroid

[danakj, 2016-10-25 00:11:44]

https://codereview.chromium.org/2443003004/diff/40001/content/browser/composi...
File content/browser/compositor/gpu_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2443003004/diff/40001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:36:
context_provider()->ContextCapabilities().flips_vertically;
On 2016/10/25 00:11:01, danakj wrote:
> On 2016/10/25 00:07:35, boliu wrote:
> > is accessing the context safe here? or is the assumption here that browser
> > compositor is single threaded, so BindtoClient and all other context access
> also
> > happen on the UI thread?
> > 
> > basically the same question for all the content changes
> 
> Yes OutputSurface is now single-threaded. Display compositor does not use
> multiple threads like LTHI (used to in the browser process sometimes)
> 
> The context is always bound before being handed to the OutputSurface now in
GPTF
> or CompositorImplAndroid

Here's where for GPTF:
https://cs.chromium.org/chromium/src/content/browser/compositor/gpu_process_t...

[danakj, 2016-10-25 00:13:29]

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

[commit-bot: I haz the power, 2016-10-25 00:14:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 00:23:03]

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

[commit-bot: I haz the power, 2016-10-25 00:23:04]

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

[boliu, 2016-10-25 00:26:36]

lgtm

[danakj, 2016-10-25 00:28:14]

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

[commit-bot: I haz the power, 2016-10-25 00:29:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 00:39:00]

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

[commit-bot: I haz the power, 2016-10-25 00:39:01]

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

[piman, 2016-10-25 00:47:44]

lgtm

https://codereview.chromium.org/2443003004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/2443003004/diff/80001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:301:
base::WeakPtrFactory<AndroidOutputSurface> weak_ptr_factory_;
I /think/ it's not necessary because AndroidOutputSurface is the sole owner of
the ContextProvider/CommandBufferProxyImpl, but I guess it doesn't really hurt?

[danakj, 2016-10-25 00:49:32]

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

[danakj, 2016-10-25 00:49:33]

+khushalsagar for blimp

[danakj, 2016-10-25 00:49:55]

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

[danakj, 2016-10-25 00:50:19]

https://codereview.chromium.org/2443003004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/2443003004/diff/80001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:301:
base::WeakPtrFactory<AndroidOutputSurface> weak_ptr_factory_;
On 2016/10/25 00:47:44, piman wrote:
> I /think/ it's not necessary because AndroidOutputSurface is the sole owner of
> the ContextProvider/CommandBufferProxyImpl, but I guess it doesn't really
hurt?

Ya I agree, but refptrs scare me.

[commit-bot: I haz the power, 2016-10-25 00:50:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Khushal, 2016-10-25 01:10:02]

https://codereview.chromium.org/2443003004/diff/100001/blimp/client/support/c...
File blimp/client/support/compositor/blimp_embedder_compositor.cc (right):

https://codereview.chromium.org/2443003004/diff/100001/blimp/client/support/c...
blimp/client/support/compositor/blimp_embedder_compositor.cc:200:
DCHECK(context_provider_);
Should we context_provider_->BindToCurrentThread() here and DCHECK that it was
successful? Since otherwise its going to fail later anyway?

https://codereview.chromium.org/2443003004/diff/100001/content/browser/render...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/2443003004/diff/100001/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:689:
context_provider_command_buffer->BindToCurrentThread();
Btw, do we mean to do any fallbacks here? IIUC when this happens with the
CompositorFrameSink, it shouts back to the LTHC to make a new
CompositorFrameSink, and we try twice before giving up.
https://cs.chromium.org/chromium/src/content/browser/renderer_host/compositor...

[Khushal, 2016-10-25 01:28:35]

https://codereview.chromium.org/2443003004/diff/100001/content/browser/render...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/2443003004/diff/100001/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:689:
context_provider_command_buffer->BindToCurrentThread();
On 2016/10/25 01:10:02, Khushal wrote:
> Btw, do we mean to do any fallbacks here? IIUC when this happens with the
> CompositorFrameSink, it shouts back to the LTHC to make a new
> CompositorFrameSink, and we try twice before giving up.
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/compositor...

Nvm, just remembered that the context with the FrameSink given to the host and
the display is shared. Its weird that we have this logic here in
DidFailToInitializeCompositorFrameSink. Will this ever happen?

[commit-bot: I haz the power, 2016-10-25 01:50:00]

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

[commit-bot: I haz the power, 2016-10-25 01:50:02]

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

[enne (OOO), 2016-10-25 18:22:59]

https://codereview.chromium.org/2443003004/diff/100001/cc/output/gl_renderer_...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/2443003004/diff/100001/cc/output/gl_renderer_...
cc/output/gl_renderer_unittest.cc:602: auto provider =
TestContextProvider::Create(std::move(context));
Should TestContextProvider just BindToCurrentThread in its ctor? Or if there are
use cases where it doesn't, just make a second class that doesn't? It just seems
like this gets repeated a lot and it's pretty verbose to have to create it
separately to make a call on it before you pass it somewhere else.

[danakj, 2016-10-25 18:54:19]

https://codereview.chromium.org/2443003004/diff/100001/cc/output/gl_renderer_...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/2443003004/diff/100001/cc/output/gl_renderer_...
cc/output/gl_renderer_unittest.cc:602: auto provider =
TestContextProvider::Create(std::move(context));
On 2016/10/25 18:22:59, enne wrote:
> Should TestContextProvider just BindToCurrentThread in its ctor? Or if there
are
> use cases where it doesn't, just make a second class that doesn't? It just
seems
> like this gets repeated a lot and it's pretty verbose to have to create it
> separately to make a call on it before you pass it somewhere else.

So.. yah. CreateWorker() will bind immediately. Layer compositor contexts don't
get bound immediately tho and that's always been the default behaviour in cc
unittests. Eventually they will too when they are created on the compositor
thread, but.. yah.

I could change all the callsites to CreateAndBind() or something instead. I'd
have to go find all the places that make a context for the layer compositor
though if I changed the default. And then we'd be missing calls to
BindToCurrentThread() that would be needed with a production ContextProvider. So
overall Idk I don't think it'd be better?

[danakj, 2016-10-25 18:58:39]

https://codereview.chromium.org/2443003004/diff/100001/blimp/client/support/c...
File blimp/client/support/compositor/blimp_embedder_compositor.cc (right):

https://codereview.chromium.org/2443003004/diff/100001/blimp/client/support/c...
blimp/client/support/compositor/blimp_embedder_compositor.cc:200:
DCHECK(context_provider_);
On 2016/10/25 01:10:02, Khushal wrote:
> Should we context_provider_->BindToCurrentThread() here and DCHECK that it was
> successful? Since otherwise its going to fail later anyway?

No this is for the layer compositor's context it will be used on the compositor
thread, so we must bind it there (we do so inside LTHI).

https://codereview.chromium.org/2443003004/diff/100001/content/browser/render...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/2443003004/diff/100001/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:689:
context_provider_command_buffer->BindToCurrentThread();
On 2016/10/25 01:28:35, Khushal wrote:
> On 2016/10/25 01:10:02, Khushal wrote:
> > Btw, do we mean to do any fallbacks here? IIUC when this happens with the
> > CompositorFrameSink, it shouts back to the LTHC to make a new
> > CompositorFrameSink, and we try twice before giving up.
> >
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/compositor...
> 
> Nvm, just remembered that the context with the FrameSink given to the host and
> the display is shared. Its weird that we have this logic here in
> DidFailToInitializeCompositorFrameSink. Will this ever happen?

We don't check that the context is not dead-on-arrival here. This code is
depending on LTHI to check that instead, which it does so after we have set up
the lost context callback (in Display), so that we don't race between the two.

It's complicated and bad IMO but I think it's correct.

[danakj, 2016-10-25 19:00:00]

https://codereview.chromium.org/2443003004/diff/100001/blimp/client/support/c...
File blimp/client/support/compositor/blimp_embedder_compositor.cc (right):

https://codereview.chromium.org/2443003004/diff/100001/blimp/client/support/c...
blimp/client/support/compositor/blimp_embedder_compositor.cc:200:
DCHECK(context_provider_);
On 2016/10/25 18:58:38, danakj wrote:
> On 2016/10/25 01:10:02, Khushal wrote:
> > Should we context_provider_->BindToCurrentThread() here and DCHECK that it
was
> > successful? Since otherwise its going to fail later anyway?
> 
> No this is for the layer compositor's context it will be used on the
compositor
> thread, so we must bind it there (we do so inside LTHI).

I lied, I guess this is shared. Blimp is single threaded on the client? I didn't
realize. Anyhow.. ya it needs to be bound before being given to the Display.

[enne (OOO), 2016-10-25 20:01:14]

https://codereview.chromium.org/2443003004/diff/100001/cc/output/gl_renderer_...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/2443003004/diff/100001/cc/output/gl_renderer_...
cc/output/gl_renderer_unittest.cc:602: auto provider =
TestContextProvider::Create(std::move(context));
On 2016/10/25 at 18:54:18, danakj wrote:
> On 2016/10/25 18:22:59, enne wrote:
> > Should TestContextProvider just BindToCurrentThread in its ctor? Or if there
are
> > use cases where it doesn't, just make a second class that doesn't? It just
seems
> > like this gets repeated a lot and it's pretty verbose to have to create it
> > separately to make a call on it before you pass it somewhere else.
> 
> So.. yah. CreateWorker() will bind immediately. Layer compositor contexts
don't get bound immediately tho and that's always been the default behaviour in
cc unittests. Eventually they will too when they are created on the compositor
thread, but.. yah.
> 
> I could change all the callsites to CreateAndBind() or something instead. I'd
have to go find all the places that make a context for the layer compositor
though if I changed the default. And then we'd be missing calls to
BindToCurrentThread() that would be needed with a production ContextProvider. So
overall Idk I don't think it'd be better?

How many places want the default vs auto-bind?

Is switching in a production ContextProvider vs using a test one for these tests
a use case you want to support?

[danakj, 2016-10-25 20:12:00]

https://codereview.chromium.org/2443003004/diff/100001/cc/output/gl_renderer_...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/2443003004/diff/100001/cc/output/gl_renderer_...
cc/output/gl_renderer_unittest.cc:602: auto provider =
TestContextProvider::Create(std::move(context));
On 2016/10/25 20:01:14, enne wrote:
> On 2016/10/25 at 18:54:18, danakj wrote:
> > On 2016/10/25 18:22:59, enne wrote:
> > > Should TestContextProvider just BindToCurrentThread in its ctor? Or if
there
> are
> > > use cases where it doesn't, just make a second class that doesn't? It just
> seems
> > > like this gets repeated a lot and it's pretty verbose to have to create it
> > > separately to make a call on it before you pass it somewhere else.
> > 
> > So.. yah. CreateWorker() will bind immediately. Layer compositor contexts
> don't get bound immediately tho and that's always been the default behaviour
in
> cc unittests. Eventually they will too when they are created on the compositor
> thread, but.. yah.
> > 
> > I could change all the callsites to CreateAndBind() or something instead.
I'd
> have to go find all the places that make a context for the layer compositor
> though if I changed the default. And then we'd be missing calls to
> BindToCurrentThread() that would be needed with a production ContextProvider.
So
> overall Idk I don't think it'd be better?
> 
> How many places want the default vs auto-bind?

Any context handed to LayerTreeHostImpl needs to not be bound on the main
thread.

> Is switching in a production ContextProvider vs using a test one for these
tests
> a use case you want to support?

Well, all our production code uses ContextProvider, and it would make it very
easy to code LTHI stop binding the context in some path for example, but tests
wouldn't notice. Esp because BindToCurrentThread is explicitly allowed to be
called twice, so we can add this default binding almost-everywhere and then
remove the production bind and tests will be fine, but we'd break chrome.

I just don't feel very safe about making such a change, and would rather get rid
of BindToCurrentThread by creating contexts on the right thread instead.

I guess an alternative I would feel safer about would be a
DeferredContextProvider interface that spits out the ContextProvider and binds
it. Then we'd get typesafe "this is bound or not" throughout the system and we
couldn't get it wrong. It's a lot like creating contexts on the right thread,
but we could just wrap the current ContextProvider instead..

[danakj, 2016-10-25 20:14:56]

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

[commit-bot: I haz the power, 2016-10-25 20:15:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[enne (OOO), 2016-10-25 20:25:10]

I still think this is a lot of boilerplate that could be cleaned up here or
later, but sure, lgtm.

On 2016/10/25 at 20:12:00, danakj wrote:
> > Is switching in a production ContextProvider vs using a test one for these
tests
> > a use case you want to support?
> 
> Well, all our production code uses ContextProvider, and it would make it very
easy to code LTHI stop binding the context in some path for example, but tests
wouldn't notice. Esp because BindToCurrentThread is explicitly allowed to be
called twice, so we can add this default binding almost-everywhere and then
remove the production bind and tests will be fine, but we'd break chrome.

Tests wouldn't notice already because they're already explicitly calling
BindToCurrentThread in ways that are test-specific.  I'm really not sure what
your fears are here.

[danakj, 2016-10-25 20:27:56]

On Tue, Oct 25, 2016 at 1:25 PM, enne@chromium.org via
chromiumcodereview-hr.appspot.com <
reply@chromiumcodereview-hr.appspotmail.com> wrote:

> I still think this is a lot of boilerplate that could be cleaned up here or
> later, but sure, lgtm.
>
> On 2016/10/25 at 20:12:00, danakj wrote:
> > > Is switching in a production ContextProvider vs using a test one for
> these
> tests
> > > a use case you want to support?
> >
> > Well, all our production code uses ContextProvider, and it would make it
> very
> easy to code LTHI stop binding the context in some path for example, but
> tests
> wouldn't notice. Esp because BindToCurrentThread is explicitly allowed to
> be
> called twice, so we can add this default binding almost-everywhere and then
> remove the production bind and tests will be fine, but we'd break chrome.
>
> Tests wouldn't notice already because they're already explicitly calling
> BindToCurrentThread in ways that are test-specific. I'm really not sure
> what
> your fears are here.
>

Yes it's true. I guess it's that I had to add these explicitly, you are
reading them as a reviewer etc. If we do it in the TestCP class, now its
implicit everywhere and we can no longer see that it's in places that make
sense.


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

-- 
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.

[Khushal, 2016-10-25 20:45:06]

lgtm.

https://codereview.chromium.org/2443003004/diff/100001/blimp/client/support/c...
File blimp/client/support/compositor/blimp_embedder_compositor.cc (right):

https://codereview.chromium.org/2443003004/diff/100001/blimp/client/support/c...
blimp/client/support/compositor/blimp_embedder_compositor.cc:200:
DCHECK(context_provider_);
On 2016/10/25 19:00:00, danakj wrote:
> On 2016/10/25 18:58:38, danakj wrote:
> > On 2016/10/25 01:10:02, Khushal wrote:
> > > Should we context_provider_->BindToCurrentThread() here and DCHECK that it
> was
> > > successful? Since otherwise its going to fail later anyway?
> > 
> > No this is for the layer compositor's context it will be used on the
> compositor
> > thread, so we must bind it there (we do so inside LTHI).

I actually got confused about whether it was needed because I saw you did a Bind
in CompositorImpl before giving the context to the Display. I guess you had to
do it there to query the gpu::Capabilities?

> 
> I lied, I guess this is shared. Blimp is single threaded on the client? I
didn't
> realize. Anyhow.. ya it needs to be bound before being given to the Display.

Nope, the compositor which is the equivalent of the renderer compositor is
BlimpCompositor, which is threaded.

This one is meant to be a cheap knock off of the UI compositor in Chrome, so
anyone who embeds Blimp always gets a cc::Layer as the output, and Chrome adds
that to the UI compositor's LayerTree. So when we had to make the standalone app
a Blimp embedder also, we just added this to do what the Chrome UI compositor
would do.

https://codereview.chromium.org/2443003004/diff/100001/content/browser/render...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/2443003004/diff/100001/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:689:
context_provider_command_buffer->BindToCurrentThread();
On 2016/10/25 18:58:39, danakj wrote:
> On 2016/10/25 01:28:35, Khushal wrote:
> > On 2016/10/25 01:10:02, Khushal wrote:
> > > Btw, do we mean to do any fallbacks here? IIUC when this happens with the
> > > CompositorFrameSink, it shouts back to the LTHC to make a new
> > > CompositorFrameSink, and we try twice before giving up.
> > >
> >
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/compositor...
> > 
> > Nvm, just remembered that the context with the FrameSink given to the host
and
> > the display is shared. Its weird that we have this logic here in
> > DidFailToInitializeCompositorFrameSink. Will this ever happen?
> 
> We don't check that the context is not dead-on-arrival here. This code is
> depending on LTHI to check that instead, which it does so after we have set up
> the lost context callback (in Display), so that we don't race between the two.
> 
> It's complicated and bad IMO but I think it's correct.

Ohhh. Right CompositorFrameSink will still check this. Makes sense, thanks!

[danakj, 2016-10-25 20:47:23]

https://codereview.chromium.org/2443003004/diff/100001/blimp/client/support/c...
File blimp/client/support/compositor/blimp_embedder_compositor.cc (right):

https://codereview.chromium.org/2443003004/diff/100001/blimp/client/support/c...
blimp/client/support/compositor/blimp_embedder_compositor.cc:200:
DCHECK(context_provider_);
On 2016/10/25 20:45:06, Khushal wrote:
> On 2016/10/25 19:00:00, danakj wrote:
> > On 2016/10/25 18:58:38, danakj wrote:
> > > On 2016/10/25 01:10:02, Khushal wrote:
> > > > Should we context_provider_->BindToCurrentThread() here and DCHECK that
it
> > was
> > > > successful? Since otherwise its going to fail later anyway?
> > > 
> > > No this is for the layer compositor's context it will be used on the
> > compositor
> > > thread, so we must bind it there (we do so inside LTHI).
> 
> I actually got confused about whether it was needed because I saw you did a
Bind
> in CompositorImpl before giving the context to the Display. I guess you had to
> do it there to query the gpu::Capabilities?

Ya, Display will no longer (double) bind the context. GPTF does a bind, and I
added a bind here as you suggested because nothing would bind it now until it
went to LTHI which is late.

> > 
> > I lied, I guess this is shared. Blimp is single threaded on the client? I
> didn't
> > realize. Anyhow.. ya it needs to be bound before being given to the Display.
> 
> Nope, the compositor which is the equivalent of the renderer compositor is
> BlimpCompositor, which is threaded.
> 
> This one is meant to be a cheap knock off of the UI compositor in Chrome, so
> anyone who embeds Blimp always gets a cc::Layer as the output, and Chrome adds
> that to the UI compositor's LayerTree. So when we had to make the standalone
app
> a Blimp embedder also, we just added this to do what the Chrome UI compositor
> would do.

[commit-bot: I haz the power, 2016-10-25 20:59:48]

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

[commit-bot: I haz the power, 2016-10-25 20:59:50]

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

[danakj, 2016-10-25 21:03:11]

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

[commit-bot: I haz the power, 2016-10-25 21:03:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-10-25 22:15:26]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2016-10-25 22:15:32]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-10-25 22:15:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, boliu@chromium.org, piman@chromium.org,
khushalsagar@chromium.org, enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2443003004/#ps140001
(title: "bindtoclient-pure-virtual: rebase")

[commit-bot: I haz the power, 2016-10-25 22:15:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 22:33:59]

Description was changed from

==========
cc: Make OutputSurface::BindToClient pure virtual and not return bool

The function does not need to return bool and we DCHECK it is always
true already, since the ContextProvider given to the OutputSurface can
be initialized before even creating the OutputSurface as it is not
used across threads anymore.

Since it is not cross-thread the base class code in BindToClient can
move to its constructor and the method can become pure virtual.

Drops the DidLoseOutputSurface from the OutputSurfaceClient interface
so that every implementation doesn't need to bind it - esp since only
OutputSurfaces with a context can even get lost. Instead have Display
listen to the context for loss.

And some random code cleanups in AndroidOutputSurface since it was
working around multiple threads that don't exist.

R=enne@chromium.org, piman@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Make OutputSurface::BindToClient pure virtual and not return bool

The function does not need to return bool and we DCHECK it is always
true already, since the ContextProvider given to the OutputSurface can
be initialized before even creating the OutputSurface as it is not
used across threads anymore.

Since it is not cross-thread the base class code in BindToClient can
move to its constructor and the method can become pure virtual.

Drops the DidLoseOutputSurface from the OutputSurfaceClient interface
so that every implementation doesn't need to bind it - esp since only
OutputSurfaces with a context can even get lost. Instead have Display
listen to the context for loss.

And some random code cleanups in AndroidOutputSurface since it was
working around multiple threads that don't exist.

R=enne@chromium.org, piman@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/94486a41405a0a792a0aed2324b9a841d536d853
Cr-Commit-Position: refs/heads/master@{#427511}
==========

[commit-bot: I haz the power, 2016-10-25 22:34:01]

Patchset 8 (id:??) landed as
https://crrev.com/94486a41405a0a792a0aed2324b9a841d536d853
Cr-Commit-Position: refs/heads/master@{#427511}

[commit-bot: I haz the power, 2016-10-25 22:34:21]

Failed to apply patch for cc/output/output_surface_unittest.cc:
While running git rm cc/output/output_surface_unittest.cc;
  fatal: pathspec 'cc/output/output_surface_unittest.cc' did not match any files

Patch:  D    cc/output/output_surface_unittest.cc
Index: cc/output/output_surface_unittest.cc
diff --git a/cc/output/output_surface_unittest.cc
b/cc/output/output_surface_unittest.cc
deleted file mode 100644
index
2c40578117a2b27e0374bebb14f4f8142fad3027..0000000000000000000000000000000000000000
--- a/cc/output/output_surface_unittest.cc
+++ /dev/null
@@ -1,124 +0,0 @@
-// Copyright 2013 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "cc/output/output_surface.h"
-
-#include <utility>
-
-#include "base/memory/ptr_util.h"
-#include "cc/output/output_surface_frame.h"
-#include "cc/output/software_output_device.h"
-#include "cc/test/fake_output_surface_client.h"
-#include "cc/test/test_context_provider.h"
-#include "cc/test/test_web_graphics_context_3d.h"
-#include "gpu/GLES2/gl2extchromium.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace cc {
-namespace {
-
-class TestOutputSurface : public OutputSurface {
- public:
-  explicit TestOutputSurface(
-      scoped_refptr<TestContextProvider> context_provider)
-      : OutputSurface(std::move(context_provider)) {}
-
-  explicit TestOutputSurface(
-      std::unique_ptr<SoftwareOutputDevice> software_device)
-      : OutputSurface(std::move(software_device)) {}
-
-  void EnsureBackbuffer() override {}
-  void DiscardBackbuffer() override {}
-  void BindFramebuffer() override {}
-  void Reshape(const gfx::Size& size,
-               float device_scale_factor,
-               const gfx::ColorSpace& color_space,
-               bool has_alpha) override {}
-  void SwapBuffers(OutputSurfaceFrame frame) override {
-    client_->DidReceiveSwapBuffersAck();
-  }
-  uint32_t GetFramebufferCopyTextureFormat() override {
-    // TestContextProvider has no real framebuffer, just use RGB.
-    return GL_RGB;
-  }
-  OverlayCandidateValidator* GetOverlayCandidateValidator() const override {
-    return nullptr;
-  }
-  bool IsDisplayedAsOverlayPlane() const override { return false; }
-  unsigned GetOverlayTextureId() const override { return 0; }
-  bool SurfaceIsSuspendForRecycle() const override { return false; }
-  bool HasExternalStencilTest() const override { return false; }
-  void ApplyExternalStencil() override {}
-
-  void OnSwapBuffersCompleteForTesting() {
-    client_->DidReceiveSwapBuffersAck();
-  }
-
- protected:
-};
-
-class TestSoftwareOutputDevice : public SoftwareOutputDevice {
- public:
-  TestSoftwareOutputDevice();
-  ~TestSoftwareOutputDevice() override;
-
-  // Overriden from cc:SoftwareOutputDevice
-  void DiscardBackbuffer() override;
-  void EnsureBackbuffer() override;
-
-  int discard_backbuffer_count() { return discard_backbuffer_count_; }
-  int ensure_backbuffer_count() { return ensure_backbuffer_count_; }
-
- private:
-  int discard_backbuffer_count_;
-  int ensure_backbuffer_count_;
-};
-
-TestSoftwareOutputDevice::TestSoftwareOutputDevice()
-    : discard_backbuffer_count_(0), ensure_backbuffer_count_(0) {}
-
-TestSoftwareOutputDevice::~TestSoftwareOutputDevice() {}
-
-void TestSoftwareOutputDevice::DiscardBackbuffer() {
-  SoftwareOutputDevice::DiscardBackbuffer();
-  discard_backbuffer_count_++;
-}
-
-void TestSoftwareOutputDevice::EnsureBackbuffer() {
-  SoftwareOutputDevice::EnsureBackbuffer();
-  ensure_backbuffer_count_++;
-}
-
-TEST(OutputSurfaceTest, ContextLossInformsClient) {
-  scoped_refptr<TestContextProvider> provider = TestContextProvider::Create();
-  TestOutputSurface output_surface(provider);
-
-  FakeOutputSurfaceClient client;
-  EXPECT_TRUE(output_surface.BindToClient(&client));
-
-  // Verify DidLoseOutputSurface callback is hooked up correctly.
-  EXPECT_FALSE(client.did_lose_output_surface_called());
-  output_surface.context_provider()->ContextGL()->LoseContextCHROMIUM(
-      GL_GUILTY_CONTEXT_RESET_ARB, GL_INNOCENT_CONTEXT_RESET_ARB);
-  output_surface.context_provider()->ContextGL()->Flush();
-  EXPECT_TRUE(client.did_lose_output_surface_called());
-}
-
-// TODO(danakj): Add a test for worker context failure as well when
-// OutputSurface creates/binds it.
-TEST(OutputSurfaceTest, ContextLossFailsBind) {
-  scoped_refptr<TestContextProvider> context_provider =
-      TestContextProvider::Create();
-
-  // Lose the context so BindToClient fails.
-  context_provider->UnboundTestContext3d()->set_context_lost(true);
-
-  TestOutputSurface output_surface(context_provider);
-
-  FakeOutputSurfaceClient client;
-  EXPECT_FALSE(output_surface.BindToClient(&client));
-}
-
-}  // namespace
-}  // namespace cc