RenderWidgetHostViewChildFrame should use CompositorFrameSinkSupport

RenderWidgetHostViewChildFrame should use CompositorFrameSinkSupport

This CL replaces SurfaceFactory with CompositorFrameSinkSupport, which takes us
one step closer to enabling SurfaceReferences in Chrome and moving the
DisplayCompositor out of the browser process.

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

Review-Url: https://codereview.chromium.org/2688613002
Cr-Commit-Position: refs/heads/master@{#453801}
Committed: https://chromium.googlesource.com/chromium/src/+/bd3c6dd2947f52dab4b3a7c3a8448231c5a60b97

[Saman Sami, 2017-02-08 22:37:27]

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

[Saman Sami, 2017-02-08 22:37:30]

Description was changed from

==========
RenderWidgetHostViewChildFrame should use CompositorFrameSinkSupport

BUG=682336
==========

to

==========
RenderWidgetHostViewChildFrame should use CompositorFrameSinkSupport

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

[Saman Sami, 2017-02-08 22:37:31]

Description was changed from

==========
RenderWidgetHostViewChildFrame should use CompositorFrameSinkSupport

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

to

==========
RenderWidgetHostViewChildFrame should use CompositorFrameSinkSupport

BUG=682336
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[commit-bot: I haz the power, 2017-02-08 22:38:10]

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-02-08 22:41:22]

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

[commit-bot: I haz the power, 2017-02-08 22:41:23]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Saman Sami, 2017-02-08 22:48:31]

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

[commit-bot: I haz the power, 2017-02-08 22:49:35]

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-02-08 23:03:29]

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

[commit-bot: I haz the power, 2017-02-08 23:03:30]

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_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-02-08 23:30:01]

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

[commit-bot: I haz the power, 2017-02-08 23:31:00]

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-02-08 23:34:52]

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

[commit-bot: I haz the power, 2017-02-08 23:34:53]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  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-02-15 23:06:52]

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

[commit-bot: I haz the power, 2017-02-15 23:08:04]

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-02-15 23:11:57]

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

[commit-bot: I haz the power, 2017-02-15 23:12:48]

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-02-16 00:04:52]

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

[commit-bot: I haz the power, 2017-02-16 00:04:52]

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-02-16 01:03:23]

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

[commit-bot: I haz the power, 2017-02-16 01:05: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-02-16 01:28:11]

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

[commit-bot: I haz the power, 2017-02-16 01:28:24]

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

[commit-bot: I haz the power, 2017-02-16 01:29:16]

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-02-16 01:37:57]

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

[commit-bot: I haz the power, 2017-02-16 01:39:36]

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-02-16 02:22:35]

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

[commit-bot: I haz the power, 2017-02-16 02:22:36]

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

[Saman Sami, 2017-02-16 02:23:31]

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

[commit-bot: I haz the power, 2017-02-16 02:24: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-02-16 03:19:16]

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

[commit-bot: I haz the power, 2017-02-16 03:19:18]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Saman Sami, 2017-02-16 22:35:17]

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

[commit-bot: I haz the power, 2017-02-16 22:37:17]

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-02-16 23:19:21]

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

[commit-bot: I haz the power, 2017-02-16 23:19:23]

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-02-17 15:21:51]

Description was changed from

==========
RenderWidgetHostViewChildFrame should use CompositorFrameSinkSupport

BUG=682336
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
RenderWidgetHostViewChildFrame should use CompositorFrameSinkSupport

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

[Saman Sami, 2017-02-17 15:22:44]

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

[commit-bot: I haz the power, 2017-02-17 15:22:58]

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-02-17 15:39:25]

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

[commit-bot: I haz the power, 2017-02-17 15:39:27]

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

[Saman Sami, 2017-02-17 15:41:58]

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

[commit-bot: I haz the power, 2017-02-17 15:42:21]

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-02-17 15:58:13]

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

[commit-bot: I haz the power, 2017-02-17 15:58:38]

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-02-17 16:37:02]

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

