Remove RendererCompositorFrameSink and use ClientCompositorFrameSink instead

Remove RendererCompositorFrameSink and use ClientCompositorFrameSink instead

RendererCompositorFrameSink and ClientCompositorFrameSink are pretty much
identical except for the LocalSurfaceId allocation logic. Extract
that logic into LocalSurfaceIdProvider and remove RendererCFS.

BUG=726485
TBR=sadrul@chromium.org,danakj@chromium.org

Review-Url: https://codereview.chromium.org/2923583002
Cr-Commit-Position: refs/heads/master@{#477065}
Committed: https://chromium.googlesource.com/chromium/src/+/7e6675cc4392ed5f41a9ebff1fa7f3f968f6374b

[Saman Sami, 2017-06-05 15:11:13]

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

[commit-bot: I haz the power, 2017-06-05 15:11:24]

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-06-05 15:31:07]

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

[commit-bot: I haz the power, 2017-06-05 15:31:18]

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-06-05 15:32:21]

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

[Fady Samuel, 2017-06-05 15:33:48]

https://codereview.chromium.org/2923583002/diff/1/components/viz/client/clien...
File components/viz/client/client_compositor_frame_sink.h (right):

https://codereview.chromium.org/2923583002/diff/1/components/viz/client/clien...
components/viz/client/client_compositor_frame_sink.h:19: class
LocalSurfaceIdProvider {
Make this a separate file?

https://codereview.chromium.org/2923583002/diff/1/components/viz/client/clien...
components/viz/client/client_compositor_frame_sink.h:25: class
DefaultLocalSurfaceIdProvider : public LocalSurfaceIdProvider {
Separate file?

https://codereview.chromium.org/2923583002/diff/1/components/viz/client/clien...
components/viz/client/client_compositor_frame_sink.h:34:
cc::LocalSurfaceIdAllocator local_surface_id_allocator_;
DISALLOW_COPY_AND_ASSIGN..

https://codereview.chromium.org/2923583002/diff/20001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2923583002/diff/20001/content/renderer/render...
content/renderer/render_thread_impl.cc:423: class RendererLocalSurfaceIdProvider
: public viz::LocalSurfaceIdProvider {
Separate file in content/renderer/gpu?

[Saman Sami, 2017-06-05 15:34:12]

Description was changed from

==========
Remove RendererCompositorFrameSink and use ClientCompositorFrameSink instead
==========

to

==========
Remove RendererCompositorFrameSink and use ClientCompositorFrameSink instead

BUG=726485
==========

[Saman Sami, 2017-06-05 15:46:27]

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

[commit-bot: I haz the power, 2017-06-05 15:46:48]

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-06-05 15:49:22]

https://codereview.chromium.org/2923583002/diff/1/components/viz/client/clien...
File components/viz/client/client_compositor_frame_sink.h (right):

https://codereview.chromium.org/2923583002/diff/1/components/viz/client/clien...
components/viz/client/client_compositor_frame_sink.h:19: class
LocalSurfaceIdProvider {
On 2017/06/05 15:33:48, Fady Samuel wrote:
> Make this a separate file?

Done.

https://codereview.chromium.org/2923583002/diff/1/components/viz/client/clien...
components/viz/client/client_compositor_frame_sink.h:25: class
DefaultLocalSurfaceIdProvider : public LocalSurfaceIdProvider {
On 2017/06/05 15:33:48, Fady Samuel wrote:
> Separate file?

I put in the same file as LocalSurfaceIdProvider.

https://codereview.chromium.org/2923583002/diff/1/components/viz/client/clien...
components/viz/client/client_compositor_frame_sink.h:34:
cc::LocalSurfaceIdAllocator local_surface_id_allocator_;
On 2017/06/05 15:33:48, Fady Samuel wrote:
> DISALLOW_COPY_AND_ASSIGN..

Done.

https://codereview.chromium.org/2923583002/diff/20001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2923583002/diff/20001/content/renderer/render...
content/renderer/render_thread_impl.cc:423: class RendererLocalSurfaceIdProvider
: public viz::LocalSurfaceIdProvider {
On 2017/06/05 15:33:48, Fady Samuel wrote:
> Separate file in content/renderer/gpu?

Hmmm. Is that better? This is the only file that needs it.

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

Description was changed from

==========
Remove RendererCompositorFrameSink and use ClientCompositorFrameSink instead

BUG=726485
==========

to

==========
Remove RendererCompositorFrameSink and use ClientCompositorFrameSink instead

RendererCompositorFrameSink and ClientCompositorFrameSink are pretty much
identical except for the LocalSurfaceId allocation logic. Extract
that logic into LocalSurfaceIdProvider and remove RendererCFS.

BUG=726485
==========

[Saman Sami, 2017-06-05 16:04:49]

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

[Saman Sami, 2017-06-05 16:07:27]

famuel please review components/viz
piman please review content/

[piman, 2017-06-05 16:56:26]

lgtm

https://codereview.chromium.org/2923583002/diff/40001/content/renderer/gpu/re...
File content/renderer/gpu/renderer_compositor_frame_sink.cc (left):

https://codereview.chromium.org/2923583002/diff/40001/content/renderer/gpu/re...
content/renderer/gpu/renderer_compositor_frame_sink.cc:79:
compositor_frame_sink_proxy_ = new RendererCompositorFrameSinkProxy(this);
nit: Can we remove RendererCompositorFrameSinkProxy and
CompositorForwardingMessageFilter now?

If we ever find that we need them again, we can always resurrect from source
control.

[Saman Sami, 2017-06-05 17:01:22]

Unfortunately, those are still used by SynchronousCompositorFrameSink.

On 2017/06/05 16:56:26, piman wrote:
> lgtm
> 
>
https://codereview.chromium.org/2923583002/diff/40001/content/renderer/gpu/re...
> File content/renderer/gpu/renderer_compositor_frame_sink.cc (left):
> 
>
https://codereview.chromium.org/2923583002/diff/40001/content/renderer/gpu/re...
> content/renderer/gpu/renderer_compositor_frame_sink.cc:79:
> compositor_frame_sink_proxy_ = new RendererCompositorFrameSinkProxy(this);
> nit: Can we remove RendererCompositorFrameSinkProxy and
> CompositorForwardingMessageFilter now?
> 
> If we ever find that we need them again, we can always resurrect from source
> control.

[piman, 2017-06-05 18:04:50]

On Mon, Jun 5, 2017 at 10:01 AM, <samans@chromium.org> wrote:

> Unfortunately, those are still used by SynchronousCompositorFrameSink.
>

How, I don't see it.  SynchronousCompositorFrameSink
uses SynchronousCompositorFilter, but that's a different thing.

>
>
> On 2017/06/05 16:56:26, piman wrote:
> > lgtm
> >
> >
> https://codereview.chromium.org/2923583002/diff/40001/
> content/renderer/gpu/renderer_compositor_frame_sink.cc
> > File content/renderer/gpu/renderer_compositor_frame_sink.cc (left):
> >
> >
> https://codereview.chromium.org/2923583002/diff/40001/
> content/renderer/gpu/renderer_compositor_frame_sink.cc#oldcode79
> > content/renderer/gpu/renderer_compositor_frame_sink.cc:79:
> > compositor_frame_sink_proxy_ = new RendererCompositorFrameSinkPro
> xy(this);
> > nit: Can we remove RendererCompositorFrameSinkProxy and
> > CompositorForwardingMessageFilter now?
> >
> > If we ever find that we need them again, we can always resurrect from
> source
> > control.
>
>
>
> https://codereview.chromium.org/2923583002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Saman Sami, 2017-06-05 18:17:31]

Sorry for lack of clarity. CompositorForwardingMessageFilter is being used by
CompositorExternalBeginFrameSource which is passed to
SynchronousCompositorFrameSink at construction. RendererCompositorFrameSinkProxy
was defined in renderer_compositor_frame_sink.h and is already gone.

On 2017/06/05 18:04:50, piman wrote:
> On Mon, Jun 5, 2017 at 10:01 AM, <mailto:samans@chromium.org> wrote:
> 
> > Unfortunately, those are still used by SynchronousCompositorFrameSink.
> >
> 
> How, I don't see it.  SynchronousCompositorFrameSink
> uses SynchronousCompositorFilter, but that's a different thing.
> 
> >
> >
> > On 2017/06/05 16:56:26, piman wrote:
> > > lgtm
> > >
> > >
> > https://codereview.chromium.org/2923583002/diff/40001/
> > content/renderer/gpu/renderer_compositor_frame_sink.cc
> > > File content/renderer/gpu/renderer_compositor_frame_sink.cc (left):
> > >
> > >
> > https://codereview.chromium.org/2923583002/diff/40001/
> > content/renderer/gpu/renderer_compositor_frame_sink.cc#oldcode79
> > > content/renderer/gpu/renderer_compositor_frame_sink.cc:79:
> > > compositor_frame_sink_proxy_ = new RendererCompositorFrameSinkPro
> > xy(this);
> > > nit: Can we remove RendererCompositorFrameSinkProxy and
> > > CompositorForwardingMessageFilter now?
> > >
> > > If we ever find that we need them again, we can always resurrect from
> > source
> > > control.
> >
> >
> >
> > https://codereview.chromium.org/2923583002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Saman Sami, 2017-06-05 18:18:22]

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

[commit-bot: I haz the power, 2017-06-05 18:18:52]

Dry run: CQ is trying da patch.

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

[chromium-reviews, 2017-06-05 18:31:14]

On Mon, Jun 5, 2017 at 11:17 AM, <samans@chromium.org> wrote:

> Sorry for lack of clarity. CompositorForwardingMessageFilter is being
> used by
> CompositorExternalBeginFrameSource which is passed to
> SynchronousCompositorFrameSink at construction.
> RendererCompositorFrameSinkProxy
> was defined in renderer_compositor_frame_sink.h and is already gone.
>

Ok, thanks for the clarification. LGTM stands.

Is there a plan to switch the SynchronousCompositorFrameSink IPC needs to
mojo?


>
> On 2017/06/05 18:04:50, piman wrote:
> > On Mon, Jun 5, 2017 at 10:01 AM, <mailto:samans@chromium.org> wrote:
> >
> > > Unfortunately, those are still used by SynchronousCompositorFrameSink.
> > >
> >
> > How, I don't see it. SynchronousCompositorFrameSink
> > uses SynchronousCompositorFilter, but that's a different thing.
> >
> > >
> > >
> > > On 2017/06/05 16:56:26, piman wrote:
> > > > lgtm
> > > >
> > > >
> > > https://codereview.chromium.org/2923583002/diff/40001/
> > > content/renderer/gpu/renderer_compositor_frame_sink.cc
> > > > File content/renderer/gpu/renderer_compositor_frame_sink.cc (left):
> > > >
> > > >
> > > https://codereview.chromium.org/2923583002/diff/40001/
> > > content/renderer/gpu/renderer_compositor_frame_sink.cc#oldcode79
> > > > content/renderer/gpu/renderer_compositor_frame_sink.cc:79:
> > > > compositor_frame_sink_proxy_ = new RendererCompositorFrameSinkPro
> > > xy(this);
> > > > nit: Can we remove RendererCompositorFrameSinkProxy and
> > > > CompositorForwardingMessageFilter now?
> > > >
> > > > If we ever find that we need them again, we can always resurrect from
> > > source
> > > > control.
> > >
> > >
> > >
> > > https://codereview.chromium.org/2923583002/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2923583002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Saman Sami, 2017-06-05 19:14:25]

We don't currently have a plan for it unfortunately. I guess eventually
everything will be mojo, but at the moment it doesn't sound like a high priority
task. If we could use MojoCompositorFrameSink the switch would be good for
unifying code paths, but SynchronousCFS is different enough that
MojoCompositorFrameSink probably doesn't work and a special mojo interface is
needed. So generally I believe it's going to be a non-trivial task without a lot
to gain from.

On 2017/06/05 18:31:14, chromium-reviews wrote:
> On Mon, Jun 5, 2017 at 11:17 AM, <mailto:samans@chromium.org> wrote:
> 
> > Sorry for lack of clarity. CompositorForwardingMessageFilter is being
> > used by
> > CompositorExternalBeginFrameSource which is passed to
> > SynchronousCompositorFrameSink at construction.
> > RendererCompositorFrameSinkProxy
> > was defined in renderer_compositor_frame_sink.h and is already gone.
> >
> 
> Ok, thanks for the clarification. LGTM stands.
> 
> Is there a plan to switch the SynchronousCompositorFrameSink IPC needs to
> mojo?
> 
> 
> >
> > On 2017/06/05 18:04:50, piman wrote:
> > > On Mon, Jun 5, 2017 at 10:01 AM, <mailto:samans@chromium.org> wrote:
> > >
> > > > Unfortunately, those are still used by SynchronousCompositorFrameSink.
> > > >
> > >
> > > How, I don't see it. SynchronousCompositorFrameSink
> > > uses SynchronousCompositorFilter, but that's a different thing.
> > >
> > > >
> > > >
> > > > On 2017/06/05 16:56:26, piman wrote:
> > > > > lgtm
> > > > >
> > > > >
> > > > https://codereview.chromium.org/2923583002/diff/40001/
> > > > content/renderer/gpu/renderer_compositor_frame_sink.cc
> > > > > File content/renderer/gpu/renderer_compositor_frame_sink.cc (left):
> > > > >
> > > > >
> > > > https://codereview.chromium.org/2923583002/diff/40001/
> > > > content/renderer/gpu/renderer_compositor_frame_sink.cc#oldcode79
> > > > > content/renderer/gpu/renderer_compositor_frame_sink.cc:79:
> > > > > compositor_frame_sink_proxy_ = new RendererCompositorFrameSinkPro
> > > > xy(this);
> > > > > nit: Can we remove RendererCompositorFrameSinkProxy and
> > > > > CompositorForwardingMessageFilter now?
> > > > >
> > > > > If we ever find that we need them again, we can always resurrect from
> > > > source
> > > > > control.
> > > >
> > > >
> > > >
> > > > https://codereview.chromium.org/2923583002/
> > > >
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups
> > > "Chromium-reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> >
> >
> > https://codereview.chromium.org/2923583002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Fady Samuel, 2017-06-05 19:17:14]

I too worry that android_webview will grow increasingly crufty over time. It
would be nice to find someone to think more about mojoification of the
android_webview, but unfortunately our team is currently focused on Chrome OS.

On 2017/06/05 19:14:25, Saman Sami wrote:
> We don't currently have a plan for it unfortunately. I guess eventually
> everything will be mojo, but at the moment it doesn't sound like a high
priority
> task. If we could use MojoCompositorFrameSink the switch would be good for
> unifying code paths, but SynchronousCFS is different enough that
> MojoCompositorFrameSink probably doesn't work and a special mojo interface is
> needed. So generally I believe it's going to be a non-trivial task without a
lot
> to gain from.
> 
> On 2017/06/05 18:31:14, chromium-reviews wrote:
> > On Mon, Jun 5, 2017 at 11:17 AM, <mailto:samans@chromium.org> wrote:
> > 
> > > Sorry for lack of clarity. CompositorForwardingMessageFilter is being
> > > used by
> > > CompositorExternalBeginFrameSource which is passed to
> > > SynchronousCompositorFrameSink at construction.
> > > RendererCompositorFrameSinkProxy
> > > was defined in renderer_compositor_frame_sink.h and is already gone.
> > >
> > 
> > Ok, thanks for the clarification. LGTM stands.
> > 
> > Is there a plan to switch the SynchronousCompositorFrameSink IPC needs to
> > mojo?
> > 
> > 
> > >
> > > On 2017/06/05 18:04:50, piman wrote:
> > > > On Mon, Jun 5, 2017 at 10:01 AM, <mailto:samans@chromium.org> wrote:
> > > >
> > > > > Unfortunately, those are still used by SynchronousCompositorFrameSink.
> > > > >
> > > >
> > > > How, I don't see it. SynchronousCompositorFrameSink
> > > > uses SynchronousCompositorFilter, but that's a different thing.
> > > >
> > > > >
> > > > >
> > > > > On 2017/06/05 16:56:26, piman wrote:
> > > > > > lgtm
> > > > > >
> > > > > >
> > > > > https://codereview.chromium.org/2923583002/diff/40001/
> > > > > content/renderer/gpu/renderer_compositor_frame_sink.cc
> > > > > > File content/renderer/gpu/renderer_compositor_frame_sink.cc (left):
> > > > > >
> > > > > >
> > > > > https://codereview.chromium.org/2923583002/diff/40001/
> > > > > content/renderer/gpu/renderer_compositor_frame_sink.cc#oldcode79
> > > > > > content/renderer/gpu/renderer_compositor_frame_sink.cc:79:
> > > > > > compositor_frame_sink_proxy_ = new RendererCompositorFrameSinkPro
> > > > > xy(this);
> > > > > > nit: Can we remove RendererCompositorFrameSinkProxy and
> > > > > > CompositorForwardingMessageFilter now?
> > > > > >
> > > > > > If we ever find that we need them again, we can always resurrect
from
> > > > > source
> > > > > > control.
> > > > >
> > > > >
> > > > >
> > > > > https://codereview.chromium.org/2923583002/
> > > > >
> > > >
> > > > --
> > > > You received this message because you are subscribed to the Google
Groups
> > > > "Chromium-reviews" group.
> > > > To unsubscribe from this group and stop receiving emails from it, send
an
> > > email
> > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> > >
> > >
> > > https://codereview.chromium.org/2923583002/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[Saman Sami, 2017-06-05 19:20:24]

samans@chromium.org changed reviewers:
+ danakj@chromium.org, sadrul@chromium.org

[Saman Sami, 2017-06-05 19:20:53]

Description was changed from

==========
Remove RendererCompositorFrameSink and use ClientCompositorFrameSink instead

RendererCompositorFrameSink and ClientCompositorFrameSink are pretty much
identical except for the LocalSurfaceId allocation logic. Extract
that logic into LocalSurfaceIdProvider and remove RendererCFS.

BUG=726485
==========

to

==========
Remove RendererCompositorFrameSink and use ClientCompositorFrameSink instead

RendererCompositorFrameSink and ClientCompositorFrameSink are pretty much
identical except for the LocalSurfaceId allocation logic. Extract
that logic into LocalSurfaceIdProvider and remove RendererCFS.

BUG=726485
TBR=sadrul@chromium.org,danakj@chromium.org
==========

[Fady Samuel, 2017-06-05 19:27:37]

lgtm

[Saman Sami, 2017-06-05 19:57:54]

The CQ bit was unchecked by samans@chromium.org

[Saman Sami, 2017-06-05 19:57:59]

The CQ bit was checked by samans@chromium.org

[Saman Sami, 2017-06-05 19:57:59]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2923583002/#ps60001
(title: "Fix windows")

[commit-bot: I haz the power, 2017-06-05 19:58: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-06-05 20:16:11]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496692679514620,
"parent_rev": "27175dace3576a3e351348cf8272c20b0d3aceec", "commit_rev":
"7e6675cc4392ed5f41a9ebff1fa7f3f968f6374b"}

[commit-bot: I haz the power, 2017-06-05 20:16:23]

Description was changed from

==========
Remove RendererCompositorFrameSink and use ClientCompositorFrameSink instead

RendererCompositorFrameSink and ClientCompositorFrameSink are pretty much
identical except for the LocalSurfaceId allocation logic. Extract
that logic into LocalSurfaceIdProvider and remove RendererCFS.

BUG=726485
TBR=sadrul@chromium.org,danakj@chromium.org
==========

to

==========
Remove RendererCompositorFrameSink and use ClientCompositorFrameSink instead

RendererCompositorFrameSink and ClientCompositorFrameSink are pretty much
identical except for the LocalSurfaceId allocation logic. Extract
that logic into LocalSurfaceIdProvider and remove RendererCFS.

BUG=726485
TBR=sadrul@chromium.org,danakj@chromium.org

Review-Url: https://codereview.chromium.org/2923583002
Cr-Commit-Position: refs/heads/master@{#477065}
Committed:
https://chromium.googlesource.com/chromium/src/+/7e6675cc4392ed5f41a9ebff1fa7...
==========

[commit-bot: I haz the power, 2017-06-05 20:16:25]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/7e6675cc4392ed5f41a9ebff1fa7...