blimp: Set up the CompositorDependencies for blimp in Chrome.

blimp: Set up the CompositorDependencies for blimp in Chrome.

Add ChromeCompositorDependencies to implement the CompositorDependencies
for blimp in Chrome. The class passes through to the
ui::ContextProviderFactory to create the contexts.

TBR=jochen

BUG=611105
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/8ecea2e7082ed06d138d5b74f8a6cff2873b6af9
Cr-Commit-Position: refs/heads/master@{#416333}

[Khushal, 2016-08-30 23:10:19]

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

[Khushal, 2016-08-30 23:11:51]

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

[Khushal, 2016-08-30 23:11:52]

+danakj for DEPS.

[David Trainor- moved to gerrit, 2016-08-31 00:28:58]

https://codereview.chromium.org/2297933002/diff/1/blimp/client/core/blimp_cli...
File blimp/client/core/blimp_client_context_impl_unittest.cc (right):

https://codereview.chromium.org/2297933002/diff/1/blimp/client/core/blimp_cli...
blimp/client/core/blimp_client_context_impl_unittest.cc:53:
base::MakeUnique<CompositorDependenciesImpl>());
I moved MockCompositorDependencies out to blimp/client/support/compositor since
that looked cleaner (in case anything did try to trigger a ContextProvider).

https://codereview.chromium.org/2297933002/diff/1/blimp/client/feature/compos...
File blimp/client/feature/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2297933002/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.cc:177:
weak_ptr_factory_.GetWeakPtr()));
Return after this?

https://codereview.chromium.org/2297933002/diff/1/blimp/client/feature/compos...
File blimp/client/feature/compositor/blimp_compositor.h (right):

https://codereview.chromium.org/2297933002/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.h:109: void
OnContextProviderCreated(
Provider -> Providers?

https://codereview.chromium.org/2297933002/diff/1/blimp/client/public/composi...
File blimp/client/public/compositor/compositor_dependencies.cc (right):

https://codereview.chromium.org/2297933002/diff/1/blimp/client/public/composi...
blimp/client/public/compositor/compositor_dependencies.cc:12:
CompositorDependencies::ContextProviders::ContextProviders() = default;
Did this not work in the header?

https://codereview.chromium.org/2297933002/diff/1/chrome/browser/android/blim...
File chrome/browser/android/blimp/chrome_compositor_dependencies.cc (right):

https://codereview.chromium.org/2297933002/diff/1/chrome/browser/android/blim...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:67: // MAKE THE
SETTINGS!!!!!
yes plz!  In another patch maybe pull out the CompositorDependenciesImpl
LayerTreeSettings creator to a static helper so this can use it in the short
term?

https://codereview.chromium.org/2297933002/diff/1/chrome/browser/android/blim...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:96:
shared_main_thread_worker_context_ = nullptr;
What happens to any outstanding requests if this dies while we're talking to the
context provider factory?

https://codereview.chromium.org/2297933002/diff/1/chrome/browser/android/blim...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:131: }
What if this returns false?

https://codereview.chromium.org/2297933002/diff/1/chrome/browser/android/blim...
File chrome/browser/android/blimp/chrome_compositor_dependencies.h (right):

https://codereview.chromium.org/2297933002/diff/1/chrome/browser/android/blim...
chrome/browser/android/blimp/chrome_compositor_dependencies.h:43: bool
worker_context_request_pending_;
Add some comments around why we're doing it this way and how it works.

[Khushal, 2016-08-31 21:22:51]

https://codereview.chromium.org/2297933002/diff/1/blimp/client/core/blimp_cli...
File blimp/client/core/blimp_client_context_impl_unittest.cc (right):

https://codereview.chromium.org/2297933002/diff/1/blimp/client/core/blimp_cli...
blimp/client/core/blimp_client_context_impl_unittest.cc:53:
base::MakeUnique<CompositorDependenciesImpl>());
On 2016/08/31 00:28:58, David Trainor wrote:
> I moved MockCompositorDependencies out to blimp/client/support/compositor
since
> that looked cleaner (in case anything did try to trigger a ContextProvider).

Rebase took care of it.

https://codereview.chromium.org/2297933002/diff/1/blimp/client/feature/compos...
File blimp/client/feature/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2297933002/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.cc:177:
weak_ptr_factory_.GetWeakPtr()));
On 2016/08/31 00:28:58, David Trainor wrote:
> Return after this?

Good catch. Done.

https://codereview.chromium.org/2297933002/diff/1/blimp/client/feature/compos...
File blimp/client/feature/compositor/blimp_compositor.h (right):

