Replacing SurfaceReferenceBase and SequenceSurfaceReference with Closures

cc/surfaces/surface_reference_factory.h

Index: cc/surfaces/surface_reference_factory.h
diff --git a/cc/surfaces/surface_reference_factory.h b/cc/surfaces/surface_reference_factory.h
index a4a32afa5667029acdef1c326bc267fbc5f76e92..c870a634affe0de3a5bafbc1c1e0c91cdf806d49 100644
--- a/cc/surfaces/surface_reference_factory.h
+++ b/cc/surfaces/surface_reference_factory.h
@@ -5,24 +5,22 @@
 #ifndef CC_SURFACES_SURFACE_REFERENCE_FACTORY_H_
 #define CC_SURFACES_SURFACE_REFERENCE_FACTORY_H_
 
+#include "base/callback_forward.h"
 #include "base/memory/ref_counted.h"
 #include "cc/surfaces/surface_id.h"
 #include "cc/surfaces/surface_reference_owner.h"
 
 namespace cc {
 
-class SurfaceReferenceBase;
-
-// Creates surface references. Returns an object of type
-// SurfaceReferenceBase which holds on to its corresponding
-// surface reference until destruction. The referenced surface
-// will be kept alive as long as there is a reference to it.
+// Creates surface references. The referenced surface will be kept alive as
+// long as there is a reference to it.
 class SurfaceReferenceFactory
     : public base::RefCountedThreadSafe<SurfaceReferenceFactory> {
  public:
-  virtual std::unique_ptr<SurfaceReferenceBase> CreateReference(
-      SurfaceReferenceOwner* owner,
-      const SurfaceId& surface_id) const = 0;
+  // Creates a reference to the surface with the given surface id and returns
+  // a closure that can be called to remove the reference.

[danakj, 2017/01/10 21:12:06]

"must be called"? It can only be called a single time also right?

[Fady Samuel, 2017/01/10 21:33:07]

Yes it can only be called a single time.

[Saman Sami, 2017/01/10 21:56:14]

Done.

+  virtual base::Closure CreateReference(SurfaceReferenceOwner* owner,
+                                        const SurfaceId& surface_id) const = 0;
 
   SurfaceReferenceFactory() = default;

[danakj, 2017/01/10 21:12:06]

why does this class have a constructor?

[Saman Sami, 2017/01/10 21:56:14]

"call to implicitly-deleted default constructor of
'cc::SequenceSurfaceReferenceFactory'" I guess because of
DISALLOW_COPY_AND_ASSIGN?

[danakj, 2017/01/11 15:25:28]

Oh, there is no need for DISALLOW_COPY_AND_ASSIGN for an interface IMO. I'd drop
both and keep the smallest LOC possible for the interface - just the virtual
methods. It should be the subclass that is refcounted (tho better to not use at
all) also, not the interface. There are some cases where you have to refcount
the interface because of multiple inheritance problems but I don't think this is
one. Maybe we can improve this separately.

[Saman Sami, 2017/01/11 16:19:16]

But we have scoped_refptr<SurfaceReferenceFactory>. Doesn't that mean it should
be ref counted?

 
@@ -30,11 +28,8 @@ class SurfaceReferenceFactory
   virtual ~SurfaceReferenceFactory() = default;
 
  private:
-  friend class SurfaceReferenceBase;
   friend class base::RefCountedThreadSafe<SurfaceReferenceFactory>;
 
-  virtual void DestroyReference(SurfaceReferenceBase* surface_ref) const = 0;
-
   DISALLOW_COPY_AND_ASSIGN(SurfaceReferenceFactory);
 };