[commit-bot: I haz the power, 2017-02-17 16:37:24]

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-02-17 17:15:16]

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

[commit-bot: I haz the power, 2017-02-17 17:15:18]

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

[Fady Samuel, 2017-02-17 17:27:22]

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

[Fady Samuel, 2017-02-17 17:27:23]

https://codereview.chromium.org/2688613002/diff/260001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2688613002/diff/260001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:103:
support_.reset();
This isn't necessary.

[Saman Sami, 2017-02-17 17:28:04]

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

[commit-bot: I haz the power, 2017-02-17 17:28:32]

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

[Fady Samuel, 2017-02-17 17:29:00]

https://codereview.chromium.org/2688613002/diff/260001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support_client.h (right):

https://codereview.chromium.org/2688613002/diff/260001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_client.h:21: class CC_SURFACES_EXPORT
CompositorFrameSinkSupportClient {
Undo this change.

https://codereview.chromium.org/2688613002/diff/260001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/2688613002/diff/260001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:54: public
cc::CompositorFrameSinkSupportClient {
Use NON_EXPORTED_BASE

[Saman Sami, 2017-02-17 17:31:35]

https://codereview.chromium.org/2688613002/diff/260001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support_client.h (right):

https://codereview.chromium.org/2688613002/diff/260001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_client.h:21: class CC_SURFACES_EXPORT
CompositorFrameSinkSupportClient {
On 2017/02/17 17:29:00, Fady Samuel wrote:
> Undo this change.

Acknowledged.

https://codereview.chromium.org/2688613002/diff/260001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2688613002/diff/260001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:103:
support_.reset();
On 2017/02/17 17:27:23, Fady Samuel wrote:
> This isn't necessary.

Actually, if I omit it a new CompositorFrameSinkSupport is created before the
last one is destroyed which is bad. Some dchecks will fail for registering
SurfaceFactoryClient twice.

https://codereview.chromium.org/2688613002/diff/260001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/2688613002/diff/260001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:54: public
cc::CompositorFrameSinkSupportClient {
On 2017/02/17 17:29:00, Fady Samuel wrote:
> Use NON_EXPORTED_BASE

Acknowledged.

[commit-bot: I haz the power, 2017-02-17 18:16:47]

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

[commit-bot: I haz the power, 2017-02-17 18:16:48]

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-02-23 22:46:13]

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

[commit-bot: I haz the power, 2017-02-23 22:47:24]

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-02-23 22:50:56]

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

[commit-bot: I haz the power, 2017-02-23 22:50:57]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  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-02-23 22:54:27]

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

[commit-bot: I haz the power, 2017-02-23 22:55:33]

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

[Fady Samuel, 2017-02-23 23:26:50]

https://codereview.chromium.org/2688613002/diff/320001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support_client.h (right):

https://codereview.chromium.org/2688613002/diff/320001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_client.h:21: class CC_SURFACES_EXPORT
CompositorFrameSinkSupportClient {
Undo this.

https://codereview.chromium.org/2688613002/diff/320001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/2688613002/diff/320001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:54: public
cc::CompositorFrameSinkSupportClient {
NON_EXPORTED_BASE

[Fady Samuel, 2017-02-23 23:29:31]

https://codereview.chromium.org/2688613002/diff/320001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2688613002/diff/320001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:88: void
RenderWidgetHostViewChildFrame::CreateCompositorFrameSinkSupport() {
Can you simply merge ResetCompsoitorFrameSinkSupport and
CreateCompositorFrameSinkSupport?

[Fady Samuel, 2017-02-23 23:29:31]

https://codereview.chromium.org/2688613002/diff/320001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2688613002/diff/320001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:88: void
RenderWidgetHostViewChildFrame::CreateCompositorFrameSinkSupport() {
Can you simply merge ResetCompsoitorFrameSinkSupport and
CreateCompositorFrameSinkSupport?

[commit-bot: I haz the power, 2017-02-23 23:38:55]

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

[commit-bot: I haz the power, 2017-02-23 23:38:57]

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-02-24 16:44:48]

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

[commit-bot: I haz the power, 2017-02-24 16:45:04]

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-02-24 16:46:16]

https://codereview.chromium.org/2688613002/diff/320001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2688613002/diff/320001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:88: void
RenderWidgetHostViewChildFrame::CreateCompositorFrameSinkSupport() {
On 2017/02/23 23:29:31, Fady Samuel wrote:
> Can you simply merge ResetCompsoitorFrameSinkSupport and
> CreateCompositorFrameSinkSupport?

I'm not sure how I should do that. What if I only want to reset and not create?

[Charlie Reis, 2017-02-24 17:09:37]

Description was changed from

==========
RenderWidgetHostViewChildFrame should use CompositorFrameSinkSupport

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

to

==========
RenderWidgetHostViewChildFrame should use CompositorFrameSinkSupport

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

[commit-bot: I haz the power, 2017-02-24 18:01:53]

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

[commit-bot: I haz the power, 2017-02-24 18:01:54]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2017-02-24 18:54:31]

PTAL

[Fady Samuel, 2017-02-24 19:11:58]

lgtm + nits

https://codereview.chromium.org/2688613002/diff/340001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support_client.h (right):

https://codereview.chromium.org/2688613002/diff/340001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_client.h:9: #include
"cc/surfaces/surfaces_export.h"
remove this.

https://codereview.chromium.org/2688613002/diff/340001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_client.h:21: class CC_SURFACES_EXPORT
CompositorFrameSinkSupportClient {
remove CC_SURFACES_EXPORT

https://codereview.chromium.org/2688613002/diff/340001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/2688613002/diff/340001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:54: public
cc::CompositorFrameSinkSupportClient {
NON_EXPORTED_BASE

[Saman Sami, 2017-02-24 19:18:30]

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

[Saman Sami, 2017-02-24 19:20:09]

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

[commit-bot: I haz the power, 2017-02-24 19:20:36]

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-02-24 19:23:55]

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

[commit-bot: I haz the power, 2017-02-24 19:24: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-02-24 19:25:36]

jbauman@: Please take a look.

[Saman Sami, 2017-02-24 21:28:17]

Description was changed from

==========
RenderWidgetHostViewChildFrame should use CompositorFrameSinkSupport

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

to

==========
RenderWidgetHostViewChildFrame should use CompositorFrameSinkSupport

This CL replaces SurfaceFactory with CompositorFrameSinkSupport, which takes us
one step closer to enabling SurfaceReferences in Chrome and moving the
DisplayCompositor out of the browser process.

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

[Saman Sami, 2017-02-24 21:29:47]

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

[Saman Sami, 2017-02-24 21:31:19]

jam@: Please review content/

[commit-bot: I haz the power, 2017-02-24 21:58:49]

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

[commit-bot: I haz the power, 2017-02-24 21:58:50]

Dry run: This issue passed the CQ dry run.

[jbauman, 2017-02-25 00:16:07]

https://codereview.chromium.org/2688613002/diff/380001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2688613002/diff/380001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:346: true /*
is_swap_ack */, cc::ReturnedResourceArray()));
This removes the optimization to avoid sending an unnecessary IPC to reclaim
resources if an ack would be sent later. Ideally we'd support that optimization
everywhere.

[Saman Sami, 2017-02-25 00:30:46]

https://codereview.chromium.org/2688613002/diff/380001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2688613002/diff/380001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:346: true /*
is_swap_ack */, cc::ReturnedResourceArray()));
On 2017/02/25 00:16:07, jbauman wrote:
> This removes the optimization to avoid sending an unnecessary IPC to reclaim
> resources if an ack would be sent later. Ideally we'd support that
optimization
> everywhere.

