[cc] Pass on BeginFrameAcks from CompositorEBFS through RWHVAura, DFH.

content/renderer/gpu/compositor_external_begin_frame_source.cc

Index: content/renderer/gpu/compositor_external_begin_frame_source.cc
diff --git a/content/renderer/gpu/compositor_external_begin_frame_source.cc b/content/renderer/gpu/compositor_external_begin_frame_source.cc
index 90f5554ba3915d1deb71674f38f7eaaed7716598..23b5d5402b283af9fd8ea06277dae87c8fda6763 100644
--- a/content/renderer/gpu/compositor_external_begin_frame_source.cc
+++ b/content/renderer/gpu/compositor_external_begin_frame_source.cc
@@ -55,6 +55,12 @@ void CompositorExternalBeginFrameSource::RemoveObserver(
   external_begin_frame_source_.RemoveObserver(obs);
 }
 
+void CompositorExternalBeginFrameSource::DidFinishFrame(
+    cc::BeginFrameObserver* obs,
+    const cc::BeginFrameAck& ack) {
+  external_begin_frame_source_.DidFinishFrame(obs, ack);
+}
+
 bool CompositorExternalBeginFrameSource::IsThrottled() const {
   return true;
 }
@@ -66,7 +72,9 @@ void CompositorExternalBeginFrameSource::OnNeedsBeginFrames(
 
 void CompositorExternalBeginFrameSource::OnDidFinishFrame(
     const cc::BeginFrameAck& ack) {
-  // TODO(eseckler): Pass on the ack to the view host.
+  // If there was damage, ViewHostMsg_SwapCompositorFrame includes the ack.
+  if (!ack.has_damage)
+    Send(new ViewHostMsg_BeginFrameDidNotSwap(routing_id_, ack));

[piman, 2017/03/14 20:06:44]

So, there are a few cases where we bundle IPC messages with
ViewHostMsg_SwapCompositorFrame, so that they arrive consistently with the frame
and we avoid transient states - see RenderWidget::QueueMessage. This is used for
example for FrameHostMsg_VisualStateResponse, ViewHostMsg_DidFirstPaintAfterLoad
or ViewHostMsg_DidFirstVisuallyNonEmptyPaint.

There is logic to make sure those messages are sent even if the compositor
doesn't generate a frame, e.g. if we canceled the commit because of no damage.

What I'm wondering is if we should do the same thing in that case as we do for
ViewHostMsg_SwapCompositorFrame, that is, bundle those queued messages together
with ViewHostMsg_BeginFrameDidNotSwap in the case we didn't do a frame, to
provide the same consistency property. It would also reduce the number of
IPCs/thread hops.

[Eric Seckler, 2017/03/15 11:56:48]

This sounds like an interesting optimization, but it's probably not
straight-forward :)

One question on my mind would be whether breaking the SwapPromise for the queued
messages is actually always accompanied by a BeginFrameDidNotSwap (for the
current BeginFrame), or if they might also be broken while a CompositorFrame is
produced by the impl thread (b/c of animations and such).

In either case, we'd have to bring the queued messages from the broken
SwapPromise into the impl thread (if they didn't get there yet) and then attach
them to the next BeginFrameDidNotSwap (or SwapCompositorFrame) IPC, right?

Would it make sense to / would we be able to do something similar for
MojoCompositorFrameSink communication?

Anyway, shall we explore this separately from this change? :)

[piman, 2017/03/15 21:17:10]

I think ideally if the compositor is making frames after the commit (that was
canceled because e.g. no updates), we ought to queue the messages with those
frames. Which we probably don't do today.
Yes I think that's the idea
We talked about this with fsamuel and others about the need to have a system to
support all this in the new world because those IPCs don't make a lot of sense
to send directly to the DisplayCompositor. See some discussion here:
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-mojo/sy4ASAZr0b...
Sure, but may be worth a todo / looping back to a bug

 }
 
 void CompositorExternalBeginFrameSource::OnMessageReceived(