blimp: Move compositing, input and render widget feature to client/core.

blimp: Move compositing, input and render widget feature to client/core.

This patch integrates the BlimpCompositor, the BlimpInputManager and
the RenderWidget feature with BlimpContents framework in
blimp/client/core. In the process of moving these, it also establishes
the class structre in the client code to mirror the structure of their
counterparts on the Engine.

1) BlimpRenderWidget maps to content's render widget and maintains the
compositing and input state. BlimpInputManager is still in the
compositor, will be moved to the render widget in a follow-up patch.

2) BlimpCompositor maps to the RenderWidgetCompositor.

3) BlimpContents maps to the WebContents, and thus is the delegate for
the RenderWidgetFeature. The lifetime of the BlimpRenderWidgets on the
client is controlled by their counterparts on the engine.

This change also consolidates the compositor dependencies into a
seperate CompositorDepsProvider, with a mode for direct rendering which
is used by the standalone app to allow the BlimpCompositor to render
content directly to the native widget. Support for a mode for
delegated rendering will be added in a follow up patch and will be
used when Blimp is embedded by Clank.

[Khushal, 2016-08-11 21:56:25]

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

[Khushal, 2016-08-11 21:56:26]

I still have to hook in the BlimpStats. Kevin has a patch close to landing which
changes it into a single instance with a static getter. Will rebase off of that
and add it.

[Khushal, 2016-08-11 21:56:46]

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

[commit-bot: I haz the power, 2016-08-11 21:57:51]

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-11 22:06:17]

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

[commit-bot: I haz the power, 2016-08-11 22:06:18]

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...)
  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_clobber_rel_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_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[nyquist, 2016-08-12 07:34:28]

just some initial comments.
looking forward to the upcoming rebases!

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

https://codereview.chromium.org/2241623002/diff/1/blimp/client/app/android/bl...
blimp/client/app/android/blimp_view.cc:185: return
blimp_contents_->GetActiveWidget()->OnTouchEvent(event);
Opposite of //cc, where curly brackets are ... "optional"... , in //blimp we use
curlies for one-liners like this...

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

https://codereview.chromium.org/2241623002/diff/1/blimp/client/app/linux/blim...
blimp/client/app/linux/blimp_display_manager.h:11: #include
"blimp/client/core/compositor/compositor_deps_provider.h"
Needed?

https://codereview.chromium.org/2241623002/diff/1/blimp/client/app/linux/blim...
blimp/client/app/linux/blimp_display_manager.h:26: class CompositorDepsProvider;
Needed?