Yes, I'm aware of this. I think the solution is that CompositorFrameSinkSupport
should pass the returned resources directly to DidReceiveCompositorFrameAck
instead of calling it without a parameter and passing the resources to
ReclaimResources. This can be done in another CL, but this CL should work
properly (though not 100% optimally) without it.

[Saman Sami, 2017-02-27 16:11:32]

jbauman@: I've sent out another CL that should let us keep the optimization. If
you have any other comments about this CL please let me know. Thanks.

[jam, 2017-02-27 16:14:37]

Can creis review this? I'm not familiar with this

[jam, 2017-02-27 16:15:07]

jam@chromium.org changed reviewers:
+ creis@chromium.org

[Saman Sami, 2017-02-27 16:16:08]

+creis@: Please review content/.

[Fady Samuel, 2017-02-27 16:18:38]

On 2017/02/27 16:11:32, Saman Sami wrote:
> jbauman@: I've sent out another CL that should let us keep the optimization.
If
> you have any other comments about this CL please let me know. Thanks.

TBH, I'm not a fan of this "optimization". I'm the one who introduced it several
months ago as an attempt at cleanup. Later we felt that
DidReceiveCompositorFrameAck should go away entirely with unified BeginFrame in
place. I think we should move in that direction as opposed to matching existing
behavior. I don't think a single extra IPC per frame will have a significant
impact on performance in the interim, WDYT?

