Use MojoCompositorFrameSink in RendererCompositorFrameSink

Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's essential for MUS that all clients submit frames using Mojo. This CL gets rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.
To this end, I introduced content::mojom::FrameSinkProvider, an interface that takes 
the widget id (= legacy IPC routing id) and provides a MojoCompositorFrameSinkPtr to
be used in RendererCompositorFrameSink. The frames go to RenderWidgetHostImpl 
and are processed the same way they used to be.

Note: Due to crbug.com/709689, we keep sending BeginFrames using Chrome
IPC.

We have some performance numbers available for smoothness.top_25_smooth at
Patch Set 35. There is no metric that is consistently better than or worse than ToT.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2774373002
Cr-Commit-Position: refs/heads/master@{#463845}
Committed: https://chromium.googlesource.com/chromium/src/+/2040988b1345e6ce738bd03ab7ccd5f4b73a956b

[Saman Sami, 2017-03-27 20:19:57]

Description was changed from

==========
WIP: RendererCompositorFrameSink should use MojoCompositorFrameSink
==========

to

==========
WIP: RendererCompositorFrameSink should use MojoCompositorFrameSink
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Saman Sami, 2017-04-06 02:17:16]

Description was changed from

==========
WIP: RendererCompositorFrameSink should use MojoCompositorFrameSink
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
WIP: RendererCompositorFrameSink should use MojoCompositorFrameSink
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Saman Sami, 2017-04-06 02:35:28]

Description was changed from

==========
WIP: RendererCompositorFrameSink should use MojoCompositorFrameSink
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
WIP: Use MojoCompositorFrameSink in RendererCompositorFrameSink

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

[Saman Sami, 2017-04-06 14:54:12]

Patchset #15 (id:280001) has been deleted

[Saman Sami, 2017-04-06 17:55:43]

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

[commit-bot: I haz the power, 2017-04-06 17:56:59]

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

[Saman Sami, 2017-04-06 18:07:03]

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

[commit-bot: I haz the power, 2017-04-06 18:08:14]

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

[Saman Sami, 2017-04-06 18:28:21]

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

[commit-bot: I haz the power, 2017-04-06 18:29: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, 2017-04-06 18:48:58]

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

[commit-bot: I haz the power, 2017-04-06 18:48:59]

Dry run: Try jobs failed on following builders:
  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...)

[Saman Sami, 2017-04-06 21:47:12]

Description was changed from

==========
WIP: Use MojoCompositorFrameSink in RendererCompositorFrameSink

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

to

==========
WIP: Use MojoCompositorFrameSink in RendererCompositorFrameSink

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

[Saman Sami, 2017-04-06 21:47:31]

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

[commit-bot: I haz the power, 2017-04-06 21:49:06]

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, 2017-04-06 22:18:09]

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

[commit-bot: I haz the power, 2017-04-06 22:18:10]

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

[Saman Sami, 2017-04-07 00:27:33]

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

[commit-bot: I haz the power, 2017-04-07 00:28:05]

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, 2017-04-07 00:47:49]

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

[commit-bot: I haz the power, 2017-04-07 00:47:49]

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

[Saman Sami, 2017-04-07 01:32:41]

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

[commit-bot: I haz the power, 2017-04-07 01:33:43]

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, 2017-04-07 01:56:22]

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

[commit-bot: I haz the power, 2017-04-07 01:56:23]

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

[Saman Sami, 2017-04-07 15:57:58]

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

[commit-bot: I haz the power, 2017-04-07 15:58:18]

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, 2017-04-07 16:01:29]

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

[commit-bot: I haz the power, 2017-04-07 16:01:30]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Saman Sami, 2017-04-07 16:18:48]

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

[commit-bot: I haz the power, 2017-04-07 16:19:22]

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, 2017-04-07 16:40:23]

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

[commit-bot: I haz the power, 2017-04-07 16:40:24]

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

[Saman Sami, 2017-04-07 16:47:55]

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

[commit-bot: I haz the power, 2017-04-07 16:48:37]

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, 2017-04-07 17:11:35]

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

[commit-bot: I haz the power, 2017-04-07 17:11:36]

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

[Saman Sami, 2017-04-07 19:01:19]

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

[commit-bot: I haz the power, 2017-04-07 19:02:01]

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

[Saman Sami, 2017-04-07 19:04:35]

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

[commit-bot: I haz the power, 2017-04-07 19:05:34]

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, 2017-04-07 20:11:12]

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

[commit-bot: I haz the power, 2017-04-07 20:11:13]

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

[Saman Sami, 2017-04-08 16:26:11]

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

[commit-bot: I haz the power, 2017-04-08 16:26:23]

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, 2017-04-08 17:17:17]

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

[commit-bot: I haz the power, 2017-04-08 17:17:18]

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

[Saman Sami, 2017-04-09 01:31:26]

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

[commit-bot: I haz the power, 2017-04-09 01:31:32]

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, 2017-04-09 04:20:30]

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

[commit-bot: I haz the power, 2017-04-09 04:20:31]

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

[Saman Sami, 2017-04-09 15:09:39]

Description was changed from

==========
WIP: Use MojoCompositorFrameSink in RendererCompositorFrameSink

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

[Saman Sami, 2017-04-09 15:10:21]

