SurfaceDependencyTracker: Only observe BeginFrames if a deadline is needed

cc/surfaces/surface_dependency_tracker.cc

Index: cc/surfaces/surface_dependency_tracker.cc
diff --git a/cc/surfaces/surface_dependency_tracker.cc b/cc/surfaces/surface_dependency_tracker.cc
index 652ecfe731b3d97edb77d43b00664451f00da6bf..583baec3ee8174f46cf232d65b9bf67024c2d060 100644
--- a/cc/surfaces/surface_dependency_tracker.cc
+++ b/cc/surfaces/surface_dependency_tracker.cc
@@ -19,12 +19,9 @@ SurfaceDependencyTracker::SurfaceDependencyTracker(
     BeginFrameSource* begin_frame_source)
     : surface_manager_(surface_manager),
       begin_frame_source_(begin_frame_source) {
-  begin_frame_source_->AddObserver(this);
 }
 
-SurfaceDependencyTracker::~SurfaceDependencyTracker() {
-  begin_frame_source_->RemoveObserver(this);
-}
+SurfaceDependencyTracker::~SurfaceDependencyTracker() = default;
 
 void SurfaceDependencyTracker::RequestSurfaceResolution(Surface* surface) {
   DCHECK(surface->HasPendingFrame());
@@ -52,8 +49,10 @@ void SurfaceDependencyTracker::RequestSurfaceResolution(Surface* surface) {
   if (!blocked_surfaces_by_id_.count(surface->surface_id()))
     blocked_surfaces_by_id_.insert(surface->surface_id());
 
-  if (needs_deadline && !frames_since_deadline_set_)
+  if (needs_deadline && !frames_since_deadline_set_) {
     frames_since_deadline_set_ = 0;
+    begin_frame_source_->AddObserver(this);

[vmpstr, 2017/05/15 15:49:39]

I wonder if we should have some sort of a scoped object that would remove itself
from the begin_frame_source_ observer list on destruction.

My concern is that it's hard to reason whether any of the operations that remove
the observer are guaranteed to happen before the dtor of this class is called. 

You would also not need the && frames_since_deadline_set_ checks, since you can
just reset the observer thing when it's already empty without any bad behavior. 

[Fady Samuel, 2017/05/29 17:46:54]

I've introduced a SurfaceDependencyDeadline to deal with this.

+  }
 }
 
 void SurfaceDependencyTracker::OnSurfaceActivated(Surface* surface) {
@@ -79,8 +78,10 @@ void SurfaceDependencyTracker::OnSurfaceDependenciesChanged(
 
   // If there are no more dependencies to resolve then we don't need to have a
   // deadline.
-  if (blocked_surfaces_from_dependency_.empty())
+  if (blocked_surfaces_from_dependency_.empty() && frames_since_deadline_set_) {
     frames_since_deadline_set_.reset();
+    begin_frame_source_->RemoveObserver(this);
+  }
 }
 
 void SurfaceDependencyTracker::OnSurfaceDiscarded(Surface* surface) {
@@ -109,8 +110,10 @@ void SurfaceDependencyTracker::OnSurfaceDiscarded(Surface* surface) {
     }
   }
 
-  if (blocked_surfaces_from_dependency_.empty())
+  if (blocked_surfaces_from_dependency_.empty() && frames_since_deadline_set_) {
     frames_since_deadline_set_.reset();
+    begin_frame_source_->RemoveObserver(this);
+  }
 
   blocked_surfaces_by_id_.erase(surface->surface_id());
 
@@ -161,6 +164,7 @@ void SurfaceDependencyTracker::OnBeginFrame(const BeginFrameArgs& args) {
   }
 
   frames_since_deadline_set_.reset();
+  begin_frame_source_->RemoveObserver(this);
 }
 
 const BeginFrameArgs& SurfaceDependencyTracker::LastUsedBeginFrameArgs() const {
@@ -180,8 +184,10 @@ void SurfaceDependencyTracker::NotifySurfaceIdAvailable(
   blocked_surfaces_from_dependency_.erase(it);
   // If there are no more blockers in the system, then we no longer need to
   // have a deadline.
-  if (blocked_surfaces_from_dependency_.empty())
+  if (blocked_surfaces_from_dependency_.empty() && frames_since_deadline_set_) {
     frames_since_deadline_set_.reset();
+    begin_frame_source_->RemoveObserver(this);
+  }
 
   // Tell each surface about the availability of its blocker.
   for (const SurfaceId& blocked_surface_by_id : blocked_surfaces_by_id) {