Remove SurfaceFactory And SurfaceFactoryClient

cc/surfaces/compositor_frame_sink_support.h

 // Copyright 2016 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.
 
 #ifndef CC_SURFACES_COMPOSITOR_FRAME_SINK_SUPPORT_H_
 #define CC_SURFACES_COMPOSITOR_FRAME_SINK_SUPPORT_H_
 
 #include <memory>
 #include <unordered_set>
 #include <vector>
 
 #include "base/compiler_specific.h"
 #include "base/memory/weak_ptr.h"
 #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,
       SurfaceManager* surface_manager,
       const FrameSinkId& frame_sink_id,
       bool is_root,
       bool handles_frame_sink_id_invalidation,
       bool needs_sync_points);
 
   ~CompositorFrameSinkSupport() override;
 
   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;
 
   // FrameSinkManagerClient implementation.
   void SetBeginFrameSource(BeginFrameSource* begin_frame_source) override;
 
   void EvictFrame();
   void SetNeedsBeginFrame(bool needs_begin_frame);
   void BeginFrameDidNotSwap(const BeginFrameAck& ack);
   void SubmitCompositorFrame(const LocalSurfaceId& local_surface_id,
                              CompositorFrame frame);
   void RequestCopyOfSurface(std::unique_ptr<CopyOutputRequest> request);
   void ClaimTemporaryReference(const SurfaceId& surface_id);
 
  protected:
   CompositorFrameSinkSupport(CompositorFrameSinkSupportClient* client,
                              const FrameSinkId& frame_sink_id,
                              bool is_root,
                              bool handles_frame_sink_id_invalidation);
 
   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
   // root references if |display_| is not null.
   void UpdateSurfaceReferences(const SurfaceId& last_surface_id,
                                const LocalSurfaceId& local_surface_id);
 
   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,
                        const gfx::Rect& damage_rect);
 
   // BeginFrameObserver implementation.
   void OnBeginFrame(const BeginFrameArgs& args) override;
   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_;
 
   SurfaceManager* surface_manager_ = nullptr;
 
   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;
   ReturnedResourceArray surface_returned_resources_;
 
   // The begin frame source being observered. Null if none.
   BeginFrameSource* begin_frame_source_ = nullptr;
 
   // The last begin frame args generated by the begin frame source.
   BeginFrameArgs last_begin_frame_args_;
 
   // Whether a request for begin frames has been issued.
   bool needs_begin_frame_ = false;
 
   // Whether or not a frame observer has been added.
   bool added_frame_observer_ = false;
 
   // Track the surface references for the surface corresponding to this
   // compositor frame sink.
   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().
   // A surfaceSequence's validity is bound to the lifetime of the parent
   // FrameSink that created it. We track the lifetime of FrameSinks through
   // RegisterFrameSinkId and InvalidateFrameSinkId. During startup and GPU
   // restart, a SurfaceSequence created by the top most layer compositor may be
   // used prior to the creation of the associated CompositorFrameSinkSupport.
   // CompositorFrameSinkSupport is created asynchronously when a new GPU channel
   // is established. Once we switch to SurfaceReferences, this ordering concern
   // goes away and we can remove this bool.
   const bool handles_frame_sink_id_invalidation_;
 
   base::WeakPtrFactory<CompositorFrameSinkSupport> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(CompositorFrameSinkSupport);
 };
 
 }  // namespace cc
 
 #endif  // CC_SURFACES_COMPOSITOR_FRAME_SINK_SUPPORT_H_