https://codereview.chromium.org/2297933002/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.h:109: void
OnContextProviderCreated(
On 2016/08/31 00:28:58, David Trainor wrote:
> Provider -> Providers?

Done.

https://codereview.chromium.org/2297933002/diff/1/blimp/client/public/composi...
File blimp/client/public/compositor/compositor_dependencies.cc (right):

https://codereview.chromium.org/2297933002/diff/1/blimp/client/public/composi...
blimp/client/public/compositor/compositor_dependencies.cc:12:
CompositorDependencies::ContextProviders::ContextProviders() = default;
On 2016/08/31 00:28:58, David Trainor wrote:
> Did this not work in the header?

It would but then I couldn't forward declare the ContextProvider, and I thought
the preferred style was to not define in the header?

https://codereview.chromium.org/2297933002/diff/1/chrome/browser/android/blim...
File chrome/browser/android/blimp/chrome_compositor_dependencies.cc (right):

https://codereview.chromium.org/2297933002/diff/1/chrome/browser/android/blim...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:67: // MAKE THE
SETTINGS!!!!!
On 2016/08/31 00:28:58, David Trainor wrote:
> yes plz!  In another patch maybe pull out the CompositorDependenciesImpl
> LayerTreeSettings creator to a static helper so this can use it in the short
> term?

Done. I just moved it to internal for now, when Chrome can make the settings, we
can make it embedder provided.

https://codereview.chromium.org/2297933002/diff/1/chrome/browser/android/blim...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:96:
shared_main_thread_worker_context_ = nullptr;
On 2016/08/31 00:28:58, David Trainor wrote:
> What happens to any outstanding requests if this dies while we're talking to
the
> context provider factory?

Good call. I think the compositor expects the worker context to be valid, so I
just added a check to make sure its still valid when we get the compositor
context back, and otherwise return null.

https://codereview.chromium.org/2297933002/diff/1/chrome/browser/android/blim...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:131: }
On 2016/08/31 00:28:58, David Trainor wrote:
> What if this returns false?

If the bind fails, then we just report the failure to the caller. And then they
can make another request if they want.

https://codereview.chromium.org/2297933002/diff/1/chrome/browser/android/blim...
File chrome/browser/android/blimp/chrome_compositor_dependencies.h (right):

https://codereview.chromium.org/2297933002/diff/1/chrome/browser/android/blim...
chrome/browser/android/blimp/chrome_compositor_dependencies.h:43: bool
worker_context_request_pending_;
On 2016/08/31 00:28:58, David Trainor wrote:
> Add some comments around why we're doing it this way and how it works.

Done.

[Khushal, 2016-08-31 21:46:06]

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

[David Trainor- moved to gerrit, 2016-08-31 21:49:58]

lgtm % nits

https://codereview.chromium.org/2297933002/diff/40001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2297933002/diff/40001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor.cc:288:
settings->abort_commit_before_output_surface_creation = false;
Add comment why this is here.

https://codereview.chromium.org/2297933002/diff/40001/blimp/client/public/bli...
File blimp/client/public/blimp_client_context.h (right):

https://codereview.chromium.org/2297933002/diff/40001/blimp/client/public/bli...
blimp/client/public/blimp_client_context.h:27: class CompositorDependencies;
double the forward declare, double the fun!

[Khushal, 2016-08-31 22:04:57]

https://codereview.chromium.org/2297933002/diff/40001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2297933002/diff/40001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor.cc:288:
settings->abort_commit_before_output_surface_creation = false;
On 2016/08/31 21:49:58, David Trainor wrote:
> Add comment why this is here.

Done.

https://codereview.chromium.org/2297933002/diff/40001/blimp/client/public/bli...
File blimp/client/public/blimp_client_context.h (right):

https://codereview.chromium.org/2297933002/diff/40001/blimp/client/public/bli...
blimp/client/public/blimp_client_context.h:27: class CompositorDependencies;
On 2016/08/31 21:49:58, David Trainor wrote:
> double the forward declare, double the fun!

May be I should add one more? :P
Done.

[Khushal, 2016-09-01 05:17:00]

Description was changed from

==========
blimp: Set up the CompositorDependencies for blimp in Chrome.

Add ChromeCompositorDependencies to implement the CompositorDependencies
for blimp in Chrome. The class passes through to the
ui::ContextProviderFactory to create the contexts.

BUG=611105
==========

to

==========
blimp: Set up the CompositorDependencies for blimp in Chrome.

Add ChromeCompositorDependencies to implement the CompositorDependencies
for blimp in Chrome. The class passes through to the
ui::ContextProviderFactory to create the contexts.

BUG=611105
==========

[Khushal, 2016-09-01 05:17:00]

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

[Khushal, 2016-09-01 05:17:20]

khushalsagar@chromium.org changed reviewers:
+ dpranke@chromium.org
- agrieve@chromium.org

[Khushal, 2016-09-01 05:17:32]

khushalsagar@chromium.org changed reviewers:
- dpranke@chromium.org

[danakj, 2016-09-01 19:26:06]

https://codereview.chromium.org/2297933002/diff/80001/chrome/browser/android/...
File chrome/browser/android/blimp/chrome_compositor_dependencies.cc (right):

https://codereview.chromium.org/2297933002/diff/80001/chrome/browser/android/...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:18: if
(!context_provider)
Why would it not just be a failure |result| in this case?

https://codereview.chromium.org/2297933002/diff/80001/chrome/browser/android/...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:22: return
context_provider->ContextGL()->GetGraphicsResetStatusKHR() ==
Why do you check this when the context is created? It's racey at best.

https://codereview.chromium.org/2297933002/diff/80001/chrome/browser/android/...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:133:
context_provider_factory_->CreateOffscreenContextProvider(
This code here is significantly more complicated than it needs to be because
your async API here is to make the context provider instead of to get the
GpuChannelHost. If it was the latter, you'd make 1 async call and just make 2
context providers off it (see RenderThreadImpl or GpuProcessTransportFactory for
examples).

[Khushal, 2016-09-01 19:39:57]

https://codereview.chromium.org/2297933002/diff/80001/chrome/browser/android/...
File chrome/browser/android/blimp/chrome_compositor_dependencies.cc (right):

https://codereview.chromium.org/2297933002/diff/80001/chrome/browser/android/...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:18: if
(!context_provider)
On 2016/09/01 19:26:06, danakj wrote:
> Why would it not just be a failure |result| in this case?

Oh, I'm calling it from GetContextProviders also, where it can be null. May be I
should just do a null check there itself.

https://codereview.chromium.org/2297933002/diff/80001/chrome/browser/android/...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:22: return
context_provider->ContextGL()->GetGraphicsResetStatusKHR() ==
On 2016/09/01 19:26:06, danakj wrote:
> Why do you check this when the context is created? It's racey at best.

So I only do this after the worker context is bound to the current thread. Isn't
this the same that the RenderThreadImpl does?

https://codereview.chromium.org/2297933002/diff/80001/chrome/browser/android/...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:133:
context_provider_factory_->CreateOffscreenContextProvider(
On 2016/09/01 19:26:06, danakj wrote:
> This code here is significantly more complicated than it needs to be because
> your async API here is to make the context provider instead of to get the
> GpuChannelHost. If it was the latter, you'd make 1 async call and just make 2
> context providers off it (see RenderThreadImpl or GpuProcessTransportFactory
for
> examples).

I had to do it this way because we needed to create a
ContextProviderCommandBuffer which is a content private detail...

[danakj, 2016-09-01 19:55:20]

https://codereview.chromium.org/2297933002/diff/80001/chrome/browser/android/...
File chrome/browser/android/blimp/chrome_compositor_dependencies.cc (right):

https://codereview.chromium.org/2297933002/diff/80001/chrome/browser/android/...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:22: return
context_provider->ContextGL()->GetGraphicsResetStatusKHR() ==
On 2016/09/01 19:39:57, Khushal wrote:
> On 2016/09/01 19:26:06, danakj wrote:
> > Why do you check this when the context is created? It's racey at best.
> 
> So I only do this after the worker context is bound to the current thread.
Isn't
> this the same that the RenderThreadImpl does?

I think it does it when checking if it should reuse it or not, not when it
creates it to see if it's valid. Maybe both of these should only be done when
checking if we should recreate or not?

https://codereview.chromium.org/2297933002/diff/80001/chrome/browser/android/...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:133:
context_provider_factory_->CreateOffscreenContextProvider(
On 2016/09/01 19:39:57, Khushal wrote:
> On 2016/09/01 19:26:06, danakj wrote:
> > This code here is significantly more complicated than it needs to be because
> > your async API here is to make the context provider instead of to get the
> > GpuChannelHost. If it was the latter, you'd make 1 async call and just make
2
> > context providers off it (see RenderThreadImpl or GpuProcessTransportFactory
> for
> > examples).
> 
> I had to do it this way because we needed to create a
> ContextProviderCommandBuffer which is a content private detail...

What if you just made a sync method to make context providers given a
GpuChannelHost returned from an async method?

[Khushal, 2016-09-01 20:10:24]

https://codereview.chromium.org/2297933002/diff/80001/chrome/browser/android/...
File chrome/browser/android/blimp/chrome_compositor_dependencies.cc (right):

https://codereview.chromium.org/2297933002/diff/80001/chrome/browser/android/...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:22: return
context_provider->ContextGL()->GetGraphicsResetStatusKHR() ==
On 2016/09/01 19:55:20, danakj wrote:
> On 2016/09/01 19:39:57, Khushal wrote:
> > On 2016/09/01 19:26:06, danakj wrote:
> > > Why do you check this when the context is created? It's racey at best.
> > 
> > So I only do this after the worker context is bound to the current thread.
> Isn't
> > this the same that the RenderThreadImpl does?
> 
> I think it does it when checking if it should reuse it or not, not when it
> creates it to see if it's valid. Maybe both of these should only be done when
> checking if we should recreate or not?

This one also checks it only during the re-use point. The second time we check
it when the compositor context is created, is that unnecessary? Also, would
become much cleaner if we remove the 2 async calls.

https://codereview.chromium.org/2297933002/diff/80001/chrome/browser/android/...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:133:
context_provider_factory_->CreateOffscreenContextProvider(
On 2016/09/01 19:55:20, danakj wrote:
> On 2016/09/01 19:39:57, Khushal wrote:
> > On 2016/09/01 19:26:06, danakj wrote:
> > > This code here is significantly more complicated than it needs to be
because
> > > your async API here is to make the context provider instead of to get the
> > > GpuChannelHost. If it was the latter, you'd make 1 async call and just
make
> 2
> > > context providers off it (see RenderThreadImpl or
GpuProcessTransportFactory
> > for
> > > examples).
> > 
> > I had to do it this way because we needed to create a
> > ContextProviderCommandBuffer which is a content private detail...
> 
> What if you just made a sync method to make context providers given a
> GpuChannelHost returned from an async method?

I could just hide it all in the ContextProviderFactory class, have one async
call to create the ContextProviders and internally it could use the
GpuChannelHost to create both. I think the reason I didn't do it initially was
because of the notion of shared context, sievers@ suggested that the worker
context for the browser compositors shouldn't be shared with the
BlimpCompositors. Is that something we should worry about? And even if so, may
be I can just let that class know that the contexts are for blimp so it would
cache that worker context separately?

[danakj, 2016-09-01 20:14:01]

https://codereview.chromium.org/2297933002/diff/80001/chrome/browser/android/...
File chrome/browser/android/blimp/chrome_compositor_dependencies.cc (right):

https://codereview.chromium.org/2297933002/diff/80001/chrome/browser/android/...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:133:
context_provider_factory_->CreateOffscreenContextProvider(
On 2016/09/01 20:10:23, Khushal wrote:
> On 2016/09/01 19:55:20, danakj wrote:
> > On 2016/09/01 19:39:57, Khushal wrote:
> > > On 2016/09/01 19:26:06, danakj wrote:
> > > > This code here is significantly more complicated than it needs to be
> because
> > > > your async API here is to make the context provider instead of to get
the
> > > > GpuChannelHost. If it was the latter, you'd make 1 async call and just
> make
> > 2
> > > > context providers off it (see RenderThreadImpl or
> GpuProcessTransportFactory
> > > for
> > > > examples).
> > > 
> > > I had to do it this way because we needed to create a
> > > ContextProviderCommandBuffer which is a content private detail...
> > 
> > What if you just made a sync method to make context providers given a
> > GpuChannelHost returned from an async method?
> 
> I could just hide it all in the ContextProviderFactory class, have one async
> call to create the ContextProviders and internally it could use the
> GpuChannelHost to create both. I think the reason I didn't do it initially was
> because of the notion of shared context, sievers@ suggested that the worker
> context for the browser compositors shouldn't be shared with the
> BlimpCompositors. Is that something we should worry about? And even if so, may
> be I can just let that class know that the contexts are for blimp so it would
> cache that worker context separately?

They have LTHI on diff threads right, so no they shouldn't share. It seems like
you could make that work by saying which one you're making. Though I'd prefer
functions that return 1 thing, and just have separate functions I think. Not
entirely sure why I feel that way.. so whatever, either works, but we should
avoid 2 async calls in a row to set these up.

[Khushal, 2016-09-01 20:47:37]

https://codereview.chromium.org/2297933002/diff/80001/chrome/browser/android/...
File chrome/browser/android/blimp/chrome_compositor_dependencies.cc (right):

https://codereview.chromium.org/2297933002/diff/80001/chrome/browser/android/...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:133:
context_provider_factory_->CreateOffscreenContextProvider(
On 2016/09/01 20:14:01, danakj wrote:
> On 2016/09/01 20:10:23, Khushal wrote:
> > On 2016/09/01 19:55:20, danakj wrote:
> > > On 2016/09/01 19:39:57, Khushal wrote:
> > > > On 2016/09/01 19:26:06, danakj wrote:
> > > > > This code here is significantly more complicated than it needs to be
> > because
> > > > > your async API here is to make the context provider instead of to get
> the
> > > > > GpuChannelHost. If it was the latter, you'd make 1 async call and just
> > make
> > > 2
> > > > > context providers off it (see RenderThreadImpl or
> > GpuProcessTransportFactory
> > > > for
> > > > > examples).
> > > > 
> > > > I had to do it this way because we needed to create a
> > > > ContextProviderCommandBuffer which is a content private detail...
> > > 
> > > What if you just made a sync method to make context providers given a
> > > GpuChannelHost returned from an async method?
> > 
> > I could just hide it all in the ContextProviderFactory class, have one async
> > call to create the ContextProviders and internally it could use the
> > GpuChannelHost to create both. I think the reason I didn't do it initially
was
> > because of the notion of shared context, sievers@ suggested that the worker
> > context for the browser compositors shouldn't be shared with the
> > BlimpCompositors. Is that something we should worry about? And even if so,
may
> > be I can just let that class know that the contexts are for blimp so it
would
> > cache that worker context separately?
> 
> They have LTHI on diff threads right, so no they shouldn't share. It seems
like
> you could make that work by saying which one you're making. Though I'd prefer
> functions that return 1 thing, and just have separate functions I think. Not
> entirely sure why I feel that way.. so whatever, either works, but we should
> avoid 2 async calls in a row to set these up.

So there are 3 ways I could think of doing this.

1) Ask the ContextProviderFactory for a gpu::GpuChannelHost, then provide it
back to the factory in a sync call to get the ContextProvider. This way there is
1 async call for the GpuChannelHost followed by 2 sync calls for the
ContextProviders. I'll have to add a DEPS for gpu too here then.

2) Ask the ContextProviderFactory for both the contexts in an async call and
have it cache the context internally for blimp.

3) Use 2 async calls the way we do right now.

We were hoping that the ContextProviderFactoryImpl class could hopefully be used
for aura too, there is nothing platform specific there and we can avoid
duplicacy in code. So I'll let you decide which one would looks better. :)

[danakj, 2016-09-01 21:00:00]

On Thu, Sep 1, 2016 at 1:47 PM, <khushalsagar@chromium.org> wrote:

>
> https://codereview.chromium.org/2297933002/diff/80001/
> chrome/browser/android/blimp/chrome_compositor_dependencies.cc
> File chrome/browser/android/blimp/chrome_compositor_dependencies.cc
> (right):
>
> https://codereview.chromium.org/2297933002/diff/80001/
> chrome/browser/android/blimp/chrome_compositor_dependencies.cc#newcode133
> chrome/browser/android/blimp/chrome_compositor_dependencies.cc:133:
> context_provider_factory_->CreateOffscreenContextProvider(
> On 2016/09/01 20:14:01, danakj wrote:
> > On 2016/09/01 20:10:23, Khushal wrote:
> > > On 2016/09/01 19:55:20, danakj wrote:
> > > > On 2016/09/01 19:39:57, Khushal wrote:
> > > > > On 2016/09/01 19:26:06, danakj wrote:
> > > > > > This code here is significantly more complicated than it needs
> to be
> > > because
> > > > > > your async API here is to make the context provider instead of
> to get
> > the
> > > > > > GpuChannelHost. If it was the latter, you'd make 1 async call
> and just
> > > make
> > > > 2
> > > > > > context providers off it (see RenderThreadImpl or
> > > GpuProcessTransportFactory
> > > > > for
> > > > > > examples).
> > > > >
> > > > > I had to do it this way because we needed to create a
> > > > > ContextProviderCommandBuffer which is a content private
> detail...
> > > >
> > > > What if you just made a sync method to make context providers
> given a
> > > > GpuChannelHost returned from an async method?
> > >
> > > I could just hide it all in the ContextProviderFactory class, have
> one async
> > > call to create the ContextProviders and internally it could use the
> > > GpuChannelHost to create both. I think the reason I didn't do it
> initially was
> > > because of the notion of shared context, sievers@ suggested that the
> worker
> > > context for the browser compositors shouldn't be shared with the
> > > BlimpCompositors. Is that something we should worry about? And even
> if so, may
> > > be I can just let that class know that the contexts are for blimp so
> it would
> > > cache that worker context separately?
> >
> > They have LTHI on diff threads right, so no they shouldn't share. It
> seems like
> > you could make that work by saying which one you're making. Though I'd
> prefer
> > functions that return 1 thing, and just have separate functions I
> think. Not
> > entirely sure why I feel that way.. so whatever, either works, but we
> should
> > avoid 2 async calls in a row to set these up.
>
> So there are 3 ways I could think of doing this.
>
> 1) Ask the ContextProviderFactory for a gpu::GpuChannelHost, then
> provide it back to the factory in a sync call to get the
> ContextProvider. This way there is 1 async call for the GpuChannelHost
> followed by 2 sync calls for the ContextProviders. I'll have to add a
> DEPS for gpu too here then.
>
> 2) Ask the ContextProviderFactory for both the contexts in an async call
> and have it cache the context internally for blimp.
>
> 3) Use 2 async calls the way we do right now.
>
> We were hoping that the ContextProviderFactoryImpl class could hopefully
> be used for aura too, there is nothing platform specific there and we
> can avoid duplicacy in code. So I'll let you decide which one would
> looks better. :)
>

I choose 1) then :)


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

