cc: Dedup IPCs to return resources to client

content/browser/renderer_host/delegated_frame_host.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 "content/browser/renderer_host/delegated_frame_host.h"
 
 #include <algorithm>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/memory/ptr_util.h"
 #include "base/time/default_tick_clock.h"
 #include "cc/output/compositor_frame.h"
-#include "cc/output/compositor_frame_ack.h"
 #include "cc/output/copy_output_request.h"
 #include "cc/resources/single_release_callback.h"
 #include "cc/resources/texture_mailbox.h"
 #include "cc/surfaces/surface.h"
 #include "cc/surfaces/surface_factory.h"
 #include "cc/surfaces/surface_hittest.h"
 #include "cc/surfaces/surface_manager.h"
 #include "components/display_compositor/gl_helper.h"
 #include "content/browser/compositor/surface_utils.h"
 #include "content/browser/gpu/compositor_util.h"
@@ skipping 375 matching lines @@
   gfx::Size frame_size = root_pass->output_rect.size();
   gfx::Size frame_size_in_dip =
       gfx::ConvertSizeToDIP(frame_device_scale_factor, frame_size);
 
   gfx::Rect damage_rect = root_pass->damage_rect;
   damage_rect.Intersect(gfx::Rect(frame_size));
   gfx::Rect damage_rect_in_dip =
       gfx::ConvertRectToDIP(frame_device_scale_factor, damage_rect);
 
   if (ShouldSkipFrame(frame_size_in_dip)) {
-    cc::CompositorFrameAck ack;
+    cc::ReturnedResourceArray resources;
     cc::TransferableResource::ReturnResources(frame_data->resource_list,
-                                              &ack.resources);
+                                              &resources);
 
     skipped_latency_info_list_.insert(skipped_latency_info_list_.end(),
                                       frame.metadata.latency_info.begin(),
                                       frame.metadata.latency_info.end());
 
-    client_->DelegatedFrameHostSendCompositorSwapAck(output_surface_id, ack);
+    client_->DelegatedFrameHostSendReclaimCompositorResources(
+        output_surface_id, true /* is_swap_ack*/, resources);
     skipped_frames_ = true;
     return;
   }
 
   if (skipped_frames_) {
     skipped_frames_ = false;
     damage_rect = gfx::Rect(frame_size);
     damage_rect_in_dip = gfx::Rect(frame_size_in_dip);
 
     // Give the same damage rect to the compositor.
     cc::RenderPass* root_pass = frame_data->render_pass_list.back().get();
     root_pass->damage_rect = damage_rect;
   }
 
   if (output_surface_id != last_output_surface_id_) {
     // Resource ids are scoped by the output surface.
     // If the originating output surface doesn't match the last one, it
     // indicates the renderer's output surface may have been recreated, in which
     // case we should recreate the DelegatedRendererLayer, to avoid matching
     // resources from the old one with resources from the new one which would
     // have the same id. Changing the layer to showing painted content destroys
     // the DelegatedRendererLayer.
     EvictDelegatedFrame();
 
     surface_factory_.reset();
     if (!surface_returned_resources_.empty())
-      SendReturnedDelegatedResources(last_output_surface_id_);
+      SendReclaimCompositorResources(last_output_surface_id_,
+                                     false /* is_swap_ack */);
 
     last_output_surface_id_ = output_surface_id;
   }
   bool skip_frame = false;
   pending_delegated_ack_count_++;
 
   background_color_ = frame.metadata.root_background_color;
 
   if (frame_size.IsEmpty()) {
     DCHECK(frame_data->resource_list.empty());
@@ skipping 43 matching lines @@
 
   UpdateGutters();
 
   if (!damage_rect_in_dip.IsEmpty())
     client_->DelegatedFrameHostGetLayer()->OnDelegatedFrameDamage(
         damage_rect_in_dip);
 
   // Note that |compositor_| may be reset by SetShowSurface or
   // SetShowDelegatedContent above.
   if (!compositor_ || skip_frame) {
-    SendDelegatedFrameAck(output_surface_id);
+    SendReclaimCompositorResources(output_surface_id, false /* is_swap_ack */);

[piman, 2016/07/13 20:59:14]

this one is a swap ack.

[Fady Samuel, 2016/07/13 22:12:16]

Done.

[danakj, 2016/07/13 22:15:03]

Are there (can there be) tests that cover these mistakes?

[Fady Samuel, 2016/07/14 02:50:10]

This is now covered by
RenderWidgetHostViewAuraTest.ResettingCompositorReturnsResources

   } else {
     can_lock_compositor_ = NO_PENDING_COMMIT;
   }
   if (!surface_id_.is_null())
     delegated_frame_evictor_->SwappedFrame(
         client_->DelegatedFrameHostIsVisible());
   // Note: the frame may have been evicted immediately.
 }
 
 void DelegatedFrameHost::ClearDelegatedFrame() {
   if (!surface_id_.is_null())
     EvictDelegatedFrame();
 }
 