[Saman Sami, 2017-02-27 16:27:20]

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

[commit-bot: I haz the power, 2017-02-27 16:27: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-02-27 17:39:17]

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

[commit-bot: I haz the power, 2017-02-27 17:39:18]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-02-27 17:42:41]

creis@chromium.org changed reviewers:
+ lfg@chromium.org

[Charlie Reis, 2017-02-27 17:42:42]

[+lfg]

I'm happy to rubber stamp once lfg@ reviews it, since he's much more familiar
with the compositor support here than I am.

[lfg, 2017-02-27 22:58:25]

Mostly looks good, just a few questions.

https://codereview.chromium.org/2688613002/diff/400001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2688613002/diff/400001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:651: if
(support_)
Why would support_ be null?

https://codereview.chromium.org/2688613002/diff/400001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:688: if
(support_)
Same as above.

https://codereview.chromium.org/2688613002/diff/400001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/2688613002/diff/400001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:222:
std::unique_ptr<cc::CompositorFrameSinkSupport> support_;
Can this be a member instead of allocating on the constructor?

https://codereview.chromium.org/2688613002/diff/400001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:259: bool
needs_begin_frame_ = false;
Is this needed? Can't you just do host_->needs_begin_frames() ?

[Saman Sami, 2017-02-28 02:39:39]

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

[commit-bot: I haz the power, 2017-02-28 02:40:26]

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-02-28 02:45:22]

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

[commit-bot: I haz the power, 2017-02-28 02:45: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-02-28 02:47:12]

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

[commit-bot: I haz the power, 2017-02-28 02:47:58]

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-02-28 03:16:14]

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

[commit-bot: I haz the power, 2017-02-28 03:16:15]

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

[Saman Sami, 2017-02-28 19:15:25]

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

[commit-bot: I haz the power, 2017-02-28 19:16:05]

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-02-28 19:17:10]

lfg@: I replied to your comments. Please take another look.

https://codereview.chromium.org/2688613002/diff/400001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2688613002/diff/400001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:651: if
(support_)
On 2017/02/27 22:58:25, lfg wrote:
> Why would support_ be null?

I don't fully understand how RenderWidget stuff work, but I tried removing it
and now I'm getting seg faults in this method. I imagine maybe
SetCrossProcessFrameConnector(nullptr) can leave us with a null support? Here is
one of the tests that failed:
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux...

https://codereview.chromium.org/2688613002/diff/400001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:688: if
(support_)
On 2017/02/27 22:58:25, lfg wrote:
> Same as above.

The one above turned out to be necessary. I feel safer if I have this check here
too.

https://codereview.chromium.org/2688613002/diff/400001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/2688613002/diff/400001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:222:
std::unique_ptr<cc::CompositorFrameSinkSupport> support_;
On 2017/02/27 22:58:25, lfg wrote:
> Can this be a member instead of allocating on the constructor?

But it needs to be reset in some places.