-- 
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-09-01 23:10:29]

Changed it to option 1. Could you take another look Dana?

[Khushal, 2016-09-01 23:35:06]

khushalsagar@chromium.org changed reviewers:
+ piman@chromium.org
- vmpstr@chromium.org

[Khushal, 2016-09-01 23:35:07]

+piman for content change, I'm sorry about the back and forth on this. Hopefully
this should be final. :)

[danakj, 2016-09-01 23:35:17]

danakj@chromium.org changed reviewers:
+ vmpstr@chromium.org
- piman@chromium.org

[danakj, 2016-09-01 23:35:19]

I guess this gets a bunch simpler when we move ContextProviderCommandBuffer from
content/ to gpu/.

I just looked at the chrome/ parts but they LGTM overall, with a few comments.

https://codereview.chromium.org/2297933002/diff/120001/chrome/browser/android...
File chrome/browser/android/blimp/chrome_compositor_dependencies.cc (right):

https://codereview.chromium.org/2297933002/diff/120001/chrome/browser/android...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:73:
pending_requests_.push_back(callback);
It feels like you could just bind the callback into the OnGpuChannelEstablished
and remove a few LOC dealing with pending_requests_ and trying to answer more
than one at a time.

https://codereview.chromium.org/2297933002/diff/120001/chrome/browser/android...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:80: const
scoped_refptr<gpu::GpuChannelHost>& gpu_channel_host,
receive refptrs by value

