content/blimp: Set up hooks for enabling LTHRemote in the renderer.

blimp/engine/renderer/scheduler.cc

Index: blimp/engine/renderer/scheduler.cc
diff --git a/blimp/engine/renderer/scheduler.cc b/blimp/engine/renderer/scheduler.cc
new file mode 100644
index 0000000000000000000000000000000000000000..c063e6022deee7ea72748db49937c6f07838e86b
--- /dev/null
+++ b/blimp/engine/renderer/scheduler.cc
@@ -0,0 +1,93 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "blimp/engine/renderer/scheduler.h"
+
+#include "base/logging.h"
+
+namespace blimp {
+namespace engine {
+namespace {
+// This is the temporary frame delay to keep pages which make animation requests
+// but don't mutate the state on the engine from running main frames
+// back-to-back. We need smarter throttling of engine updates. See
+// crbug.com/597829.
+constexpr base::TimeDelta kDefaultFrameDelay =
+    base::TimeDelta::FromMilliseconds(30);
+}  // namespace
+
+Scheduler::Scheduler(scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+                     SchedulerClient* client)
+    : Scheduler(kDefaultFrameDelay, std::move(task_runner), client) {}
+
+Scheduler::Scheduler(base::TimeDelta frame_delay,
+                     scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+                     SchedulerClient* client)
+    : next_frame_time_(base::TimeTicks::Now()),
+      frame_delay_(frame_delay),
+      client_(client) {
+  DCHECK(client_);
+  frame_tick_timer_.SetTaskRunner(std::move(task_runner));
+}
+
+Scheduler::~Scheduler() = default;
+
+void Scheduler::SetNeedsFrameUpdate() {
+  if (needs_frame_update_)
+    return;
+
+  needs_frame_update_ = true;
+  ScheduleMainFrameIfNecessary();
+}
+
+void Scheduler::DidSendFrameUpdateToClient() {
+  DCHECK(!frame_ack_pending_) << "We can have only frame update in flight";

[Wez, 2016/10/19 22:01:34]

nit: "...only one frame..."

That said, I'd recommend not bothering with additional log text unless there's
actually interested data to log - if this DCHECK fires then the stack identifies
the condition pretty clearly, doesn't it?

[Khushal, 2016/10/20 01:29:33]

The reason why this is an error is because the Scheduler is implementing the
protocol policy of having only one frame with a pending ack at a time. It nice
to have a helpful log that makes the error that we have here explicit.

+
+  frame_ack_pending_ = true;
+  ScheduleMainFrameIfNecessary();
+}
+
+void Scheduler::DidReceiveFrameUpdateAck() {
+  DCHECK(frame_ack_pending_);
+
+  frame_ack_pending_ = false;
+  ScheduleMainFrameIfNecessary();
+}
+
+void Scheduler::ScheduleMainFrameIfNecessary() {
+  // If we can't produce main frame updates right now, don't schedule a task.
+  if (!CanProduceMainFrames()) {
+    return;
+  }
+
+  // If a task has already been scheduled, we don't need to schedule again.
+  if (frame_tick_timer_.IsRunning())
+    return;
+
+  base::TimeDelta delay = next_frame_time_ - base::TimeTicks::Now();
+  frame_tick_timer_.Start(
+      FROM_HERE, delay,
+      base::Bind(&Scheduler::StartMainFrame, base::Unretained(this)));
+}
+
+bool Scheduler::CanProduceMainFrames() const {
+  return needs_frame_update_ && !frame_ack_pending_;

[Wez, 2016/10/19 22:01:34]

This logic doesn't seem to match the function name; the function name is
*Can*ProduceMainFrames, whereas the logic is "did the caller explicitly indicate
that they want a frame, AND did the last frame we produced get ACK'd" - i.e. not
whether we *can* produce a frame, but whether we *should*, in some sense.

Renaming the function to ReadyForNextFrameUpdate() might help clarify the intent
here.

[Khushal, 2016/10/20 01:29:33]

I just renamed it to ShouldProduceMainFrames, that looks clear enough.

+}
+
+void Scheduler::StartMainFrame() {
+  DCHECK(needs_frame_update_);
+
+  // If an update was sent to the client since this request, we can not start
+  // another frame. Early out here, we'll come back when an Ack is received from
+  // the client.
+  if (frame_ack_pending_)
+    return;
+
+  needs_frame_update_ = false;
+  next_frame_time_ = base::TimeTicks::Now() + frame_delay_;

[Wez, 2016/10/19 22:15:27]

nit: Is this the desired next-frame-time, bearing in mind that StartMainFrame
may be triggered in response to SetNeedsFrameUpdate OR
DidReceiveFrameUpdateAck()?  i.e. if we have a continuously updating page, and a
30ms frame-delay, and a 15ms round-trip delay then we'll actually get frames
generated every 45ms, not every 30ms.

[Khushal, 2016/10/20 01:29:33]

Yeah, but if it is triggered in response to DidReceiveFrameUpdateAck, the delay
used will still be based on the time when the last frame was produced.

The |next_frame_time_| is set each time we start a frame update. If we sent an
update at 0ms, the next frame time will be 30 ms. Now for a round trip of 15 ms,
we get an ack at 15ms. The delay used for scheduling the task for starting the
frame will be |next_frame_time_| - base::TimeTicks::Now(), i.e., 30 ms - 15ms =
15ms. So the frame will still kick-off at 30 ms.

Even if the round trip is longer than the frame delay itself. For instance, for
a round trip of 45 ms, the timer will run StartMainFrame at 30ms, early out
because we'll have the frame ack pending and not update the |next_frame_time_|.
Now when we get the ack at 45 ms, the delay for starting the frame will be, 30ms
- 45ms = -15ms, which will schedule the task with no delay. So this should make
sure that there is at least a 30 ms delay between any 2 frames, in the case
where the network round trip is lower than that, or if running main frames is
not resulting in any updates to the client at all.

[Wez, 2016/10/20 23:53:27]

Acknowledged.

+  client_->StartFrameUpdate();
+}
+
+}  // namespace engine
+}  // namespace blimp