+aelias, +boliu as well, because this also applies equally to DelegatedFrameHostAndroid.

I happen to be deleting 50 lines of cruft from DelegatedFrameHostAndroid in https://codereview.chromium.org/2707403004 .  That will leaves the Android one pretty simple and bare-bones, so this seems like primarily a desktop issue.

Cool, thanks for the update, Alexander. I've marked  crbug.com/689754  as a blocker for this as well.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/71ca55f2b59fe14c1a803f00c2c219502fd5d9bb

commit 71ca55f2b59fe14c1a803f00c2c219502fd5d9bb
Author: samans <samans@chromium.org>
Date: Wed Mar 01 23:50:07 2017

In an effort to simplify DelegatedFrameHost, this CL moves the code for
copying latency info from old surface to new surface to cc.

This needs to be done because the code for capturing screenshots depends
on the fact that that a particular latency info reaches cc::Display. When
resizing, the latency info can remain in the old surface which is not
drawn, and screenshot times out. A previous CL of mine had this problem
and got reverted.

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

Review-Url: https://codereview.chromium.org/2721763002
Cr-Commit-Position: refs/heads/master@{#454101}

[modify] https://crrev.com/71ca55f2b59fe14c1a803f00c2c219502fd5d9bb/cc/surfaces/compositor_frame_sink_support_unittest.cc
[modify] https://crrev.com/71ca55f2b59fe14c1a803f00c2c219502fd5d9bb/cc/surfaces/surface.cc
[modify] https://crrev.com/71ca55f2b59fe14c1a803f00c2c219502fd5d9bb/cc/surfaces/surface.h
[modify] https://crrev.com/71ca55f2b59fe14c1a803f00c2c219502fd5d9bb/content/browser/renderer_host/delegated_frame_host.cc

I'm not convinced that skipping frames adds much value. I would try to delete that code. https://cs.chromium.org/chromium/src/content/browser/renderer_host/delegated_frame_host.cc?q=ShouldSkipFrame+package:%5Echromium$&l=397

EvictDelegatedFrame right before ResetCompositorFrameSinkSupport can probably go away: https://cs.chromium.org/chromium/src/content/browser/renderer_host/delegated_frame_host.cc?q=ShouldSkipFrame+package:%5Echromium$&l=429

Guttering should happen in the display compositor instead of in DelegatedFrameHost.

I'm just writing out thoughts on tasks that need to be done. Frame eviction is specialized right now. I think we want to build a generalized system that uses surface references and surface synchronization.

Hit testing logic should move out of DelegatedFrameHost.

Resize Lock should be replaced with surface synchronization.

Capture needs to be rethought. https://bugs.chromium.org/p/chromium/issues/detail?id=695429

@#10 Skipping frames was important to make sure resize happens quickly if the renderer is in high latency mode - basically discarding any frame from a previous resize in the pipeline and avoiding throttling from the parent compositor. Maybe with surface synchronization + UBF it never applies any more?

#17: Yea, that's my thinking. With UBF, we won't actually generate frames any quicker if we ACK immediately, right? That would only happen if we turn off vsync? 

I think it'd be useful to review how we resize. Right now it's completely decoupled from UBF (ViewMsg_Resize + throttling in RenderWidgetHostImpl). OTOH, when we do resize, I think it's worth considering if we want to be throttled by vsync, since input events that trigger resize in the UI are not, and so waiting for vsync would make the renderer resize slower and cause more guttering. The renderer main thread is likely to be a significant bottleneck (full relayout + full repaint) so we want to start it as soon as possible. However we could also want to extend the BeginFrame deadline to reflect the idea that we're ok waiting a bit longer for a correct frame (CompositorLock logic today, moves into the DisplayCompositor in the future).

Hm, so input throttling/interaction with the compositor is a bit tricky to reason about but..

- In the UI, we apply input events immediately to the "main-thread" side of the compositor. But they won't appear until we commit. The commit is tied to the UBF, so we will only resize the UI once per begin-frame/vsync.

- In the renderer, same idea. But if the browser sends multiple resizes to the renderer, and it ends up committing a different size than the one the UI compositor committed, we're going to gutter and be sad for no good reason.

#20: These things are largely mitigated by surface sync. The display compositor will keep the UI's CompositorFrame in a pending state UNTIL the renderer submits the appropriate CompositorFrame.

+eseckler, +brianderson. With that said, there are some subtle interactions between the display scheduler and surface sync. I'm working out the details with scheduling folks.

  if (view_ && frame.metadata.content_source_id >= current_content_source_id_) {
    view_->SubmitCompositorFrame(local_surface_id, std::move(frame));
    view_->DidReceiveRendererFrame();
  } else {
    cc::ReturnedResourceArray resources;
    cc::TransferableResource::ReturnResources(frame.resource_list, &resources);
    renderer_compositor_frame_sink_->DidReceiveCompositorFrameAck(resources);
  }

I know content_source_id is going away soon but I bet we can move this logic to CompositorFrameSinkSupport from RenderWidgetHostImpl::SubmitCompositorFrame which is in the spirit of simplifying DelegatedFrameHost.

Really, this bug is about simplifying/eliminiating about the entire:

RenderWidgetHostImpl::SubmitCompositorFrame => RenderWidgetHostViewAura::SubmitCompositorFrame => DelegatedFrameHost::SubmitCompositorFrame flow because it will not exist in Mushrome or Mus+Ash.

  if (!ui::LatencyInfo::Verify(frame.metadata.latency_info,
                               "RenderWidgetHostImpl::OnSwapCompositorFrame")) {
    std::vector<ui::LatencyInfo>().swap(frame.metadata.latency_info);
  }

...

  latency_tracker_.OnSwapCompositorFrame(&frame.metadata.latency_info);

It ought to be possible to move this code into CompositorFrameSupport? Doing so generalizes it for OOPIF, the browser, and Ash.

I'm not a fan of this validity check:

  if (local_surface_id == last_local_surface_id_ &&
      (frame_size != last_frame_size_ ||
       device_scale_factor != last_device_scale_factor_)) {
    DLOG(ERROR) << "Renderer submitted frame of wrong size to its surface."
                << " Expected: size=" << last_frame_size_.ToString()
                << ",scale=" << last_device_scale_factor_
                << " Received: size=" << frame_size.ToString()
                << ",scale=" << device_scale_factor;
    return;
  }


because it does not generalize to OOPIFs, Ash, and the browser process.

I really really wish we could add it to CompositorFrameSinkSupport.

Antoine earlier expressed a concern that rejection should happen earlier: https://codereview.chromium.org/2757953002/

I worry that all future clients: video surfaces, offscreen canvas, OOPIFs, etc and check for it. Indeed, they don't check for it today, which is a bit worrisome.

I do feel like it makes sense to have the invariant that a cc::Surface has a fixed SurfaceInfo for the entirety of its lifetime.

Maybe we don't kill the client if it submits a bad frame, but instead we skip the frame and the tell the "DisplayCompositorHost" (now called FrameSinkManagerHost, aka the browser) about it, and have it decide what to do.

Yes, it's a deep chain of methods but it seems more "right" in the sense that a Surface is a sequence of CompositorFrames that all share a set of common properties (in particular "SurfaceInfo")....WDYT Antoine

#27: Since latency info is passed to the latency tracker shortly thereafter, I'm not sure we should move this code to CompositorFrameSinkSupport.

#26: Yes, we can move that even though it feels weird, but the code for stopping the timer has to stay there.

#28: I'm a fan of also doing this check in the display compositor, but as long as DelegatedFrameHost is as complicated as it is right now, I feel safer doing this check early on.

#29: We need to move it for Mus/Mushrome/Mus+Ash/Cuttlefish/Catfish/Jellyfish/pick your cute name of the quarter, so really we need to think about how latency tracking logic should be refactored to accommodate that. 

This task is covered under other bugs already.