https://codereview.chromium.org/2297933002/diff/120001/chrome/browser/android...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:88: case
ui::ContextProviderFactory::GpuChannelHostResult::
if you "using ui::ContextProviderFactory::GpuChannelHostResult" in this function
then these cases can be shorter.

https://codereview.chromium.org/2297933002/diff/120001/chrome/browser/android...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:127:
IsAsyncWorkerContextEnabled()
I'm not sure what the answer is, maybe talk with reveman about it, but it
stresses me out a bit that we are checking command line flags here and in
RenderThreadImpl and they are eventually going to not agree with each other on
decisions being made.

https://codereview.chromium.org/2297933002/diff/120001/chrome/browser/android...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:138:
shared_context, gpu_channel_host.get());
Why is this passing raw pointer to gpu_channel_host, when we're going to be
reffing it over there?

https://codereview.chromium.org/2297933002/diff/120001/chrome/browser/android...
File chrome/browser/android/blimp/chrome_compositor_dependencies.h (right):

https://codereview.chromium.org/2297933002/diff/120001/chrome/browser/android...
chrome/browser/android/blimp/chrome_compositor_dependencies.h:43:
std::list<ContextProviderCallback> pending_requests_;
pls no std::list, it is sloooow cuz pointers are terrible. vector or stack or
queue or deque or something (prob std::queue?)