samans@chromium.org changed reviewers:
+ rockot@chromium.org

[Saman Sami, 2017-04-10 02:16:19]

rockot@: I still have some work to do, mostly fixing crbug.com/709689, but this
CL seems to be functional and in a good state overall. Can you take a look
please?

[Saman Sami, 2017-04-10 02:16:55]

samans@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Saman Sami, 2017-04-10 02:18:01]

fsamuel@ PTAL

[Fady Samuel, 2017-04-10 02:21:03]

This is a big change. Please add an indepth description of what you're doing
here. What flag are you introducing and why? What changes have you made?
Summarize changes in behavior and INTENT. Thanks! This is awesome though! :D

[Saman Sami, 2017-04-10 02:21:09]

Description was changed from

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's in the core of MUS that all clients submit frames using Mojo. This CL gets
rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

[Saman Sami, 2017-04-10 02:24:58]

I might actually not land behind a flag. I think if we keep sending BeginFrames
using Chrome IPC then the regression should be gone, but I haven't tried that
yet. This CL is not fully ready so yes definitely some work is necessary but I
thought I could use some suggestions. Thanks for taking a look!

[Fady Samuel, 2017-04-10 02:28:38]

I haven't reviewed everything but it looks like you're not actually using the
switch you introduced. Could you please not remove the Chrome IPC just yet?
Thanks!

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
File content/browser/frame_sink_provider_impl.cc (right):

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
content/browser/frame_sink_provider_impl.cc:15: if (binding_.is_bound()) {
Should this be a DCHECK instead or can a malicious renderer cause this to get
called twice?

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
content/browser/frame_sink_provider_impl.cc:24: int32_t widget_id,
routing ID right?

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
File content/browser/frame_sink_provider_impl.h (right):

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
content/browser/frame_sink_provider_impl.h:13: class FrameSinkProviderImpl :
public mojom::FrameSinkProvider {
Class level description of what this does.

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
content/browser/frame_sink_provider_impl.h:15: FrameSinkProviderImpl(int32_t
process_id);
nit: explicit

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
content/browser/frame_sink_provider_impl.h:22: int32_t widget_id,
This is actually the routing_id, right?

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
content/browser/frame_sink_provider_impl.h:27: int32_t process_id_;
nit: const int32_t?

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
content/browser/frame_sink_provider_impl.h:28:
mojo::Binding<mojom::FrameSinkProvider> binding_;
DISALLOW_COPY_AND_ASSIGN...

https://codereview.chromium.org/2774373002/diff/580001/content/common/frame_s...
File content/common/frame_sink_provider.mojom (right):

https://codereview.chromium.org/2774373002/diff/580001/content/common/frame_s...
content/common/frame_sink_provider.mojom:5: interface FrameSinkProvider {
Explain what this interface is about?

https://codereview.chromium.org/2774373002/diff/580001/content/common/frame_s...
content/common/frame_sink_provider.mojom:7: int32 widget_id,
cc.mojom.MojoCompositorFrameSink& request,
This is the routing ID?

https://codereview.chromium.org/2774373002/diff/580001/content/public/app/moj...
File content/public/app/mojo/content_browser_manifest.json (right):

https://codereview.chromium.org/2774373002/diff/580001/content/public/app/moj...
content/public/app/mojo/content_browser_manifest.json:31:
"content::mojom::FrameSinkProvider",
indentation is off.

[Saman Sami, 2017-04-10 02:37:48]

I think it makes sense to get rid of Chrome IPC if I can fix the regression?
Yes, the switch is not used right now. I'll either remove it or actually use it.

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
File content/browser/frame_sink_provider_impl.cc (right):

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
content/browser/frame_sink_provider_impl.cc:15: if (binding_.is_bound()) {
On 2017/04/10 02:28:37, Fady Samuel wrote:
> Should this be a DCHECK instead or can a malicious renderer cause this to get
> called twice?

Since the request comes from the renderer, I think we shouldn't DCHECK. rockot
can provide a better answer.

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
content/browser/frame_sink_provider_impl.cc:24: int32_t widget_id,
On 2017/04/10 02:28:37, Fady Samuel wrote:
> routing ID right?

Those two are basically synonymous (I'm pretty sure I saw it used in other
places, I don't think I created this term?) and I would rather not use the term
that has Chrome IPC roots.

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
File content/browser/frame_sink_provider_impl.h (right):

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
content/browser/frame_sink_provider_impl.h:13: class FrameSinkProviderImpl :
public mojom::FrameSinkProvider {
On 2017/04/10 02:28:38, Fady Samuel wrote:
> Class level description of what this does.

Yes absolutely.

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
content/browser/frame_sink_provider_impl.h:15: FrameSinkProviderImpl(int32_t
process_id);
On 2017/04/10 02:28:38, Fady Samuel wrote:
> nit: explicit

Acknowledged.

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
content/browser/frame_sink_provider_impl.h:22: int32_t widget_id,
On 2017/04/10 02:28:38, Fady Samuel wrote:
> This is actually the routing_id, right?

Yes but see my comment in cc file please.

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
content/browser/frame_sink_provider_impl.h:27: int32_t process_id_;
On 2017/04/10 02:28:38, Fady Samuel wrote:
> nit: const int32_t?

Acknowledged.

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
content/browser/frame_sink_provider_impl.h:28:
mojo::Binding<mojom::FrameSinkProvider> binding_;
On 2017/04/10 02:28:38, Fady Samuel wrote:
> DISALLOW_COPY_AND_ASSIGN...

Acknowledged.

https://codereview.chromium.org/2774373002/diff/580001/content/public/app/moj...
File content/public/app/mojo/content_browser_manifest.json (right):

https://codereview.chromium.org/2774373002/diff/580001/content/public/app/moj...
content/public/app/mojo/content_browser_manifest.json:31:
"content::mojom::FrameSinkProvider",
On 2017/04/10 02:28:38, Fady Samuel wrote:
> indentation is off.

I lost faith in git cl format.

[Saman Sami, 2017-04-10 15:05:19]

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

[commit-bot: I haz the power, 2017-04-10 15:05:35]

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

[Saman Sami, 2017-04-10 15:05:51]

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
File content/browser/frame_sink_provider_impl.h (right):

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
content/browser/frame_sink_provider_impl.h:13: class FrameSinkProviderImpl :
public mojom::FrameSinkProvider {
On 2017/04/10 02:28:38, Fady Samuel wrote:
> Class level description of what this does.

Done.

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
content/browser/frame_sink_provider_impl.h:15: FrameSinkProviderImpl(int32_t
process_id);
On 2017/04/10 02:28:38, Fady Samuel wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
content/browser/frame_sink_provider_impl.h:27: int32_t process_id_;
On 2017/04/10 02:28:38, Fady Samuel wrote:
> nit: const int32_t?

Done.

https://codereview.chromium.org/2774373002/diff/580001/content/browser/frame_...
content/browser/frame_sink_provider_impl.h:28:
mojo::Binding<mojom::FrameSinkProvider> binding_;
On 2017/04/10 02:28:38, Fady Samuel wrote:
> DISALLOW_COPY_AND_ASSIGN...

Done.

https://codereview.chromium.org/2774373002/diff/580001/content/common/frame_s...
File content/common/frame_sink_provider.mojom (right):

https://codereview.chromium.org/2774373002/diff/580001/content/common/frame_s...
content/common/frame_sink_provider.mojom:5: interface FrameSinkProvider {
On 2017/04/10 02:28:38, Fady Samuel wrote:
> Explain what this interface is about?

Done.

https://codereview.chromium.org/2774373002/diff/580001/content/public/app/moj...
File content/public/app/mojo/content_browser_manifest.json (right):

https://codereview.chromium.org/2774373002/diff/580001/content/public/app/moj...
content/public/app/mojo/content_browser_manifest.json:31:
"content::mojom::FrameSinkProvider",
On 2017/04/10 02:28:38, Fady Samuel wrote:
> indentation is off.

Done.

[Saman Sami, 2017-04-10 15:10:01]

Description was changed from

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's in the core of MUS that all clients submit frames using Mojo. This CL gets
rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's in the core of MUS that all clients submit frames using Mojo. This CL gets
rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.
To this end, I introduced content::mojom::FrameSinkProvider, an interface that
takes 
the widget id (= legacy IPC routing id) and provides a
MojoCompositorFrameSinkPtr to
be used in RendererCompositorFrameSink. The frames currently go to
RenderWidgetHostImpl and are processed the same way they used to be.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

[Saman Sami, 2017-04-10 15:10:23]

Description was changed from

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's in the core of MUS that all clients submit frames using Mojo. This CL gets
rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.
To this end, I introduced content::mojom::FrameSinkProvider, an interface that
takes 
the widget id (= legacy IPC routing id) and provides a
MojoCompositorFrameSinkPtr to
be used in RendererCompositorFrameSink. The frames currently go to
RenderWidgetHostImpl and are processed the same way they used to be.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's in the core of MUS that all clients submit frames using Mojo. This CL gets
rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.
To this end, I introduced content::mojom::FrameSinkProvider, an interface that
takes 
the widget id (= legacy IPC routing id) and provides a
MojoCompositorFrameSinkPtr to
be used in RendererCompositorFrameSink. The frames go to RenderWidgetHostImpl 
and are processed the same way they used to be.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

[Saman Sami, 2017-04-10 15:13:09]

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

[commit-bot: I haz the power, 2017-04-10 15:13:29]

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

[Saman Sami, 2017-04-10 16:00:32]

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

[commit-bot: I haz the power, 2017-04-10 16:01:03]

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, 2017-04-10 16:19:29]

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

[commit-bot: I haz the power, 2017-04-10 16:19:30]

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

[Saman Sami, 2017-04-10 16:38:57]

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

[commit-bot: I haz the power, 2017-04-10 16:39: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, 2017-04-10 17:09:31]

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

[commit-bot: I haz the power, 2017-04-10 17:09:32]

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

[Saman Sami, 2017-04-10 17:42:07]

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

[commit-bot: I haz the power, 2017-04-10 17:42:39]

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

[Saman Sami, 2017-04-10 18:34:12]

Description was changed from

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's in the core of MUS that all clients submit frames using Mojo. This CL gets
rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.
To this end, I introduced content::mojom::FrameSinkProvider, an interface that
takes 
the widget id (= legacy IPC routing id) and provides a
MojoCompositorFrameSinkPtr to
be used in RendererCompositorFrameSink. The frames go to RenderWidgetHostImpl 
and are processed the same way they used to be.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's in the core of MUS that all clients submit frames using Mojo. This CL gets
rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.
To this end, I introduced content::mojom::FrameSinkProvider, an interface that
takes 
the widget id (= legacy IPC routing id) and provides a
MojoCompositorFrameSinkPtr to
be used in RendererCompositorFrameSink. The frames go to RenderWidgetHostImpl 
and are processed the same way they used to be.

Note: due to crbug.com/709689, we keep sending BeginFrames using Chrome
IPC.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

[Saman Sami, 2017-04-10 18:34:25]

Description was changed from

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's in the core of MUS that all clients submit frames using Mojo. This CL gets
rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.
To this end, I introduced content::mojom::FrameSinkProvider, an interface that
takes 
the widget id (= legacy IPC routing id) and provides a
MojoCompositorFrameSinkPtr to
be used in RendererCompositorFrameSink. The frames go to RenderWidgetHostImpl 
and are processed the same way they used to be.

Note: due to crbug.com/709689, we keep sending BeginFrames using Chrome
IPC.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's in the core of MUS that all clients submit frames using Mojo. This CL gets
rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.
To this end, I introduced content::mojom::FrameSinkProvider, an interface that
takes 
the widget id (= legacy IPC routing id) and provides a
MojoCompositorFrameSinkPtr to
be used in RendererCompositorFrameSink. The frames go to RenderWidgetHostImpl 
and are processed the same way they used to be.

Note: Due to crbug.com/709689, we keep sending BeginFrames using Chrome
IPC.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

[Saman Sami, 2017-04-10 19:06:36]

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

[commit-bot: I haz the power, 2017-04-10 19:07:41]

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

[Saman Sami, 2017-04-10 19:19:08]

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

[commit-bot: I haz the power, 2017-04-10 19:20:02]

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

[Saman Sami, 2017-04-10 19:53:56]

Description was changed from

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's in the core of MUS that all clients submit frames using Mojo. This CL gets
rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.
To this end, I introduced content::mojom::FrameSinkProvider, an interface that
takes 
the widget id (= legacy IPC routing id) and provides a
MojoCompositorFrameSinkPtr to
be used in RendererCompositorFrameSink. The frames go to RenderWidgetHostImpl 
and are processed the same way they used to be.

Note: Due to crbug.com/709689, we keep sending BeginFrames using Chrome
IPC.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's in the core of MUS that all clients submit frames using Mojo. This CL gets
rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.
To this end, I introduced content::mojom::FrameSinkProvider, an interface that
takes 
the widget id (= legacy IPC routing id) and provides a
MojoCompositorFrameSinkPtr to
be used in RendererCompositorFrameSink. The frames go to RenderWidgetHostImpl 
and are processed the same way they used to be.

Note: Due to crbug.com/709689, we keep sending BeginFrames using Chrome
IPC.

We have some performance number available at Patch Set 35. 

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

[Saman Sami, 2017-04-10 19:54:34]

Description was changed from

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's in the core of MUS that all clients submit frames using Mojo. This CL gets
rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.
To this end, I introduced content::mojom::FrameSinkProvider, an interface that
takes 
the widget id (= legacy IPC routing id) and provides a
MojoCompositorFrameSinkPtr to
be used in RendererCompositorFrameSink. The frames go to RenderWidgetHostImpl 
and are processed the same way they used to be.

Note: Due to crbug.com/709689, we keep sending BeginFrames using Chrome
IPC.

We have some performance number available at Patch Set 35. 

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's in the core of MUS that all clients submit frames using Mojo. This CL gets
rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.
To this end, I introduced content::mojom::FrameSinkProvider, an interface that
takes 
the widget id (= legacy IPC routing id) and provides a
MojoCompositorFrameSinkPtr to
be used in RendererCompositorFrameSink. The frames go to RenderWidgetHostImpl 
and are processed the same way they used to be.

Note: Due to crbug.com/709689, we keep sending BeginFrames using Chrome
IPC.

We have some performance numbers available at Patch Set 35. 

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

[Saman Sami, 2017-04-10 19:56:20]

Description was changed from

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's in the core of MUS that all clients submit frames using Mojo. This CL gets
rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.
To this end, I introduced content::mojom::FrameSinkProvider, an interface that
takes 
the widget id (= legacy IPC routing id) and provides a
MojoCompositorFrameSinkPtr to
be used in RendererCompositorFrameSink. The frames go to RenderWidgetHostImpl 
and are processed the same way they used to be.

Note: Due to crbug.com/709689, we keep sending BeginFrames using Chrome
IPC.

We have some performance numbers available at Patch Set 35. 

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's in the core of MUS that all clients submit frames using Mojo. This CL gets
rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.
To this end, I introduced content::mojom::FrameSinkProvider, an interface that
takes 
the widget id (= legacy IPC routing id) and provides a
MojoCompositorFrameSinkPtr to
be used in RendererCompositorFrameSink. The frames go to RenderWidgetHostImpl 
and are processed the same way they used to be.

Note: Due to crbug.com/709689, we keep sending BeginFrames using Chrome
IPC.

We have some performance numbers available for smoothness.top_25_smooth at
Patch Set 35. 

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

[Saman Sami, 2017-04-10 19:58:24]

Description was changed from

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's in the core of MUS that all clients submit frames using Mojo. This CL gets
rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.
To this end, I introduced content::mojom::FrameSinkProvider, an interface that
takes 
the widget id (= legacy IPC routing id) and provides a
MojoCompositorFrameSinkPtr to
be used in RendererCompositorFrameSink. The frames go to RenderWidgetHostImpl 
and are processed the same way they used to be.

Note: Due to crbug.com/709689, we keep sending BeginFrames using Chrome
IPC.

We have some performance numbers available for smoothness.top_25_smooth at
Patch Set 35. 

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's in the core of MUS that all clients submit frames using Mojo. This CL gets
rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.
To this end, I introduced content::mojom::FrameSinkProvider, an interface that
takes 
the widget id (= legacy IPC routing id) and provides a
MojoCompositorFrameSinkPtr to
be used in RendererCompositorFrameSink. The frames go to RenderWidgetHostImpl 
and are processed the same way they used to be.

Note: Due to crbug.com/709689, we keep sending BeginFrames using Chrome
IPC.

We have some performance numbers available for smoothness.top_25_smooth at
Patch Set 35. There is no metric that is consistently better than or worse than
ToT.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

[Fady Samuel, 2017-04-10 20:01:07]

lgtm + nits. This is awesome! I'm excited to see the progress on this front! :-)

https://codereview.chromium.org/2774373002/diff/740001/content/renderer/gpu/r...
File content/renderer/gpu/renderer_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2774373002/diff/740001/content/renderer/gpu/r...
content/renderer/gpu/renderer_compositor_frame_sink.cc:164:
IPC_MESSAGE_HANDLER(ViewMsg_BeginFrame, OnBeginFrameIPC)
nit: It's weird to have the "IPC" in there. Delete?

https://codereview.chromium.org/2774373002/diff/740001/content/test/fake_rend...
File content/test/fake_renderer_compositor_frame_sink.h (right):

https://codereview.chromium.org/2774373002/diff/740001/content/test/fake_rend...
content/test/fake_renderer_compositor_frame_sink.h:13: class
FakeRendererCompositorFrameSink
nit: Class level description?

https://codereview.chromium.org/2774373002/diff/740001/content/test/fake_rend...
content/test/fake_renderer_compositor_frame_sink.h:43: cc::ReturnedResourceArray
last_reclaimed_resources_;
nit: DISALLOW_COPY_AND_ASSIGN...

[Saman Sami, 2017-04-10 20:04:03]

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

[Saman Sami, 2017-04-10 20:05:00]

samans@chromium.org changed reviewers:
+ dcheng@chromium.org
- piman@chromium.org

[Saman Sami, 2017-04-10 20:06:06]

samans@chromium.org changed reviewers:
- dcheng@chromium.org

[Saman Sami, 2017-04-10 20:06:28]

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

[Saman Sami, 2017-04-10 20:15:01]

Thanks for the LGTM!

piman@ PTAL

https://codereview.chromium.org/2774373002/diff/740001/content/renderer/gpu/r...
File content/renderer/gpu/renderer_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2774373002/diff/740001/content/renderer/gpu/r...
content/renderer/gpu/renderer_compositor_frame_sink.cc:164:
IPC_MESSAGE_HANDLER(ViewMsg_BeginFrame, OnBeginFrameIPC)
On 2017/04/10 20:01:07, Fady Samuel wrote:
> nit: It's weird to have the "IPC" in there. Delete?

I wanted to have a different method name than the mojo method. What name do you
suggest?

[Saman Sami, 2017-04-10 20:16:29]

Description was changed from

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's in the core of MUS that all clients submit frames using Mojo. This CL gets
rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.
To this end, I introduced content::mojom::FrameSinkProvider, an interface that
takes 
the widget id (= legacy IPC routing id) and provides a
MojoCompositorFrameSinkPtr to
be used in RendererCompositorFrameSink. The frames go to RenderWidgetHostImpl 
and are processed the same way they used to be.

Note: Due to crbug.com/709689, we keep sending BeginFrames using Chrome
IPC.

We have some performance numbers available for smoothness.top_25_smooth at
Patch Set 35. There is no metric that is consistently better than or worse than
ToT.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's essential for MUS that all clients submit frames using Mojo. This CL gets
rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.
To this end, I introduced content::mojom::FrameSinkProvider, an interface that
takes 
the widget id (= legacy IPC routing id) and provides a
MojoCompositorFrameSinkPtr to
be used in RendererCompositorFrameSink. The frames go to RenderWidgetHostImpl 
and are processed the same way they used to be.

Note: Due to crbug.com/709689, we keep sending BeginFrames using Chrome
IPC.

We have some performance numbers available for smoothness.top_25_smooth at
Patch Set 35. There is no metric that is consistently better than or worse than
ToT.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

[Saman Sami, 2017-04-10 20:52:33]

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

[commit-bot: I haz the power, 2017-04-10 20:53:15]

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, 2017-04-10 22:04:29]

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

[commit-bot: I haz the power, 2017-04-10 22:04:30]

Dry run: 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_...)

[Saman Sami, 2017-04-10 22:10:35]

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

[commit-bot: I haz the power, 2017-04-10 22:11:38]

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, 2017-04-10 23:01:28]

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

[commit-bot: I haz the power, 2017-04-10 23:01:30]

Dry run: 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_...)

[Saman Sami, 2017-04-10 23:25:49]

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

[commit-bot: I haz the power, 2017-04-10 23:26:12]

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

[Ken Rockot(use gerrit already), 2017-04-10 23:31:11]

lgtm

https://codereview.chromium.org/2774373002/diff/760001/content/browser/frame_...
File content/browser/frame_sink_provider_impl.cc (right):

https://codereview.chromium.org/2774373002/diff/760001/content/browser/frame_...
content/browser/frame_sink_provider_impl.cc:15: if (binding_.is_bound())
Some surprising and unexpected behavior could result from code which
accidentally ends up binding the same provider twice. Maybe this could DCHECK
instead? And then you could add an explicit Unbind which you call from
RPHI::ProcessDied?

https://codereview.chromium.org/2774373002/diff/760001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2774373002/diff/760001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1378:
frame_sink_provider_.Bind(std::move(request));
nit: Maybe BindFrameSinkProvider is a better name than CreateFrameSinkProvider?

[piman, 2017-04-10 23:35:58]

https://codereview.chromium.org/2774373002/diff/760001/content/browser/frame_...
File content/browser/frame_sink_provider_impl.h (right):

https://codereview.chromium.org/2774373002/diff/760001/content/browser/frame_...
content/browser/frame_sink_provider_impl.h:16: class FrameSinkProviderImpl :
public mojom::FrameSinkProvider {
nit: should this be in content/browser/renderer_host?

https://codereview.chromium.org/2774373002/diff/760001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2774373002/diff/760001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:2604:
renderer_compositor_frame_sink_.get());
So, if the MojoCompositorFrameSinkClient is recreated, here, we'll call into the
view, passing the new one, while it still has a raw pointer to the old one
(which I assume is deleted on l.2601). Is that safe - is there a chance the view
might access the old one to try to return resources or something?
If so, should we keep a reference to the old bindings until after this?

https://codereview.chromium.org/2774373002/diff/760001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:2683:
renderer_compositor_frame_sink_->DidReceiveCompositorFrameAck();
Could we avoid splitting this into 2 IPCs?

https://codereview.chromium.org/2774373002/diff/760001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2774373002/diff/760001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1117:
renderer_compositor_frame_sink_->DidReceiveCompositorFrameAck();
ditto: can we coalesce the IPCs?

https://codereview.chromium.org/2774373002/diff/760001/content/renderer/gpu/r...
File content/renderer/gpu/renderer_compositor_frame_sink.cc (left):

https://codereview.chromium.org/2774373002/diff/760001/content/renderer/gpu/r...
content/renderer/gpu/renderer_compositor_frame_sink.cc:147: routing_id_,
compositor_frame_sink_id_, local_surface_id_, frame));
I worry a little bit about the expectations of ordering wrt other ViewHost
messages - unless IPCs are properly ordered wrt Mojo?
I don't have specific/concrete thing that could go wrong, but we should be on
the lookout for weird races with other "visual" things, e.g. focus, selection,
popup menus, etc.

[Saman Sami, 2017-04-11 00:09:52]

https://codereview.chromium.org/2774373002/diff/760001/content/browser/frame_...
File content/browser/frame_sink_provider_impl.cc (right):

https://codereview.chromium.org/2774373002/diff/760001/content/browser/frame_...
content/browser/frame_sink_provider_impl.cc:15: if (binding_.is_bound())
On 2017/04/10 23:31:11, Ken Rockot wrote:
> Some surprising and unexpected behavior could result from code which
> accidentally ends up binding the same provider twice. Maybe this could DCHECK
> instead? And then you could add an explicit Unbind which you call from
> RPHI::ProcessDied?

Is it appropriate to add a dcheck when a malicious renderer can trigger it?
Maybe I should just print an error and reject the request?

https://codereview.chromium.org/2774373002/diff/760001/content/browser/frame_...
File content/browser/frame_sink_provider_impl.h (right):

https://codereview.chromium.org/2774373002/diff/760001/content/browser/frame_...
content/browser/frame_sink_provider_impl.h:16: class FrameSinkProviderImpl :
public mojom::FrameSinkProvider {
On 2017/04/10 23:35:57, piman wrote:
> nit: should this be in content/browser/renderer_host?

Yes.

https://codereview.chromium.org/2774373002/diff/760001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2774373002/diff/760001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:2604:
renderer_compositor_frame_sink_.get());
That's a good point. I think it's better to do that.

https://codereview.chromium.org/2774373002/diff/760001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:2683:
renderer_compositor_frame_sink_->DidReceiveCompositorFrameAck();
On 2017/04/10 23:35:58, piman wrote:
> Could we avoid splitting this into 2 IPCs?

I proposed that, but other folks objected because DidReceiveCompositorFrameAck
is going away. I don't disagree with you though. Is the overhead of an
additional IPC significant?

https://codereview.chromium.org/2774373002/diff/760001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2774373002/diff/760001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1117:
renderer_compositor_frame_sink_->DidReceiveCompositorFrameAck();
On 2017/04/10 23:35:58, piman wrote:
> ditto: can we coalesce the IPCs?

I think things can get better if I we send the IPCs in DelegatedFrameHostAndroid
instead of here. I'll try.

https://codereview.chromium.org/2774373002/diff/760001/content/renderer/gpu/r...
File content/renderer/gpu/renderer_compositor_frame_sink.cc (left):

https://codereview.chromium.org/2774373002/diff/760001/content/renderer/gpu/r...
content/renderer/gpu/renderer_compositor_frame_sink.cc:147: routing_id_,
compositor_frame_sink_id_, local_surface_id_, frame));
Mojo doesn't preserve ordering with Chrome IPC. I understand your worry but this
is a risk that we have to take for our project because in mus the frames go to a
different process and the ordering with Chrome IPC can't be preserved. We should
definitely try to be as careful as possible before landing, but if we can't
think of any problems we should take the risk for sake of making progress in
mus. We've been doing something similar in mus+ash and I don't think folks have
noticed any problem with focus, popup, etc.

