blimp: Move BlimpCompositor to use delegated rendering.

blimp: Move BlimpCompositor to use delegated rendering.

Currently the BlimpCompositor is the compositor that owns the
cc::Display and delegated frames from it are directed to the display
by the BlimpDelegatingOutputSurface. The lifetime of the display and
the surfaces system is tied to the lifetime of the cc::OutputSurface,
which is tied to the lifetime of the native widget.

This diverges from the behavior expected when Blimp is embedded in
Clank, which owns the Surfaces system and holds the UI compositor,
which is the parent compositor that will embed content from the
BlimpCompositor.

This patch adds a BrowserCompositor to the blimp app, which performs
the work of the UI compositor in Chrome. This ensures that Blimp always
shares its content using a cc::SurfaceLayer, and the embedder is
expected to own the cc::Surfaces system and push this content to the
cc::Display.

BUG=639950
Committed: https://crrev.com/a498580e603b732faa90ccd61b99bb36271e4308
Cr-Commit-Position: refs/heads/master@{#414325}

[Khushal, 2016-08-22 20:58:35]

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

[Khushal, 2016-08-22 20:58:36]

Need to update the tests and the linux client. The app works great. PTAL.

[David Trainor- moved to gerrit, 2016-08-22 23:58:10]

small nits.

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

https://codereview.chromium.org/2266863003/diff/1/blimp/client/app/compositor...
blimp/client/app/compositor/browser_compositor.cc:116:
root_layer_->RemoveAllChildren();
Is all of this teardown needed?

https://codereview.chromium.org/2266863003/diff/1/blimp/client/app/compositor...
blimp/client/app/compositor/browser_compositor.cc:130: // Update the host.
newline before this

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

https://codereview.chromium.org/2266863003/diff/1/blimp/client/app/compositor...
blimp/client/app/compositor/browser_compositor.h:32: // Move to Compositor Deps
thingy after David's patch.
Do we need the deps provider here?  I can add the code, but if isn't used by the
internal BlimpCompositor it might not be worth it!

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

https://codereview.chromium.org/2266863003/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/delegated_output_surface.cc:94:
output_surface_thread_pipe_->SwapBuffers(std::move(frame));
DCHECK(output_surface_thread_pipe_)?

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

https://codereview.chromium.org/2266863003/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.h:31: }
I've heard for small forward declaration namespaces we don't have to/shouldn't
add the closing comment, but if you're doing it do we need one here as well?

https://codereview.chromium.org/2266863003/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.h:45: class
BlimpGpuMemoryBufferManager;
Why the Blimp subclass?

https://codereview.chromium.org/2266863003/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.h:120: void
WillBeginMainFrame() override {}
w00t so much nicer!

[Khushal, 2016-08-23 02:53:12]

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

https://codereview.chromium.org/2266863003/diff/1/blimp/client/app/compositor...
blimp/client/app/compositor/browser_compositor.cc:116:
root_layer_->RemoveAllChildren();
On 2016/08/22 23:58:10, David Trainor wrote:
> Is all of this teardown needed?

Actually no. We have the SetSurface cleanup in Clank, but I think that's to
remove the reference from the GpuSurfaceTracker, which we don't need. Thank for
pointing this out, we do need to clean up the reference with the SurfaceManager,
added that.

https://codereview.chromium.org/2266863003/diff/1/blimp/client/app/compositor...
blimp/client/app/compositor/browser_compositor.cc:130: // Update the host.
On 2016/08/22 23:58:10, David Trainor wrote:
> newline before this

Done.

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

https://codereview.chromium.org/2266863003/diff/1/blimp/client/app/compositor...
blimp/client/app/compositor/browser_compositor.h:32: // Move to Compositor Deps
thingy after David's patch.
On 2016/08/22 23:58:10, David Trainor wrote:
> Do we need the deps provider here?  I can add the code, but if isn't used by
the
> internal BlimpCompositor it might not be worth it!

I just meant that these singletons should be moved to the CompositorDepsProvider
so they can be shared between this compositor and the BlimpCompositor. Updated
the TODO for you since you are setting them up for the BlimpCompositor in your
next patch anyway.

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

https://codereview.chromium.org/2266863003/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/delegated_output_surface.cc:94:
output_surface_thread_pipe_->SwapBuffers(std::move(frame));
On 2016/08/22 23:58:10, David Trainor wrote:
> DCHECK(output_surface_thread_pipe_)?

If we are derefencing it in the next line, it will crash anyway and we get the
same stack trace. I used to DCHECK in these cases, until I realized that.

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

https://codereview.chromium.org/2266863003/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.h:31: }
On 2016/08/22 23:58:10, David Trainor wrote:
> I've heard for small forward declaration namespaces we don't have to/shouldn't
> add the closing comment, but if you're doing it do we need one here as well?

Its nice to do that. Done.

https://codereview.chromium.org/2266863003/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.h:45: class
BlimpGpuMemoryBufferManager;
On 2016/08/22 23:58:10, David Trainor wrote:
> Why the Blimp subclass?

Actually Not here. Its needed in the manager for the LayerTreeSettings but not
here.

https://codereview.chromium.org/2266863003/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.h:120: void
WillBeginMainFrame() override {}
On 2016/08/22 23:58:10, David Trainor wrote:
> w00t so much nicer!

:D

[Khushal, 2016-08-23 05:14:34]

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

https://codereview.chromium.org/2266863003/diff/60001/blimp/BUILD.gn#newcode98
blimp/BUILD.gn:98: # which can be used in this file only.
Had to do the JNI hookup for moving the BlimpCompositor test to android. Tommy
and I had discussed this target for tests in the previous patch
(https://codereview.chromium.org/2241623002/).

[David Trainor- moved to gerrit, 2016-08-23 16:43:40]

re JNI hookup, makes sense to me.  We need this to run the blimp net tests on
Android anyway, so now we can reenable those as well :).

https://codereview.chromium.org/2266863003/diff/80001/blimp/BUILD.gn
File blimp/BUILD.gn (right):

https://codereview.chromium.org/2266863003/diff/80001/blimp/BUILD.gn#newcode90
blimp/BUILD.gn:90: # While we could directly include them in the test target,
but in order to
Fix wording a bit.  Remove "While"?

https://codereview.chromium.org/2266863003/diff/80001/blimp/BUILD.gn#newcode101
blimp/BUILD.gn:101: visibility = [ ":*" ]
Can we be explicit about visibility here since the comment seems pretty explicit
about who should depend on this?

https://codereview.chromium.org/2266863003/diff/80001/blimp/client/test/BUILD.gn
File blimp/client/test/BUILD.gn (right):

https://codereview.chromium.org/2266863003/diff/80001/blimp/client/test/BUILD...
blimp/client/test/BUILD.gn:16: "//base/test:test_support",
"Just to be sure... let's double up on the test support!" :P

https://codereview.chromium.org/2266863003/diff/80001/blimp/client/test/BUILD...
blimp/client/test/BUILD.gn:20: "//mojo/edk/system",
"Same with the edk system!" ;)

