RendererCompositorFrameSink should receive BeginFrame messages

RendererCompositorFrameSink should receive BeginFrame messages

Currently, BeginFrame messsages are routed to 
CompositorExternalBeginFrameSource. Once we switch to 
MojoCompositorFrameSinkClient, we can't have ReclaimResources
messages go to one class and BeginFrame messages go to another. 
RendererCompositorFrameSink must handle all BeginFrame-related messages.

BUG=704971
Review-Url: https://codereview.chromium.org/2773023002
Cr-Commit-Position: refs/heads/master@{#459859}
Committed: https://chromium.googlesource.com/chromium/src/+/09812d36f72be34ba5e68441222750b4b0a6b3a9

[Saman Sami, 2017-03-24 02:37:54]

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

[commit-bot: I haz the power, 2017-03-24 02:38:09]

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-03-24 02:52:00]

Patchset #1 (id:1) has been deleted

[Saman Sami, 2017-03-24 02:52:05]

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

[commit-bot: I haz the power, 2017-03-24 02:52:32]

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-03-24 03:11:36]

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

[commit-bot: I haz the power, 2017-03-24 03:11:56]

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-03-24 03:19:42]

Description was changed from

==========
Remove CompositorExternalBeginFrameSource
==========

to

==========
Remove CompositorExternalBeginFrameSource

Once we switch to MojoCompositorFrameSinkClient, we can't have ReclaimResources
messages go to one class and BeginFrame messages go to another. 
RendererCompositorFrameSink must handle all BeginFrame-related messages.
==========

[commit-bot: I haz the power, 2017-03-24 04:13:34]

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

[commit-bot: I haz the power, 2017-03-24 04:13:34]

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-03-24 04:40:37]

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

[commit-bot: I haz the power, 2017-03-24 04:40:59]

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-03-24 06:09:32]

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

[commit-bot: I haz the power, 2017-03-24 06:09:33]

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-03-24 18:32:28]

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

[commit-bot: I haz the power, 2017-03-24 18:34: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-03-24 18:40:38]

Description was changed from

==========
Remove CompositorExternalBeginFrameSource

Once we switch to MojoCompositorFrameSinkClient, we can't have ReclaimResources
messages go to one class and BeginFrame messages go to another. 
RendererCompositorFrameSink must handle all BeginFrame-related messages.
==========

to

==========
RendererCompositorFrameSink should receive BeginFrame messages

Once we switch to MojoCompositorFrameSinkClient, we can't have ReclaimResources
messages go to one class and BeginFrame messages go to another. 
RendererCompositorFrameSink must handle all BeginFrame-related messages.

BUG=704971
==========

[Saman Sami, 2017-03-24 18:40:55]

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

[Saman Sami, 2017-03-24 18:41:38]

PTAL

[Saman Sami, 2017-03-24 18:42:24]

Description was changed from

==========
RendererCompositorFrameSink should receive BeginFrame messages

Once we switch to MojoCompositorFrameSinkClient, we can't have ReclaimResources
messages go to one class and BeginFrame messages go to another. 
RendererCompositorFrameSink must handle all BeginFrame-related messages.

BUG=704971
==========

to

==========
RendererCompositorFrameSink should receive BeginFrame messages

Currently, BeginFrame messsages are routed to 
CompositorExternalBeginFrameSource. Once we switch to 
MojoCompositorFrameSinkClient, we can't have ReclaimResources
messages go to one class and BeginFrame messages go to another. 
RendererCompositorFrameSink must handle all BeginFrame-related messages.

BUG=704971
==========

[Fady Samuel, 2017-03-24 18:51:49]

lgtm

[Saman Sami, 2017-03-24 18:53:44]

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

[Saman Sami, 2017-03-24 18:54:52]

enne@ and piman@ Please review all files. Thanks.

[Fady Samuel, 2017-03-24 21:30:31]

https://codereview.chromium.org/2773023002/diff/80001/content/renderer/gpu/re...
File content/renderer/gpu/renderer_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2773023002/diff/80001/content/renderer/gpu/re...
content/renderer/gpu/renderer_compositor_frame_sink.cc:157:
IPC_MESSAGE_HANDLER(ViewMsg_BeginFrame, OnBeginFrame)
Where was this handled previously?

[Saman Sami, 2017-03-24 21:37:16]

https://codereview.chromium.org/2773023002/diff/80001/content/renderer/gpu/re...
File content/renderer/gpu/renderer_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2773023002/diff/80001/content/renderer/gpu/re...
content/renderer/gpu/renderer_compositor_frame_sink.cc:157:
IPC_MESSAGE_HANDLER(ViewMsg_BeginFrame, OnBeginFrame)
On 2017/03/24 21:30:31, Fady Samuel wrote:
> Where was this handled previously?

Here
https://cs.chromium.org/chromium/src/content/renderer/gpu/compositor_external...
We used to pass a CompositorExternalBeginFrameSource to
RendererCompositorFrameSink instead of having a cc::ExternalBeginFrameSource in
RendererCompositorFrameSink.

[Saman Sami, 2017-03-24 21:42:18]

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

[commit-bot: I haz the power, 2017-03-24 21:42:49]

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-03-24 22:04:55]

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

[commit-bot: I haz the power, 2017-03-24 22:04:56]

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

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