[Saman Sami, 2017-04-11 00:48:05]

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

[commit-bot: I haz the power, 2017-04-11 00:48:45]

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, 2017-04-11 02:00:00]

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

[commit-bot: I haz the power, 2017-04-11 02:00:01]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2017-04-11 04:07:53]

On 2017/04/11 at 00:09:52, samans wrote:
>
https://codereview.chromium.org/2774373002/diff/760001/content/browser/frame_...
> File content/browser/frame_sink_provider_impl.cc (right):
> 
>
https://codereview.chromium.org/2774373002/diff/760001/content/browser/frame_...
> content/browser/frame_sink_provider_impl.cc:15: if (binding_.is_bound())
> On 2017/04/10 23:31:11, Ken Rockot wrote:
> > Some surprising and unexpected behavior could result from code which
> > accidentally ends up binding the same provider twice. Maybe this could
DCHECK
> > instead? And then you could add an explicit Unbind which you call from
> > RPHI::ProcessDied?
> 
> Is it appropriate to add a dcheck when a malicious renderer can trigger it?
Maybe I should just print an error and reject the request?

A right, good point. That sounds like a better idea.

> 
>
https://codereview.chromium.org/2774373002/diff/760001/content/browser/frame_...
> File content/browser/frame_sink_provider_impl.h (right):
> 
>
https://codereview.chromium.org/2774373002/diff/760001/content/browser/frame_...
> content/browser/frame_sink_provider_impl.h:16: class FrameSinkProviderImpl :
public mojom::FrameSinkProvider {
> On 2017/04/10 23:35:57, piman wrote:
> > nit: should this be in content/browser/renderer_host?
> 
> Yes.
> 
>
https://codereview.chromium.org/2774373002/diff/760001/content/browser/render...
> File content/browser/renderer_host/render_widget_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2774373002/diff/760001/content/browser/render...
> content/browser/renderer_host/render_widget_host_impl.cc:2604:
renderer_compositor_frame_sink_.get());
> That's a good point. I think it's better to do that.
> 
>
https://codereview.chromium.org/2774373002/diff/760001/content/browser/render...
> content/browser/renderer_host/render_widget_host_impl.cc:2683:
renderer_compositor_frame_sink_->DidReceiveCompositorFrameAck();
> On 2017/04/10 23:35:58, piman wrote:
> > Could we avoid splitting this into 2 IPCs?
> 
> I proposed that, but other folks objected because DidReceiveCompositorFrameAck
is going away. I don't disagree with you though. Is the overhead of an
additional IPC significant?
> 
>
https://codereview.chromium.org/2774373002/diff/760001/content/browser/render...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://codereview.chromium.org/2774373002/diff/760001/content/browser/render...
> content/browser/renderer_host/render_widget_host_view_android.cc:1117:
renderer_compositor_frame_sink_->DidReceiveCompositorFrameAck();
> On 2017/04/10 23:35:58, piman wrote:
> > ditto: can we coalesce the IPCs?
> 
> I think things can get better if I we send the IPCs in
DelegatedFrameHostAndroid instead of here. I'll try.
> 
>
https://codereview.chromium.org/2774373002/diff/760001/content/renderer/gpu/r...
> File content/renderer/gpu/renderer_compositor_frame_sink.cc (left):
> 
>
https://codereview.chromium.org/2774373002/diff/760001/content/renderer/gpu/r...
> content/renderer/gpu/renderer_compositor_frame_sink.cc:147: routing_id_,
compositor_frame_sink_id_, local_surface_id_, frame));
> Mojo doesn't preserve ordering with Chrome IPC. I understand your worry but
this is a risk that we have to take for our project because in mus the frames go
to a different process and the ordering with Chrome IPC can't be preserved. We
should definitely try to be as careful as possible before landing, but if we
can't think of any problems we should take the risk for sake of making progress
in mus. We've been doing something similar in mus+ash and I don't think folks
have noticed any problem with focus, popup, etc.

