RendererCompositorFrameSink should handle local surface id allocation

RendererCompositorFrameSink should handle local surface id allocation

We are planning to use MojoCompositorFrameSink in the renderer process,
which means RendererCompositorFrameSink must be aware of LocalSurfaceId
of the surface it's submitting to.

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

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

[Saman Sami, 2017-03-03 19:26:47]

Description was changed from

==========
RendererCompositorFrameSink should take care of local surface id generation

BUG=697864
==========

to

==========
RendererCompositorFrameSink should take care of local surface id generation

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

[Saman Sami, 2017-03-03 19:27:02]

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

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

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-03 19:43:30]

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

[commit-bot: I haz the power, 2017-03-03 19:43:55]

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-03 19:54:32]

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

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

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-03-03 20:04:14]

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

[commit-bot: I haz the power, 2017-03-03 20:04:29]

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

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

[commit-bot: I haz the power, 2017-03-03 20:17:04]

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-03-03 20:25:59]

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

[commit-bot: I haz the power, 2017-03-03 20:26:23]

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-03 20:37:53]

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

[commit-bot: I haz the power, 2017-03-03 20:38:25]

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-03 20:59:08]

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

[commit-bot: I haz the power, 2017-03-03 20:59:09]

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-03-03 21:06:40]

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

[commit-bot: I haz the power, 2017-03-03 21:07:02]

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-03 21:15:35]

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

[commit-bot: I haz the power, 2017-03-03 21:15:37]

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

[Saman Sami, 2017-03-03 21:21:44]

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

[commit-bot: I haz the power, 2017-03-03 21:22: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-03-03 21:25:15]

Description was changed from

==========
RendererCompositorFrameSink should take care of local surface id generation

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

to

==========
RendererCompositorFrameSink should handle local surface id allocation

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

[commit-bot: I haz the power, 2017-03-03 21:31:56]

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

[commit-bot: I haz the power, 2017-03-03 21:31:57]

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

[Saman Sami, 2017-03-10 16:30:36]

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

[commit-bot: I haz the power, 2017-03-10 16:31:52]

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-10 16:36:24]

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

[commit-bot: I haz the power, 2017-03-10 16:36:25]

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...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
  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...)

[Saman Sami, 2017-03-10 22:46:36]

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

[commit-bot: I haz the power, 2017-03-10 22:47:11]

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-10 22:50:30]

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

[commit-bot: I haz the power, 2017-03-10 22:50:31]

Dry run: Try jobs failed on following builders:
  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-03-10 23:09:14]

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

[commit-bot: I haz the power, 2017-03-10 23:09: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-03-10 23:16:37]

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

[commit-bot: I haz the power, 2017-03-10 23:16:38]

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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  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-03-10 23:37:09]

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

[commit-bot: I haz the power, 2017-03-10 23:37:46]

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

[commit-bot: I haz the power, 2017-03-11 00:05:04]

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

[commit-bot: I haz the power, 2017-03-11 00:05:05]

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-03-13 20:55:43]

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

[commit-bot: I haz the power, 2017-03-13 20:56:41]

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-13 21:04:53]

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

[commit-bot: I haz the power, 2017-03-13 21:04:55]

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_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-03-13 21:09:24]

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

[commit-bot: I haz the power, 2017-03-13 21:10:10]

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-13 21:35:29]

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

[commit-bot: I haz the power, 2017-03-13 21:36:14]

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-13 22:34:26]

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

[commit-bot: I haz the power, 2017-03-13 22:34:27]

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-03-15 03:26:34]

Description was changed from

==========
RendererCompositorFrameSink should handle local surface id allocation

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

to

==========
RendererCompositorFrameSink should handle local surface id allocation

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

[Saman Sami, 2017-03-15 03:27:15]

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

[commit-bot: I haz the power, 2017-03-15 03:27: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-03-15 03:30:31]

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

[commit-bot: I haz the power, 2017-03-15 03:30:56]

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-15 04:33:40]

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

[commit-bot: I haz the power, 2017-03-15 04:33:41]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2017-03-15 12:05:45]

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