https://codereview.chromium.org/2297933002/diff/120001/content/browser/render...
File content/browser/renderer_host/context_provider_factory_impl_android.cc
(right):

https://codereview.chromium.org/2297933002/diff/120001/content/browser/render...
content/browser/renderer_host/context_provider_factory_impl_android.cc:162:
gpu_channel_host, gpu::GPU_STREAM_DEFAULT, gpu::GpuStreamPriority::NORMAL,
this gpu_channel_host is an implicit conversion to refptr. make all the
functions here receive this by value, and std::move() it around instead.

https://codereview.chromium.org/2297933002/diff/120001/content/browser/render...
content/browser/renderer_host/context_provider_factory_impl_android.cc:186:
std::list<GpuChannelHostCallback> gpu_channel_requests =
Normally I'd see this written as

std::list<T> requests;
requests.swap(gpu_channel_requests_);

(ps why are we using std::list :( oh noes)

[danakj, 2016-09-01 23:39:06]

On 2016/09/01 23:35:19, danakj wrote:
> I guess this gets a bunch simpler when we move ContextProviderCommandBuffer
from
> content/ to gpu/.

Which is https://bugs.chromium.org/p/chromium/issues/detail?id=586380 fwiw.

[Khushal, 2016-09-02 00:24:33]

Thanks Dana, done!

https://codereview.chromium.org/2297933002/diff/120001/chrome/browser/android...
File chrome/browser/android/blimp/chrome_compositor_dependencies.cc (right):

https://codereview.chromium.org/2297933002/diff/120001/chrome/browser/android...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:73:
pending_requests_.push_back(callback);
On 2016/09/01 23:35:18, danakj wrote:
> It feels like you could just bind the callback into the
OnGpuChannelEstablished
> and remove a few LOC dealing with pending_requests_ and trying to answer more
> than one at a time.

Oh yes, didn't realize without the 2 async calls, this makes this so much
simpler too. Thanks! Done.

https://codereview.chromium.org/2297933002/diff/120001/chrome/browser/android...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:80: const
scoped_refptr<gpu::GpuChannelHost>& gpu_channel_host,
On 2016/09/01 23:35:18, danakj wrote:
> receive refptrs by value

Is there a reason why that is better? I thought it does an unnecessary ref and
unref if the caller doesn't need a copy?

https://codereview.chromium.org/2297933002/diff/120001/chrome/browser/android...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:88: case
ui::ContextProviderFactory::GpuChannelHostResult::
On 2016/09/01 23:35:18, danakj wrote:
> if you "using ui::ContextProviderFactory::GpuChannelHostResult" in this
function
> then these cases can be shorter.

Done.

https://codereview.chromium.org/2297933002/diff/120001/chrome/browser/android...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:127:
IsAsyncWorkerContextEnabled()
On 2016/09/01 23:35:18, danakj wrote:
> I'm not sure what the answer is, maybe talk with reveman about it, but it
> stresses me out a bit that we are checking command line flags here and in
> RenderThreadImpl and they are eventually going to not agree with each other on
> decisions being made.

I think all of this is in compositor_util in content/. May be the gpu specific
switches could be in gpu/ and we could use the same code here?

https://codereview.chromium.org/2297933002/diff/120001/chrome/browser/android...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:138:
shared_context, gpu_channel_host.get());
On 2016/09/01 23:35:18, danakj wrote:
> Why is this passing raw pointer to gpu_channel_host, when we're going to be
> reffing it over there?

Done.

https://codereview.chromium.org/2297933002/diff/120001/chrome/browser/android...
File chrome/browser/android/blimp/chrome_compositor_dependencies.h (right):

https://codereview.chromium.org/2297933002/diff/120001/chrome/browser/android...
chrome/browser/android/blimp/chrome_compositor_dependencies.h:43:
std::list<ContextProviderCallback> pending_requests_;
On 2016/09/01 23:35:19, danakj wrote:
> pls no std::list, it is sloooow cuz pointers are terrible. vector or stack or
> queue or deque or something (prob std::queue?)

