Remove SurfaceFactory::Create and SurfaceFactory::Destroy

cc/surfaces/surface_factory.cc

Index: cc/surfaces/surface_factory.cc
diff --git a/cc/surfaces/surface_factory.cc b/cc/surfaces/surface_factory.cc
index 36a79ee9e9ad0557aa92978a661bb961dd56ed6b..53365dc523c32be7d17ebcdb16695e99317640fa 100644
--- a/cc/surfaces/surface_factory.cc
+++ b/cc/surfaces/surface_factory.cc
@@ -26,10 +26,6 @@ SurfaceFactory::SurfaceFactory(const FrameSinkId& frame_sink_id,
       weak_factory_(this) {}
 
 SurfaceFactory::~SurfaceFactory() {
-  if (!surface_map_.empty()) {

[danakj, 2016/11/10 23:57:58]

Can we enforce this still?

[Fady Samuel, 2016/11/11 00:03:01]

Why? What does that solve? This CL eliminates explicit Create/Destroy in order
to better match the CompositorFrameSink API. The last surface will be destroyed
here. 

[danakj, 2016/11/11 00:10:47]

Cuz this is why there's all the null checks unless I'm misunderstanding.

[Saman Sami, 2016/11/11 17:49:58]

I got rid of the null checks. I still don't have this error message in my last
patch.

-    LOG(ERROR) << "SurfaceFactory has " << surface_map_.size()
-               << " entries in map on destruction.";
-  }
   DestroyAll();
 }
 
@@ -39,6 +35,7 @@ void SurfaceFactory::DestroyAll() {
       manager_->Destroy(std::move(pair.second));
   }
   surface_map_.clear();
+  last_local_frame_id_.reset();
 }
 
 void SurfaceFactory::Reset() {
@@ -82,6 +79,9 @@ void SurfaceFactory::SubmitCompositorFrame(const LocalFrameId& local_frame_id,
                                            CompositorFrame frame,
                                            const DrawCallback& callback) {
   TRACE_EVENT0("cc", "SurfaceFactory::SubmitCompositorFrame");
+  if (!last_local_frame_id_ || local_frame_id != *last_local_frame_id_) {
+    Create(local_frame_id);
+  }
   OwningSurfaceMap::iterator it = surface_map_.find(local_frame_id);
   DCHECK(it != surface_map_.end());
   DCHECK(it->second->factory().get() == this);
@@ -101,6 +101,9 @@ void SurfaceFactory::SubmitCompositorFrame(const LocalFrameId& local_frame_id,
     TRACE_EVENT_INSTANT0("cc", "Damage not visible.", TRACE_EVENT_SCOPE_THREAD);
     it->second->RunDrawCallbacks();
   }
+  if (last_local_frame_id_ && local_frame_id != *last_local_frame_id_)
+    Destroy(*last_local_frame_id_);
+  last_local_frame_id_ = local_frame_id;

[danakj, 2016/11/10 23:57:58]

instead of last_ maybe this is current_ ? last sounds like previous and seems
misleading here

[Saman Sami, 2016/11/11 17:49:59]

Done.

 }
 
 void SurfaceFactory::RequestCopyOfSurface(
@@ -118,7 +121,8 @@ void SurfaceFactory::RequestCopyOfSurface(
 
 void SurfaceFactory::WillDrawSurface(const LocalFrameId& id,
                                      const gfx::Rect& damage_rect) {
-  client_->WillDrawSurface(id, damage_rect);
+  if (client_)

[Fady Samuel, 2016/11/10 23:33:37]

Is this check necessary?

[Saman Sami, 2016/11/11 17:49:58]

Removed

+    client_->WillDrawSurface(id, damage_rect);
 }
 
 void SurfaceFactory::ReceiveFromChild(