[Fady Samuel, 2017-03-15 12:05:48]

I agree that updating the factory in SurfaceManager is the right solution but
this needs a unit test. I recommend that change along with the unit test be done
separately, first.

https://codereview.chromium.org/2728183002/diff/320001/cc/surfaces/surface_ma...
File cc/surfaces/surface_manager.cc (right):

https://codereview.chromium.org/2728183002/diff/320001/cc/surfaces/surface_ma...
cc/surfaces/surface_manager.cc:125: surface->set_factory(surface_factory);
This is subtle and needs a unit test. Parent refers to a child, child
CompositorFrameSinkSupport goes away, and a new one is created, and the child
submits a new CompositorFrame, that ought to exercise this. I'd recommend doing
this in a separate patch.

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

https://codereview.chromium.org/2728183002/diff/320001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:353:
cc::LocalSurfaceId local_surface_id,
const cc::LocalSurfaceId&

https://codereview.chromium.org/2728183002/diff/320001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:398:
cc::LocalSurfaceId local_surface_id,
const cc::LocalSurfaceId&

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

https://codereview.chromium.org/2728183002/diff/320001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:109:
cc::LocalSurfaceId local_surface_id,
const cc::LocalSurfaceId&

https://codereview.chromium.org/2728183002/diff/320001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:194:
cc::LocalSurfaceId local_surface_id,
const cc::LocalSurfaceId&

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

https://codereview.chromium.org/2728183002/diff/320001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_guest.cc:284:
cc::LocalSurfaceId local_surface_id,
const cc::LocalSurfaceId&

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

https://codereview.chromium.org/2728183002/diff/320001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_guest.h:96:
cc::LocalSurfaceId local_surface_id,
const cc::LocalSurfaceId&

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
File content/browser/renderer_host/browser_compositor_view_mac.h (right):

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/browser_compositor_view_mac.h:65:
cc::LocalSurfaceId local_surface_id,
const cc::LocalSurfaceId&

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
File content/browser/renderer_host/browser_compositor_view_mac.mm (right):

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/browser_compositor_view_mac.mm:273:
cc::LocalSurfaceId local_surface_id,
const cc::LocalSurfaceId&

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
File content/browser/renderer_host/delegated_frame_host.cc (right):

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/delegated_frame_host.cc:375: cc::LocalSurfaceId
local_surface_id,
const cc::LocalSurfaceId&

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
File content/browser/renderer_host/delegated_frame_host.h (right):

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/delegated_frame_host.h:126: cc::LocalSurfaceId
local_surface_id,
const cc::LocalSurfaceId&

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

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1145:
cc::LocalSurfaceId local_surface_id,
const cc::LocalSurfaceId&

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1200:
cc::LocalSurfaceId local_surface_id,
const cc::LocalSurfaceId&

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.h:134:
cc::LocalSurfaceId local_surface_id,
const cc::LocalSurfaceId&

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.h:304:
cc::LocalSurfaceId local_surface_id,
const cc::LocalSurfaceId&

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:910:
cc::LocalSurfaceId local_surface_id,
const cc::LocalSurfaceId&

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.h:168:
cc::LocalSurfaceId local_surface_id,
const cc::LocalSurfaceId&

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:219:
cc::LocalSurfaceId local_surface_id,
const cc::LocalSurfaceId&

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.h (right):

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.h:308:
cc::LocalSurfaceId local_surface_id,
const cc::LocalSurfaceId&

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:1431:
cc::LocalSurfaceId local_surface_id,
const cc::LocalSurfaceId&

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