https://codereview.chromium.org/2688613002/diff/400001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:259: bool
needs_begin_frame_ = false;
On 2017/02/27 22:58:25, lfg wrote:
> Is this needed? Can't you just do host_->needs_begin_frames() ?

The last patch makes this change. If it works, we can keep it that way.

[lfg, 2017-02-28 19:42:41]

On 2017/02/28 19:17:10, Saman Sami wrote:
> lfg@: I replied to your comments. Please take another look.

Thanks for looking into this, I believe that the crashes are happening because
the renderer is sending ViewHostMsg_SetNeedsBeginFrames after the
CrossProcessFrameConnector is destroyed. The CrossProcessFrameConnector is owned
by the RenderFrameProxyHost, so its lifetime is associated to the parent frame,
not the child frame.

Can you give this a try:

1. Make CompositorFrameSinkSupport a member of RenderWidgetHostViewChildFrame
and initialize it in the constructor.
2. There's no need to create/destroy CompositorFrameSinkSupport, since nothing
should ever change. You'll only need to register/unregister the
FrameSinkHierarchy.
3. After this, support_ will always exist, so no need for the if checks.

[Saman Sami, 2017-02-28 19:51:06]

On 2017/02/28 19:42:41, lfg wrote:
> On 2017/02/28 19:17:10, Saman Sami wrote:
> > lfg@: I replied to your comments. Please take another look.
> 
> Thanks for looking into this, I believe that the crashes are happening because
> the renderer is sending ViewHostMsg_SetNeedsBeginFrames after the
> CrossProcessFrameConnector is destroyed. The CrossProcessFrameConnector is
owned
> by the RenderFrameProxyHost, so its lifetime is associated to the parent
frame,
> not the child frame.
> 
> Can you give this a try:
> 
> 1. Make CompositorFrameSinkSupport a member of RenderWidgetHostViewChildFrame
> and initialize it in the constructor.
> 2. There's no need to create/destroy CompositorFrameSinkSupport, since nothing
> should ever change. You'll only need to register/unregister the
> FrameSinkHierarchy.
> 3. After this, support_ will always exist, so no need for the if checks.
Thanks for your suggestion. Currently RenderWidgetHostViewChildFrame calls
Register/UnregisterSurfaceFactoryClient in SetCrossProcessFrameConnector, which
is now done in CompositorFrameSinkSupport. This is why I felt the need to reset
and recreate the support object in SetCrossProcessFrameConnector. If it's okay
not to register/unregister (if you are not sure I can check) then I can remove
the CompositorFrameSinkSupport reset in SetCrossProcessFrameConnector, but
unfortunately we believe the reset in ProcessCompositorFrame needs to stay
because it's equivalent to SurfaceFactory::Reset, so I don't think
CompositorFrameSinkSupport can become a member variable.

[lfg, 2017-02-28 20:03:14]

On 2017/02/28 19:51:06, Saman Sami wrote:
> On 2017/02/28 19:42:41, lfg wrote:
> > On 2017/02/28 19:17:10, Saman Sami wrote:
> > > lfg@: I replied to your comments. Please take another look.
> > 
> > Thanks for looking into this, I believe that the crashes are happening
because
> > the renderer is sending ViewHostMsg_SetNeedsBeginFrames after the
> > CrossProcessFrameConnector is destroyed. The CrossProcessFrameConnector is
> owned
> > by the RenderFrameProxyHost, so its lifetime is associated to the parent
> frame,
> > not the child frame.
> > 
> > Can you give this a try:
> > 
> > 1. Make CompositorFrameSinkSupport a member of
RenderWidgetHostViewChildFrame
> > and initialize it in the constructor.
> > 2. There's no need to create/destroy CompositorFrameSinkSupport, since
nothing
> > should ever change. You'll only need to register/unregister the
> > FrameSinkHierarchy.
> > 3. After this, support_ will always exist, so no need for the if checks.
> Thanks for your suggestion. Currently RenderWidgetHostViewChildFrame calls
> Register/UnregisterSurfaceFactoryClient in SetCrossProcessFrameConnector,
which
> is now done in CompositorFrameSinkSupport. This is why I felt the need to
reset
> and recreate the support object in SetCrossProcessFrameConnector. If it's okay
> not to register/unregister (if you are not sure I can check) then I can remove
> the CompositorFrameSinkSupport reset in SetCrossProcessFrameConnector, but
> unfortunately we believe the reset in ProcessCompositorFrame needs to stay
> because it's equivalent to SurfaceFactory::Reset, so I don't think
> CompositorFrameSinkSupport can become a member variable.

