Remove SurfaceFactory::Create and SurfaceFactory::Destroy

cc/surfaces/surface_factory.cc

 // Copyright 2014 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 "cc/surfaces/surface_factory.h"
 
 #include <utility>
 
 #include "base/trace_event/trace_event.h"
 #include "cc/output/compositor_frame.h"
 #include "cc/output/copy_output_request.h"
 #include "cc/surfaces/surface.h"
 #include "cc/surfaces/surface_factory_client.h"
 #include "cc/surfaces/surface_manager.h"
 #include "ui/gfx/geometry/size.h"
 
 namespace cc {
 SurfaceFactory::SurfaceFactory(const FrameSinkId& frame_sink_id,
                                SurfaceManager* manager,
                                SurfaceFactoryClient* client)
     : frame_sink_id_(frame_sink_id),
       manager_(manager),
       client_(client),
       holder_(client),
       needs_sync_points_(true),
       weak_factory_(this) {}
 
 SurfaceFactory::~SurfaceFactory() {
-  if (!surface_map_.empty()) {
-    LOG(ERROR) << "SurfaceFactory has " << surface_map_.size()
-               << " entries in map on destruction.";
-  }
-  DestroyAll();
+  EvictFrame();

[danakj, 2016/11/12 00:27:21]

Is there a reason to support having a surface here, or could we dcheck that
current_surface_ was evicted already? (similar to the old weaker log error)

 }
 
-void SurfaceFactory::DestroyAll() {
-  if (manager_) {
-    for (auto& pair : surface_map_)
-      manager_->Destroy(std::move(pair.second));
-  }
-  surface_map_.clear();
+void SurfaceFactory::EvictFrame() {
+  if (!current_surface_)
+    return;
+  if (manager_)
+    manager_->Destroy(std::move(current_surface_));
+  current_surface_.reset();
 }
 
 void SurfaceFactory::Reset() {
-  DestroyAll();
+  EvictFrame();
   // Disown Surfaces that are still alive so that they don't try to unref
   // resources that we're not tracking any more.
   weak_factory_.InvalidateWeakPtrs();
   holder_.Reset();
 }
 
-void SurfaceFactory::Create(const LocalFrameId& local_frame_id) {
-  auto surface(base::MakeUnique<Surface>(
-      SurfaceId(frame_sink_id_, local_frame_id), weak_factory_.GetWeakPtr()));
-  manager_->RegisterSurface(surface.get());
-  DCHECK(!surface_map_.count(local_frame_id));
-  surface_map_[local_frame_id] = std::move(surface);
-}
-
-void SurfaceFactory::Destroy(const LocalFrameId& local_frame_id) {
-  OwningSurfaceMap::iterator it = surface_map_.find(local_frame_id);
-  DCHECK(it != surface_map_.end());
-  DCHECK(it->second->factory().get() == this);
-  std::unique_ptr<Surface> surface(std::move(it->second));
-  surface_map_.erase(it);
-  if (manager_)
-    manager_->Destroy(std::move(surface));
-}
-
-void SurfaceFactory::SetPreviousFrameSurface(const LocalFrameId& new_id,
-                                             const LocalFrameId& old_id) {
-  OwningSurfaceMap::iterator it = surface_map_.find(new_id);
-  DCHECK(it != surface_map_.end());
-  Surface* old_surface =
-      manager_->GetSurfaceForId(SurfaceId(frame_sink_id_, old_id));
-  if (old_surface) {
-    it->second->SetPreviousFrameSurface(old_surface);
-  }
-}
-
 void SurfaceFactory::SubmitCompositorFrame(const LocalFrameId& local_frame_id,
                                            CompositorFrame frame,
                                            const DrawCallback& callback) {
   TRACE_EVENT0("cc", "SurfaceFactory::SubmitCompositorFrame");
-  OwningSurfaceMap::iterator it = surface_map_.find(local_frame_id);
-  DCHECK(it != surface_map_.end());
-  DCHECK(it->second->factory().get() == this);
+  DCHECK(local_frame_id.is_valid());
+  std::unique_ptr<Surface> surface;
+  bool create_new_surface =
+      (!current_surface_ ||
+       local_frame_id != current_surface_->surface_id().local_frame_id());
+  if (create_new_surface)
+    surface = Create(local_frame_id);
+  else
+    surface = std::move(current_surface_);
   // Tell the SurfaceManager if this is the first frame submitted with this
   // LocalFrameId.
-  if (!it->second->HasFrame()) {
+  if (!surface->HasFrame()) {
     float device_scale_factor = frame.metadata.device_scale_factor;
     gfx::Size frame_size;
     // CompositorFrames may not be populated with a RenderPass in unit tests.
     if (!frame.render_pass_list.empty())
       frame_size = frame.render_pass_list[0]->output_rect.size();
-    manager_->SurfaceCreated(it->second->surface_id(), frame_size,
+    manager_->SurfaceCreated(surface->surface_id(), frame_size,
                              device_scale_factor);
   }
-  it->second->QueueFrame(std::move(frame), callback);
+  surface->QueueFrame(std::move(frame), callback);
   if (!manager_->SurfaceModified(SurfaceId(frame_sink_id_, local_frame_id))) {
     TRACE_EVENT_INSTANT0("cc", "Damage not visible.", TRACE_EVENT_SCOPE_THREAD);
-    it->second->RunDrawCallbacks();
+    surface->RunDrawCallbacks();
   }
+  if (current_surface_ && create_new_surface) {
+    surface->SetPreviousFrameSurface(current_surface_.get());

[piman, 2016/11/12 00:19:10]

I think doing this implicitly makes sense, but I'm not 100% sure that we don't
expect full damage in some places on e.g. resize. Before this change, damage was
implicitly being discarded on a surface change, because of no previous surface.
So we should definitely watch for issues on resize on platforms that support
partial swaps, such as (I believe) Windows, Chrome OS and possibly Mac. IIRC
it's disabled/not implemented on Linux and Android

If there are issues, we should probably EvictFrame in places where we used to
recreate a surface and did not do SetPreviousFrameSurface.

[danakj, 2016/11/12 00:27:21]

Should on linux with --disable-gpu

+    Destroy(std::move(current_surface_));
+  }
+  current_surface_ = std::move(surface);
 }
 
 void SurfaceFactory::RequestCopyOfSurface(
-    const LocalFrameId& local_frame_id,
     std::unique_ptr<CopyOutputRequest> copy_request) {
-  OwningSurfaceMap::iterator it = surface_map_.find(local_frame_id);
-  if (it == surface_map_.end()) {
+  if (!current_surface_) {
     copy_request->SendEmptyResult();
     return;
   }
-  DCHECK(it->second->factory().get() == this);
-  it->second->RequestCopyOfOutput(std::move(copy_request));
-  manager_->SurfaceModified(SurfaceId(frame_sink_id_, local_frame_id));
+  DCHECK(current_surface_->factory().get() == this);
+  current_surface_->RequestCopyOfOutput(std::move(copy_request));
+  manager_->SurfaceModified(current_surface_->surface_id());
 }
 
 void SurfaceFactory::WillDrawSurface(const LocalFrameId& id,
                                      const gfx::Rect& damage_rect) {
   client_->WillDrawSurface(id, damage_rect);
 }
 
 void SurfaceFactory::ReceiveFromChild(
     const TransferableResourceArray& resources) {
   holder_.ReceiveFromChild(resources);
 }
 
 void SurfaceFactory::RefResources(const TransferableResourceArray& resources) {
   holder_.RefResources(resources);
 }
 
 void SurfaceFactory::UnrefResources(const ReturnedResourceArray& resources) {
   holder_.UnrefResources(resources);
 }
 
+std::unique_ptr<Surface> SurfaceFactory::Create(
+    const LocalFrameId& local_frame_id) {
+  auto surface = base::MakeUnique<Surface>(
+      SurfaceId(frame_sink_id_, local_frame_id), weak_factory_.GetWeakPtr());
+  manager_->RegisterSurface(surface.get());
+  return surface;
+}
+
+void SurfaceFactory::Destroy(std::unique_ptr<Surface> surface) {
+  if (manager_)

[danakj, 2016/11/12 00:27:21]

I'm confused by the if (manager_) checks sometimes but not others. When can the
manager_ be null?

[Saman Sami, 2016/11/14 23:39:01]

I have no idea. I didn't write this. We can get rid of it in a future CL if
possible.

[danakj, 2016/11/15 00:43:40]

I would greatly appreciate that we do, unneeded null checks frustrate me :)

+    manager_->Destroy(std::move(surface));
+}
+
 }  // namespace cc