Make ui::Compositor BeginFrame subscription robust

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 176 matching lines @@
 
   CancelCompositorLock();
   DCHECK(!compositor_lock_);
 
   FOR_EACH_OBSERVER(CompositorObserver, observer_list_,
                     OnCompositingShuttingDown(this));
 
   FOR_EACH_OBSERVER(CompositorAnimationObserver, animation_observer_list_,
                     OnCompositingShuttingDown(this));
 
-  DCHECK(begin_frame_observer_list_.empty());

[brianderson, 2015/10/21 23:12:59]

Why is this no longer valid?

[jdduke (slow), 2015/10/21 23:46:16]

ObserverList does that for us (the second "true" argument to the template).

-
   if (root_layer_)
     root_layer_->ResetCompositor();
 
   // 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 143 matching lines @@
     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) {
-  DCHECK(std::find(begin_frame_observer_list_.begin(),

[brianderson, 2015/10/21 23:12:59]

Any way to keep this check?

[jdduke (slow), 2015/10/21 23:46:16]

ObserverList also does that for us.

-                   begin_frame_observer_list_.end(), observer) ==
-         begin_frame_observer_list_.end());
+  if (!begin_frame_observer_list_.might_have_observers())
+    host_->SetChildrenNeedBeginFrames(true);
 
-  if (begin_frame_observer_list_.empty())
-    host_->SetChildrenNeedBeginFrames(true);
+  begin_frame_observer_list_.AddObserver(observer);
 
   if (missed_begin_frame_args_.IsValid())
     observer->OnSendBeginFrame(missed_begin_frame_args_);
-
-  begin_frame_observer_list_.push_back(observer);
 }
 
 void Compositor::RemoveBeginFrameObserver(
     CompositorBeginFrameObserver* observer) {
-  auto it = std::find(begin_frame_observer_list_.begin(),
-                      begin_frame_observer_list_.end(), observer);
-  DCHECK(it != begin_frame_observer_list_.end());
-  begin_frame_observer_list_.erase(it);
+  begin_frame_observer_list_.RemoveObserver(observer);
 
-  if (begin_frame_observer_list_.empty()) {
+  if (!begin_frame_observer_list_.might_have_observers()) {

[danakj, 2015/10/22 18:35:59]

Do you need to do this here still, since it is done in
SendBeginFramesToChildren?

Can you leave a comment explaining here and on the one in
SendBeginFramesToChildren to explain them?

[jdduke (slow), 2015/10/22 18:39:00]

I considered that, I suppose it depends on the semantics we want.

If we don't mind a lingering unsubscribe for a single frame, that would be fine
with me, though I'd still add a comment here  detailing when the
SendBeginFramesToChildren call will occur. I'll make the change and we can go
from there.

     host_->SetChildrenNeedBeginFrames(false);
     missed_begin_frame_args_ = cc::BeginFrameArgs();
   }
 }
 
 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())
@@ skipping 52 matching lines @@
                     OnCompositingStarted(this, start_time));
 }
 
 void Compositor::DidAbortSwapBuffers() {
   FOR_EACH_OBSERVER(CompositorObserver,
                     observer_list_,
                     OnCompositingAborted(this));
 }
 
 void Compositor::SendBeginFramesToChildren(const cc::BeginFrameArgs& args) {
-  for (auto observer : begin_frame_observer_list_)
-    observer->OnSendBeginFrame(args);
+  FOR_EACH_OBSERVER(CompositorBeginFrameObserver, begin_frame_observer_list_,
+                    OnSendBeginFrame(args));
+
+  if (!begin_frame_observer_list_.might_have_observers()) {
+    host_->SetChildrenNeedBeginFrames(false);
+    missed_begin_frame_args_ = cc::BeginFrameArgs();
+    return;
+  }
 
   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(
@@ skipping 24 matching lines @@
                     observer_list_,
                     OnCompositingLockStateChanged(this));
 }
 
 void Compositor::CancelCompositorLock() {
   if (compositor_lock_)
     compositor_lock_->CancelLock();
 }
 
 }  // namespace ui