Introducing SurfaceReferenceFactory

cc/surfaces/surface_embedding_base.h

Index: cc/surfaces/surface_embedding_base.h
diff --git a/cc/surfaces/surface_embedding_base.h b/cc/surfaces/surface_embedding_base.h
new file mode 100644
index 0000000000000000000000000000000000000000..7dd484d079c4d6132f2eed06d3290825ff67afdf
--- /dev/null
+++ b/cc/surfaces/surface_embedding_base.h
@@ -0,0 +1,87 @@
+// Copyright (c) 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_SURFACE_EMBEDDING_BASE_H_
+#define CC_SURFACES_SURFACE_EMBEDDING_BASE_H_
+
+#include <memory>
+
+#include "base/memory/ptr_util.h"
+#include "cc/surfaces/surface_id.h"
+#include "cc/surfaces/surface_sequence.h"
+#include "cc/surfaces/surface_sequence_generator.h"
+#include "ui/gfx/geometry/size.h"
+
+namespace cc {
+
+struct SurfaceInfo {

[Fady Samuel, 2016/12/07 23:29:05]

Put this in a separate file?

+  SurfaceId id;
+  float scale = 1.f;
+  gfx::Size size;
+
+  SurfaceInfo() {}
+  SurfaceInfo(const SurfaceId& id, float scale, gfx::Size size)
+      : id(id), scale(scale), size(size) {}
+  bool operator==(const SurfaceInfo& info) const {

[kylechar, 2016/12/08 02:02:46]

You should define != too if you define ==.

+    return id == info.id && scale == info.scale && size == info.size;
+  }
+};
+
+class CompositorFrameMetadata;
+class SurfaceEmbedding;
+typedef std::unique_ptr<SurfaceEmbedding> SurfaceEmbeddingPtr;
+
+class SurfaceEmbeddingOwner {
+ public:
+  virtual SurfaceSequenceGenerator* GetSurfaceSequenceGenerator() = 0;
+};
+
+class SurfaceEmbedding {

[kylechar, 2016/12/08 02:02:46]

One class per header and file should be named after that class.

+ public:
+  explicit SurfaceEmbedding(const SurfaceInfo& info) : info_(info) {}
+  ~SurfaceEmbedding();

[kylechar, 2016/12/08 02:02:46]

virtual ~SurfaceEmbedding();

+
+  void RemoveReference() {

[kylechar, 2016/12/08 02:02:46]

Function definitions should be in the .cc file unless they're trivial, eg. int
foo() const { return foo_; }

+    if (!has_ref_)
+      return;
+    DCHECK(id().is_valid());
+    RemoveReferenceImpl();
+    has_ref_ = false;
+  }
+
+  SurfaceEmbeddingPtr CreateReference(SurfaceEmbeddingOwner* owner) {

[Fady Samuel, 2016/12/07 23:29:05]

How about simply making this a pure virtual function?

+    SurfaceEmbeddingPtr clone = Clone();
+    clone->AddReferenceImpl(owner);
+    clone->has_ref_ = true;
+    return clone;
+  }
+
+  void DelegateReference(CompositorFrameMetadata* metadata) {

[Fady Samuel, 2016/12/07 23:29:05]

DelegateReference is a confusing name: How about just
RemoveReference(CompsoitorFrameMetadata* metdata)?

+    DelegateReferenceImpl(metadata);
+    has_ref_ = false;
+  }
+
+  const SurfaceInfo& info() { return info_; }
+  const SurfaceId& id() const { return info_.id; }
+  float scale() const { return info_.scale; }
+  const gfx::Size& size() const { return info_.size; }
+
+  SurfaceEmbeddingPtr Clone() {

[Fady Samuel, 2016/12/08 12:49:24]

If you want a clone then just make Clone() a pure virtual in the base class. I'm
not a fan of this * / *Impl template pattern used all over the place.

+    DCHECK(!has_ref_);
+    return SurfaceEmbeddingPtr(CloneImpl());
+  }
+
+ private:
+  virtual void AddReferenceImpl(SurfaceEmbeddingOwner*) = 0;
+  virtual void RemoveReferenceImpl() = 0;
+  virtual void DelegateReferenceImpl(CompositorFrameMetadata*) = 0;
+  virtual SurfaceEmbedding* CloneImpl() = 0;
+
+  const SurfaceInfo info_;
+  bool has_ref_ = false;
+};
+
+}  // namespace cc
+
+#endif  // CC_SURFACES_SURFACE_EMBEDDING_BASE_H_