Replace CompositorFrameSinkSupport::WillDrawSurface With RepeatingCallback

cc/surfaces/surface_aggregator.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_aggregator.h"
 
 #include <stddef.h>
 
 #include <map>
 
 #include "base/bind.h"
 #include "base/containers/adapters.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/trace_event/trace_event.h"
 #include "cc/base/math_util.h"
 #include "cc/output/compositor_frame.h"
 #include "cc/quads/draw_quad.h"
 #include "cc/quads/render_pass_draw_quad.h"
 #include "cc/quads/shared_quad_state.h"
 #include "cc/quads/solid_color_draw_quad.h"
 #include "cc/quads/surface_draw_quad.h"
 #include "cc/quads/texture_draw_quad.h"
 #include "cc/resources/resource_provider.h"
 #include "cc/surfaces/surface.h"
 #include "cc/surfaces/surface_factory.h"

[Fady Samuel, 2017/04/20 12:45:48]

Is this still necessary?

[Alex Z., 2017/04/20 13:58:05]

Done.

 #include "cc/surfaces/surface_manager.h"
 #include "cc/trees/blocking_task_runner.h"
 
 namespace cc {
 namespace {
 
 // Maximum bucket size for the UMA stats.
 constexpr int kUmaStatMaxSurfaces = 30;
 
 const char kUmaValidSurface[] =
@@ skipping 712 matching lines @@
 
   CHECK(debug_weak_this.get());
   for (const auto& surface_id : frame.metadata.referenced_surfaces) {
     if (!contained_surfaces_.count(surface_id)) {
       result->undrawn_surfaces.insert(surface_id);
       PrewalkTree(surface_id, false, 0, result);
     }
   }
 
   CHECK(debug_weak_this.get());
-  if (surface->factory()) {
-    surface->factory()->WillDrawSurface(
-        surface->surface_id().local_surface_id(), damage_rect);
-  }
+  if (!damage_rect.IsEmpty())

[danakj, 2017/04/20 13:56:45]

Can you leave a TODO that this shouldn't need to call the callback when the
damage is from |surface| and not from |child_surfaces|?

[Alex Z., 2017/04/20 14:05:03]

Done.

+    surface->RunWillDrawCallbacks(damage_rect);

[danakj, 2017/04/20 13:56:45]

Also, DelegatedFrameHost doesn't need to check for an empty rect now.

[Alex Z., 2017/04/20 14:05:03]

Done. Does DFH need to check the local surface id? When would the DFH
have a different local surface id from the surface?

[danakj, 2017/04/20 14:18:43]

I'm not sure, you could look at when each is set to reason it out?

Also - are there no unit tests that verify that WillDraw happens, I didn't see
one here? Would you be able to add one if not?

[Alex Z., 2017/04/20 17:42:26]

DelegatedFrameHost updates local_surface_id_ in 
SubmitdCompositorFrame after submitting the frame to SurfaceFactory.
I replaced the if in DFH with a DCHECK and the check failed during
resizing. It seems that there a race.
SurfaceAggregatorTest.SimpleFrame verifies that WillDraw happens.

 
   CHECK(debug_weak_this.get());
   for (const auto& render_pass : frame.render_pass_list) {
     if (!render_pass->copy_requests.empty()) {
       int remapped_pass_id = RemapPassId(render_pass->id, surface_id);
       copy_request_passes_.insert(remapped_pass_id);
     }
   }
 
   referenced_surfaces_.erase(referenced_surfaces_.find(surface->surface_id()));
@@ skipping 156 matching lines @@
 }
 
 void SurfaceAggregator::SetOutputColorSpace(
     const gfx::ColorSpace& blending_color_space,
     const gfx::ColorSpace& output_color_space) {
   blending_color_space_ = blending_color_space;
   output_color_space_ = output_color_space;
 }
 
 }  // namespace cc