[Khushal, 2016-08-24 16:13:43]

I moved all the threaded complexity within the DelegatedOutputSurface itself, so
BlimpCompositor don't need to care. :D

https://codereview.chromium.org/2266863003/diff/80001/blimp/BUILD.gn
File blimp/BUILD.gn (right):

https://codereview.chromium.org/2266863003/diff/80001/blimp/BUILD.gn#newcode90
blimp/BUILD.gn:90: # While we could directly include them in the test target,
but in order to
On 2016/08/23 16:43:39, David Trainor wrote:
> Fix wording a bit.  Remove "While"?

Done.

https://codereview.chromium.org/2266863003/diff/80001/blimp/BUILD.gn#newcode101
blimp/BUILD.gn:101: visibility = [ ":*" ]
On 2016/08/23 16:43:39, David Trainor wrote:
> Can we be explicit about visibility here since the comment seems pretty
explicit
> about who should depend on this?

That's the thing, since the real target that has to depend on this is generated
it ends up *blimp_unittest* and we can't do that when specifying gn targets. And
since this is testonly, even with everything in this file depending on it, we
figured its reasonable.

https://codereview.chromium.org/2266863003/diff/80001/blimp/client/test/BUILD.gn
File blimp/client/test/BUILD.gn (right):

https://codereview.chromium.org/2266863003/diff/80001/blimp/client/test/BUILD...
blimp/client/test/BUILD.gn:16: "//base/test:test_support",
On 2016/08/23 16:43:39, David Trainor wrote:
> "Just to be sure... let's double up on the test support!" :P

moar is better!! :P

