Add BeginFrameObserverProxy

content/browser/renderer_host/begin_frame_manager.cc

Index: content/browser/renderer_host/begin_frame_manager.cc
diff --git a/content/browser/renderer_host/begin_frame_manager.cc b/content/browser/renderer_host/begin_frame_manager.cc
new file mode 100644
index 0000000000000000000000000000000000000000..369290ee5c6af56979215b593d609f13a2046a0d
--- /dev/null
+++ b/content/browser/renderer_host/begin_frame_manager.cc
@@ -0,0 +1,71 @@
+// Copyright 2015 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 "content/browser/renderer_host/begin_frame_manager.h"
+
+namespace content {
+
+BeginFrameManager::BeginFrameManager(BeginFrameManagerClient* client)
+    : needs_begin_frames_(false),
+      client_(client),
+      compositor_(nullptr) {
+}
+BeginFrameManager::~BeginFrameManager() {
+}
+
+void BeginFrameManager::OnSetNeedsBeginFrames(bool needs_begin_frames) {

[danakj, 2015/03/17 18:56:45]

This looks like it should be just "SetNeedsBeginFrames()"

[simonhong, 2015/03/19 15:48:45]

Done.

+  if (needs_begin_frames_ == needs_begin_frames)
+    return;
+
+  needs_begin_frames_ = needs_begin_frames;
+
+  // In some cases, BeginFrame message is requested before |client_|'s window is
+  // added in the root window hierarchy.
+  if (!compositor_)
+    return;
+
+  if (needs_begin_frames)
+    StartObservingBeginFrames();
+  else
+    StopObservingBeginFrames();
+}
+
+void BeginFrameManager::SetCompositor(ui::Compositor* compositor) {
+  DCHECK(!compositor_);

[danakj, 2015/03/17 18:56:45]

I'm a little unclear about this DCHECK with the if() early out below.

You don't want them setting a compositor more than once. But you want to allow
SetCompositor with a null compositor? But ignore it? That seems awkward, can you
explain? Can we DCHECK(compositor) as well?

Or... Why not just allow null here, and you don't need a separate Reset method?

[simonhong, 2015/03/19 15:48:45]

DCHECK(compositor) is added.
I think using Reset method can make  more easier and readable code.

+
+  if (!compositor)
+    return;
+
+  compositor_ = compositor;
+
+  if (needs_begin_frames_)
+    StartObservingBeginFrames();
+}
+
+void BeginFrameManager::ResetCompositor() {
+  if (!compositor_)
+    return;
+
+  if (needs_begin_frames_)
+    StopObservingBeginFrames();
+
+  compositor_ = nullptr;
+}
+
+void BeginFrameManager::OnSendBeginFrame(const cc::BeginFrameArgs& args) {
+  client_->SendBeginFrame(args);
+  last_sent_begin_frame_args_ = args;

[danakj, 2015/03/17 18:56:45]

Would it be equivalent to early out in here instead of passing the
last_sent_..._args_ to the AddBeginFrameObserver and having it skip sending? Or
can compositor send the same args more than once? Cuz if it's the same, it seems
a bit nicer to it locally here rather than passing things around.

[simonhong, 2015/03/19 15:48:46]

Hmm, this is not for checking sending same args twice.
It helps Compositor. Please check below comment.

+}
+
+void BeginFrameManager::StartObservingBeginFrames() {
+  DCHECK(compositor_);
+  compositor_->AddBeginFrameObserver(this, last_sent_begin_frame_args_);

[danakj, 2015/03/17 18:56:45]

can you add some comments explaining the purpose of thie last_sent_..._args_
thing as well? Is it for moving to a new compositor? Or...? What kinda
situations will make this used? What does it do or prevent?

[simonhong, 2015/03/19 15:48:45]

I commented its purpose in header file.
It is for two situation.
One is moving to a new compositor like you said when tab is detached and
attached.
The Other is needs_begin_frame_ is changed from false to true.
Passing this value can help Compositor whether Compositor passing args
immediately or not.

+}
+
+void BeginFrameManager::StopObservingBeginFrames() {
+  DCHECK(compositor_);
+  compositor_->RemoveBeginFrameObserver(this);
+}
+
+}  // namespace content