-void DelegatedFrameHost::SendDelegatedFrameAck(uint32_t output_surface_id) {
-  cc::CompositorFrameAck ack;
-  if (!surface_returned_resources_.empty())
-    ack.resources.swap(surface_returned_resources_);
-  client_->DelegatedFrameHostSendCompositorSwapAck(output_surface_id, ack);
+void DelegatedFrameHost::SendReclaimCompositorResources(
+    uint32_t output_surface_id,
+    bool is_swap_ack) {
+  client_->DelegatedFrameHostSendReclaimCompositorResources(
+      output_surface_id, is_swap_ack, surface_returned_resources_);
+  surface_returned_resources_.clear();
   DCHECK_GT(pending_delegated_ack_count_, 0);
   pending_delegated_ack_count_--;

[piman, 2016/07/13 20:59:14]

this (the DCHECK and the decrement) should only happen if is_swap_ack is true.

[Fady Samuel, 2016/07/13 22:12:16]

Done.

[Fady Samuel, 2016/07/14 02:50:10]

This is now covered by RenderWidgetHostViewAuraTest.ReturnedResources

 }
 
 void DelegatedFrameHost::SurfaceDrawn(uint32_t output_surface_id,
                                       cc::SurfaceDrawStatus drawn) {
-  SendDelegatedFrameAck(output_surface_id);
-}
-
-void DelegatedFrameHost::SendReturnedDelegatedResources(
-    uint32_t output_surface_id) {
-  cc::CompositorFrameAck ack;
-  DCHECK(!surface_returned_resources_.empty());
-  ack.resources.swap(surface_returned_resources_);
-
-  client_->DelegatedFrameHostSendReclaimCompositorResources(output_surface_id,
-                                                            ack);
+  SendReclaimCompositorResources(output_surface_id, true /* is_swap_ack */);
 }
 
 void DelegatedFrameHost::ReturnResources(
     const cc::ReturnedResourceArray& resources) {
   if (resources.empty())
     return;
   std::copy(resources.begin(), resources.end(),
             std::back_inserter(surface_returned_resources_));
   if (!pending_delegated_ack_count_)
-    SendReturnedDelegatedResources(last_output_surface_id_);
+    SendReclaimCompositorResources(last_output_surface_id_,
+                                   false /* is_swap_ack */);
 }
 
 void DelegatedFrameHost::WillDrawSurface(const cc::SurfaceId& id,
                                          const gfx::Rect& damage_rect) {
   // Frame subscribers are only interested in changes to the target surface, so
   // do not attempt capture if |damage_rect| is empty.  This prevents the draws
   // of parent surfaces from triggering extra frame captures, which can affect
   // smoothness.
   if (id != surface_id_ || damage_rect.IsEmpty())
     return;
@@ skipping 333 matching lines @@
     cc::SurfaceManager* manager = factory->GetSurfaceManager();
     new_layer->SetShowSurface(
         surface_id_, base::Bind(&SatisfyCallback, base::Unretained(manager)),
         base::Bind(&RequireCallback, base::Unretained(manager)),
         current_surface_size_, current_scale_factor_,
         current_frame_size_in_dip_);
   }
 }
 
 }  // namespace content