Good point. Cool thing, no need for this anymore.

https://codereview.chromium.org/2297933002/diff/120001/content/browser/render...
File content/browser/renderer_host/context_provider_factory_impl_android.cc
(right):

https://codereview.chromium.org/2297933002/diff/120001/content/browser/render...
content/browser/renderer_host/context_provider_factory_impl_android.cc:162:
gpu_channel_host, gpu::GPU_STREAM_DEFAULT, gpu::GpuStreamPriority::NORMAL,
On 2016/09/01 23:35:19, danakj wrote:
> this gpu_channel_host is an implicit conversion to refptr. make all the
> functions here receive this by value, and std::move() it around instead.

Didn't see that it was a refptr. Done.

https://codereview.chromium.org/2297933002/diff/120001/content/browser/render...
content/browser/renderer_host/context_provider_factory_impl_android.cc:186:
std::list<GpuChannelHostCallback> gpu_channel_requests =
On 2016/09/01 23:35:19, danakj wrote:
> Normally I'd see this written as
> 
> std::list<T> requests;
> requests.swap(gpu_channel_requests_);
> 
> (ps why are we using std::list :( oh noes)

Much better. Done.

[danakj, 2016-09-02 00:30:56]

https://codereview.chromium.org/2297933002/diff/120001/chrome/browser/android...
File chrome/browser/android/blimp/chrome_compositor_dependencies.cc (right):

https://codereview.chromium.org/2297933002/diff/120001/chrome/browser/android...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:80: const
scoped_refptr<gpu::GpuChannelHost>& gpu_channel_host,
On 2016/09/02 00:24:32, Khushal wrote:
> On 2016/09/01 23:35:18, danakj wrote:
> > receive refptrs by value
> 
> Is there a reason why that is better? I thought it does an unnecessary ref and
> unref if the caller doesn't need a copy?

The caller should std::move if it doesnt need a copy, so no this doesn't add an
additional ref. It's better because if you already have an rvalue the compiler
doesnt have to generate a temporary lvalue to make a const& to, which is an
extra ref.

https://codereview.chromium.org/2297933002/diff/120001/chrome/browser/android...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:127:
IsAsyncWorkerContextEnabled()
On 2016/09/02 00:24:32, Khushal wrote:
> On 2016/09/01 23:35:18, danakj wrote:
> > I'm not sure what the answer is, maybe talk with reveman about it, but it
> > stresses me out a bit that we are checking command line flags here and in
> > RenderThreadImpl and they are eventually going to not agree with each other
on
> > decisions being made.
> 
> I think all of this is in compositor_util in content/.

That's content/browser though.

> May be the gpu specific switches could be in gpu/ and we could use the same
code here?

We need something shared for the renderer or something. Consolidating logic into
compositor_util would help tho, that's not a bad idea, break it down there so
you only get a single flag to the renderer. That's harder tho not impossible to
bitrot.

[danakj, 2016-09-02 00:35:29]

Thanks :)

https://codereview.chromium.org/2297933002/diff/140001/chrome/browser/android...
File chrome/browser/android/blimp/chrome_compositor_dependencies.h (right):

https://codereview.chromium.org/2297933002/diff/140001/chrome/browser/android...
chrome/browser/android/blimp/chrome_compositor_dependencies.h:8: #include
<queue>
not used in the end?

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

https://codereview.chromium.org/2297933002/diff/140001/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:647: const
scoped_refptr<gpu::GpuChannelHost>& gpu_channel_host,
This should be by value too.

https://codereview.chromium.org/2297933002/diff/140001/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:673: gpu_channel_host);
And std::move() here

https://codereview.chromium.org/2297933002/diff/140001/content/browser/render...
File content/browser/renderer_host/context_provider_factory_impl_android.cc
(right):

https://codereview.chromium.org/2297933002/diff/140001/content/browser/render...
content/browser/renderer_host/context_provider_factory_impl_android.cc:187:
std::queue<GpuChannelHostCallback> gpu_channel_requests;
nitty-nit: i like when variables don't have the same name as member variables
with only the trailing underscore to differentiate

[piman, 2016-09-02 01:07:14]

piman@chromium.org changed reviewers:
+ piman@chromium.org

[piman, 2016-09-02 01:07:15]

LGTM after Dana's comments are addressed.

[Khushal, 2016-09-02 01:13:35]

https://codereview.chromium.org/2297933002/diff/120001/chrome/browser/android...
File chrome/browser/android/blimp/chrome_compositor_dependencies.cc (right):

https://codereview.chromium.org/2297933002/diff/120001/chrome/browser/android...
chrome/browser/android/blimp/chrome_compositor_dependencies.cc:80: const
scoped_refptr<gpu::GpuChannelHost>& gpu_channel_host,
On 2016/09/02 00:30:56, danakj wrote:
> On 2016/09/02 00:24:32, Khushal wrote:
> > On 2016/09/01 23:35:18, danakj wrote:
> > > receive refptrs by value
> > 
> > Is there a reason why that is better? I thought it does an unnecessary ref
and
> > unref if the caller doesn't need a copy?
> 
> The caller should std::move if it doesnt need a copy, so no this doesn't add
an
> additional ref. It's better because if you already have an rvalue the compiler
> doesnt have to generate a temporary lvalue to make a const& to, which is an
> extra ref.

Sorry if this is getting repetitive, I just want to make sure I understand this
correctly. Thanks for taking a look at this.

So I know we do std::move if the caller doesn't want to keep a copy. What I
actually meant to ask was about the case when the caller wants to keep it, but
the function I'm passing the refptr too doesn't need a copy. In this case, the
ContextProviderFactoryImpl needs to keep the GpuChannelHost for all the
callbacks it is responding to, but this function only uses it to pass it back to
the ContextProviderFactoryImpl. In this case does using const& avoid creating an
additional copy of the refptr? And how would passing by value be better?

[Khushal, 2016-09-02 16:26:47]

https://codereview.chromium.org/2297933002/diff/140001/chrome/browser/android...
File chrome/browser/android/blimp/chrome_compositor_dependencies.h (right):