[Saman Sami, 2017-04-11 16:09:32]

samans@chromium.org changed reviewers:
+ tsepez@chromium.org

[Saman Sami, 2017-04-11 16:09:34]

tsepez Please review IPC for security

[Saman Sami, 2017-04-11 16:13:45]

piman PTAL

https://codereview.chromium.org/2774373002/diff/760001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2774373002/diff/760001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1117:
renderer_compositor_frame_sink_->DidReceiveCompositorFrameAck();
On 2017/04/11 00:09:52, Saman Sami wrote:
> On 2017/04/10 23:35:58, piman wrote:
> > ditto: can we coalesce the IPCs?
> 
> I think things can get better if I we send the IPCs in
DelegatedFrameHostAndroid
> instead of here. I'll try.

Moving to DelegatedFrameHostAndroid doesn't help. Let's talk on slack with Fady
about this.

[Tom Sepez, 2017-04-11 17:21:53]

On 2017/04/11 16:09:34, Saman Sami wrote:
> tsepez Please review IPC for security

Sounds like there's still some ongoing work here -- ping me when it's done. 
Thanks.

[Saman Sami, 2017-04-11 17:34:10]

On 2017/04/11 17:21:53, Tom Sepez wrote:
> On 2017/04/11 16:09:34, Saman Sami wrote:
> > tsepez Please review IPC for security
> 
> Sounds like there's still some ongoing work here -- ping me when it's done. 
> Thanks.