https://codereview.chromium.org/2728183002/diff/320001/content/renderer/gpu/r...
content/renderer/gpu/renderer_compositor_frame_sink.cc:116: if
(!local_surface_id_.is_valid() || frame_size != last_frame_size_ ||
As discussed offline you need all the conditions checked by
DelegatedFrameHostAndroid but most only apply to Android so maybe you can put
them behind an #ifdef. Maybe a helper method would be useful here.

https://codereview.chromium.org/2728183002/diff/320001/content/test/test_rend...
File content/test/test_render_view_host.cc (right):

https://codereview.chromium.org/2728183002/diff/320001/content/test/test_rend...
content/test/test_render_view_host.cc:176: cc::LocalSurfaceId local_surface_id,
const cc::LocalSurfaceId&

https://codereview.chromium.org/2728183002/diff/320001/content/test/test_rend...
File content/test/test_render_view_host.h (right):

https://codereview.chromium.org/2728183002/diff/320001/content/test/test_rend...
content/test/test_render_view_host.h:90: cc::LocalSurfaceId local_surface_id,
const cc::LocalSurfaceId&

https://codereview.chromium.org/2728183002/diff/320001/ui/android/delegated_f...
File ui/android/delegated_frame_host_android.cc (right):

https://codereview.chromium.org/2728183002/diff/320001/ui/android/delegated_f...
ui/android/delegated_frame_host_android.cc:81: cc::LocalSurfaceId
local_surface_id,
const cc::LocalSurfaceId&

https://codereview.chromium.org/2728183002/diff/320001/ui/android/delegated_f...
ui/android/delegated_frame_host_android.cc:86: if (!current_frame_ ||
surface_size != current_frame_->surface_size ||
This can be simplified once you move the conditions to
RendererCompositorFrameSink...

If the local_surface_id has changed then 

1. DestroyDelegatedContent();
2. content_layer_ = CreateSurfaceLayer(...);
3. AddChild(content_layer_);

https://codereview.chromium.org/2728183002/diff/320001/ui/android/delegated_f...
File ui/android/delegated_frame_host_android.h (right):

https://codereview.chromium.org/2728183002/diff/320001/ui/android/delegated_f...
ui/android/delegated_frame_host_android.h:49: void
SubmitCompositorFrame(cc::LocalSurfaceId local_surface_id,
const cc::LocalSurfaceId&

[Saman Sami, 2017-03-15 14:17:26]

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

[commit-bot: I haz the power, 2017-03-15 14:17:41]

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-15 14:23:14]

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

[commit-bot: I haz the power, 2017-03-15 14:23:15]

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-03-15 14:30:12]

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

[commit-bot: I haz the power, 2017-03-15 14:30: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-03-15 15:18:05]

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

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

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2017-03-15 20:37:15]

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

[commit-bot: I haz the power, 2017-03-15 20:38:12]

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-15 20:41:22]

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

[commit-bot: I haz the power, 2017-03-15 20:42:07]

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-15 22:04:10]

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

[commit-bot: I haz the power, 2017-03-15 22:04:11]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2017-03-16 18:33:08]

PTAL

https://codereview.chromium.org/2728183002/diff/320001/cc/surfaces/surface_ma...
File cc/surfaces/surface_manager.cc (right):

https://codereview.chromium.org/2728183002/diff/320001/cc/surfaces/surface_ma...
cc/surfaces/surface_manager.cc:125: surface->set_factory(surface_factory);
On 2017/03/15 12:05:47, Fady Samuel wrote:
> This is subtle and needs a unit test. Parent refers to a child, child
> CompositorFrameSinkSupport goes away, and a new one is created, and the child
> submits a new CompositorFrame, that ought to exercise this. I'd recommend
doing
> this in a separate patch.

Done.

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

https://codereview.chromium.org/2728183002/diff/320001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:353:
cc::LocalSurfaceId local_surface_id,
On 2017/03/15 12:05:47, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

https://codereview.chromium.org/2728183002/diff/320001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:398:
cc::LocalSurfaceId local_surface_id,
On 2017/03/15 12:05:47, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

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

https://codereview.chromium.org/2728183002/diff/320001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:109:
cc::LocalSurfaceId local_surface_id,
On 2017/03/15 12:05:47, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

https://codereview.chromium.org/2728183002/diff/320001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:194:
cc::LocalSurfaceId local_surface_id,
On 2017/03/15 12:05:47, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

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

https://codereview.chromium.org/2728183002/diff/320001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_guest.cc:284:
cc::LocalSurfaceId local_surface_id,
On 2017/03/15 12:05:47, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

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

