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

blimp/engine/renderer/scheduler.h

+// 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.
+
+#ifndef BLIMP_ENGINE_RENDERER_SCHEDULER_H_
+#define BLIMP_ENGINE_RENDERER_SCHEDULER_H_
+
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "base/timer/timer.h"
+
+namespace blimp {
+namespace engine {
+
+class SchedulerClient;
+
+// Responsible for scheduling frame updates sent to the client.

[Wez, 2016/10/13 16:53:52]

Can you elaborate on *how* it schedules them?  All I see here are some void()
functions and a next_frame_time() API.  How are they used?  What is the |client|
for?

[Khushal, 2016/10/13 18:47:27]

Added a comment. I don't think we need to get into the details about how exactly
the class works internally, so I mentioned the signals that it needs to make
these decisions.
What the |client| is for is document on that interface.

[Wez, 2016/10/18 01:49:42]

I'm not asking for details of how we decide when to schedule, but of how this
class is used. :)

[Khushal, 2016/10/18 05:10:03]

Ah, added a comment about using the client interface. Better?

+class Scheduler {
+ public:
+  Scheduler(scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+            SchedulerClient* client);

[Wez, 2016/10/13 16:53:52]

Who owns this? What is its lifetime?

[Khushal, 2016/10/13 18:47:27]

I don't know if there is an established pattern for this, but from what I've
seen in code, if a class takes a Delegate/Client interface in its ctor, it is
assumed that it will outlive the class. We document if a pointer can be null,
otherwise it is implicitly assumed that it can't be. And it doesn't seem of much
benefit to me to have this be stated explicitly on every class.
Happy to add a comment if you feel otherwise though.

[Wez, 2016/10/18 01:49:42]

If it is made explicit that |client| is a caller-supplied delegate then that's
fine, but I don't think that is presently the case.

[Khushal, 2016/10/18 05:10:03]

The class takes the client in its ctor, I thought that was explicit enough. Or I
guess the pattern that I'm used to seeing/using,
https://cs.chromium.org/chromium/src/blimp/engine/session/page_load_tracker.h
for instance.
Added a comment nonetheless. :)

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

If you pass a bare pointer to a function, ctor, whatever, then it isn't clear
whether it's (1) used by that function, (2) retained and used for the duration
of the receiving objects' lifetime, (3) owned by the recipient or (4)
ref-counted, (though thankfully we have ways to express #3 & #4 these days :)

Hence we need to at least clarify #1 vs #2 :)

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

I understand that in general when you take a bare pointer, you need to clarify
what the its lifetime expectations are, i.e., for how much duration are you
expecting the caller to keep that object alive.

What I meant to say was that its a very common pattern in the code base to
inject a FooClient* or FooDelegate* when constructing Foo, and I have rarely
seen comments on Foo specifying the lifetime of these interfaces in this case.
It is assumed that the delegate or client will outlive Foo and comments
generally specify when there is a divergence from this expectation.

+  virtual ~Scheduler();
+
+  void SetNeedsMainFrame();

[Wez, 2016/10/13 16:53:52]

What is a "main frame" in the context of this scheduler?

[Khushal, 2016/10/13 18:47:27]

Added a comment. I kept the word main frame mostly to stay consistent with the
nomenclature we use to refer to frames produced by blink, so it would be obvious
to anyone looking at the code as to what frame we're talking about.

[Wez, 2016/10/18 01:49:42]

Understood. However, I think it would be clearer if we had less Blink-specific
naming here, e.g. if we just had StartFrameUpdate() and SetNeedsFrame() or
ForceFrameUpdate(), for example (SetNeedsMainFrame() strictly describes what the
call does, but in spirit is not a very helpful name ;)

[Khushal, 2016/10/18 05:10:03]

Sure, I don't really have a strong preference on this, either way is good.
May be I've gotten to used to cc/blink naming. :P

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

Perhaps... :P

Can we rename this to indicate what it means, though, rather than how it
implements it?  This call MUST be made whenever there is a change to the content
that warrants generating a new frame.  So something like ScheduleFrameUpdate()
would seem more appropriate.  Similarly, I'd reword the comment to make it
explicit that this API must be called every time there is new content to render
- i.e. that the scheduler won't keep scheduling frames unless you keep calling
this.

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

Actually that comment will not be accurate. Since calling this does not
necessarily mean that there is any new content to render. It will also be called
for requestAnimationFrame for instance, where the page just wants to do
something on the next vsync but the page content might not have changed at all.

The other thing is I don't think this class should have to care about the
reasons why a caller needs to start a frame. You call it when you want the
scheduler to tell you when its a good time to start a frame update, why you need
that frame is up to you.

I've renamed the method to ScheduleFrameUpdate if that is more descriptive.

+  void DidSendFrameUpdateToClient();
+  void DidReceiveFrameUpdateAck();

[Wez, 2016/10/13 16:53:52]

SetNeedsMainFrame(), at least, needs explaining.

[Khushal, 2016/10/13 18:47:27]

Done.

+
+  base::TimeTicks next_frame_time() const { return next_frame_time_; }
+
+ protected:
+  // protected for testing.
+  Scheduler(base::TimeDelta frame_delay,
+            scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+            SchedulerClient* client);
+
+ private:
+  void ScheduleMainFrameIfNecessary();
+  bool CanProduceMainFrames() const;

[Wez, 2016/10/13 16:53:52]

Under what circumstance can we _not_ produce "main frames"?

[Khushal, 2016/10/13 18:47:27]

Done.

+  void StartMainFrame();
+
+  // Set to true if the |client_| has requested us to schedule main frames.
+  bool needs_main_frame_ = false;
+
+  // Set to true if a frame update was sent to the client and the ack is
+  // pending.
+  bool frame_ack_pending_ = false;
+
+  // The time at which the next main frame update can be run.
+  base::TimeTicks next_frame_time_;
+
+  // The delay to use between consecutive frames.
+  base::TimeDelta frame_delay_;
+
+  base::OneShotTimer frame_tick_timer_;
+
+  SchedulerClient* client_;

[Wez, 2016/10/13 16:53:52]

nit: Initialize to nullptr here, just in case?

[Khushal, 2016/10/13 18:47:27]

The ctor takes it as an argument, so is it necessary?

[Wez, 2016/10/18 01:49:42]

No, just suggested for consistency w/ the other members.

+
+  base::WeakPtrFactory<Scheduler> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(Scheduler);
+};
+
+}  // namespace engine
+}  // namespace blimp
+
+#endif  // BLIMP_ENGINE_RENDERER_SCHEDULER_H_