Expose Blimp dependencies to the embedder

Expose Blimp dependencies to the embedder

Move Blimp compositor dependencies out to an interface that can be
implemented by the embedder.  Update blimp/client/app to use this
interface.

BUG=611105
Committed: https://crrev.com/7cb0bd5a02b78ac0b8631a1329baafe89e1ff915
Cr-Commit-Position: refs/heads/master@{#415097}

[David Trainor- moved to gerrit, 2016-08-24 21:57:35]

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

[Khushal, 2016-08-24 23:22:46]

https://codereview.chromium.org/2274323002/diff/1/blimp/client/app/android/bl...
File blimp/client/app/android/blimp_compositor_manager_android.h (left):

https://codereview.chromium.org/2274323002/diff/1/blimp/client/app/android/bl...
blimp/client/app/android/blimp_compositor_manager_android.h:30: class
BlimpCompositorManagerAndroid : public BlimpCompositorManager {
Yes, die file die!! :D

https://codereview.chromium.org/2274323002/diff/1/blimp/client/app/android/bl...
File blimp/client/app/android/blimp_view.cc (right):

https://codereview.chromium.org/2274323002/diff/1/blimp/client/app/android/bl...
blimp/client/app/android/blimp_view.cc:61: base::WrapUnique(new
BlimpCompositorDependenciesImpl(base::WrapUnique(
nit: MakeUnique!

https://codereview.chromium.org/2274323002/diff/1/blimp/client/app/compositor...
File blimp/client/app/compositor/browser_compositor.cc (right):

https://codereview.chromium.org/2274323002/diff/1/blimp/client/app/compositor...
blimp/client/app/compositor/browser_compositor.cc:153: // TODO(dtrainor,
khushalsagar): Handle ContextProvider creation failures.
If we are not handling it right now, lets LOG(FATAL) instead of failing
silently?

https://codereview.chromium.org/2274323002/diff/1/blimp/client/app/compositor...
blimp/client/app/compositor/browser_compositor.cc:159: if
(!output_surface_request_pending_ ||
When could this happen where we get here but no request is pending. Shouldn't
this be a DCHECK at the top?

https://codereview.chromium.org/2274323002/diff/1/blimp/client/app/linux/blim...
File blimp/client/app/linux/blimp_display_manager.cc (right):

https://codereview.chromium.org/2274323002/diff/1/blimp/client/app/linux/blim...
blimp/client/app/linux/blimp_display_manager.cc:34: base::WrapUnique(new
BlimpCompositorDependenciesImpl(
nit: MakeUnique!

https://codereview.chromium.org/2274323002/diff/1/blimp/client/core/composito...
File blimp/client/core/compositor/blimp_compositor_dependencies.h (right):

https://codereview.chromium.org/2274323002/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/blimp_compositor_dependencies.h:24: class
BlimpCompositorDependencies {
Just leave it concrete if you don't need subclasses?

https://codereview.chromium.org/2274323002/diff/1/blimp/client/core/composito...
File blimp/client/core/compositor/blimp_compositor_dependencies_impl.cc (right):

https://codereview.chromium.org/2274323002/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/blimp_compositor_dependencies_impl.cc:22:
DCHECK(embedder_dependencies_.get());
nit: no need to get, other places in this file too.

https://codereview.chromium.org/2274323002/diff/1/blimp/client/core/composito...
File blimp/client/core/compositor/blimp_compositor_dependencies_impl.h (right):

https://codereview.chromium.org/2274323002/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/blimp_compositor_dependencies_impl.h:13: namespace
base {
nit: terminate namespace.

https://codereview.chromium.org/2274323002/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/blimp_compositor_dependencies_impl.h:19: class
BlimpTaskGraphRunner;
Why is this not in the client namespace?

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

https://codereview.chromium.org/2274323002/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.cc:96:
DCHECK(!surface_factory_)
Can we keep the DCHECK here itself. The compositor should detach the old
OutputSurface before asking for a new one, which means we should have destroyed
the surface factory. Better to fail in the cc client callback if they are
breaking the contract.

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

https://codereview.chromium.org/2274323002/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.cc:157: if (!host_)
I was thinking about the case where the host goes away, we recreate a new one,
and then get the callback for the old one. We'll assume this host asked for it
and then it will fail. So we could add back the bool to track a pending output
surface request, reset it on destroying the host, and just bail out if that is
false. If its true, the host should most definitely be there.

https://codereview.chromium.org/2274323002/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.cc:307: settings_.reset();
I think they keep a copy, so you can cache this value forever, but then may be
the deps provider can cache it for all compositors itself.

https://codereview.chromium.org/2274323002/diff/1/blimp/client/feature/compos...
File blimp/client/feature/compositor/blimp_compositor_manager.cc (left):

https://codereview.chromium.org/2274323002/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor_manager.cc:23: }  // namespace
nit: keep the namespace?

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

https://codereview.chromium.org/2274323002/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor_manager.cc:25:
compositor_dependencies_(compositor_dependencies) {
DCHECK compositor_dependencies_?

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

https://codereview.chromium.org/2274323002/diff/1/blimp/client/public/composi...
blimp/client/public/compositor/compositor_dependencies.h:15: }
nit: terminate namespace for this and gpu.

https://codereview.chromium.org/2274323002/diff/1/blimp/client/public/composi...
blimp/client/public/compositor/compositor_dependencies.h:31: virtual
std::unique_ptr<cc::LayerTreeSettings> GetLayerTreeSettings() = 0;
Since Chrome might not be able to do this for us right now, do you want to keep
it internal? Xing can change it to public once he hooks that up in his patch.

https://codereview.chromium.org/2274323002/diff/1/blimp/client/support/compos...
File
blimp/client/support/compositor/android/compositor_dependencies_impl_android.h
(right):

https://codereview.chromium.org/2274323002/diff/1/blimp/client/support/compos...
blimp/client/support/compositor/android/compositor_dependencies_impl_android.h:20:
CompositorDependenciesImplAndroid(const gfx::Size& real_size,
I think we can get rid of the android subclass and just query it using the
gfx::DeviceDisplayInfo, the way the renderer compositor does this,
https://cs.chromium.org/chromium/src/content/renderer/gpu/render_widget_compo...

https://codereview.chromium.org/2274323002/diff/1/blimp/client/support/compos...
File blimp/client/support/compositor/blimp_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/2274323002/diff/1/blimp/client/support/compos...
blimp/client/support/compositor/blimp_gpu_memory_buffer_manager.cc:14: #include
"blimp/client/support/compositor/blimp_gpu_memory_buffer_manager.h"
nit: this should go back to the top.

[Khushal, 2016-08-25 05:02:54]

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/app/compos...
File blimp/client/app/compositor/browser_compositor.cc (right):

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/app/compos...
blimp/client/app/compositor/browser_compositor.cc:141:
base::Bind(&BrowserCompositor::OnContextProviderCreated,
I didn't see this earlier, but we are creating an offscreen context here but we
actually need to create an onscreen one with the widget.

Can we avoid adding the asynchronous context callback here, have this class
create the BlimpContextProvider for itself like right now, since we know it is
always going to be the InProcess one? The OutputSurface setup doesn't need to
change really.

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_compositor_dependencies.h (right):

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor_dependencies.h:31: class
BlimpCompositorDependencies {
May be mention that this class is all on the main/UI thread.

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_task_graph_runner.h (right):

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_task_graph_runner.h:18: // engine's dummy UI
context factory.
Update the comment? And this is only used in BlimpCompositorDependencies, can be
put in the .cc file itself.

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor.cc:168: if (!host_ ||
!output_surface_request_pending_)
Actually, we should need just the output_surface_request_pending_ check to early
out. If this value is true we must have a |host_| which is expecting this
request.

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor.cc:273:
GetEmbedderDeps()->GetLayerTreeSettings();
nit: Feel like it would be better to cache the unique_ptr in the EmbedderDeps
and return a weak reference here, the deps is expected to outlive the
compositors anyway.

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_compositor_manager.cc (right):

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor_manager.cc:52: return
base::WrapUnique(
This will become MakeUnique when you rebase. :P

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_compositor_manager.h (right):

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor_manager.h:45: // virtual void
GenerateLayerTreeSettings(cc::LayerTreeSettings* settings);
This is going to go away right? :P

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_compositor_manager_unittest.cc
(right):

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor_manager_unittest.cc:83:
base::WrapUnique(new MockCompositorDependencies())));
MakeUnique.

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_compositor_unittest.cc (right):

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor_unittest.cc:70:
compositor_dependencies_ = base::WrapUnique(new BlimpCompositorDependencies(
MakeUnique. :(

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/public/com...
File blimp/client/public/compositor/compositor_dependencies.h (right):

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/public/com...
blimp/client/public/compositor/compositor_dependencies.h:25: class
CompositorDependencies {
Comment on the lifetime? The Dependencies are expected to outlive all
BlimpContents?

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/public/com...
blimp/client/public/compositor/compositor_dependencies.h:27: using
ContextProviderCallback =
The ui::ContextProviderFactory passes an enum saying why the failure, should we
mirror a subset of that here. I think except GPU_SURFACE_HANDLE loss, we can
expect all responses back, since we only need offscreen contexts, and that
response is for an onscreen one.

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/public/com...
blimp/client/public/compositor/compositor_dependencies.h:39: // created.  It may
be asynchronous if it needs to build it though.
nit: the asynchronous bit is expected.

[David Trainor- moved to gerrit, 2016-08-26 17:15:55]

https://codereview.chromium.org/2274323002/diff/1/blimp/client/app/android/bl...
File blimp/client/app/android/blimp_compositor_manager_android.h (left):

https://codereview.chromium.org/2274323002/diff/1/blimp/client/app/android/bl...
blimp/client/app/android/blimp_compositor_manager_android.h:30: class
BlimpCompositorManagerAndroid : public BlimpCompositorManager {
On 2016/08/24 23:22:45, Khushal wrote:
> Yes, die file die!! :D

Acknowledged.

https://codereview.chromium.org/2274323002/diff/1/blimp/client/app/android/bl...
File blimp/client/app/android/blimp_view.cc (right):

https://codereview.chromium.org/2274323002/diff/1/blimp/client/app/android/bl...
blimp/client/app/android/blimp_view.cc:61: base::WrapUnique(new
BlimpCompositorDependenciesImpl(base::WrapUnique(
On 2016/08/24 23:22:45, Khushal wrote:
> nit: MakeUnique!

Done.

https://codereview.chromium.org/2274323002/diff/1/blimp/client/app/compositor...
File blimp/client/app/compositor/browser_compositor.cc (right):

https://codereview.chromium.org/2274323002/diff/1/blimp/client/app/compositor...
blimp/client/app/compositor/browser_compositor.cc:153: // TODO(dtrainor,
khushalsagar): Handle ContextProvider creation failures.
On 2016/08/24 23:22:45, Khushal wrote:
> If we are not handling it right now, lets LOG(FATAL) instead of failing
> silently?

Done.

https://codereview.chromium.org/2274323002/diff/1/blimp/client/app/compositor...
blimp/client/app/compositor/browser_compositor.cc:159: if
(!output_surface_request_pending_ ||
On 2016/08/24 23:22:45, Khushal wrote:
> When could this happen where we get here but no request is pending. Shouldn't
> this be a DCHECK at the top?

Done.

https://codereview.chromium.org/2274323002/diff/1/blimp/client/app/linux/blim...
File blimp/client/app/linux/blimp_display_manager.cc (right):

https://codereview.chromium.org/2274323002/diff/1/blimp/client/app/linux/blim...
blimp/client/app/linux/blimp_display_manager.cc:34: base::WrapUnique(new
BlimpCompositorDependenciesImpl(
On 2016/08/24 23:22:45, Khushal wrote:
> nit: MakeUnique!

Done.

https://codereview.chromium.org/2274323002/diff/1/blimp/client/core/composito...
File blimp/client/core/compositor/blimp_compositor_dependencies.h (right):

https://codereview.chromium.org/2274323002/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/blimp_compositor_dependencies.h:24: class
BlimpCompositorDependencies {
On 2016/08/24 23:22:45, Khushal wrote:
> Just leave it concrete if you don't need subclasses?

Done.

https://codereview.chromium.org/2274323002/diff/1/blimp/client/core/composito...
File blimp/client/core/compositor/blimp_compositor_dependencies_impl.cc (right):

https://codereview.chromium.org/2274323002/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/blimp_compositor_dependencies_impl.cc:22:
DCHECK(embedder_dependencies_.get());
On 2016/08/24 23:22:45, Khushal wrote:
> nit: no need to get, other places in this file too.

Done.

https://codereview.chromium.org/2274323002/diff/1/blimp/client/core/composito...
File blimp/client/core/compositor/blimp_compositor_dependencies_impl.h (right):

https://codereview.chromium.org/2274323002/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/blimp_compositor_dependencies_impl.h:13: namespace
base {
On 2016/08/24 23:22:45, Khushal wrote:
> nit: terminate namespace.

Done.

https://codereview.chromium.org/2274323002/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/blimp_compositor_dependencies_impl.h:19: class
BlimpTaskGraphRunner;
On 2016/08/24 23:22:45, Khushal wrote:
> Why is this not in the client namespace?

Oddly it was in blimp/common/compositor.  Not used anywhere else, so moved it to
blimp/client/core/compositor.

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

https://codereview.chromium.org/2274323002/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.cc:96:
DCHECK(!surface_factory_)
On 2016/08/24 23:22:46, Khushal wrote:
> Can we keep the DCHECK here itself. The compositor should detach the old
> OutputSurface before asking for a new one, which means we should have
destroyed
> the surface factory. Better to fail in the cc client callback if they are
> breaking the contract.

Done.

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

https://codereview.chromium.org/2274323002/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.cc:157: if (!host_)
On 2016/08/24 23:22:45, Khushal wrote:
> I was thinking about the case where the host goes away, we recreate a new one,
> and then get the callback for the old one. We'll assume this host asked for it
> and then it will fail. So we could add back the bool to track a pending output
> surface request, reset it on destroying the host, and just bail out if that is
> false. If its true, the host should most definitely be there.

Done.

https://codereview.chromium.org/2274323002/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.cc:307: settings_.reset();
On 2016/08/24 23:22:46, Khushal wrote:
> I think they keep a copy, so you can cache this value forever, but then may be
> the deps provider can cache it for all compositors itself.

Done.

https://codereview.chromium.org/2274323002/diff/1/blimp/client/feature/compos...
File blimp/client/feature/compositor/blimp_compositor_manager.cc (left):

https://codereview.chromium.org/2274323002/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor_manager.cc:23: }  // namespace
On 2016/08/24 23:22:46, Khushal wrote:
> nit: keep the namespace?

Done.

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

https://codereview.chromium.org/2274323002/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor_manager.cc:25:
compositor_dependencies_(compositor_dependencies) {
On 2016/08/24 23:22:46, Khushal wrote:
> DCHECK compositor_dependencies_?

Done.

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

https://codereview.chromium.org/2274323002/diff/1/blimp/client/public/composi...
blimp/client/public/compositor/compositor_dependencies.h:15: }
On 2016/08/24 23:22:46, Khushal wrote:
> nit: terminate namespace for this and gpu.

Done.

https://codereview.chromium.org/2274323002/diff/1/blimp/client/public/composi...
blimp/client/public/compositor/compositor_dependencies.h:31: virtual
std::unique_ptr<cc::LayerTreeSettings> GetLayerTreeSettings() = 0;
On 2016/08/24 23:22:46, Khushal wrote:
> Since Chrome might not be able to do this for us right now, do you want to
keep
> it internal? Xing can change it to public once he hooks that up in his patch.

I'll pull the LayerTreeSettings from //blimp/client/support out to a static
helper.

https://codereview.chromium.org/2274323002/diff/1/blimp/client/support/compos...
File
blimp/client/support/compositor/android/compositor_dependencies_impl_android.h
(right):

https://codereview.chromium.org/2274323002/diff/1/blimp/client/support/compos...
blimp/client/support/compositor/android/compositor_dependencies_impl_android.h:20:
CompositorDependenciesImplAndroid(const gfx::Size& real_size,
On 2016/08/24 23:22:46, Khushal wrote:
> I think we can get rid of the android subclass and just query it using the
> gfx::DeviceDisplayInfo, the way the renderer compositor does this,
>
https://cs.chromium.org/chromium/src/content/renderer/gpu/render_widget_compo...

Done.

https://codereview.chromium.org/2274323002/diff/1/blimp/client/support/compos...
File blimp/client/support/compositor/blimp_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/2274323002/diff/1/blimp/client/support/compos...
blimp/client/support/compositor/blimp_gpu_memory_buffer_manager.cc:14: #include
"blimp/client/support/compositor/blimp_gpu_memory_buffer_manager.h"
On 2016/08/24 23:22:46, Khushal wrote:
> nit: this should go back to the top.

Done.

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/app/compos...
File blimp/client/app/compositor/browser_compositor.cc (right):

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/app/compos...
blimp/client/app/compositor/browser_compositor.cc:141:
base::Bind(&BrowserCompositor::OnContextProviderCreated,
On 2016/08/25 05:02:53, Khushal wrote:
> I didn't see this earlier, but we are creating an offscreen context here but
we
> actually need to create an onscreen one with the widget.
> 
> Can we avoid adding the asynchronous context callback here, have this class
> create the BlimpContextProvider for itself like right now, since we know it is
> always going to be the InProcess one? The OutputSurface setup doesn't need to
> change really.

Hah yeah I ran into this this morning before I saw the comment.  Just removed
the usage of the CompositorDependencies for this context provider and built a
BlimpContextProvider, like you suggested.  Thanks!

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_compositor_dependencies.h (right):

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor_dependencies.h:31: class
BlimpCompositorDependencies {
On 2016/08/25 05:02:53, Khushal wrote:
> May be mention that this class is all on the main/UI thread.

Done.

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_task_graph_runner.h (right):

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_task_graph_runner.h:18: // engine's dummy UI
context factory.
On 2016/08/25 05:02:53, Khushal wrote:
> Update the comment? And this is only used in BlimpCompositorDependencies, can
be
> put in the .cc file itself. 

Good point. Done.

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor.cc:168: if (!host_ ||
!output_surface_request_pending_)
On 2016/08/25 05:02:53, Khushal wrote:
> Actually, we should need just the output_surface_request_pending_ check to
early
> out. If this value is true we must have a |host_| which is expecting this
> request.

I get that we only need to check one, but having it be explicit here made it
more clear to me (if I ever step away from the code for 9 months again!).  I can
remove the host_ check though.

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor.cc:273:
GetEmbedderDeps()->GetLayerTreeSettings();
On 2016/08/25 05:02:53, Khushal wrote:
> nit: Feel like it would be better to cache the unique_ptr in the EmbedderDeps
> and return a weak reference here, the deps is expected to outlive the
> compositors anyway.

Done.

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_compositor_manager.cc (right):

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor_manager.cc:52: return
base::WrapUnique(
On 2016/08/25 05:02:54, Khushal wrote:
> This will become MakeUnique when you rebase. :P

Acknowledged.

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_compositor_manager.h (right):

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor_manager.h:45: // virtual void
GenerateLayerTreeSettings(cc::LayerTreeSettings* settings);
On 2016/08/25 05:02:54, Khushal wrote:
> This is going to go away right? :P

Done.

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_compositor_manager_unittest.cc
(right):

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor_manager_unittest.cc:83:
base::WrapUnique(new MockCompositorDependencies())));
On 2016/08/25 05:02:54, Khushal wrote:
> MakeUnique.

Done.

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_compositor_unittest.cc (right):

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor_unittest.cc:70:
compositor_dependencies_ = base::WrapUnique(new BlimpCompositorDependencies(
On 2016/08/25 05:02:54, Khushal wrote:
> MakeUnique. :(

Sorry still hadn't done a personal final pass on this yet.  Thanks for catching.

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/public/com...
File blimp/client/public/compositor/compositor_dependencies.h (right):

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/public/com...
blimp/client/public/compositor/compositor_dependencies.h:25: class
CompositorDependencies {
On 2016/08/25 05:02:54, Khushal wrote:
> Comment on the lifetime? The Dependencies are expected to outlive all
> BlimpContents?

Done.

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/public/com...
blimp/client/public/compositor/compositor_dependencies.h:27: using
ContextProviderCallback =
On 2016/08/25 05:02:54, Khushal wrote:
> The ui::ContextProviderFactory passes an enum saying why the failure, should
we
> mirror a subset of that here. I think except GPU_SURFACE_HANDLE loss, we can
> expect all responses back, since we only need offscreen contexts, and that
> response is for an onscreen one.

Huh that callback doesn't return an enum either.  Maybe I'm missing the code
path.  Could you tell me where it is?  Thanks!

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/public/com...
blimp/client/public/compositor/compositor_dependencies.h:39: // created.  It may
be asynchronous if it needs to build it though.
On 2016/08/25 05:02:54, Khushal wrote:
> nit: the asynchronous bit is expected.

Done.

[David Trainor- moved to gerrit, 2016-08-26 17:23:40]

Description was changed from

==========
Expose Blimp dependencies to the embedder

This is a WIP CL.  Uploading so I can easily look at the diff.  Please
do not review yet!

BUG=
==========

to

==========
Expose Blimp dependencies to the embedder

Move Blimp compositor dependencies out to an interface that can be
implemented by the embedder.  Update blimp/client/app to use this
interface.

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

[Khushal, 2016-08-26 19:34:25]

lgtm. Thanks!

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor.cc:168: if (!host_ ||
!output_surface_request_pending_)
On 2016/08/26 17:15:54, David Trainor wrote:
> On 2016/08/25 05:02:53, Khushal wrote:
> > Actually, we should need just the output_surface_request_pending_ check to
> early
> > out. If this value is true we must have a |host_| which is expecting this
> > request.
> 
> I get that we only need to check one, but having it be explicit here made it
> more clear to me (if I ever step away from the code for 9 months again!).  I
can
> remove the host_ check though.

That's cool. Just didn't want to early out so someone incorrectly assumes that
the case was possible. You can add a DCHECK for the |host_| saying "We can never
have a pending request if the host is null" or something after the if check.

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/public/com...
File blimp/client/public/compositor/compositor_dependencies.h (right):

https://codereview.chromium.org/2274323002/diff/20001/blimp/client/public/com...
blimp/client/public/compositor/compositor_dependencies.h:27: using
ContextProviderCallback =
On 2016/08/26 17:15:54, David Trainor wrote:
> On 2016/08/25 05:02:54, Khushal wrote:
> > The ui::ContextProviderFactory passes an enum saying why the failure, should
> we
> > mirror a subset of that here. I think except GPU_SURFACE_HANDLE loss, we can
> > expect all responses back, since we only need offscreen contexts, and that
> > response is for an onscreen one.
> 
> Huh that callback doesn't return an enum either.  Maybe I'm missing the code
> path.  Could you tell me where it is?  Thanks!

Ummm, that patch hasn't landed yet. My bad.

https://codereview.chromium.org/2274323002/diff/60001/blimp/client/BUILD.gn
File blimp/client/BUILD.gn (right):

https://codereview.chromium.org/2274323002/diff/60001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:161: "//blimp/client/core/compositor",
moar compositor!!
I think I ended up doing this in my change though. Sorry.

https://codereview.chromium.org/2274323002/diff/60001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2274323002/diff/60001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor.cc:193:
DCHECK(!output_surface_);
Now that I look at it, this DCHECK will fail even if the |output_surface_| is
null because we will try to dereference it on the main  thread. That was my bad
sorry.

[Khushal, 2016-08-26 22:53:40]

https://codereview.chromium.org/2274323002/diff/80001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_compositor_dependencies.cc (right):

https://codereview.chromium.org/2274323002/diff/80001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor_dependencies.cc:32: }
nit: namespace.

[David Trainor- moved to gerrit, 2016-08-29 05:30:51]

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

[commit-bot: I haz the power, 2016-08-29 05:31:10]

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-08-29 05:35:31]

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

[commit-bot: I haz the power, 2016-08-29 05:35:32]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[David Trainor- moved to gerrit, 2016-08-29 15:55:13]

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

[David Trainor- moved to gerrit, 2016-08-29 15:55:25]

https://codereview.chromium.org/2274323002/diff/60001/blimp/client/BUILD.gn
File blimp/client/BUILD.gn (right):

https://codereview.chromium.org/2274323002/diff/60001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:161: "//blimp/client/core/compositor",
On 2016/08/26 19:34:25, Khushal wrote:
> moar compositor!!
> I think I ended up doing this in my change though. Sorry.

My fault too for not catching it :(.

https://codereview.chromium.org/2274323002/diff/60001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2274323002/diff/60001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor.cc:193:
DCHECK(!output_surface_);
On 2016/08/26 19:34:25, Khushal wrote:
> Now that I look at it, this DCHECK will fail even if the |output_surface_| is
> null because we will try to dereference it on the main  thread. That was my
bad
> sorry.

Done.

https://codereview.chromium.org/2274323002/diff/80001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_compositor_dependencies.cc (right):

https://codereview.chromium.org/2274323002/diff/80001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor_dependencies.cc:32: }
On 2016/08/26 22:53:40, Khushal wrote:
> nit: namespace.

Done.

[commit-bot: I haz the power, 2016-08-29 15:56: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-08-29 16:05:42]

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

[commit-bot: I haz the power, 2016-08-29 16:05:44]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  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-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[David Trainor- moved to gerrit, 2016-08-29 17:11:34]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2016-08-29 17:11:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
khushalsagar@chromium.org
Link to the patchset: https://codereview.chromium.org/2274323002/#ps120001
(title: "gn check failures")

[David Trainor- moved to gerrit, 2016-08-29 17:11:43]

The CQ bit was unchecked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2016-08-29 17:23:26]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2016-08-29 17:23:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
khushalsagar@chromium.org
Link to the patchset: https://codereview.chromium.org/2274323002/#ps140001
(title: "Rebased")

[commit-bot: I haz the power, 2016-08-29 17:27:27]

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

[David Trainor- moved to gerrit, 2016-08-29 19:55:42]

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

[commit-bot: I haz the power, 2016-08-29 20:13:46]

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

[David Trainor- moved to gerrit, 2016-08-29 20:35:44]

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

[commit-bot: I haz the power, 2016-08-29 21:15:35]

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

[David Trainor- moved to gerrit, 2016-08-29 23:05:20]

The CQ bit was unchecked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2016-08-29 23:05:28]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2016-08-29 23:05:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
khushalsagar@chromium.org
Link to the patchset: https://codereview.chromium.org/2274323002/#ps180001
(title: "Fix linux build break")

[commit-bot: I haz the power, 2016-08-29 23:51:13]

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-08-30 04:31:48]

Description was changed from

==========
Expose Blimp dependencies to the embedder

Move Blimp compositor dependencies out to an interface that can be
implemented by the embedder.  Update blimp/client/app to use this
interface.

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

to

==========
Expose Blimp dependencies to the embedder

Move Blimp compositor dependencies out to an interface that can be
implemented by the embedder.  Update blimp/client/app to use this
interface.

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

[commit-bot: I haz the power, 2016-08-30 04:31:56]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-08-30 04:34:57]

Description was changed from

==========
Expose Blimp dependencies to the embedder

Move Blimp compositor dependencies out to an interface that can be
implemented by the embedder.  Update blimp/client/app to use this
interface.

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

to

==========
Expose Blimp dependencies to the embedder

Move Blimp compositor dependencies out to an interface that can be
implemented by the embedder.  Update blimp/client/app to use this
interface.

BUG=611105

Committed: https://crrev.com/7cb0bd5a02b78ac0b8631a1329baafe89e1ff915
Cr-Commit-Position: refs/heads/master@{#415097}
==========

[commit-bot: I haz the power, 2016-08-30 04:34:58]

Patchset 10 (id:??) landed as
https://crrev.com/7cb0bd5a02b78ac0b8631a1329baafe89e1ff915
Cr-Commit-Position: refs/heads/master@{#415097}