Add BeginFrameObserverProxy

ui/compositor/compositor.cc

 // Copyright (c) 2012 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 "ui/compositor/compositor.h"
 
 #include <algorithm>
 #include <deque>
 
 #include "base/bind.h"
@@ skipping 138 matching lines @@
 
 Compositor::~Compositor() {
   TRACE_EVENT0("shutdown", "Compositor::destructor");
 
   CancelCompositorLock();
   DCHECK(!compositor_lock_);
 
   FOR_EACH_OBSERVER(CompositorObserver, observer_list_,
                     OnCompositingShuttingDown(this));
 
+  DCHECK(!begin_frame_observer_list_.might_have_observers());
+
   if (root_layer_)
     root_layer_->SetCompositor(NULL);
 
   // Stop all outstanding draws before telling the ContextFactory to tear
   // down any contexts that the |host_| may rely upon.
   host_.reset();
 
   context_factory_->RemoveCompositor(this);
 }
 
@@ skipping 107 matching lines @@
 void Compositor::RemoveAnimationObserver(
     CompositorAnimationObserver* observer) {
   animation_observer_list_.RemoveObserver(observer);
 }
 
 bool Compositor::HasAnimationObserver(
     const CompositorAnimationObserver* observer) const {
   return animation_observer_list_.HasObserver(observer);
 }
 
+void Compositor::AddBeginFrameObserver(
+    CompositorBeginFrameObserver* observer,
+    const cc::BeginFrameArgs& last_begin_frame_args_sent_to_observer) {
+  // If |last_begin_frame_args_| is still effective, send it to the new
+  // |observer| immediately.
+  if (last_begin_frame_args_sent_to_observer.frame_time !=
+      missed_begin_frame_args_.frame_time) {
+    observer->OnSendBeginFrame(missed_begin_frame_args_);
+  }
+
+  if (!begin_frame_observer_list_.might_have_observers())

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

From the name "might have", this feels like a bug where maybe it doesn't have an
observer, but it returns true, so we don't SetChildrenNeedBeginFrames() but we
should have.

Can you convince my why this will act correctly?

[brianderson, 2015/03/17 22:29:32]

Looks like might_have_observers() can return the incorrect value if observers
have been removed while an iterator exists (in order to avoid having to
invalidate iterators when elements are removed):

https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list...

https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list...

https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list...

Since this isn't called when an iterator exists, it should return the correct
value.

That's a lot of mental overhead to verify correctness though. It also means we
don't really need the added functionality of an ObserverList. Should we just use
a std::list instead of an ObserverList?

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

I used it to check whether the list is empty or not because ObserverList
provides only that method.
It will work as I expected as brian explained.
Anyway, I changed to use std::list instead of using ObserverList as brian
suggested.

+    host_->SetChildrenNeedBeginFrames(true);
+  begin_frame_observer_list_.AddObserver(observer);
+}
+
+void Compositor::RemoveBeginFrameObserver(
+    CompositorBeginFrameObserver* observer) {
+  DCHECK(begin_frame_observer_list_.HasObserver(observer));
+  begin_frame_observer_list_.RemoveObserver(observer);
+
+  if (!begin_frame_observer_list_.might_have_observers())

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

Dittos

+    host_->SetChildrenNeedBeginFrames(false);
+}
+
 void Compositor::BeginMainFrame(const cc::BeginFrameArgs& args) {
   FOR_EACH_OBSERVER(CompositorAnimationObserver,
                     animation_observer_list_,
                     OnAnimationStep(args.frame_time));
   if (animation_observer_list_.might_have_observers())
     host_->SetNeedsAnimate();
 }
 
 void Compositor::BeginMainFrameNotExpectedSoon() {
 }
@@ skipping 42 matching lines @@
   FOR_EACH_OBSERVER(CompositorObserver, observer_list_,
                     OnCompositingStarted(this, start_time));
 }
 
 void Compositor::DidAbortSwapBuffers() {
   FOR_EACH_OBSERVER(CompositorObserver,
                     observer_list_,
                     OnCompositingAborted(this));
 }
 
+void Compositor::SendBeginFramesToChildren(const cc::BeginFrameArgs& args) {
+  FOR_EACH_OBSERVER(CompositorBeginFrameObserver,
+                    begin_frame_observer_list_,
+                    OnSendBeginFrame(args));
+  missed_begin_frame_args_ = args;
+  missed_begin_frame_args_.type = cc::BeginFrameArgs::MISSED;
+}
+
 const cc::LayerTreeDebugState& Compositor::GetLayerTreeDebugState() const {
   return host_->debug_state();
 }
 
 void Compositor::SetLayerTreeDebugState(
     const cc::LayerTreeDebugState& debug_state) {
   host_->SetDebugState(debug_state);
 }
 
 const cc::RendererSettings& Compositor::GetRendererSettings() const {
@@ skipping 19 matching lines @@
                     observer_list_,
                     OnCompositingLockStateChanged(this));
 }
 
 void Compositor::CancelCompositorLock() {
   if (compositor_lock_)
     compositor_lock_->CancelLock();
 }
 
 }  // namespace ui