Improve CompositorFrameSinkSupport::SubmitCompositorFrame() interface

There are two versions of this method available, one inherited from
mojom::CompositorFrameSink interface and one that actually implements
the functionality. Both are public and a bit confusing as to which
should be used. After this change, callers should prefer to use the one
that is inherited from mojom::CompositorFrameSink and call the other one
only when they want to see if the call was successful or not. The first
version DCHECK's on the success of the call.

BUG=776154
TEST=none

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I26a0a23f8e2ee807c3a2ea8a5ce7004ac2801c85
Reviewed-on: https://chromium-review.googlesource.com/865475Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Commit-Queue: Mohsen Izadi <mohsen@chromium.org>
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532586}

android_webview/browser/hardware_renderer.cc

@@ -135,9 +135,8 @@ void HardwareRenderer::DrawGL(AwDrawGLInfo* draw_info) {
       device_scale_factor_ = device_scale_factor;
     }
 
-    bool result = support_->SubmitCompositorFrame(
-        child_id_, std::move(*child_compositor_frame));
-    DCHECK(result);
+    support_->SubmitCompositorFrame(child_id_,
+                                    std::move(*child_compositor_frame));
   }
 
   gfx::Transform transform(gfx::Transform::kSkipInitialization);

android_webview/browser/surfaces_instance.cc

@@ -157,8 +157,7 @@ void SurfacesInstance::DrawAndSwap(const gfx::Size& viewport,
     device_scale_factor_ = device_scale_factor;
     display_->SetLocalSurfaceId(root_id_, device_scale_factor);
   }
-  bool result = support_->SubmitCompositorFrame(root_id_, std::move(frame));
-  DCHECK(result);
+  support_->SubmitCompositorFrame(root_id_, std::move(frame));
 
   display_->Resize(viewport);
   display_->DrawAndSwap();