https://codereview.chromium.org/2297933002/diff/140001/chrome/browser/android...
chrome/browser/android/blimp/chrome_compositor_dependencies.h:8: #include
<queue>
On 2016/09/02 00:35:29, danakj wrote:
> not used in the end?

Done.

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

https://codereview.chromium.org/2297933002/diff/140001/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:647: const
scoped_refptr<gpu::GpuChannelHost>& gpu_channel_host,
On 2016/09/02 00:35:29, danakj wrote:
> This should be by value too.

Done.

https://codereview.chromium.org/2297933002/diff/140001/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:673: gpu_channel_host);
On 2016/09/02 00:35:29, danakj wrote:
> And std::move() here

Done.

https://codereview.chromium.org/2297933002/diff/140001/content/browser/render...
File content/browser/renderer_host/context_provider_factory_impl_android.cc
(right):

https://codereview.chromium.org/2297933002/diff/140001/content/browser/render...
content/browser/renderer_host/context_provider_factory_impl_android.cc:187:
std::queue<GpuChannelHostCallback> gpu_channel_requests;
On 2016/09/02 00:35:29, danakj wrote:
> nitty-nit: i like when variables don't have the same name as member variables
> with only the trailing underscore to differentiate

Done.

[Khushal, 2016-09-02 17:16:02]

Description was changed from

==========
blimp: Set up the CompositorDependencies for blimp in Chrome.

Add ChromeCompositorDependencies to implement the CompositorDependencies
for blimp in Chrome. The class passes through to the
ui::ContextProviderFactory to create the contexts.

BUG=611105
==========

to

==========
blimp: Set up the CompositorDependencies for blimp in Chrome.

Add ChromeCompositorDependencies to implement the CompositorDependencies
for blimp in Chrome. The class passes through to the
ui::ContextProviderFactory to create the contexts.

BUG=611105
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Khushal, 2016-09-02 17:37:27]

Much of what the rebase pulled in was test changes from
https://codereview.chromium.org/2297273004/ which was reverted.

Now the callers get notified when the GpuChannelEstablishFactory gives a null
host. The original change was having us retry because we assume that it is
because of a system failure to initialize the Gpu process but it probably needs
more info to know if its because the Gpu is disabled (see crbug.com/643282)? And
in that case should the fallback be to software, that's what the renderer does?
So I've added a TODO there till the discussion on the bug is resolved.

[Khushal, 2016-09-02 17:37:37]

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

[commit-bot: I haz the power, 2016-09-02 17:38:07]

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

[danakj, 2016-09-02 20:13:28]

On Fri, Sep 2, 2016 at 10:37 AM, <khushalsagar@chromium.org> wrote:

> Much of what the rebase pulled in was test changes from
> https://codereview.chromium.org/2297273004/ which was reverted.
>
> Now the callers get notified when the GpuChannelEstablishFactory gives a
> null
> host. The original change was having us retry because we assume that it is
> because of a system failure to initialize the Gpu process but it probably
> needs
> more info to know if its because the Gpu is disabled (see crbug.com/643282)?
> And
> in that case should the fallback be to software, that's what the renderer
> does?
> So I've added a TODO there till the discussion on the bug is resolved.
>

You may wanna mimic what GpuProcessTransportFactory does. It doesn't try to
use GPU if the GpuDataManager says we can't use GpuCompositing. And it
falls back to software if it fails N times in a row to establish a channel.
Take care to think about ensuring that both the Browser and the Renderer
compositors see the same thing (they both go into software compositing or
neither does, and one sees a lost context if the other has transitioned.


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

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

[commit-bot: I haz the power, 2016-09-02 20:14:23]

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

[commit-bot: I haz the power, 2016-09-02 20:14:25]

Dry run: This issue passed the CQ dry run.

[Khushal, 2016-09-02 20:19:42]

Description was changed from

==========
blimp: Set up the CompositorDependencies for blimp in Chrome.

Add ChromeCompositorDependencies to implement the CompositorDependencies
for blimp in Chrome. The class passes through to the
ui::ContextProviderFactory to create the contexts.

BUG=611105
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
blimp: Set up the CompositorDependencies for blimp in Chrome.

Add ChromeCompositorDependencies to implement the CompositorDependencies
for blimp in Chrome. The class passes through to the
ui::ContextProviderFactory to create the contexts.

TBR=jochen

BUG=611105
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Khushal, 2016-09-02 20:19:53]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-09-02 20:19:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, danakj@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2297933002/#ps200001
(title: "retry gpu process failures after crbug.com/643282")

[commit-bot: I haz the power, 2016-09-02 20:20:24]

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-09-02 20:26:15]

Description was changed from

==========
blimp: Set up the CompositorDependencies for blimp in Chrome.

Add ChromeCompositorDependencies to implement the CompositorDependencies
for blimp in Chrome. The class passes through to the
ui::ContextProviderFactory to create the contexts.

TBR=jochen

BUG=611105
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
blimp: Set up the CompositorDependencies for blimp in Chrome.

Add ChromeCompositorDependencies to implement the CompositorDependencies
for blimp in Chrome. The class passes through to the
ui::ContextProviderFactory to create the contexts.

TBR=jochen

BUG=611105
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-09-02 20:26:17]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-09-02 20:27:57]

Description was changed from

==========
blimp: Set up the CompositorDependencies for blimp in Chrome.

Add ChromeCompositorDependencies to implement the CompositorDependencies
for blimp in Chrome. The class passes through to the
ui::ContextProviderFactory to create the contexts.

TBR=jochen

BUG=611105
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
blimp: Set up the CompositorDependencies for blimp in Chrome.

Add ChromeCompositorDependencies to implement the CompositorDependencies
for blimp in Chrome. The class passes through to the
ui::ContextProviderFactory to create the contexts.

TBR=jochen