I don't think there will be a lot of changes, but I'll ping you after piman
LGTMed so the CL is more finalized.

[Saman Sami, 2017-04-11 22:36:40]

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

[commit-bot: I haz the power, 2017-04-11 22:37:30]

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

[Saman Sami, 2017-04-11 22:40:04]

piman PTAL I merged IPCs.

[piman, 2017-04-11 22:50:21]

LGTM, thanks!

[Tom Sepez, 2017-04-11 22:58:11]

messages LGTM

[Fady Samuel, 2017-04-11 23:13:17]

On 2017/04/10 20:15:01, Saman Sami wrote:
> Thanks for the LGTM!
> 
> piman@ PTAL
> 
>
https://codereview.chromium.org/2774373002/diff/740001/content/renderer/gpu/r...
> File content/renderer/gpu/renderer_compositor_frame_sink.cc (right):
> 
>
https://codereview.chromium.org/2774373002/diff/740001/content/renderer/gpu/r...
> content/renderer/gpu/renderer_compositor_frame_sink.cc:164:
> IPC_MESSAGE_HANDLER(ViewMsg_BeginFrame, OnBeginFrameIPC)
> On 2017/04/10 20:01:07, Fady Samuel wrote:
> > nit: It's weird to have the "IPC" in there. Delete?
> 
> I wanted to have a different method name than the mojo method. What name do
you
> suggest?