https://codereview.chromium.org/2728183002/diff/320001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_guest.h:96:
cc::LocalSurfaceId local_surface_id,
On 2017/03/15 12:05:47, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
File content/browser/renderer_host/browser_compositor_view_mac.h (right):

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/browser_compositor_view_mac.h:65:
cc::LocalSurfaceId local_surface_id,
On 2017/03/15 12:05:47, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
File content/browser/renderer_host/browser_compositor_view_mac.mm (right):

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/browser_compositor_view_mac.mm:273:
cc::LocalSurfaceId local_surface_id,
On 2017/03/15 12:05:47, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
File content/browser/renderer_host/delegated_frame_host.cc (right):

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/delegated_frame_host.cc:375: cc::LocalSurfaceId
local_surface_id,
On 2017/03/15 12:05:48, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
File content/browser/renderer_host/delegated_frame_host.h (right):

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/delegated_frame_host.h:126: cc::LocalSurfaceId
local_surface_id,
On 2017/03/15 12:05:48, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

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

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1145:
cc::LocalSurfaceId local_surface_id,
On 2017/03/15 12:05:48, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1200:
cc::LocalSurfaceId local_surface_id,
On 2017/03/15 12:05:48, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.h:134:
cc::LocalSurfaceId local_surface_id,
On 2017/03/15 12:05:48, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.h:304:
cc::LocalSurfaceId local_surface_id,
On 2017/03/15 12:05:48, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:910:
cc::LocalSurfaceId local_surface_id,
On 2017/03/15 12:05:48, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.h:168:
cc::LocalSurfaceId local_surface_id,
On 2017/03/15 12:05:48, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:219:
cc::LocalSurfaceId local_surface_id,
On 2017/03/15 12:05:48, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.h (right):

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.h:308:
cc::LocalSurfaceId local_surface_id,
On 2017/03/15 12:05:48, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2728183002/diff/320001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:1431:
cc::LocalSurfaceId local_surface_id,
On 2017/03/15 12:05:48, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

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