I believe it should be ok to unregister the SurfaceFactoryClient in the
destructor now that I switched RWHVCF to be synchronously deleted in
Destroy(). Please, give that a try. If there are still failing tests,
we'll file a bug and I'll work on cleaning it up after.

As for the second case, can't CompositorFrameSinkSupport just implement a
Reset() function that calls SurfaceFactory::Reset() ? Why do we want to be
destroying and re-creating the object?

[Saman Sami, 2017-02-28 20:17:00]

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

[commit-bot: I haz the power, 2017-02-28 20:17: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-02-28 20:18:13]

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

[commit-bot: I haz the power, 2017-02-28 20:18:55]

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-02-28 20:23:59]

On 2017/02/28 20:03:14, lfg wrote:
> On 2017/02/28 19:51:06, Saman Sami wrote:
> > On 2017/02/28 19:42:41, lfg wrote:
> > > On 2017/02/28 19:17:10, Saman Sami wrote:
> > > > lfg@: I replied to your comments. Please take another look.
> > > 
> > > Thanks for looking into this, I believe that the crashes are happening
> because
> > > the renderer is sending ViewHostMsg_SetNeedsBeginFrames after the
> > > CrossProcessFrameConnector is destroyed. The CrossProcessFrameConnector is
> > owned
> > > by the RenderFrameProxyHost, so its lifetime is associated to the parent
> > frame,
> > > not the child frame.
> > > 
> > > Can you give this a try:
> > > 
> > > 1. Make CompositorFrameSinkSupport a member of
> RenderWidgetHostViewChildFrame
> > > and initialize it in the constructor.
> > > 2. There's no need to create/destroy CompositorFrameSinkSupport, since
> nothing
> > > should ever change. You'll only need to register/unregister the
> > > FrameSinkHierarchy.
> > > 3. After this, support_ will always exist, so no need for the if checks.
> > Thanks for your suggestion. Currently RenderWidgetHostViewChildFrame calls
> > Register/UnregisterSurfaceFactoryClient in SetCrossProcessFrameConnector,
> which
> > is now done in CompositorFrameSinkSupport. This is why I felt the need to
> reset
> > and recreate the support object in SetCrossProcessFrameConnector. If it's
okay
> > not to register/unregister (if you are not sure I can check) then I can
remove
> > the CompositorFrameSinkSupport reset in SetCrossProcessFrameConnector, but
> > unfortunately we believe the reset in ProcessCompositorFrame needs to stay
> > because it's equivalent to SurfaceFactory::Reset, so I don't think
> > CompositorFrameSinkSupport can become a member variable.
> 
> I believe it should be ok to unregister the SurfaceFactoryClient in the
> destructor now that I switched RWHVCF to be synchronously deleted in
> Destroy(). Please, give that a try. If there are still failing tests,
> we'll file a bug and I'll work on cleaning it up after.
> 
> As for the second case, can't CompositorFrameSinkSupport just implement a
> Reset() function that calls SurfaceFactory::Reset() ? Why do we want to be
> destroying and re-creating the object?

I just sent out a new patch to see how things go without resetting support in
SetCrossProcessFrameConnector.

The reason we re-create the support object is that in the future
CompositorFrameSinkSupport will live in the gpu process (See
GpuCompositorFrameSink) and on gpu restart (compositor_frame_sink_id !=
last_compositor_frame_sink_id) the support object is actually lost. We are
trying to mimic that behaviour.

[lfg, 2017-02-28 20:37:00]

lgtm