https://codereview.chromium.org/2241623002/diff/1/blimp/client/app/linux/blim...
blimp/client/app/linux/blimp_display_manager.h:62: BlimpContentsManager
blimp_contents_manager_;
Will this become BlimpClientContext possibly in the future when
//blimp/client/app is an embedder?

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/BUILD.gn
File blimp/client/core/BUILD.gn (right):

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/BUILD.gn#...
blimp/client/core/BUILD.gn:37: "//blimp/client/core/input",
Would we maybe want to add render_widget as well here, so core for sure
encapsulates all of it? Or should we just trust that they will all be included
based on deps?
Since this is just a convenience group, it somehow feels like it should just
list all of them. I don't feel strongly I think though.

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

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/blimp_compositor.h:157: cc::TaskGraphRunner*
task_graph_runner_;
It is possible to add comments for these for laymen like me? I think it'd be
helpful once they're first there.

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/contents/...
File blimp/client/core/contents/android/blimp_contents_factory.cc (right):

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/contents/...
blimp/client/core/contents/android/blimp_contents_factory.cc:30: new
BlimpContentsImpl(kDummyTabId, nullptr, nullptr);
Dude, can we just delete this and the Java-class and the Java-tests alltogether?
They're not really helping.

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/contents/...
File blimp/client/core/contents/blimp_contents_impl.h (right):

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/contents/...
blimp/client/core/contents/blimp_contents_impl.h:84: void
SetAcceleratedWidget(gfx::AcceleratedWidget widget);
This seems to be called from the linux app for now. Will chrome not need to do
that?

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/contents/...
File blimp/client/core/contents/blimp_contents_manager.h (right):

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/contents/...
blimp/client/core/contents/blimp_contents_manager.h:50: RenderWidgetFeature*
render_widget_feature_;
Do we want to comment on the lifetime of this here?

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/render_wi...
File blimp/client/core/render_widget/blimp_render_widget.cc (right):

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/render_wi...
blimp/client/core/render_widget/blimp_render_widget.cc:32: if
(compositor_deps_provider_->use_direct_rendering())
curlies throughout this file

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/render_wi...
blimp/client/core/render_widget/blimp_render_widget.cc:53: return;
braces

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/render_wi...
File blimp/client/core/render_widget/blimp_render_widget.h (right):

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/render_wi...
blimp/client/core/render_widget/blimp_render_widget.h:64: class
DirectRenderingDeps {
Could this just be a forward declaration in the days of std::unique_ptr?

[nyquist, 2016-08-12 07:34:49]

Also... This is such excite! Very wow.

[Khushal, 2016-08-13 00:03:22]

Rebased and verified the change too. You want to patch locally for the unit-test
issue, use the StreamPacketReader in the net code. Kevin is fixing an issue with
the z-lib compression code.

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

https://codereview.chromium.org/2241623002/diff/1/blimp/client/app/android/bl...
blimp/client/app/android/blimp_view.cc:185: return
blimp_contents_->GetActiveWidget()->OnTouchEvent(event);
On 2016/08/12 07:34:28, nyquist wrote:
> Opposite of //cc, where curly brackets are ... "optional"... , in //blimp we
use
> curlies for one-liners like this...

Done.
You know I'm in that land most of the time. :P

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

https://codereview.chromium.org/2241623002/diff/1/blimp/client/app/linux/blim...
blimp/client/app/linux/blimp_display_manager.h:11: #include
"blimp/client/core/compositor/compositor_deps_provider.h"
On 2016/08/12 07:34:28, nyquist wrote:
> Needed?

Oh not anymore. Done.

https://codereview.chromium.org/2241623002/diff/1/blimp/client/app/linux/blim...
blimp/client/app/linux/blimp_display_manager.h:26: class CompositorDepsProvider;
On 2016/08/12 07:34:28, nyquist wrote:
> Needed?

Done.

https://codereview.chromium.org/2241623002/diff/1/blimp/client/app/linux/blim...
blimp/client/app/linux/blimp_display_manager.h:62: BlimpContentsManager
blimp_contents_manager_;
On 2016/08/12 07:34:28, nyquist wrote:
> Will this become BlimpClientContext possibly in the future when
> //blimp/client/app is an embedder?

Yup. That looks like where we should be asking blimp to create a contents.

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/BUILD.gn
File blimp/client/core/BUILD.gn (right):

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/BUILD.gn#...
blimp/client/core/BUILD.gn:37: "//blimp/client/core/input",
On 2016/08/12 07:34:28, nyquist wrote:
> Would we maybe want to add render_widget as well here, so core for sure
> encapsulates all of it? Or should we just trust that they will all be included
> based on deps?
> Since this is just a convenience group, it somehow feels like it should just
> list all of them. I don't feel strongly I think though.

That's a good idea. Done.

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

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/blimp_compositor.h:157: cc::TaskGraphRunner*
task_graph_runner_;
On 2016/08/12 07:34:28, nyquist wrote:
> It is possible to add comments for these for laymen like me? I think it'd be
> helpful once they're first there.

Good idea. Done.

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/contents/...
File blimp/client/core/contents/android/blimp_contents_factory.cc (right):

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/contents/...
blimp/client/core/contents/android/blimp_contents_factory.cc:30: new
BlimpContentsImpl(kDummyTabId, nullptr, nullptr);
On 2016/08/12 07:34:28, nyquist wrote:
> Dude, can we just delete this and the Java-class and the Java-tests
alltogether?
> They're not really helping.

Done. But I can't get the JNI hookup for tests to work. :(

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/contents/...
File blimp/client/core/contents/blimp_contents_impl.h (right):

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/contents/...
blimp/client/core/contents/blimp_contents_impl.h:84: void
SetAcceleratedWidget(gfx::AcceleratedWidget widget);
On 2016/08/12 07:34:28, nyquist wrote:
> This seems to be called from the linux app for now. Will chrome not need to do
> that?

It's used by both linux and android but Chrome would never use this. That's why
I didn't want to add it to public yet.

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/contents/...
File blimp/client/core/contents/blimp_contents_manager.h (right):

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/contents/...
blimp/client/core/contents/blimp_contents_manager.h:50: RenderWidgetFeature*
render_widget_feature_;
On 2016/08/12 07:34:28, nyquist wrote:
> Do we want to comment on the lifetime of this here?

I guess it should be implicit that this is expected to outlive the
BlimpContentsManager. In case its not then we should add a comment stating that
it can be null. WDYT?

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/render_wi...
File blimp/client/core/render_widget/blimp_render_widget.cc (right):

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/render_wi...
blimp/client/core/render_widget/blimp_render_widget.cc:32: if
(compositor_deps_provider_->use_direct_rendering())
On 2016/08/12 07:34:28, nyquist wrote:
> curlies throughout this file

Done.

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/render_wi...
blimp/client/core/render_widget/blimp_render_widget.cc:53: return;
On 2016/08/12 07:34:28, nyquist wrote:
> braces

Done.

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/render_wi...
File blimp/client/core/render_widget/blimp_render_widget.h (right):

https://codereview.chromium.org/2241623002/diff/1/blimp/client/core/render_wi...
blimp/client/core/render_widget/blimp_render_widget.h:64: class
DirectRenderingDeps {
On 2016/08/12 07:34:28, nyquist wrote:
> Could this just be a forward declaration in the days of std::unique_ptr?

That's an awesome idea. Thanks! Done!

[nyquist, 2016-08-16 01:29:55]

https://codereview.chromium.org/2241623002/diff/40001/blimp/test/BUILD.gn
File blimp/test/BUILD.gn (right):

https://codereview.chromium.org/2241623002/diff/40001/blimp/test/BUILD.gn#new...
blimp/test/BUILD.gn:36: if (is_android) {
I think this you want this to go in //blimp:blimp_unittests.

[Khushal, 2016-08-16 18:07:48]

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

[commit-bot: I haz the power, 2016-08-16 18:08:17]

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

[Khushal, 2016-08-16 18:08:25]

https://codereview.chromium.org/2241623002/diff/40001/blimp/test/BUILD.gn
File blimp/test/BUILD.gn (right):

https://codereview.chromium.org/2241623002/diff/40001/blimp/test/BUILD.gn#new...
blimp/test/BUILD.gn:36: if (is_android) {
On 2016/08/16 01:29:55, nyquist wrote:
> I think this you want this to go in //blimp:blimp_unittests.

Done. Thanks!

[commit-bot: I haz the power, 2016-08-16 18:12:14]

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

[commit-bot: I haz the power, 2016-08-16 18:12:15]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Khushal, 2016-08-16 20:54:58]

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

[commit-bot: I haz the power, 2016-08-16 20:55:33]

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-16 21:00:18]

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

[commit-bot: I haz the power, 2016-08-16 21:00:19]

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

[Khushal, 2016-08-16 21:07:39]

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

[commit-bot: I haz the power, 2016-08-16 21:08:16]

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-16 21:13:17]

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

[commit-bot: I haz the power, 2016-08-16 21:13:18]

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

[Khushal, 2016-08-16 22:07:20]

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

[commit-bot: I haz the power, 2016-08-16 22:07:46]

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-16 22:12:42]

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