https://codereview.chromium.org/2728183002/diff/320001/content/renderer/gpu/r...
content/renderer/gpu/renderer_compositor_frame_sink.cc:116: if
(!local_surface_id_.is_valid() || frame_size != last_frame_size_ ||
On 2017/03/15 12:05:48, Fady Samuel wrote:
> As discussed offline you need all the conditions checked by
> DelegatedFrameHostAndroid but most only apply to Android so maybe you can put
> them behind an #ifdef. Maybe a helper method would be useful here.

Done.

https://codereview.chromium.org/2728183002/diff/320001/content/test/test_rend...
File content/test/test_render_view_host.cc (right):

https://codereview.chromium.org/2728183002/diff/320001/content/test/test_rend...
content/test/test_render_view_host.cc:176: cc::LocalSurfaceId local_surface_id,
On 2017/03/15 12:05:48, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

https://codereview.chromium.org/2728183002/diff/320001/content/test/test_rend...
File content/test/test_render_view_host.h (right):

https://codereview.chromium.org/2728183002/diff/320001/content/test/test_rend...
content/test/test_render_view_host.h:90: cc::LocalSurfaceId local_surface_id,
On 2017/03/15 12:05:48, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

https://codereview.chromium.org/2728183002/diff/320001/ui/android/delegated_f...
File ui/android/delegated_frame_host_android.cc (right):

https://codereview.chromium.org/2728183002/diff/320001/ui/android/delegated_f...
ui/android/delegated_frame_host_android.cc:81: cc::LocalSurfaceId
local_surface_id,
On 2017/03/15 12:05:48, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

https://codereview.chromium.org/2728183002/diff/320001/ui/android/delegated_f...
ui/android/delegated_frame_host_android.cc:86: if (!current_frame_ ||
surface_size != current_frame_->surface_size ||
On 2017/03/15 12:05:48, Fady Samuel wrote:
> This can be simplified once you move the conditions to
> RendererCompositorFrameSink...
> 
> If the local_surface_id has changed then 
> 
> 1. DestroyDelegatedContent();
> 2. content_layer_ = CreateSurfaceLayer(...);
> 3. AddChild(content_layer_);

Done.

https://codereview.chromium.org/2728183002/diff/320001/ui/android/delegated_f...
File ui/android/delegated_frame_host_android.h (right):

https://codereview.chromium.org/2728183002/diff/320001/ui/android/delegated_f...
ui/android/delegated_frame_host_android.h:49: void
SubmitCompositorFrame(cc::LocalSurfaceId local_surface_id,
On 2017/03/15 12:05:48, Fady Samuel wrote:
> const cc::LocalSurfaceId&

Done.

[Saman Sami, 2017-03-16 18:51:47]

Description was changed from

==========
RendererCompositorFrameSink should handle local surface id allocation

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

to

==========
RendererCompositorFrameSink should handle local surface id allocation

We are planning to use MojoCompositorFrameSink in the renderer process,
which means RendererCompositorFrameSink must be aware of LocalSurfaceId
of the surface it's submitting to.

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

[Saman Sami, 2017-03-16 18:59:02]

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

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

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

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

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-16 20:08:34]

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

[commit-bot: I haz the power, 2017-03-16 20:08:35]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2017-03-16 23:34:13]

LGTM! Yaay! Please pass this to piman@ for overall review! Thanks! This takes us
in the right direction but the final step is for the parent to allocate the
surface ID for the child, and pass it to the child to submit the
CompositorFrame.

[Saman Sami, 2017-03-16 23:44:30]

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

[Saman Sami, 2017-03-16 23:45:44]

piman@ PTAL

The cc/ files will land separately and will be removed from the final version of
this CL.

[piman, 2017-03-17 00:05:39]

It seems almost too easy, but I don't see anything obvious that would be wrong.
One thing below though (shouldn't be too hard to deal with?)

https://codereview.chromium.org/2728183002/diff/440001/content/browser/render...
File content/browser/renderer_host/delegated_frame_host.cc (right):

https://codereview.chromium.org/2728183002/diff/440001/content/browser/render...
content/browser/renderer_host/delegated_frame_host.cc:451: if (local_surface_id
!= local_surface_id_ || !has_frame_) {
Remembering that the renderer is untrusted, can anything bad happen if the frame
size doesn't match the previous frame (or the DSF doesn't), but the local
surface id was not changed?
In particular it looks like we would be changing frame_size_in_dip (outside the
if) but not current_surface_size_/current_scale_factor_ (inside the if),
therefore being inconsistent.

Should we refuse the message in that case?

[Fady Samuel, 2017-03-17 00:33:12]

WDYT about my comment of doing the security check some place central such as
Surface::QueueFrame or SurfaceFactory::SubmitCompositorFrame or
CompositorFrameSinkSupport::SubmitCompositorFrame so the display compositor
maintains the invariant for all clients.

https://codereview.chromium.org/2728183002/diff/440001/content/browser/render...
File content/browser/renderer_host/delegated_frame_host.cc (right):

https://codereview.chromium.org/2728183002/diff/440001/content/browser/render...
content/browser/renderer_host/delegated_frame_host.cc:451: if (local_surface_id
!= local_surface_id_ || !has_frame_) {
On 2017/03/17 00:05:39, piman wrote:
> Remembering that the renderer is untrusted, can anything bad happen if the
frame
> size doesn't match the previous frame (or the DSF doesn't), but the local
> surface id was not changed?
> In particular it looks like we would be changing frame_size_in_dip (outside
the
> if) but not current_surface_size_/current_scale_factor_ (inside the if),
> therefore being inconsistent.
> 
> Should we refuse the message in that case?

Can we make that change in surface.cc? I'd rather Surface maintain the invariant
that the size and device scale factor stays the same for the lifetime of the
surface, and if it changes then we reject the frame, and tell the display
compositor host (the browser / window server) which can then decide to kill the
process or something.

This way all clients automagically get this security measure not just
DelegatedFrameHost.

[piman, 2017-03-17 17:40:26]

https://codereview.chromium.org/2728183002/diff/440001/content/browser/render...
File content/browser/renderer_host/delegated_frame_host.cc (right):

https://codereview.chromium.org/2728183002/diff/440001/content/browser/render...
content/browser/renderer_host/delegated_frame_host.cc:451: if (local_surface_id
!= local_surface_id_ || !has_frame_) {
On 2017/03/17 00:33:12, Fady Samuel wrote:
> On 2017/03/17 00:05:39, piman wrote:
> > Remembering that the renderer is untrusted, can anything bad happen if the
> frame
> > size doesn't match the previous frame (or the DSF doesn't), but the local
> > surface id was not changed?
> > In particular it looks like we would be changing frame_size_in_dip (outside
> the
> > if) but not current_surface_size_/current_scale_factor_ (inside the if),
> > therefore being inconsistent.
> > 
> > Should we refuse the message in that case?
> 
> Can we make that change in surface.cc? I'd rather Surface maintain the
invariant
> that the size and device scale factor stays the same for the lifetime of the
> surface, and if it changes then we reject the frame, and tell the display
> compositor host (the browser / window server) which can then decide to kill
the
> process or something.
> 
> This way all clients automagically get this security measure not just
> DelegatedFrameHost.

I'm not sure what "making that change in surface.cc" means really. If we refuse
the message, we should do so before doing anything in response (e.g. changing
state, calling clients, etc.), i.e. before we even get here where surfaces are
involved.

[Saman Sami, 2017-03-20 19:24:26]

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

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

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

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

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

[Saman Sami, 2017-03-20 19:58:49]

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

[commit-bot: I haz the power, 2017-03-20 19:59: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-03-20 20:14:31]

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

[commit-bot: I haz the power, 2017-03-20 20:14:32]

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-03-20 20:33:42]

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

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

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-20 20:45:04]

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