[commit-bot: I haz the power, 2017-03-25 00:06:19]

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-03-25 01:00:41]

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

[commit-bot: I haz the power, 2017-03-25 01:00:42]

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2017-03-27 00:03:03]

https://codereview.chromium.org/2773023002/diff/100001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2773023002/diff/100001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1994: :
CreateExternalBeginFrameSource(routing_id);
Why is Android different here in that it's using a
CompositorExternalBeginFrameSource still, but everything else is using an
ExternalBeginFrameSource with RenderCompositorFrameSink as its client?

I guess my expectation here would be that everything would use
RenderCompositorFrameSink instead of CompositorExternalBeginFrameSource here and
that class could be removed.  Can you help me understand?

[Saman Sami, 2017-03-27 01:53:04]

https://codereview.chromium.org/2773023002/diff/100001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2773023002/diff/100001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1994: :
CreateExternalBeginFrameSource(routing_id);
On 2017/03/27 00:03:03, enne wrote:
> Why is Android different here in that it's using a
> CompositorExternalBeginFrameSource still, but everything else is using an
> ExternalBeginFrameSource with RenderCompositorFrameSink as its client?
> 
> I guess my expectation here would be that everything would use
> RenderCompositorFrameSink instead of CompositorExternalBeginFrameSource here
and
> that class could be removed.  Can you help me understand?

We can also have an ExternalBeginFrameSource inside
SynchronousCompositorFrameSink and get rid of
CompositorExternalBeginFrameSource, but this doesn't really concern me. My goal
is using Mojo in RendererCompositorFrameSink which requires receiving
BeginFrames directly.  I don't know enough about Android to tell why
SynchronousCompositorFrameSink exists at all and why we don't always use
RendererCompositorFrameSink.

[enne (OOO), 2017-03-27 16:46:01]

lgtm

https://codereview.chromium.org/2773023002/diff/100001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2773023002/diff/100001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1994: :
CreateExternalBeginFrameSource(routing_id);
On 2017/03/27 at 01:53:03, Saman Sami wrote:
> On 2017/03/27 00:03:03, enne wrote:
> > Why is Android different here in that it's using a
> > CompositorExternalBeginFrameSource still, but everything else is using an
> > ExternalBeginFrameSource with RenderCompositorFrameSink as its client?
> > 
> > I guess my expectation here would be that everything would use
> > RenderCompositorFrameSink instead of CompositorExternalBeginFrameSource here
and
> > that class could be removed.  Can you help me understand?
> 
> We can also have an ExternalBeginFrameSource inside
SynchronousCompositorFrameSink and get rid of
CompositorExternalBeginFrameSource, but this doesn't really concern me. My goal
is using Mojo in RendererCompositorFrameSink which requires receiving
BeginFrames directly.  I don't know enough about Android to tell why
SynchronousCompositorFrameSink exists at all and why we don't always use
RendererCompositorFrameSink.

Yeah, I was mostly just curious what the Android story was.  It sounds like
you're not sure.

This patch seems fine to land as-is, though.

[Saman Sami, 2017-03-27 17:03:47]

Thanks for the lgtm!
piman@ PTAL

[piman, 2017-03-27 18:09:54]

lgtm

[Saman Sami, 2017-03-27 18:11:26]

The CQ bit was checked by samans@chromium.org

[Saman Sami, 2017-03-27 18:11:26]

The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2773023002/#ps100001
(title: "Fixed android")

[commit-bot: I haz the power, 2017-03-27 18:12:10]

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

[commit-bot: I haz the power, 2017-03-27 19:51:54]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490638286498010,
"parent_rev": "75221e53b71cc5eb37cbfb957124025259aa5dab", "commit_rev":
"6d2cc4fb8ec6589b3b915e0977579eab65734b81"}

[commit-bot: I haz the power, 2017-03-27 19:57:46]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490638286498010,
"parent_rev": "1ac3e1c6f0c4f1a6806dc8296b829ed84662c5db", "commit_rev":
"09812d36f72be34ba5e68441222750b4b0a6b3a9"}

[commit-bot: I haz the power, 2017-03-27 19:58:34]

Description was changed from

==========
RendererCompositorFrameSink should receive BeginFrame messages

Currently, BeginFrame messsages are routed to 
CompositorExternalBeginFrameSource. Once we switch to 
MojoCompositorFrameSinkClient, we can't have ReclaimResources
messages go to one class and BeginFrame messages go to another. 
RendererCompositorFrameSink must handle all BeginFrame-related messages.

BUG=704971
==========

to

==========
RendererCompositorFrameSink should receive BeginFrame messages

Currently, BeginFrame messsages are routed to 
CompositorExternalBeginFrameSource. Once we switch to 
MojoCompositorFrameSinkClient, we can't have ReclaimResources
messages go to one class and BeginFrame messages go to another. 
RendererCompositorFrameSink must handle all BeginFrame-related messages.

BUG=704971

Review-Url: https://codereview.chromium.org/2773023002
Cr-Commit-Position: refs/heads/master@{#459859}
Committed:
https://chromium.googlesource.com/chromium/src/+/09812d36f72be34ba5e684412227...
==========

[commit-bot: I haz the power, 2017-03-27 19:58:35]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/09812d36f72be34ba5e684412227...