[commit-bot: I haz the power, 2016-08-16 22:12:43]

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...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Khushal, 2016-08-16 22:37:50]

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

[commit-bot: I haz the power, 2016-08-16 22:38:29]

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

[nyquist, 2016-08-16 23:14:58]

https://codereview.chromium.org/2241623002/diff/110001/blimp/BUILD.gn
File blimp/BUILD.gn (right):

https://codereview.chromium.org/2241623002/diff/110001/blimp/BUILD.gn#newcode88
blimp/BUILD.gn:88: java_group("blimp_unittests_java_deps") {
Could you add a comment as to why we have this weird construct?

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/app/andro...
File blimp/client/app/android/blimp_view.h (right):

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/app/andro...
blimp/client/app/android/blimp_view.h:103: std::unique_ptr<BlimpContentsImpl>
blimp_contents_;
I know this is temporary, but could you add a comment explaining the
relationship between these two and that we only support a single BlimpContents?

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl.cc:29:
base::LazyInstance<RenderWidgetFeature> g_render_widget_feature;
Discussed with dtrainor@. I think for now BlimpClientContextImpl can own it, and
you can pass a pointer to BCM in the constructor? WDYT?
The registration can happen in BCCI:BCCI() I think.

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

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/core/comp...
blimp/client/core/compositor/BUILD.gn:22: "//blimp/client/core/input",
Isn't this exposed through blimp_compositor.h?

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/core/comp...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/core/comp...
blimp/client/core/compositor/blimp_compositor.cc:50: if (host_)
Nit: the curlies?

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/core/comp...
File blimp/client/core/compositor/blimp_compositor.h (right):

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/core/comp...
blimp/client/core/compositor/blimp_compositor.h:73: virtual void
RequestOutputSurface() = 0;
What does this do?

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/core/comp...
blimp/client/core/compositor/blimp_compositor.h:105: void SetVisible(bool
visible);
Could we keep the comment on this one?

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/core/comp...
blimp/client/core/compositor/blimp_compositor.h:108: void
SetOutputSurface(std::unique_ptr<cc::OutputSurface> output_surface);
Should we explain what these two do and how calling them affect things?

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/core/comp...
blimp/client/core/compositor/blimp_compositor.h:174:
std::unique_ptr<cc::LayerTreeHost> host_;
does this default to nullptr? Or should we set that?

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

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_compositor.cc:5: #include
"blimp/client/feature/compositor/blimp_compositor.h"
Does this file still exist after your change, or should you now be referring to
blimp/client/core/compositor/blimp_compositor.h?

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_compositor.cc:7: #include
<blimp/client/core/compositor/blimp_delegating_output_surface.h>
This had a weird format and location in the file...

https://codereview.chromium.org/2241623002/diff/110001/blimp/common/BUILD.gn
File blimp/common/BUILD.gn (right):

https://codereview.chromium.org/2241623002/diff/110001/blimp/common/BUILD.gn#...
blimp/common/BUILD.gn:33: "//cc",
Are you removing the last dependency on //cc with this?

https://codereview.chromium.org/2241623002/diff/110001/blimp/test/BUILD.gn
File blimp/test/BUILD.gn (right):

https://codereview.chromium.org/2241623002/diff/110001/blimp/test/BUILD.gn#ne...
blimp/test/BUILD.gn:29: "//base",
It doesn't look like you updated these sources. Did you mean to put this in
source_set("run_all_unittests") ?

https://codereview.chromium.org/2241623002/diff/110001/blimp/test/run_all_uni...
File blimp/test/run_all_unittests.cc (right):

https://codereview.chromium.org/2241623002/diff/110001/blimp/test/run_all_uni...
blimp/test/run_all_unittests.cc:39: if (!base::android::RegisterJni(env))
Nit: I've been told we "always" use curlies in blimp-land.

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_contents_impl.cc (right):

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_impl.cc:46: if (active_widget_)
all the curlies

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_contents_impl.h (right):

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_impl.h:76: // TODO(mlliu): Should go
the BlimpContentsManager when we have that ready.
So should this still happen in a follow-up CL?

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_contents_impl_unittest.cc (right):

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_impl_unittest.cc:112:
EXPECT_NE(widget1_, nullptr);
Nit: do you want to flip these to match other EXPECT_EQs? I know it doesn't
matter... it's just in my head.

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_contents_manager.h (right):

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_manager.h:47: CompositorDepsProvider
compositor_deps_provider_;
Does his happen to provide state that will be shared across profiles, etc? If
so, should this be a singleton?

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/inpu...
File blimp/client/core/input/blimp_input_manager.h (right):

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/inpu...
blimp/client/core/input/blimp_input_manager.h:104: // Move out the gesture
provider bits from the input manager. They should
Is this a TODO?

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/rend...
File blimp/client/core/render_widget/blimp_render_widget.h (right):

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/rend...
blimp/client/core/render_widget/blimp_render_widget.h:29: // The
BlimpRenderWidget is the blimp counterpart to the RenderWidget. This is
content::RenderWidget?

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/rend...
blimp/client/core/render_widget/blimp_render_widget.h:39: int32_t GetId() const;
What is this ID? How does it relate to the ID in BlimpContents and tab ID? And
how does it relate to any IDs on the engine side?

[commit-bot: I haz the power, 2016-08-16 23:37:48]

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

[commit-bot: I haz the power, 2016-08-16 23:37:49]

Dry run: This issue passed the CQ dry run.

[Khushal, 2016-08-18 02:01:46]

Still working on comments from #7.

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_contents_impl.cc (right):

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_impl.cc:46: if (active_widget_)
On 2016/08/16 23:14:57, nyquist wrote:
> all the curlies

Done.

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_contents_impl.h (right):

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_impl.h:76: // TODO(mlliu): Should go
the BlimpContentsManager when we have that ready.
On 2016/08/16 23:14:57, nyquist wrote:
> So should this still happen in a follow-up CL?

I think it should be be the BlimpContentsManager that should be dealing with the
RenderWidgetFeature (which should be called the ContentsFeature I guess). But
this is all going to change a lot when we move to the Document model with
Helium. So we can let this be for now.

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_contents_impl_unittest.cc (right):

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_impl_unittest.cc:112:
EXPECT_NE(widget1_, nullptr);
On 2016/08/16 23:14:57, nyquist wrote:
> Nit: do you want to flip these to match other EXPECT_EQs? I know it doesn't
> matter... it's just in my head.

The macro is expected,actual so you're right. Done.

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_contents_manager.h (right):

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_manager.h:47: CompositorDepsProvider
compositor_deps_provider_;
On 2016/08/16 23:14:57, nyquist wrote:
> Does his happen to provide state that will be shared across profiles, etc? If
> so, should this be a singleton?

Yup, it has the compositor thread and the task graph runner, which should be
shared for all compositors. I've added it as a single instance, since I think
for Clank integration we would want the tests to be able to have a clean
shutdown and terminate all this state. So I've added methods to the public API
that will set up all such dependencies for us. Let me know if that should go
somewhere else instead.

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/inpu...
File blimp/client/core/input/blimp_input_manager.h (right):

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/inpu...
blimp/client/core/input/blimp_input_manager.h:104: // Move out the gesture
provider bits from the input manager. They should
On 2016/08/16 23:14:57, nyquist wrote:
> Is this a TODO?

Done.

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/rend...
File blimp/client/core/render_widget/blimp_render_widget.h (right):

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/rend...
blimp/client/core/render_widget/blimp_render_widget.h:29: // The
BlimpRenderWidget is the blimp counterpart to the RenderWidget. This is
On 2016/08/16 23:14:57, nyquist wrote:
> content::RenderWidget?

Done.

https://codereview.chromium.org/2241623002/diff/130001/blimp/client/core/rend...
blimp/client/core/render_widget/blimp_render_widget.h:39: int32_t GetId() const;
On 2016/08/16 23:14:57, nyquist wrote:
> What is this ID? How does it relate to the ID in BlimpContents and tab ID? And
> how does it relate to any IDs on the engine side?

Added a comment.

[Khushal, 2016-08-18 03:16:33]

https://codereview.chromium.org/2241623002/diff/110001/blimp/BUILD.gn
File blimp/BUILD.gn (right):

https://codereview.chromium.org/2241623002/diff/110001/blimp/BUILD.gn#newcode88
blimp/BUILD.gn:88: java_group("blimp_unittests_java_deps") {
On 2016/08/16 23:14:57, nyquist wrote:
> Could you add a comment as to why we have this weird construct?

Done.

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/app/andro...
File blimp/client/app/android/blimp_view.h (right):

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/app/andro...
blimp/client/app/android/blimp_view.h:103: std::unique_ptr<BlimpContentsImpl>
blimp_contents_;
On 2016/08/16 23:14:57, nyquist wrote:
> I know this is temporary, but could you add a comment explaining the
> relationship between these two and that we only support a single
BlimpContents?

Done. Though I think more detailed comments about these should go on
BlimpContentsManager and BlimpContents instead.

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl.cc:29:
base::LazyInstance<RenderWidgetFeature> g_render_widget_feature;
On 2016/08/16 23:14:57, nyquist wrote:
> Discussed with dtrainor@. I think for now BlimpClientContextImpl can own it,
and
> you can pass a pointer to BCM in the constructor? WDYT?
> The registration can happen in BCCI:BCCI() I think.

Makes sense. Done.
That is where we do start up the ThreadPipeManager and rest of the net code.

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

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/core/comp...
blimp/client/core/compositor/BUILD.gn:22: "//blimp/client/core/input",
On 2016/08/16 23:14:57, nyquist wrote:
> Isn't this exposed through blimp_compositor.h?

Done.

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/core/comp...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/core/comp...
blimp/client/core/compositor/blimp_compositor.cc:50: if (host_)
On 2016/08/16 23:14:57, nyquist wrote:
> Nit: the curlies?

Done.

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/core/comp...
File blimp/client/core/compositor/blimp_compositor.h (right):

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/core/comp...
blimp/client/core/compositor/blimp_compositor.h:73: virtual void
RequestOutputSurface() = 0;
On 2016/08/16 23:14:57, nyquist wrote:
> What does this do?

Added a comment.

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/core/comp...
blimp/client/core/compositor/blimp_compositor.h:105: void SetVisible(bool
visible);
On 2016/08/16 23:14:57, nyquist wrote:
> Could we keep the comment on this one?

The previous comments are not valid anymore, since I've moved all that logic to
the BlimpRenderWidget. This class is going to merge into cc/blimp soon, so I
don't want to keep much blimp specific logic here.

Also, the behavior of dropping the OutputSurface when visibility changes is used
when we have an onscreen context, since the native widget that we were drawing
to goes away, but it doesn't need to be done when we have an offscreen context,
which is what we'll use when blimp is embedded by Clank. So all that is in
DirectRenderingDeps in BlimpRenderWidget now.

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/core/comp...
blimp/client/core/compositor/blimp_compositor.h:108: void
SetOutputSurface(std::unique_ptr<cc::OutputSurface> output_surface);
On 2016/08/16 23:14:57, nyquist wrote:
> Should we explain what these two do and how calling them affect things?

Done.

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/core/comp...
blimp/client/core/compositor/blimp_compositor.h:174:
std::unique_ptr<cc::LayerTreeHost> host_;
On 2016/08/16 23:14:57, nyquist wrote:
> does this default to nullptr? Or should we set that?

It should be initialized with nullptr for the default ctor.

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

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_compositor.cc:5: #include
"blimp/client/feature/compositor/blimp_compositor.h"
On 2016/08/16 23:14:57, nyquist wrote:
> Does this file still exist after your change, or should you now be referring
to
> blimp/client/core/compositor/blimp_compositor.h?

Done.

https://codereview.chromium.org/2241623002/diff/110001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_compositor.cc:7: #include
<blimp/client/core/compositor/blimp_delegating_output_surface.h>
On 2016/08/16 23:14:57, nyquist wrote:
> This had a weird format and location in the file...

Done.

https://codereview.chromium.org/2241623002/diff/110001/blimp/common/BUILD.gn
File blimp/common/BUILD.gn (right):

https://codereview.chromium.org/2241623002/diff/110001/blimp/common/BUILD.gn#...
blimp/common/BUILD.gn:33: "//cc",
On 2016/08/16 23:14:57, nyquist wrote:
> Are you removing the last dependency on //cc with this?

Done.

https://codereview.chromium.org/2241623002/diff/110001/blimp/test/BUILD.gn
File blimp/test/BUILD.gn (right):

https://codereview.chromium.org/2241623002/diff/110001/blimp/test/BUILD.gn#ne...
blimp/test/BUILD.gn:29: "//base",
On 2016/08/16 23:14:57, nyquist wrote:
> It doesn't look like you updated these sources. Did you mean to put this in
> source_set("run_all_unittests") ?

Done.

https://codereview.chromium.org/2241623002/diff/110001/blimp/test/run_all_uni...
File blimp/test/run_all_unittests.cc (right):

https://codereview.chromium.org/2241623002/diff/110001/blimp/test/run_all_uni...
blimp/test/run_all_unittests.cc:39: if (!base::android::RegisterJni(env))
On 2016/08/16 23:14:57, nyquist wrote:
> Nit: I've been told we "always" use curlies in blimp-land.

Done.

[Khushal, 2016-08-18 03:16:43]

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

[commit-bot: I haz the power, 2016-08-18 03:17:09]

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-18 03:20:54]

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

[commit-bot: I haz the power, 2016-08-18 03:20:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nyquist, 2016-08-19 00:17:07]

https://codereview.chromium.org/2241623002/diff/170001/blimp/client/core/comp...
File blimp/client/core/compositor/compositor_deps_provider.cc (right):

https://codereview.chromium.org/2241623002/diff/170001/blimp/client/core/comp...
blimp/client/core/compositor/compositor_deps_provider.cc:93:
CompositorDepsProvider* current_compositor_deps_provider = nullptr;
Could you comment on the lifetime of this?

https://codereview.chromium.org/2241623002/diff/170001/blimp/client/core/comp...
blimp/client/core/compositor/compositor_deps_provider.cc:115: return
current_compositor_deps_provider;
DCHECK here?

[Khushal, 2016-08-23 05:11:06]

Abandoning this patch in favour of this one,
https://codereview.chromium.org/2266863003

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

https://codereview.chromium.org/2241623002/diff/170001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_compositor.cc:7: #include
<blimp/client/core/compositor/blimp_delegating_output_surface.h>
Delete this file!!

[Khushal, 2016-08-23 05:11:18]

Description was changed from

==========
blimp: Move compositing, input and render widget feature to client/core.

This patch integrates the BlimpCompositor, the BlimpInputManager and
the RenderWidget feature with BlimpContents framework in
blimp/client/core. In the process of moving these, it also establishes
the class structre in the client code to mirror the structure of their
counterparts on the Engine.

1) BlimpRenderWidget maps to content's render widget and maintains the
compositing and input state. BlimpInputManager is still in the
compositor, will be moved to the render widget in a follow-up patch.

