cc/blimp: Add synchronization for scroll/scale state.

blimp/client/core/compositor/blimp_compositor.cc

Index: blimp/client/core/compositor/blimp_compositor.cc
diff --git a/blimp/client/core/compositor/blimp_compositor.cc b/blimp/client/core/compositor/blimp_compositor.cc
index 2866c44682eebb89be95e2cab08c6f1c05543d3c..e8158356aa946a40465e9fb11383bda3345ca947 100644
--- a/blimp/client/core/compositor/blimp_compositor.cc
+++ b/blimp/client/core/compositor/blimp_compositor.cc
@@ -87,10 +87,7 @@ BlimpCompositor::BlimpCompositor(
             ->CreateClientPictureCache();
     compositor_state_deserializer_ =
         base::MakeUnique<cc::CompositorStateDeserializer>(
-            host_.get(), std::move(client_picture_cache),
-            base::Bind(&BlimpCompositor::LayerScrolled,
-                       weak_ptr_factory_.GetWeakPtr()),
-            this);
+            host_.get(), std::move(client_picture_cache), this);
   }
 }
 
@@ -121,6 +118,12 @@ void BlimpCompositor::NotifyWhenDonePendingCommits(base::Closure callback) {
 }
 
 void BlimpCompositor::UpdateLayerTreeHost() {
+  DCHECK(use_threaded_layer_tree_host_);
+
+  // UpdateLayerTreeHost marks the end of reporting of any deltas from the impl
+  // thread. So send a client state update if the local state was modified now.
+  SendClientStateUpdateIfPossible();
+
   if (pending_frame_update_) {
     DCHECK(use_threaded_layer_tree_host_);
     compositor_state_deserializer_->DeserializeCompositorUpdate(
@@ -130,6 +133,22 @@ void BlimpCompositor::UpdateLayerTreeHost() {
     frame_ack.set_frame_ack(true);
     client_->SendCompositorMessage(frame_ack);
   }
+
+  // Send back any deltas that have not yet been resolved on the main thread
+  // back to the impl thread.
+  compositor_state_deserializer_->SendUnappliedDeltasToLayerTreeHost();
+}
+
+void BlimpCompositor::ApplyViewportDeltas(
+    const gfx::Vector2dF& inner_delta,
+    const gfx::Vector2dF& outer_delta,
+    const gfx::Vector2dF& elastic_overscroll_delta,
+    float page_scale,
+    float top_controls_delta) {
+  DCHECK(use_threaded_layer_tree_host_);
+  compositor_state_deserializer_->ApplyViewportDeltas(
+      inner_delta, outer_delta, elastic_overscroll_delta, page_scale,
+      top_controls_delta);
 }
 
 void BlimpCompositor::RequestNewCompositorFrameSink() {
@@ -149,6 +168,10 @@ void BlimpCompositor::DidInitializeCompositorFrameSink() {
 void BlimpCompositor::DidCommitAndDrawFrame() {
   BlimpStats::GetInstance()->Add(BlimpStats::COMMIT, 1);
 
+  // TODO(khushalsagar): Fix before landing, the code here assumes that every

[aelias_OOO_until_Jul13, 2016/10/26 04:04:12]

Why are there extra commit, is this because the acks count as a commit?  Perhaps
they ought to be a separate message type that doesn't cause a CC commit per se? 
(This problem is actually the Blimp analogue of aborted commits...)

[Khushal, 2016/10/26 19:02:35]

Yeah, its because what would have aborted commits in the general case, a state
update from the impl thread, now always leads to a commit because we have to
send unresolved deltas back. I don't think it should be that expensive though,
since we shouldn't have a tree sync, push properties will only be for layers
with scroll changes, and we shouldn't need to rebuild property trees either.

And I think the code here is using this outstanding_commits_ logic to know when
a frame received from the engine was drawn, so it can make a copy request on the
Layer after that.

May be the easier way to do this is to use SwapPromises?

[aelias_OOO_until_Jul13, 2016/10/27 02:09:55]

Indeed, aborted commits are something of a microoptimization so I don't think
it's mandatory for Blimp to have them.  Your overall commit rate is likely an
order of magnitude lower than a local renderer anyway.
Yes, SwapPromises sounds like a good fit for that.

+  // commit came from the engine. :X.
+  return;
+
   DCHECK_GT(outstanding_commits_, 0U);
   outstanding_commits_--;
 
@@ -174,16 +197,29 @@ void BlimpCompositor::OnCompositorMessageReceived(
   }
 
   DCHECK(use_threaded_layer_tree_host_);
-  DCHECK(message->has_layer_tree_host())
-      << "The engine only sends frame updates";
-  DCHECK(!pending_frame_update_)
-      << "We should have only a single frame in flight";
-
-  UMA_HISTOGRAM_MEMORY_KB("Blimp.Compositor.CommitSizeKb",
-                          (float)message->ByteSize() / 1024);
-  pending_frame_update_ = std::move(message);
-  outstanding_commits_++;
-  host_->SetNeedsAnimate();
+  cc::proto::CompositorMessage* message_received = message.get();
+
+  if (message_received->has_layer_tree_host()) {
+    DCHECK(!pending_frame_update_)
+        << "We should have only a single frame in flight";
+
+    UMA_HISTOGRAM_MEMORY_KB("Blimp.Compositor.CommitSizeKb",
+                            (float)message->ByteSize() / 1024);
+    pending_frame_update_ = std::move(message);
+    outstanding_commits_++;
+    host_->SetNeedsAnimate();
+  }
+
+  if (message_received->client_state_update_ack()) {
+    DCHECK(client_state_update_ack_pending_);
+
+    client_state_update_ack_pending_ = false;
+    compositor_state_deserializer_->DidApplyStateUpdatesOnEngine();
+
+    // If there are any updates that we have queued because we were waiting for
+    // an ack, send them now.
+    SendClientStateUpdateIfPossible();
+  }
 }
 
 void BlimpCompositor::HandleCompositorMessageToImpl(
@@ -322,22 +358,28 @@ void BlimpCompositor::ReturnResources(
           proxy_client_, resources));
 }
 
-bool BlimpCompositor::ShouldRetainClientScroll(
-    int engine_layer_id,
-    const gfx::ScrollOffset& new_offset) {
-  // TODO(khushalsagar): Update when adding scroll/scale sync. See
-  // crbug.com/648442.
-  return true;
+void BlimpCompositor::DidUpdateLocalState() {
+  client_state_modified_ = true;
 }
 
-bool BlimpCompositor::ShouldRetainClientPageScale(float new_page_scale) {
-  // TODO(khushalsagar): Update when adding scroll/scale sync. See
-  // crbug.com/648442.
-  return true;
-}
+void BlimpCompositor::SendClientStateUpdateIfPossible() {

[aelias_OOO_until_Jul13, 2016/10/26 04:04:12]

I don't really think "IfPossible" method names give useful information, I'd
suggest just "FlushClientState"

[Khushal, 2016/10/26 19:02:35]

Done.

+  // If the client state has not been modified, we don't need to send an update.
+  if (!client_state_modified_)
+    return;
 
-void BlimpCompositor::LayerScrolled(int engine_layer_id) {
-  DCHECK(use_threaded_layer_tree_host_);
+  // If we had sent an update and an ack for it is still pending, we can't send
+  // another update till the ack is received.
+  if (client_state_update_ack_pending_)
+    return;
+
+  cc::proto::CompositorMessage message;
+  message.set_frame_ack(false);
+  compositor_state_deserializer_->PullClientStateUpdate(
+      message.mutable_client_state_update());
+
+  client_state_modified_ = false;
+  client_state_update_ack_pending_ = true;
+  client_->SendCompositorMessage(message);
 }
 
 CompositorDependencies* BlimpCompositor::GetEmbedderDeps() {