Fair enough. LGTM

[saman, 2017-04-11 23:16:38]

The CQ bit was unchecked by samans@google.com

[saman, 2017-04-11 23:16:48]

The CQ bit was checked by samans@google.com

[saman, 2017-04-11 23:16:49]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2774373002/#ps820001
(title: "Rebased, dedup IPC")

[commit-bot: I haz the power, 2017-04-11 23:17:26]

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, 2017-04-11 23:58:39]

CQ is committing da patch.

Bot data: {"patchset_id": 820001, "attempt_start_ts": 1491952608755210,
"parent_rev": "51ce668aebf388516f2349a0b55bc622c14e3f4c", "commit_rev":
"2040988b1345e6ce738bd03ab7ccd5f4b73a956b"}

[commit-bot: I haz the power, 2017-04-11 23:59:36]

Description was changed from

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's essential for MUS that all clients submit frames using Mojo. This CL gets
rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.
To this end, I introduced content::mojom::FrameSinkProvider, an interface that
takes 
the widget id (= legacy IPC routing id) and provides a
MojoCompositorFrameSinkPtr to
be used in RendererCompositorFrameSink. The frames go to RenderWidgetHostImpl 
and are processed the same way they used to be.

Note: Due to crbug.com/709689, we keep sending BeginFrames using Chrome
IPC.