2) BlimpCompositor maps to the RenderWidgetCompositor.

3) BlimpContents maps to the WebContents, and thus is the delegate for
the RenderWidgetFeature. The lifetime of the BlimpRenderWidgets on the
client is controlled by their counterparts on the engine.

This change also consolidates the compositor dependencies into a
seperate CompositorDepsProvider, with a mode for direct rendering which
is used by the standalone app to allow the BlimpCompositor to render
content directly to the native widget. Support for a mode for
delegated rendering will be added in a follow up patch and will be
used when Blimp is embedded by Clank.
==========

to

==========
blimp: Move compositing, input and render widget feature to client/core.

This patch integrates the BlimpCompositor, the BlimpInputManager and
the RenderWidget feature with BlimpContents framework in
blimp/client/core. In the process of moving these, it also establishes
the class structre in the client code to mirror the structure of their
counterparts on the Engine.

1) BlimpRenderWidget maps to content's render widget and maintains the
compositing and input state. BlimpInputManager is still in the
compositor, will be moved to the render widget in a follow-up patch.

2) BlimpCompositor maps to the RenderWidgetCompositor.

3) BlimpContents maps to the WebContents, and thus is the delegate for
the RenderWidgetFeature. The lifetime of the BlimpRenderWidgets on the
client is controlled by their counterparts on the engine.

This change also consolidates the compositor dependencies into a
seperate CompositorDepsProvider, with a mode for direct rendering which
is used by the standalone app to allow the BlimpCompositor to render
content directly to the native widget. Support for a mode for
delegated rendering will be added in a follow up patch and will be
used when Blimp is embedded by Clank.
==========