[commit-bot: I haz the power, 2017-03-20 20:45:46]

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

[commit-bot: I haz the power, 2017-03-20 21:16:05]

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

[commit-bot: I haz the power, 2017-03-20 21:16:07]

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_...)
  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-03-20 21:22:25]

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

[commit-bot: I haz the power, 2017-03-20 21:22:57]

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-20 21:30:12]

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

[commit-bot: I haz the power, 2017-03-20 21:31: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-03-20 22:16:21]

PTAL
I added code for rejecting messages when the renderer does not allocate a new
local surface id when it should.

[Saman Sami, 2017-03-20 22:17:46]

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

[Saman Sami, 2017-03-20 22:18:23]

tsepez@: Please review IPC.

[Saman Sami, 2017-03-20 22:21:50]

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

[Saman Sami, 2017-03-20 22:25:31]

boliu@: Please review *android* and RendererCompositorFrameSink

[piman, 2017-03-20 22:51:45]

LGTM % nits / question

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

https://codereview.chromium.org/2728183002/diff/580001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1834: NOTREACHED();
nit: not NOTREACHED because it can be reached from a malicious renderer.
DLOG(ERROR), and/or bad_message::ReceivedBadMessage is appropriate.

https://codereview.chromium.org/2728183002/diff/580001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1846: NOTREACHED();
nit: ditto

https://codereview.chromium.org/2728183002/diff/580001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1849:
last_local_surface_id_ = local_surface_id;
To confirm: is checking the last local_surface_id enough? I.e. if a renderer
tries to quickly ping-pong between 2 local_surface_id with arbitrary sizes/DSF,
we won't try to reference an old version of the surface, right?

[boliu, 2017-03-20 23:06:01]

I don't own RendererCompositorFrameSink, although it lgtm. And android lgtm %
comment

https://codereview.chromium.org/2728183002/diff/580001/ui/android/delegated_f...
File ui/android/delegated_frame_host_android.h (right):

https://codereview.chromium.org/2728183002/diff/580001/ui/android/delegated_f...
ui/android/delegated_frame_host_android.h:100: bool has_transparent_background_;
this is not initialized

