Remove SurfaceFactory And SurfaceFactoryClient

cc/surfaces/compositor_frame_sink_support.h

Index: cc/surfaces/compositor_frame_sink_support.h
diff --git a/cc/surfaces/compositor_frame_sink_support.h b/cc/surfaces/compositor_frame_sink_support.h
index 33e44f9e5423ea1d88860ec1446b236a6691266d..c59ed1323c0bf0d1fb48111a02f9256be43bff2d 100644
--- a/cc/surfaces/compositor_frame_sink_support.h
+++ b/cc/surfaces/compositor_frame_sink_support.h
@@ -14,23 +14,27 @@
 #include "cc/output/compositor_frame.h"
 #include "cc/scheduler/begin_frame_source.h"
 #include "cc/surfaces/framesink_manager_client.h"
+#include "cc/surfaces/pending_frame_observer.h"
 #include "cc/surfaces/referenced_surface_tracker.h"
-#include "cc/surfaces/surface_factory.h"
-#include "cc/surfaces/surface_factory_client.h"
 #include "cc/surfaces/surface_id.h"
+#include "cc/surfaces/surface_resource_holder.h"
 #include "cc/surfaces/surface_resource_holder_client.h"
 #include "cc/surfaces/surfaces_export.h"
 
 namespace cc {
+namespace test {
+class CompositorFrameSinkSupportTest;
+}
 
 class CompositorFrameSinkSupportClient;
+class SurfaceAggregator;
 class SurfaceManager;
 
 class CC_SURFACES_EXPORT CompositorFrameSinkSupport
-    : public SurfaceFactoryClient,
-      public BeginFrameObserver,
+    : public BeginFrameObserver,
       public SurfaceResourceHolderClient,
-      public FrameSinkManagerClient {
+      public FrameSinkManagerClient,
+      public PendingFrameObserver {
  public:
   static std::unique_ptr<CompositorFrameSinkSupport> Create(
       CompositorFrameSinkSupportClient* client,
@@ -44,19 +48,14 @@ class CC_SURFACES_EXPORT CompositorFrameSinkSupport
 
   const FrameSinkId& frame_sink_id() const { return frame_sink_id_; }
 
-  Surface* current_surface_for_testing() {
-    return surface_factory_->current_surface_for_testing();
-  }
+  Surface* current_surface_for_testing() { return current_surface_.get(); }
+  SurfaceManager* surface_manager() { return surface_manager_; }
+  bool needs_sync_points() { return needs_sync_points_; }
 
   const ReferencedSurfaceTracker& ReferenceTrackerForTesting() const {
     return reference_tracker_;
   }
 
-  // SurfaceFactoryClient implementation.
-  void ReferencedSurfacesChanged(
-      const LocalSurfaceId& local_surface_id,
-      const std::vector<SurfaceId>* active_referenced_surfaces) override;
-
   // SurfaceResourceHolderClient implementation.
   void ReturnResources(const ReturnedResourceArray& resources) override;
 
@@ -80,6 +79,33 @@ class CC_SURFACES_EXPORT CompositorFrameSinkSupport
   void Init(SurfaceManager* surface_manager, bool needs_sync_points);
 
  private:
+  // Surface calls ReceiveFromChild from Surface::QueueFrame to receive and

[danakj, 2017/05/01 22:21:39]

It looks like this is creating a method-call pattern of 

A::Foo() -> B::Bar() -> A::Baz()

Which creates a cycle between classes and is usually a code smell issue for me.
Why would this class call Surface::QueueFrame to have it call back to
ReceiveFromChild? Why wouldn't this class just do its ReceiveFromChild work on
its own, then call QueueFrame (or whatever order works)?

[Alex Z., 2017/05/02 14:46:17]

CompositorFrameSinkSupport now calls ReceiveFromChild prior to 
QueueFrame. I'm still thinking about Ref/UnrefResources.

fsamuel@ doesn't want to expose them because they are not part of the 
public CompositorFrameSink interface.

+  // track the resources referenced from the CompositorFrame regardless of
+  // whether it's pending or active.
+  // Surface calls UnrefResources in
+  // Surface::UnrefFrameResourcesAndRunDrawCallback. It unrefs resources
+  // referenced from a the CompositorFrame when it's no longer needed (e.g.
+  // when the Surface is destroyed, or when the frame is activated).
+  // TODO(staraz): Make a SurfaceClient interface so Surface has access to these
+  // methods without having to be a friend.
+  friend class Surface;
+
+  // SurfaceAggregator calls RefResources in PrewalkTree to validate the

[danakj, 2017/05/01 22:21:39]

I don't understand this pattern here of making friends for classes. If the
method is used externally, then why isn't it public? It looks like that would
cover tests, too, which are just testing the public API used by the aggregator.
Can we make Ref/Unref public?

[Alex Z., 2017/05/02 16:34:32]

Ref/UnrefResources are public now. I added a TODO to find a workaround 
and set them back to private in the future.

+  // resources.
+  // SurfaceAggregator calls UnrefResources in UnrefHelper, which is passed to
+  // ResourceProvider::CreateChild as |return_callback|.
+  friend class SurfaceAggregator;
+
+  // CompositorFrameSinkSupportTest calls RefResources and UnrefResources in its
+  // resources life time tests. The tests verify that the resources referenced
+  // by a combination of CompositorFrames and external references are returned
+  // at the correct occasion.
+  friend class test::CompositorFrameSinkSupportTest;
+
+  void ReceiveFromChild(const TransferableResourceArray& resources);
+  void RefResources(const TransferableResourceArray& resources);
+  void UnrefResources(const ReturnedResourceArray& resources);
+
   // Update surface references with SurfaceManager for current CompositorFrame
   // that has |local_surface_id|. UpdateReferences() must be called on
   // |reference_tracker_| before calling this. Will add and remove top-level
@@ -89,6 +115,9 @@ class CC_SURFACES_EXPORT CompositorFrameSinkSupport
 
   void AddTopLevelRootReference(const SurfaceId& surface_id);
   void RemoveTopLevelRootReference(const SurfaceId& surface_id);
+  void ReferencedSurfacesChanged(
+      const LocalSurfaceId& local_surface_id,
+      const std::vector<SurfaceId>* active_referenced_surfaces);
 
   void DidReceiveCompositorFrameAck();
   void WillDrawSurface(const LocalSurfaceId& local_surface_id,
@@ -99,7 +128,18 @@ class CC_SURFACES_EXPORT CompositorFrameSinkSupport
   const BeginFrameArgs& LastUsedBeginFrameArgs() const override;
   void OnBeginFrameSourcePausedChanged(bool paused) override;
 
+  // PendingFrameObserver implementation.
+  void OnSurfaceActivated(Surface* surface) override;
+  void OnSurfaceDependenciesChanged(
+      Surface* surface,
+      const SurfaceDependencies& added_dependencies,
+      const SurfaceDependencies& removed_dependencies) override;
+  void OnSurfaceDiscarded(Surface* surface) override;
+
   void UpdateNeedsBeginFramesInternal();
+  std::unique_ptr<Surface> CreateSurface(
+      const LocalSurfaceId& local_surface_id);
+  void DestroyCurrentSurface();
 
   CompositorFrameSinkSupportClient* const client_;
 
@@ -107,7 +147,9 @@ class CC_SURFACES_EXPORT CompositorFrameSinkSupport
 
   const FrameSinkId frame_sink_id_;
 
-  std::unique_ptr<SurfaceFactory> surface_factory_;
+  SurfaceResourceHolder surface_resource_holder_;
+
+  std::unique_ptr<Surface> current_surface_;
   // Counts the number of CompositorFrames that have been submitted and have not
   // yet received an ACK.
   int ack_pending_count_ = 0;
@@ -130,6 +172,8 @@ class CC_SURFACES_EXPORT CompositorFrameSinkSupport
   ReferencedSurfaceTracker reference_tracker_;
 
   const bool is_root_;
+  bool needs_sync_points_;
+  bool seen_first_frame_activation_ = false;
 
   // TODO(staraz): Remove this flag once ui::Compositor no longer needs to call
   // RegisterFrameSinkId().