We have some performance numbers available for smoothness.top_25_smooth at
Patch Set 35. There is no metric that is consistently better than or worse than
ToT.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Use MojoCompositorFrameSink in RendererCompositorFrameSink

It's essential for MUS that all clients submit frames using Mojo. This CL gets
rid of
legacy IPC in RendererCompositorFrameSink and uses Mojo instead.
To this end, I introduced content::mojom::FrameSinkProvider, an interface that
takes 
the widget id (= legacy IPC routing id) and provides a
MojoCompositorFrameSinkPtr to
be used in RendererCompositorFrameSink. The frames go to RenderWidgetHostImpl 
and are processed the same way they used to be.

Note: Due to crbug.com/709689, we keep sending BeginFrames using Chrome
IPC.

We have some performance numbers available for smoothness.top_25_smooth at
Patch Set 35. There is no metric that is consistently better than or worse than
ToT.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2774373002
Cr-Commit-Position: refs/heads/master@{#463845}
Committed:
https://chromium.googlesource.com/chromium/src/+/2040988b1345e6ce738bd03ab7cc...
==========

[commit-bot: I haz the power, 2017-04-11 23:59:38]

Committed patchset #42 (id:820001) as
https://chromium.googlesource.com/chromium/src/+/2040988b1345e6ce738bd03ab7cc...