On 2017/02/28 20:23:59, Saman Sami wrote:
> The reason we re-create the support object is that in the future
> CompositorFrameSinkSupport will live in the gpu process (See
> GpuCompositorFrameSink) and on gpu restart (compositor_frame_sink_id !=
> last_compositor_frame_sink_id) the support object is actually lost. We are
> trying to mimic that behaviour.

Ok, this explains some of the unneeded complexity here. Is there a design doc
for this?

[Charlie Reis, 2017-02-28 21:00:45]

Rubber stamp LGTM, given lfg's review.  Thanks!

[commit-bot: I haz the power, 2017-02-28 21:33:57]

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

[commit-bot: I haz the power, 2017-02-28 21:33:59]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2017-03-01 01:46:21]

On 2017/02/28 21:00:45, Charlie Reis (slow) wrote:
> Rubber stamp LGTM, given lfg's review.  Thanks!

Thank you guys for the lgtms!

lfg@: I don't think I ever encountered a design doc talking about
CompositorFrameSinkSupport. This is all part of the plan to move the display
compositor to the gpu process but I don't know a good design doc about it. I
think fsamuel@ can answer your question better.

[jbauman, 2017-03-01 01:50:06]

lgtm. I guess we can live without the resource reclamation optimization for now.

[Saman Sami, 2017-03-01 01:53:28]

On 2017/03/01 01:46:21, Saman Sami wrote:
> On 2017/02/28 21:00:45, Charlie Reis (slow) wrote:
> > Rubber stamp LGTM, given lfg's review.  Thanks!
> 
> Thank you guys for the lgtms!
> 
> lfg@: I don't think I ever encountered a design doc talking about
> CompositorFrameSinkSupport. This is all part of the plan to move the display
> compositor to the gpu process but I don't know a good design doc about it. I
> think fsamuel@ can answer your question better.

jbauman@: I'm not sure if you are convinced by what Fady said. I'm going to land
this, and if you still feel like we need that optimization, please let me know
and we can have a discussion about it with Fady and we can fix it in my other CL
(https://codereview.chromium.org/2720803002/) if necessary. For now, I'll assume
things are good the way they are.

[Saman Sami, 2017-03-01 01:54:02]

On 2017/03/01 01:50:06, jbauman wrote:
> lgtm. I guess we can live without the resource reclamation optimization for
now.

Thanks! I just saw your lgtm.

[Saman Sami, 2017-03-01 01:55:29]

The CQ bit was checked by samans@chromium.org

[Saman Sami, 2017-03-01 01:55:30]

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/2688613002/#ps520001
(title: "c")

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

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-01 02:54:07]

CQ is committing da patch.

Bot data: {"patchset_id": 520001, "attempt_start_ts": 1488333329719720,
"parent_rev": "0431b6bc6c9bf5624cccf1121285392b07b74ec6", "commit_rev":
"bd3c6dd2947f52dab4b3a7c3a8448231c5a60b97"}

[commit-bot: I haz the power, 2017-03-01 02:54:46]

Description was changed from

==========
RenderWidgetHostViewChildFrame should use CompositorFrameSinkSupport

This CL replaces SurfaceFactory with CompositorFrameSinkSupport, which takes us
one step closer to enabling SurfaceReferences in Chrome and moving the
DisplayCompositor out of the browser process.

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

to

==========
RenderWidgetHostViewChildFrame should use CompositorFrameSinkSupport

This CL replaces SurfaceFactory with CompositorFrameSinkSupport, which takes us
one step closer to enabling SurfaceReferences in Chrome and moving the
DisplayCompositor out of the browser process.

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

Review-Url: https://codereview.chromium.org/2688613002
Cr-Commit-Position: refs/heads/master@{#453801}
Committed:
https://chromium.googlesource.com/chromium/src/+/bd3c6dd2947f52dab4b3a7c3a844...
==========

[commit-bot: I haz the power, 2017-03-01 02:54:48]

Committed patchset #27 (id:520001) as
https://chromium.googlesource.com/chromium/src/+/bd3c6dd2947f52dab4b3a7c3a844...