[commit-bot: I haz the power, 2017-03-20 23:10:00]

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

[commit-bot: I haz the power, 2017-03-20 23:10:01]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2017-03-20 23:20:41]

LGTM, given the discussion we've had in the past about uniquenes of surface IDs.

[Saman Sami, 2017-03-21 02:10:25]

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

https://codereview.chromium.org/2728183002/diff/580001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1849:
last_local_surface_id_ = local_surface_id;
On 2017/03/20 22:51:45, piman wrote:
> To confirm: is checking the last local_surface_id enough? I.e. if a renderer
> tries to quickly ping-pong between 2 local_surface_id with arbitrary
sizes/DSF,
> we won't try to reference an old version of the surface, right?

Good question. If the renderer alternates between two local surface ids then
it's possible that we submit frames of different size to the same surface, but
should we care? I'm not sure how much we should care if a client is trying to
sabotage its own surfaces.

[Saman Sami, 2017-03-21 16:31:57]

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

[commit-bot: I haz the power, 2017-03-21 16:32: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-03-21 16:33:38]

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

https://codereview.chromium.org/2728183002/diff/580001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1834: NOTREACHED();
On 2017/03/20 22:51:45, piman wrote:
> nit: not NOTREACHED because it can be reached from a malicious renderer.
> DLOG(ERROR), and/or bad_message::ReceivedBadMessage is appropriate.

Done.

https://codereview.chromium.org/2728183002/diff/580001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1846: NOTREACHED();
On 2017/03/20 22:51:45, piman wrote:
> nit: ditto

Done.

https://codereview.chromium.org/2728183002/diff/580001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1849:
last_local_surface_id_ = local_surface_id;
On 2017/03/20 22:51:45, piman wrote:
> To confirm: is checking the last local_surface_id enough? I.e. if a renderer
> tries to quickly ping-pong between 2 local_surface_id with arbitrary
sizes/DSF,
> we won't try to reference an old version of the surface, right?

I guess my point is that we cared about two consecutive frames because if the
renderer doesn't update its LocalSurfaceId properly, DelegatedFrameHost can be
left in an inconsistent state, but in your ping-pong case this will not happen.

https://codereview.chromium.org/2728183002/diff/580001/ui/android/delegated_f...
File ui/android/delegated_frame_host_android.h (right):

https://codereview.chromium.org/2728183002/diff/580001/ui/android/delegated_f...
ui/android/delegated_frame_host_android.h:100: bool has_transparent_background_;
On 2017/03/20 23:06:01, boliu wrote:
> this is not initialized

Done.

[commit-bot: I haz the power, 2017-03-21 16:56:56]

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

[commit-bot: I haz the power, 2017-03-21 16:56:58]

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

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

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

[commit-bot: I haz the power, 2017-03-21 17:02: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-03-21 18:15:21]

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

[commit-bot: I haz the power, 2017-03-21 18:15:22]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2017-03-21 18:15:54]

The CQ bit was checked by samans@chromium.org

[Saman Sami, 2017-03-21 18:15:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, tsepez@chromium.org, boliu@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2728183002/#ps620001
(title: "c")

[commit-bot: I haz the power, 2017-03-21 18:16:20]

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-21 18:43:25]

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

[commit-bot: I haz the power, 2017-03-21 18:43:30]

Failed to apply patch for content/renderer/gpu/renderer_compositor_frame_sink.h:
While running git apply --index -p1;
  error: patch failed: content/renderer/gpu/renderer_compositor_frame_sink.h:103
  error: content/renderer/gpu/renderer_compositor_frame_sink.h: patch does not
apply

Patch:       content/renderer/gpu/renderer_compositor_frame_sink.h
Index: content/renderer/gpu/renderer_compositor_frame_sink.h
diff --git a/content/renderer/gpu/renderer_compositor_frame_sink.h
b/content/renderer/gpu/renderer_compositor_frame_sink.h
index
d81fb36d5e9cb43282b5617c64bf11a4c24a1ee8..3bf2ccbcd479a8380ce6107fa208817a8bbdd2a8
100644
--- a/content/renderer/gpu/renderer_compositor_frame_sink.h
+++ b/content/renderer/gpu/renderer_compositor_frame_sink.h
@@ -17,11 +17,15 @@
 #include "base/threading/platform_thread.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
