Make cc::CompositorFrames movable [Part 2 of 2]

cc/surfaces/surface.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.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 13 matching lines @@
 
 Surface::Surface(SurfaceId id, SurfaceFactory* factory)
     : surface_id_(id),
       previous_frame_surface_id_(id),
       factory_(factory->AsWeakPtr()),
       frame_index_(kFrameIndexStart),
       destroyed_(false) {}
 
 Surface::~Surface() {
   ClearCopyRequests();
-  if (current_frame_ && factory_) {
-    UnrefFrameResources(current_frame_->delegated_frame_data.get());
+  if (current_frame_.delegated_frame_data && factory_) {
+    UnrefFrameResources(current_frame_.delegated_frame_data.get());
   }
   if (!draw_callback_.is_null())
     draw_callback_.Run(SurfaceDrawStatus::DRAW_SKIPPED);
 }
 
 void Surface::SetPreviousFrameSurface(Surface* surface) {
   DCHECK(surface);
   frame_index_ = surface->frame_index() + 1;
   previous_frame_surface_id_ = surface->surface_id();
 }
 
 void Surface::SetGpuMemoryBufferClientId(int gpu_memory_buffer_client_id) {
   // This should only be set once.
   DCHECK_EQ(-1, gpu_memory_buffer_client_id_);
   DCHECK_NE(gpu_memory_buffer_client_id, gpu_memory_buffer_client_id_);
   gpu_memory_buffer_client_id_ = gpu_memory_buffer_client_id;
 }
 
-void Surface::QueueFrame(std::unique_ptr<CompositorFrame> frame,
-                         const DrawCallback& callback) {
+void Surface::QueueFrame(CompositorFrame frame, const DrawCallback& callback) {
   DCHECK(factory_);
   ClearCopyRequests();
 
-  if (frame) {
-    TakeLatencyInfo(&frame->metadata.latency_info);
+  if (frame.delegated_frame_data) {
+    TakeLatencyInfo(&frame.metadata.latency_info);
   }
 
-  std::unique_ptr<CompositorFrame> previous_frame = std::move(current_frame_);
+  CompositorFrame previous_frame = std::move(current_frame_);
   current_frame_ = std::move(frame);
 
-  if (current_frame_) {
+  if (current_frame_.delegated_frame_data) {
     factory_->ReceiveFromChild(
-        current_frame_->delegated_frame_data->resource_list);
+        current_frame_.delegated_frame_data->resource_list);
   }
 
   // Empty frames shouldn't be drawn and shouldn't contribute damage, so don't
   // increment frame index for them.
-  if (current_frame_ &&
-      !current_frame_->delegated_frame_data->render_pass_list.empty())
+  if (current_frame_.delegated_frame_data &&
+      !current_frame_.delegated_frame_data->render_pass_list.empty()) {
     ++frame_index_;
+  }
 
   previous_frame_surface_id_ = surface_id();
 
   std::vector<SurfaceId> new_referenced_surfaces;
-  if (current_frame_) {
-    new_referenced_surfaces = current_frame_->metadata.referenced_surfaces;
-  }
+  new_referenced_surfaces = current_frame_.metadata.referenced_surfaces;
 
-  if (previous_frame) {
-    UnrefFrameResources(previous_frame->delegated_frame_data.get());
-  }
+  if (previous_frame.delegated_frame_data)
+    UnrefFrameResources(previous_frame.delegated_frame_data.get());
+
   if (!draw_callback_.is_null())
     draw_callback_.Run(SurfaceDrawStatus::DRAW_SKIPPED);
   draw_callback_ = callback;
 
   bool referenced_surfaces_changed =
       (referenced_surfaces_ != new_referenced_surfaces);
   referenced_surfaces_ = new_referenced_surfaces;
   std::vector<uint32_t> satisfies_sequences;
-  if (current_frame_)
-    current_frame_->metadata.satisfies_sequences.swap(satisfies_sequences);
+  current_frame_.metadata.satisfies_sequences.swap(satisfies_sequences);

[danakj, 2016/06/27 19:53:00]

you can use move instead for this, and merge it with the variable definition.

[Fady Samuel, 2016/06/28 17:19:10]

Done.

+
   if (referenced_surfaces_changed || !satisfies_sequences.empty()) {
     // Notify the manager that sequences were satisfied either if some new
     // sequences were satisfied, or if the set of referenced surfaces changed
     // to force a GC to happen.
     factory_->manager()->DidSatisfySequences(surface_id_.id_namespace(),
                                              &satisfies_sequences);
   }
 }
 
 void Surface::RequestCopyOfOutput(
     std::unique_ptr<CopyOutputRequest> copy_request) {
-  if (current_frame_ &&
-      !current_frame_->delegated_frame_data->render_pass_list.empty()) {
+  if (current_frame_.delegated_frame_data &&
+      !current_frame_.delegated_frame_data->render_pass_list.empty()) {
     std::vector<std::unique_ptr<CopyOutputRequest>>& copy_requests =
-        current_frame_->delegated_frame_data->render_pass_list.back()
+        current_frame_.delegated_frame_data->render_pass_list.back()
             ->copy_requests;
 
     if (void* source = copy_request->source()) {
       // Remove existing CopyOutputRequests made on the Surface by the same
       // source.
       auto to_remove =
           std::remove_if(copy_requests.begin(), copy_requests.end(),
                          [source](const std::unique_ptr<CopyOutputRequest>& x) {
                            return x->source() == source;
                          });
       copy_requests.erase(to_remove, copy_requests.end());
     }
     copy_requests.push_back(std::move(copy_request));
   } else {
     copy_request->SendEmptyResult();
   }
 }
 
 void Surface::TakeCopyOutputRequests(
     std::multimap<RenderPassId, std::unique_ptr<CopyOutputRequest>>*
         copy_requests) {
   DCHECK(copy_requests->empty());
-  if (current_frame_) {
+  if (current_frame_.delegated_frame_data) {
     for (const auto& render_pass :
-         current_frame_->delegated_frame_data->render_pass_list) {
+         current_frame_.delegated_frame_data->render_pass_list) {
       for (auto& request : render_pass->copy_requests) {
         copy_requests->insert(
             std::make_pair(render_pass->id, std::move(request)));
       }
       render_pass->copy_requests.clear();
     }
   }
 }
 
 const CompositorFrame* Surface::GetEligibleFrame() {
-  return current_frame_.get();
+  return &current_frame_;

[danakj, 2016/06/27 19:53:00]

There are callers that check if this is null. What does it mean that it is never
null for them? And we should change them accordingly. I suspect we need to
maintain a "has current_frame_" vs "does not have" state but I'd like to hear
your analysis

[Fady Samuel, 2016/06/28 17:19:10]

I've let the compiler help me with the analysis. :-) I've made GetEligibleFrame
return a const reference instead and I've updated callsites to check
delegated_frame_data instead. WDYT?

[danakj, 2016/06/28 19:01:09]

Ya ok this should work for now. Eventually we're not going to have a
DelegatedFrameData I don't think, we'll just collapse it into
CompositorFrameData, so we'll have to write this patch again basically. I think
you could avoid that by using a "has a frame" state instead in this CL.

[Fady Samuel, 2016/06/28 19:24:19]

The primary goal of this CL is to make Surface hold a cc::CompositorFrame
directly.

I think the easiest way to introduce a HasFrame() is to get rid of
delegated_frame_data and update all checks for not-nullness to use HasFrame().
The compiler can help us verify we did it right. 

My goal is also to get cc/output/compositor_frame.h and
cc/ipc/compositor_frame.mojom to look the same so I'm happy to make that change
in a separate CL.

 }
 
 void Surface::TakeLatencyInfo(std::vector<ui::LatencyInfo>* latency_info) {
-  if (!current_frame_)
+  if (!current_frame_.delegated_frame_data)
     return;
   if (latency_info->empty()) {
-    current_frame_->metadata.latency_info.swap(*latency_info);
+    current_frame_.metadata.latency_info.swap(*latency_info);
     return;
   }
-  std::copy(current_frame_->metadata.latency_info.begin(),
-            current_frame_->metadata.latency_info.end(),
+  std::copy(current_frame_.metadata.latency_info.begin(),
+            current_frame_.metadata.latency_info.end(),
             std::back_inserter(*latency_info));
-  current_frame_->metadata.latency_info.clear();
+  current_frame_.metadata.latency_info.clear();
 }
 
 void Surface::RunDrawCallbacks(SurfaceDrawStatus drawn) {
   if (!draw_callback_.is_null()) {
     DrawCallback callback = draw_callback_;
     draw_callback_ = DrawCallback();
     callback.Run(drawn);
   }
 }
 
@@ skipping 17 matching lines @@
 void Surface::UnrefFrameResources(DelegatedFrameData* frame_data) {
   ReturnedResourceArray resources;
   TransferableResource::ReturnResources(frame_data->resource_list, &resources);
   // No point in returning same sync token to sender.
   for (auto& resource : resources)
     resource.sync_token.Clear();
   factory_->UnrefResources(resources);
 }
 
 void Surface::ClearCopyRequests() {
-  if (current_frame_) {
+  if (current_frame_.delegated_frame_data) {
     for (const auto& render_pass :
-         current_frame_->delegated_frame_data->render_pass_list) {
+         current_frame_.delegated_frame_data->render_pass_list) {
       for (const auto& copy_request : render_pass->copy_requests)
         copy_request->SendEmptyResult();
     }
   }
 }
 
 }  // namespace cc