https://codereview.chromium.org/2266863003/diff/80001/blimp/client/test/BUILD...
blimp/client/test/BUILD.gn:20: "//mojo/edk/system",
On 2016/08/23 16:43:39, David Trainor wrote:
> "Same with the edk system!" ;)

Done.

[Khushal, 2016-08-24 16:14:34]

On 2016/08/23 16:43:40, David Trainor wrote:
> re JNI hookup, makes sense to me.  We need this to run the blimp net tests on
> Android anyway, so now we can reenable those as well :).

Oh cool. Do you want me to do that within this patch?

> 
> https://codereview.chromium.org/2266863003/diff/80001/blimp/BUILD.gn
> File blimp/BUILD.gn (right):
> 
> https://codereview.chromium.org/2266863003/diff/80001/blimp/BUILD.gn#newcode90
> blimp/BUILD.gn:90: # While we could directly include them in the test target,
> but in order to
> Fix wording a bit.  Remove "While"?
> 
>
https://codereview.chromium.org/2266863003/diff/80001/blimp/BUILD.gn#newcode101
> blimp/BUILD.gn:101: visibility = [ ":*" ]
> Can we be explicit about visibility here since the comment seems pretty
explicit
> about who should depend on this?
> 
>
https://codereview.chromium.org/2266863003/diff/80001/blimp/client/test/BUILD.gn
> File blimp/client/test/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2266863003/diff/80001/blimp/client/test/BUILD...
> blimp/client/test/BUILD.gn:16: "//base/test:test_support",
> "Just to be sure... let's double up on the test support!" :P
> 
>
https://codereview.chromium.org/2266863003/diff/80001/blimp/client/test/BUILD...
> blimp/client/test/BUILD.gn:20: "//mojo/edk/system",
> "Same with the edk system!" ;)

[David Trainor- moved to gerrit, 2016-08-24 23:09:34]

lgtm % nits

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/app/compo...
File blimp/client/app/compositor/browser_compositor.cc (right):

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/app/compo...
blimp/client/app/compositor/browser_compositor.cc:44: ~DisplayOutputSurface()
override {}
= default?

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/core/comp...
File blimp/client/core/compositor/BUILD.gn (right):

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/core/comp...
blimp/client/core/compositor/BUILD.gn:33: source_set("compositor_support") {
Can we remove this and put these in the "compositor" target above?

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/core/comp...
File blimp/client/core/compositor/delegated_output_surface.h (right):

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/core/comp...
blimp/client/core/compositor/delegated_output_surface.h:47: // called on
compositor thread.
called -> Called

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/core/comp...
File blimp/client/core/compositor/delegated_output_surface_unittest.cc (right):

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/core/comp...
blimp/client/core/compositor/delegated_output_surface_unittest.cc:222:
TEST_F(DelegatedOutputSurfaceTestBindFails, NoRegistrations) {}
Can we put the test flow in here?

SetupTest(true/false);
// Do your thing
EndTest
WaitForIdle
EXPECT_EQ_STUFFZ()
?

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/core/comp...
blimp/client/core/compositor/delegated_output_surface_unittest.cc:240:
TEST_F(DelegatedOutputSurfaceTestBindSucceeds, SwapBuffers) {}
Who calls EndTest?  Could we just put most of this logic in the test body?

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_compositor.cc:193: // manager must outlive
compositors using it.
manager -> Manager

[Khushal, 2016-08-25 02:35:39]

All done. CQing! :D

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/app/compo...
File blimp/client/app/compositor/browser_compositor.cc (right):

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/app/compo...
blimp/client/app/compositor/browser_compositor.cc:44: ~DisplayOutputSurface()
override {}
On 2016/08/24 23:09:33, David Trainor wrote:
> = default?

Done.

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/core/comp...
File blimp/client/core/compositor/BUILD.gn (right):

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/core/comp...
blimp/client/core/compositor/BUILD.gn:33: source_set("compositor_support") {
On 2016/08/24 23:09:33, David Trainor wrote:
> Can we remove this and put these in the "compositor" target above?

Done.

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/core/comp...
File blimp/client/core/compositor/delegated_output_surface.h (right):

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/core/comp...
blimp/client/core/compositor/delegated_output_surface.h:47: // called on
compositor thread.
On 2016/08/24 23:09:33, David Trainor wrote:
> called -> Called

Removed.

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/core/comp...
File blimp/client/core/compositor/delegated_output_surface_unittest.cc (right):

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/core/comp...
blimp/client/core/compositor/delegated_output_surface_unittest.cc:222:
TEST_F(DelegatedOutputSurfaceTestBindFails, NoRegistrations) {}
On 2016/08/24 23:09:34, David Trainor wrote:
> Can we put the test flow in here?
> 
> SetupTest(true/false);
> // Do your thing
> EndTest
> WaitForIdle
> EXPECT_EQ_STUFFZ()
> ?

Done.

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/core/comp...
blimp/client/core/compositor/delegated_output_surface_unittest.cc:240:
TEST_F(DelegatedOutputSurfaceTestBindSucceeds, SwapBuffers) {}
On 2016/08/24 23:09:34, David Trainor wrote:
> Who calls EndTest?  Could we just put most of this logic in the test body?

Done.

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2266863003/diff/100001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_compositor.cc:193: // manager must outlive
compositors using it.
On 2016/08/24 23:09:34, David Trainor wrote:
> manager -> Manager

Done.

[Khushal, 2016-08-25 02:35:44]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-08-25 02:35:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2266863003/#ps120001
(title: "addressed comments + final class fix.")

[commit-bot: I haz the power, 2016-08-25 02:36:37]

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-25 02:40:32]

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

[commit-bot: I haz the power, 2016-08-25 02:40:34]

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

[Khushal, 2016-08-25 03:00:36]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-08-25 03:00:37]

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

