+1...I wonder if any tests would fail with this change?

I've had a look at this issue. It seems that the first version of CompositorFrameSinkSupport::SubmitCompositorFrame() which is overridden from mojom::CompositorFrameSink is never called. Actually, I can't see why CompositorFrameSinkSupport implements mojom::CompositorFrameSink! Apparently no one calls into CompositorFrameSinkSupport through mojom::CompositorFrameSink.

It seems to me that we can simply remove that inheritance and the first version of SubmitCompositorFrame() and everything should be fine. Other functions inherited from mojom::CompositorFrameSink (i.e., SetNeedsBeginFrame() and DidNotProduceFrame()) are still needed, just no need to override anything.

fsamuel@, samans@: WDYT?

CompositorFrameSinkSupport::SubmitCompositorFrame is called via mojom::CompositorFrameSink when doing mojo calls.

+kylechar because he introduced the inheritance.

I made CompositorFrameSinkSupport implement mojom::CompositorFrameSink for two reasons:

1. We don't want the the CompositorFrameSinkSupport and mojom::CompositorFrameSink interfaces to diverge. Everything in the browser/renderer will need use mojom::CompositorFrameSink soon.
2. It would be possible to make CompositorFrameSinkImpl and RootCompositorFrameSinkImpl inherit from CompositorFrameSinkSupport and remove a bunch of forwarding functions. I finished some more cleanup that makes this possible recently.

We could make CFFS no longer inherit from mojom::CFS, although I'd be a little worried about divergence. Removing the function that returns a bool could also work, but things DCHECK on that bool value, and the mojom function can't have a return value.

A false is basically a surface invariants violation. Kyle last week suggested alternative plumbing for surface invariants violations to the HostFrameSinkManager. Once that work is done we can get rid of the bool version of SubmitCompositorFrame I think.

Re C#6: Apparently it is not called through mojom::CompositorFrameSink interface at all and calls through that interface go to (Root)CompositorFrameSinkImpl which in turn calls the second version. In other words, the fact that CompositorFrameSinkSupport inherits mojom::CompositorFrameSink is not used in the code as far as I can tell other than forcing the same interface as mojom::CompositorFrameSink.

Re C#8: These points make sense to me except that we have already diverged the interfaces by adding the second version of SubmitCompositorFrame() that is used everywhere while the first version is not used anywhere.

If we want to keep the inheritance, I think samans@'s approach in the bug description is the best we can do. This way all calls to the second version that do not check the return value would automatically switch to call the first version which performs a DCHECK on the result. In cases that we need the return value (there are few places in code that check it), we can call the second version and use |result|.

I've prepared a CL with this change and if the CQ dry run is successful on it (i.e. the newly added DECHECK does not fail any test) I'll publish it for review: https://crrev/c/865475.

#10: Calls from the renderer do not go through RootCompositorFrameSinkImpl.

Re #11: But they go through CompositorFrameSinkImpl. Right? By (Root)CompositorFrameSinkImpl I meant both CompositorFrameSinkImpl and RootCompositorFrameSinkImpl.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a4f6bb4eb9f44a1be9ffb827a9b8cdb0680aa333

commit a4f6bb4eb9f44a1be9ffb827a9b8cdb0680aa333
Author: Mohsen Izadi <mohsen@chromium.org>
Date: Mon Jan 29 21:03:39 2018

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/865475
Reviewed-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}
[modify] https://crrev.com/a4f6bb4eb9f44a1be9ffb827a9b8cdb0680aa333/android_webview/browser/hardware_renderer.cc
[modify] https://crrev.com/a4f6bb4eb9f44a1be9ffb827a9b8cdb0680aa333/android_webview/browser/surfaces_instance.cc
[modify] https://crrev.com/a4f6bb4eb9f44a1be9ffb827a9b8cdb0680aa333/components/viz/service/display/surface_aggregator_unittest.cc
[modify] https://crrev.com/a4f6bb4eb9f44a1be9ffb827a9b8cdb0680aa333/components/viz/service/frame_sinks/compositor_frame_sink_impl.cc
[modify] https://crrev.com/a4f6bb4eb9f44a1be9ffb827a9b8cdb0680aa333/components/viz/service/frame_sinks/compositor_frame_sink_support.cc
[modify] https://crrev.com/a4f6bb4eb9f44a1be9ffb827a9b8cdb0680aa333/components/viz/service/frame_sinks/compositor_frame_sink_support.h
[modify] https://crrev.com/a4f6bb4eb9f44a1be9ffb827a9b8cdb0680aa333/components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc
[modify] https://crrev.com/a4f6bb4eb9f44a1be9ffb827a9b8cdb0680aa333/components/viz/service/frame_sinks/direct_layer_tree_frame_sink.cc
[modify] https://crrev.com/a4f6bb4eb9f44a1be9ffb827a9b8cdb0680aa333/components/viz/service/frame_sinks/root_compositor_frame_sink_impl.cc
[modify] https://crrev.com/a4f6bb4eb9f44a1be9ffb827a9b8cdb0680aa333/components/viz/service/frame_sinks/surface_synchronization_unittest.cc
[modify] https://crrev.com/a4f6bb4eb9f44a1be9ffb827a9b8cdb0680aa333/components/viz/service/surfaces/surface_unittest.cc
[modify] https://crrev.com/a4f6bb4eb9f44a1be9ffb827a9b8cdb0680aa333/components/viz/test/test_layer_tree_frame_sink.cc
[modify] https://crrev.com/a4f6bb4eb9f44a1be9ffb827a9b8cdb0680aa333/content/browser/renderer_host/delegated_frame_host.cc
[modify] https://crrev.com/a4f6bb4eb9f44a1be9ffb827a9b8cdb0680aa333/content/browser/renderer_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/a4f6bb4eb9f44a1be9ffb827a9b8cdb0680aa333/content/renderer/android/synchronous_layer_tree_frame_sink.cc
[modify] https://crrev.com/a4f6bb4eb9f44a1be9ffb827a9b8cdb0680aa333/ui/android/delegated_frame_host_android.cc
[modify] https://crrev.com/a4f6bb4eb9f44a1be9ffb827a9b8cdb0680aa333/ui/aura/local/layer_tree_frame_sink_local.cc