BUG=611105
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/8ecea2e7082ed06d138d5b74f8a6cff2873b6af9
Cr-Commit-Position: refs/heads/master@{#416333}
==========

[commit-bot: I haz the power, 2016-09-02 20:27:59]

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

[Khushal, 2016-09-02 23:01:11]

On 2016/09/02 20:13:28, danakj wrote:
> On Fri, Sep 2, 2016 at 10:37 AM, <mailto:khushalsagar@chromium.org> wrote:
> 
> > Much of what the rebase pulled in was test changes from
> > https://codereview.chromium.org/2297273004/ which was reverted.
> >
> > Now the callers get notified when the GpuChannelEstablishFactory gives a
> > null
> > host. The original change was having us retry because we assume that it is
> > because of a system failure to initialize the Gpu process but it probably
> > needs
> > more info to know if its because the Gpu is disabled (see crbug.com/643282)?
> > And
> > in that case should the fallback be to software, that's what the renderer
> > does?
> > So I've added a TODO there till the discussion on the bug is resolved.
> >
> 
> You may wanna mimic what GpuProcessTransportFactory does. It doesn't try to
> use GPU if the GpuDataManager says we can't use GpuCompositing. And it
> falls back to software if it fails N times in a row to establish a channel.
> Take care to think about ensuring that both the Browser and the Renderer
> compositors see the same thing (they both go into software compositing or
> neither does, and one sees a lost context if the other has transitioned.
> 

I'll let sievers@ take it from here, he would know if there is a reason we don't
do it for android. From what I saw at least in the renderer the software path
happens on android too so we can definitely have the case where the renderer
goes to software but the browser doesn't.

> 
> >
> > https://codereview.chromium.org/2297933002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

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

On Fri, Sep 2, 2016 at 4:01 PM, <khushalsagar@chromium.org> wrote:

> On 2016/09/02 20:13:28, danakj wrote:
> > On Fri, Sep 2, 2016 at 10:37 AM, <mailto:khushalsagar@chromium.org>
> wrote:
> >
> > > Much of what the rebase pulled in was test changes from
> > > https://codereview.chromium.org/2297273004/ which was reverted.
> > >
> > > Now the callers get notified when the GpuChannelEstablishFactory gives
> a
> > > null
> > > host. The original change was having us retry because we assume that
> it is
> > > because of a system failure to initialize the Gpu process but it
> probably
> > > needs
> > > more info to know if its because the Gpu is disabled (see
> crbug.com/643282)?
> > > And
> > > in that case should the fallback be to software, that's what the
> renderer
> > > does?
> > > So I've added a TODO there till the discussion on the bug is resolved.
> > >
> >
> > You may wanna mimic what GpuProcessTransportFactory does. It doesn't try
> to
> > use GPU if the GpuDataManager says we can't use GpuCompositing. And it
> > falls back to software if it fails N times in a row to establish a
> channel.
> > Take care to think about ensuring that both the Browser and the Renderer
> > compositors see the same thing (they both go into software compositing or
> > neither does, and one sees a lost context if the other has transitioned.
> >
>
> I'll let sievers@ take it from here, he would know if there is a reason
> we don't
> do it for android. From what I saw at least in the renderer the software
> path
> happens on android too so we can definitely have the case where the
> renderer
> goes to software but the browser doesn't.
>

Oh right blimp is android. Android does not do software compositing at all.
It tries forever for GPU and crashes if it can't.

You're right the renderer could fallback without the browser, that's a
weird race between browser/renderer that has come up recently a couple
times and we could do better. But CompositorImpl would crash on fewer tries
https://cs.chromium.org/chromium/src/content/browser/renderer_host/compositor...
(2 tries) whereas RenderWidgetCompositor tries 4, so it kinda works in a
roundabout way.


>
> >
> > >
> > > https://codereview.chromium.org/2297933002/
> > >
> >
> > --
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2297933002/
>

-- 
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-09-02 23:33:50]

On 2016/09/02 23:10:24, danakj wrote:
> On Fri, Sep 2, 2016 at 4:01 PM, <mailto:khushalsagar@chromium.org> wrote:
> 
> > On 2016/09/02 20:13:28, danakj wrote:
> > > On Fri, Sep 2, 2016 at 10:37 AM, <mailto:khushalsagar@chromium.org>
> > wrote:
> > >
> > > > Much of what the rebase pulled in was test changes from
> > > > https://codereview.chromium.org/2297273004/ which was reverted.
> > > >
> > > > Now the callers get notified when the GpuChannelEstablishFactory gives
> > a
> > > > null
> > > > host. The original change was having us retry because we assume that
> > it is
> > > > because of a system failure to initialize the Gpu process but it
> > probably
> > > > needs
> > > > more info to know if its because the Gpu is disabled (see
> > crbug.com/643282)?
> > > > And
> > > > in that case should the fallback be to software, that's what the
> > renderer
> > > > does?
> > > > So I've added a TODO there till the discussion on the bug is resolved.
> > > >
> > >
> > > You may wanna mimic what GpuProcessTransportFactory does. It doesn't try
> > to
> > > use GPU if the GpuDataManager says we can't use GpuCompositing. And it
> > > falls back to software if it fails N times in a row to establish a
> > channel.
> > > Take care to think about ensuring that both the Browser and the Renderer
> > > compositors see the same thing (they both go into software compositing or
> > > neither does, and one sees a lost context if the other has transitioned.
> > >
> >
> > I'll let sievers@ take it from here, he would know if there is a reason
> > we don't
> > do it for android. From what I saw at least in the renderer the software
> > path
> > happens on android too so we can definitely have the case where the
> > renderer
> > goes to software but the browser doesn't.
> >
> 
> Oh right blimp is android. Android does not do software compositing at all.
> It tries forever for GPU and crashes if it can't.
> 
> You're right the renderer could fallback without the browser, that's a
> weird race between browser/renderer that has come up recently a couple
> times and we could do better. But CompositorImpl would crash on fewer tries
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/compositor...
> (2 tries) whereas RenderWidgetCompositor tries 4, so it kinda works in a
> roundabout way.

Ah okay. Its kinda fine right now then. May be nicer to crash the renderer with
a log too to match what the browser does. Also CompositorImpl would crash at
that place if the context initialization failed. But if we don't create a Gpu
process itself, it will just sit there idly. :P

> 
> 
> >
> > >
> > > >
> > > > https://codereview.chromium.org/2297933002/
> > > >
> > >
> > > --
> > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> >
> >
> > https://codereview.chromium.org/2297933002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.