+#include "cc/input/selection.h"
 #include "cc/output/begin_frame_args.h"
 #include "cc/output/compositor_frame_sink.h"
 #include "cc/scheduler/begin_frame_source.h"
+#include "cc/surfaces/local_surface_id.h"
+#include "cc/surfaces/local_surface_id_allocator.h"
 #include "content/renderer/gpu/compositor_forwarding_message_filter.h"
 #include "ipc/ipc_sync_message_filter.h"
+#include "ui/gfx/selection_bound.h"
 
 namespace IPC {
 class Message;
@@ -94,6 +98,9 @@ class RendererCompositorFrameSink
                                     const cc::ReturnedResourceArray&
resources);
   bool Send(IPC::Message* message);
 
+  bool ShouldAllocateNewLocalSurfaceId(const cc::CompositorFrame& frame);
+  void UpdateFrameData(const cc::CompositorFrame& frame);
+
   scoped_refptr<CompositorForwardingMessageFilter>
       compositor_frame_sink_filter_;
   CompositorForwardingMessageFilter::Handler
@@ -103,6 +110,22 @@ class RendererCompositorFrameSink
   scoped_refptr<FrameSwapMessageQueue> frame_swap_message_queue_;
   std::unique_ptr<cc::BeginFrameSource> begin_frame_source_;
   int routing_id_;
+
+  cc::LocalSurfaceId local_surface_id_;
+  cc::LocalSurfaceIdAllocator id_allocator_;
+  struct {
+    gfx::Size frame_size;
+    float device_scale_factor;
+#ifdef OS_ANDROID
+    float top_controls_height;
+    float top_controls_shown_ratio;
+    float bottom_controls_height;
+    float bottom_controls_shown_ratio;
+    cc::Selection<gfx::SelectionBound> viewport_selection;
+    bool has_transparent_background;
+#endif
+  } current_frame_data_;
+
   base::ThreadChecker thread_checker_;
 };

[Saman Sami, 2017-03-21 19:11:41]

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

[commit-bot: I haz the power, 2017-03-21 19:12:29]

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-21 19:44:39]

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

[commit-bot: I haz the power, 2017-03-21 19:44:41]

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-03-21 20:26:42]

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

[commit-bot: I haz the power, 2017-03-21 20:27: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-03-21 21:10:46]

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

[commit-bot: I haz the power, 2017-03-21 21:10:47]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2017-03-21 21:24:57]

The CQ bit was checked by samans@chromium.org

[Saman Sami, 2017-03-21 21:24:58]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
fsamuel@chromium.org, boliu@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2728183002/#ps640001
(title: "rebase")

[commit-bot: I haz the power, 2017-03-21 21:25:48]

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-21 21:36:32]

CQ is committing da patch.

Bot data: {"patchset_id": 640001, "attempt_start_ts": 1490131497609940,
"parent_rev": "424dfda9499d7db5034a9312d85126aa0d91b1c1", "commit_rev":
"e7398c55e00a085cc03f10e16f3f42fe37f980ac"}

[commit-bot: I haz the power, 2017-03-21 21:37:25]

Description was changed from

==========
RendererCompositorFrameSink should handle local surface id allocation

We are planning to use MojoCompositorFrameSink in the renderer process,
which means RendererCompositorFrameSink must be aware of LocalSurfaceId
of the surface it's submitting to.

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

to

==========
RendererCompositorFrameSink should handle local surface id allocation

We are planning to use MojoCompositorFrameSink in the renderer process,
which means RendererCompositorFrameSink must be aware of LocalSurfaceId
of the surface it's submitting to.

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

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

[commit-bot: I haz the power, 2017-03-21 21:37:27]

Committed patchset #33 (id:640001) as
https://chromium.googlesource.com/chromium/src/+/e7398c55e00a085cc03f10e16f3f...