@@ -202,8 +201,7 @@ void SurfacesInstance::SetSolidColorRootFrame() {
       viz::BeginFrameAck::CreateManualAckWithDamage();
   frame.metadata.referenced_surfaces = child_ids_;
   frame.metadata.device_scale_factor = device_scale_factor_;
-  bool result = support_->SubmitCompositorFrame(root_id_, std::move(frame));
-  DCHECK(result);
+  support_->SubmitCompositorFrame(root_id_, std::move(frame));
 }
 
 void SurfacesInstance::DidReceiveCompositorFrameAck(

components/viz/service/display/surface_aggregator_unittest.cc

@@ -693,6 +693,7 @@ TEST_F(SurfaceAggregatorValidSurfaceTest, FallbackSurfaceReference) {
   testing::Mock::VerifyAndClearExpectations(&aggregated_damage_callback);
 
   // Submit the fallback again to create some damage then aggregate again.
+  fallback_child_local_surface_id = allocator_.GenerateId();
   SubmitCompositorFrame(fallback_child_support.get(), fallback_child_passes,
                         arraysize(fallback_child_passes),
                         fallback_child_local_surface_id, device_scale_factor_1);

components/viz/service/frame_sinks/compositor_frame_sink_impl.cc

@@ -43,8 +43,10 @@ void CompositorFrameSinkImpl::SubmitCompositorFrame(
     CompositorFrame frame,
     mojom::HitTestRegionListPtr hit_test_region_list,
     uint64_t submit_time) {
-  if (!support_->SubmitCompositorFrame(local_surface_id, std::move(frame),
-                                       std::move(hit_test_region_list))) {
+  bool success;
+  support_->SubmitCompositorFrame(local_surface_id, std::move(frame),
+                                  std::move(hit_test_region_list), &success);
+  if (!success) {
     DLOG(ERROR) << "SubmitCompositorFrame failed for " << local_surface_id;
     compositor_frame_sink_binding_.CloseWithReason(
         1, "Surface invariants violation");

components/viz/service/frame_sinks/compositor_frame_sink_support.cc

@@ -170,18 +170,23 @@ void CompositorFrameSinkSupport::SubmitCompositorFrame(
     CompositorFrame frame,
     mojom::HitTestRegionListPtr hit_test_region_list,
     uint64_t submit_time) {
+  bool success;
   SubmitCompositorFrame(local_surface_id, std::move(frame),
-                        std::move(hit_test_region_list));
+                        std::move(hit_test_region_list), &success);
+  DCHECK(success);
 }
 
-bool CompositorFrameSinkSupport::SubmitCompositorFrame(
+void CompositorFrameSinkSupport::SubmitCompositorFrame(
     const LocalSurfaceId& local_surface_id,
     CompositorFrame frame,
-    mojom::HitTestRegionListPtr hit_test_region_list) {
+    mojom::HitTestRegionListPtr hit_test_region_list,
+    bool* success) {
   TRACE_EVENT1("viz", "CompositorFrameSinkSupport::SubmitCompositorFrame",
                "FrameSinkId", frame_sink_id_.ToString());
   DCHECK(local_surface_id.is_valid());
   DCHECK(!frame.render_pass_list.empty());
+  DCHECK(success);
+  *success = true;
 
   uint64_t frame_index = ++last_frame_index_;
   ++ack_pending_count_;
@@ -240,7 +245,8 @@ bool CompositorFrameSinkSupport::SubmitCompositorFrame(
         DidPresentCompositorFrame(frame.metadata.presentation_token,
                                   base::TimeTicks(), base::TimeDelta(), 0);
       }
-      return false;
+      *success = false;
+      return;
     }
 
     current_surface = CreateSurface(surface_info);
@@ -261,7 +267,8 @@ bool CompositorFrameSinkSupport::SubmitCompositorFrame(
           : Surface::PresentedCallback());
   if (!result) {
     EvictCurrentSurface();
-    return false;
+    *success = false;
+    return;
   }
 
   if (prev_surface && prev_surface != current_surface) {
@@ -274,8 +281,6 @@ bool CompositorFrameSinkSupport::SubmitCompositorFrame(
 
   if (begin_frame_source_)
     begin_frame_source_->DidFinishFrame(this);
-
-  return true;
 }
 
 void CompositorFrameSinkSupport::UpdateSurfaceReferences(

components/viz/service/frame_sinks/compositor_frame_sink_support.h

@@ -82,21 +82,24 @@ class VIZ_SERVICE_EXPORT CompositorFrameSinkSupport
   void SetNeedsBeginFrame(bool needs_begin_frame) override;
   void SetWantsAnimateOnlyBeginFrames() override;
   void DidNotProduceFrame(const BeginFrameAck& ack) override;
-  void SubmitCompositorFrame(const LocalSurfaceId& local_surface_id,
-                             CompositorFrame frame,
-                             mojom::HitTestRegionListPtr hit_test_region_list,
-                             uint64_t submit_time) override;
+  void SubmitCompositorFrame(
+      const LocalSurfaceId& local_surface_id,
+      CompositorFrame frame,
+      mojom::HitTestRegionListPtr hit_test_region_list = nullptr,
+      uint64_t submit_time = 0) override;
 
   void EvictCurrentSurface();
 
   // Submits a new CompositorFrame to |local_surface_id|. If |local_surface_id|
   // hasn't been submitted to before then a new Surface will be created for it.
-  // Returns false if |frame| was rejected due to invalid data.
-  // TODO(kylechar): Merge the two SubmitCompositorFrame() methods.
-  bool SubmitCompositorFrame(
-      const LocalSurfaceId& local_surface_id,
-      CompositorFrame frame,
-      mojom::HitTestRegionListPtr hit_test_region_list = nullptr);
+  // Sets |success| to false if |frame| was rejected due to invalid data.
+  // SubmitCompositorFrame() that is inherited from mojom::CompositorFrameSink
+  // calls this one and DCHECK's |success|. Callers should prefer to call the
+  // other one unless they really want to check the value of |success|.
+  void SubmitCompositorFrame(const LocalSurfaceId& local_surface_id,
+                             CompositorFrame frame,
+                             mojom::HitTestRegionListPtr hit_test_region_list,
+                             bool* success);
 
   // CapturableFrameSink implementation.
   void AttachCaptureClient(CapturableFrameSink::Client* client) override;

components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc

@@ -494,18 +494,25 @@ TEST_F(CompositorFrameSinkSupportTest, MonotonicallyIncreasingLocalSurfaceIds) {
   LocalSurfaceId local_surface_id4(5, 3, kArbitraryToken);
   LocalSurfaceId local_surface_id5(8, 1, kArbitraryToken);
   LocalSurfaceId local_surface_id6(9, 3, kArbitraryToken);
-  EXPECT_TRUE(support->SubmitCompositorFrame(local_surface_id1,
-                                             MakeDefaultCompositorFrame()));
-  EXPECT_TRUE(support->SubmitCompositorFrame(local_surface_id2,
-                                             MakeDefaultCompositorFrame()));
-  EXPECT_TRUE(support->SubmitCompositorFrame(local_surface_id3,
-                                             MakeDefaultCompositorFrame()));
-  EXPECT_FALSE(support->SubmitCompositorFrame(local_surface_id4,
-                                              MakeDefaultCompositorFrame()));
-  EXPECT_FALSE(support->SubmitCompositorFrame(local_surface_id5,
-                                              MakeDefaultCompositorFrame()));
-  EXPECT_TRUE(support->SubmitCompositorFrame(local_surface_id6,
-                                             MakeDefaultCompositorFrame()));
+  bool success;
+  support->SubmitCompositorFrame(
+      local_surface_id1, MakeDefaultCompositorFrame(), nullptr, &success);
+  EXPECT_TRUE(success);
+  support->SubmitCompositorFrame(
+      local_surface_id2, MakeDefaultCompositorFrame(), nullptr, &success);
+  EXPECT_TRUE(success);
+  support->SubmitCompositorFrame(
+      local_surface_id3, MakeDefaultCompositorFrame(), nullptr, &success);
+  EXPECT_TRUE(success);
+  support->SubmitCompositorFrame(
+      local_surface_id4, MakeDefaultCompositorFrame(), nullptr, &success);
+  EXPECT_FALSE(success);
+  support->SubmitCompositorFrame(
+      local_surface_id5, MakeDefaultCompositorFrame(), nullptr, &success);
+  EXPECT_FALSE(success);
+  support->SubmitCompositorFrame(
+      local_surface_id6, MakeDefaultCompositorFrame(), nullptr, &success);
+  EXPECT_TRUE(success);
 
   support->EvictCurrentSurface();
   manager_.InvalidateFrameSinkId(kAnotherArbitraryFrameSinkId);
@@ -655,8 +662,10 @@ TEST_F(CompositorFrameSinkSupportTest, ZeroFrameSize) {
   SurfaceId id(support_->frame_sink_id(), local_surface_id_);
   auto frame =
       CompositorFrameBuilder().AddRenderPass(gfx::Rect(), gfx::Rect()).Build();
-  EXPECT_FALSE(
-      support_->SubmitCompositorFrame(local_surface_id_, std::move(frame)));
+  bool success;
+  support_->SubmitCompositorFrame(local_surface_id_, std::move(frame), nullptr,
+                                  &success);
+  EXPECT_FALSE(success);
   EXPECT_FALSE(GetSurfaceForId(id));
 }
 
@@ -668,8 +677,10 @@ TEST_F(CompositorFrameSinkSupportTest, ZeroDeviceScaleFactor) {
                    .AddDefaultRenderPass()
                    .SetDeviceScaleFactor(0.f)
                    .Build();
-  EXPECT_FALSE(
-      support_->SubmitCompositorFrame(local_surface_id_, std::move(frame)));
+  bool success;
+  support_->SubmitCompositorFrame(local_surface_id_, std::move(frame), nullptr,
+                                  &success);
+  EXPECT_FALSE(success);
   EXPECT_FALSE(GetSurfaceForId(id));
 }
 
@@ -682,8 +693,10 @@ TEST_F(CompositorFrameSinkSupportTest, FrameSizeMismatch) {
   auto frame = CompositorFrameBuilder()
                    .AddRenderPass(gfx::Rect(5, 5), gfx::Rect())
                    .Build();
-  EXPECT_TRUE(
-      support_->SubmitCompositorFrame(local_surface_id_, std::move(frame)));
+  bool success;
+  support_->SubmitCompositorFrame(local_surface_id_, std::move(frame), nullptr,
+                                  &success);
+  EXPECT_TRUE(success);
   EXPECT_TRUE(GetSurfaceForId(id));
 
   // Submit a frame with size (5,4). This frame should be rejected and the
@@ -691,8 +704,9 @@ TEST_F(CompositorFrameSinkSupportTest, FrameSizeMismatch) {
   frame = CompositorFrameBuilder()
               .AddRenderPass(gfx::Rect(5, 4), gfx::Rect())
               .Build();
-  EXPECT_FALSE(
-      support_->SubmitCompositorFrame(local_surface_id_, std::move(frame)));
+  support_->SubmitCompositorFrame(local_surface_id_, std::move(frame), nullptr,
+                                  &success);
+  EXPECT_FALSE(success);
   manager_.surface_manager()->GarbageCollectSurfaces();
   EXPECT_FALSE(GetSurfaceForId(id));
 }
@@ -708,8 +722,10 @@ TEST_F(CompositorFrameSinkSupportTest, DeviceScaleFactorMismatch) {
                    .AddDefaultRenderPass()
                    .SetDeviceScaleFactor(0.5f)
                    .Build();
-  EXPECT_TRUE(
-      support_->SubmitCompositorFrame(local_surface_id_, std::move(frame)));
+  bool success;
+  support_->SubmitCompositorFrame(local_surface_id_, std::move(frame), nullptr,
+                                  &success);
+  EXPECT_TRUE(success);
   EXPECT_TRUE(GetSurfaceForId(id));
 
   // Submit a frame with device scale factor of 0.4. This frame should be
@@ -718,8 +734,9 @@ TEST_F(CompositorFrameSinkSupportTest, DeviceScaleFactorMismatch) {
               .AddDefaultRenderPass()
               .SetDeviceScaleFactor(0.4f)
               .Build();
-  EXPECT_FALSE(
-      support_->SubmitCompositorFrame(local_surface_id_, std::move(frame)));
+  support_->SubmitCompositorFrame(local_surface_id_, std::move(frame), nullptr,
+                                  &success);
+  EXPECT_FALSE(success);
   manager_.surface_manager()->GarbageCollectSurfaces();
   EXPECT_FALSE(GetSurfaceForId(id));
 }

components/viz/service/frame_sinks/direct_layer_tree_frame_sink.cc

@@ -112,9 +112,7 @@ void DirectLayerTreeFrameSink::SubmitCompositorFrame(CompositorFrame frame) {
     display_->SetLocalSurfaceId(local_surface_id_, device_scale_factor_);
   }
 
-  bool result =
-      support_->SubmitCompositorFrame(local_surface_id_, std::move(frame));
-  DCHECK(result);
+  support_->SubmitCompositorFrame(local_surface_id_, std::move(frame));
 }
 
 void DirectLayerTreeFrameSink::DidNotProduceFrame(const BeginFrameAck& ack) {

components/viz/service/frame_sinks/root_compositor_frame_sink_impl.cc

@@ -103,8 +103,10 @@ void RootCompositorFrameSinkImpl::SubmitCompositorFrame(
     display_->SetLocalSurfaceId(local_surface_id, frame.device_scale_factor());
   }
 
-  if (!support_->SubmitCompositorFrame(local_surface_id, std::move(frame),
-                                       std::move(hit_test_region_list))) {
+  bool success;
+  support_->SubmitCompositorFrame(local_surface_id, std::move(frame),
+                                  std::move(hit_test_region_list), &success);
+  if (!success) {
     DLOG(ERROR) << "SubmitCompositorFrame failed for " << local_surface_id;
     compositor_frame_sink_binding_.Close();
     OnClientConnectionLost();

components/viz/service/frame_sinks/surface_synchronization_unittest.cc

@@ -1995,8 +1995,8 @@ TEST_F(SurfaceSynchronizationTest, LatestInFlightSurface) {
 
   // Submit a child CompositorFrame to a new SurfaceId and verify that
   // GetLatestInFlightSurface returns the right surface.
-  EXPECT_TRUE(child_support1().SubmitCompositorFrame(
-      child_id3.local_surface_id(), MakeDefaultCompositorFrame()));
+  child_support1().SubmitCompositorFrame(child_id3.local_surface_id(),
+                                         MakeDefaultCompositorFrame());
 
   // Verify that there is a temporary reference for child_id3.
   EXPECT_TRUE(HasTemporaryReference(child_id3));

components/viz/service/surfaces/surface_unittest.cc

@@ -58,32 +58,6 @@ TEST(SurfaceTest, PresentationCallback) {
     support->SubmitCompositorFrame(local_surface_id, std::move(frame));
     testing::Mock::VerifyAndClearExpectations(&client);
   }
-
-  {
-    // Submits a frame with token 3 and different size. This frame with token 3
-    // will be discarded immediately.
-    CompositorFrame frame = CompositorFrameBuilder()
-                                .AddRenderPass(gfx::Rect(400, 400), kDamageRect)
-                                .SetPresentationToken(3)
-                                .Build();
-    EXPECT_CALL(client, DidDiscardCompositorFrame(3)).Times(1);
-    support->SubmitCompositorFrame(local_surface_id, std::move(frame));
-    testing::Mock::VerifyAndClearExpectations(&client);
-  }
-
-  {
-    // Submits a frame with token 4 and different scale factor, this frame with
-    // token 4 will be discarded immediately.
-    CompositorFrame frame =
-        CompositorFrameBuilder()
-            .AddRenderPass(gfx::Rect(kSurfaceSize), kDamageRect)
-            .SetDeviceScaleFactor(2.f)
-            .SetPresentationToken(4)
-            .Build();
-    EXPECT_CALL(client, DidDiscardCompositorFrame(2)).Times(1);
-    EXPECT_CALL(client, DidDiscardCompositorFrame(4)).Times(1);
-    support->SubmitCompositorFrame(local_surface_id, std::move(frame));
-  }
 }
 
 TEST(SurfaceTest, SurfaceIds) {

components/viz/test/test_layer_tree_frame_sink.cc

@@ -163,9 +163,7 @@ void TestLayerTreeFrameSink::SubmitCompositorFrame(CompositorFrame frame) {
     device_scale_factor_ = device_scale_factor;
   }
 
-  bool result =
-      support_->SubmitCompositorFrame(local_surface_id_, std::move(frame));
-  DCHECK(result);
+  support_->SubmitCompositorFrame(local_surface_id_, std::move(frame));
 
   for (auto& copy_request : copy_requests_)
     support_->RequestCopyOfSurface(std::move(copy_request));

content/browser/renderer_host/delegated_frame_host.cc

@@ -507,9 +507,8 @@ void DelegatedFrameHost::SubmitCompositorFrame(
 
     // If surface synchronization is off, then OnFirstSurfaceActivation will be
     // called in the same call stack.
-    bool result = support_->SubmitCompositorFrame(
-        local_surface_id, std::move(frame), std::move(hit_test_region_list));
-    DCHECK(result);
+    support_->SubmitCompositorFrame(local_surface_id, std::move(frame),
+                                    std::move(hit_test_region_list));
   }
 }
 

content/browser/renderer_host/render_widget_host_view_child_frame.cc

@@ -580,9 +580,8 @@ void RenderWidgetHostViewChildFrame::ProcessCompositorFrame(
   current_surface_size_ = frame.size_in_pixels();
   current_surface_scale_factor_ = frame.device_scale_factor();
 
-  bool result = support_->SubmitCompositorFrame(
-      local_surface_id, std::move(frame), std::move(hit_test_region_list));
-  DCHECK(result);
+  support_->SubmitCompositorFrame(local_surface_id, std::move(frame),
+                                  std::move(hit_test_region_list));
   has_frame_ = true;
 
   if (last_received_local_surface_id_ != local_surface_id ||

content/renderer/android/synchronous_layer_tree_frame_sink.cc

@@ -322,12 +322,10 @@ void SynchronousLayerTreeFrameSink::SubmitCompositorFrame(
         viz::SurfaceId(kChildFrameSinkId, child_local_surface_id_),
         base::nullopt, SK_ColorWHITE, false);
 
-    bool result = child_support_->SubmitCompositorFrame(child_local_surface_id_,
-                                                        std::move(frame));
-    DCHECK(result);
-    result = root_support_->SubmitCompositorFrame(root_local_surface_id_,
-                                                  std::move(embed_frame));
-    DCHECK(result);
+    child_support_->SubmitCompositorFrame(child_local_surface_id_,
+                                          std::move(frame));
+    root_support_->SubmitCompositorFrame(root_local_surface_id_,
+                                         std::move(embed_frame));
     display_->DrawAndSwap();
   } else {
     // For hardware draws we send the whole frame to the client so it can draw

ui/android/delegated_frame_host_android.cc

@@ -92,9 +92,7 @@ void DelegatedFrameHostAndroid::SubmitCompositorFrame(
         viz::SurfaceId(frame_sink_id_, local_surface_id), 1.f, frame_size);
     has_transparent_background_ = root_pass->has_transparent_background;
 
-    bool result =
-        support_->SubmitCompositorFrame(local_surface_id, std::move(frame));
-    DCHECK(result);
+    support_->SubmitCompositorFrame(local_surface_id, std::move(frame));
 
     content_layer_ =
         CreateSurfaceLayer(surface_info_, !has_transparent_background_);

ui/aura/local/layer_tree_frame_sink_local.cc

@@ -87,9 +87,7 @@ void LayerTreeFrameSinkLocal::SubmitCompositorFrame(
 
   DCHECK(local_surface_id_.is_valid());
 
-  bool result =
-      support_->SubmitCompositorFrame(local_surface_id_, std::move(frame));
-  DCHECK(result);
+  support_->SubmitCompositorFrame(local_surface_id_, std::move(frame));
 }
 
 void LayerTreeFrameSinkLocal::DidNotProduceFrame(