[commit-bot: I haz the power, 2016-08-25 03:01:10]

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-25 03:05:59]

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

[commit-bot: I haz the power, 2016-08-25 03:06:01]

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...)
  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_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Khushal, 2016-08-25 04:13:15]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-08-25 04:13:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2266863003/#ps160001
(title: "make gn happy")

[commit-bot: I haz the power, 2016-08-25 04:13:46]

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-25 04:20:57]

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

[commit-bot: I haz the power, 2016-08-25 04:20:58]

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

[Khushal, 2016-08-25 05:03:50]

The CQ bit was checked by khushalsagar@chromium.org

[commit-bot: I haz the power, 2016-08-25 05:04:22]

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-25 05:37:09]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-08-25 05:39:54]

Description was changed from

==========
blimp: Move BlimpCompositor to use delegated rendering.

Currently the BlimpCompositor is the compositor that owns the
cc::Display and delegated frames from it are directed to the display
by the BlimpDelegatingOutputSurface. The lifetime of the display and
the surfaces system is tied to the lifetime of the cc::OutputSurface,
which is tied to the lifetime of the native widget.

This diverges from the behavior expected when Blimp is embedded in
Clank, which owns the Surfaces system and holds the UI compositor,
which is the parent compositor that will embed content from the
BlimpCompositor.

This patch adds a BrowserCompositor to the blimp app, which performs
the work of the UI compositor in Chrome. This ensures that Blimp always
shares its content using a cc::SurfaceLayer, and the embedder is
expected to own the cc::Surfaces system and push this content to the
cc::Display.

BUG=639950
==========

to

==========
blimp: Move BlimpCompositor to use delegated rendering.

Currently the BlimpCompositor is the compositor that owns the
cc::Display and delegated frames from it are directed to the display
by the BlimpDelegatingOutputSurface. The lifetime of the display and
the surfaces system is tied to the lifetime of the cc::OutputSurface,
which is tied to the lifetime of the native widget.

This diverges from the behavior expected when Blimp is embedded in
Clank, which owns the Surfaces system and holds the UI compositor,
which is the parent compositor that will embed content from the
BlimpCompositor.

This patch adds a BrowserCompositor to the blimp app, which performs
the work of the UI compositor in Chrome. This ensures that Blimp always
shares its content using a cc::SurfaceLayer, and the embedder is
expected to own the cc::Surfaces system and push this content to the
cc::Display.

BUG=639950

Committed: https://crrev.com/a498580e603b732faa90ccd61b99bb36271e4308
Cr-Commit-Position: refs/heads/master@{#414325}
==========

[commit-bot: I haz the power, 2016-08-25 05:39:56]

Patchset 9 (id:??) landed as
https://crrev.com/a498580e603b732faa90ccd61b99bb36271e4308
Cr-Commit-Position: refs/heads/master@{#414325}