Replacing SurfaceReferenceBase and SequenceSurfaceReference with Closures

The sole purpose of SurfaceReferenceBase and SequenceSurfaceReference
was to be able to return the reference later on, but the same goal can
be achieved using closures with much less complexity. From now on,
SurfaceReferenceFactory::CreateReference returns a closure that returns
the reference once its called.

TBR=sadrul@chromium.org

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2616403003
Cr-Commit-Position: refs/heads/master@{#442984}

base/callback_unittest.cc

@@ -115,6 +115,17 @@ TEST_F(CallbackTest, Reset) {
   EXPECT_TRUE(callback_a_.Equals(null_callback_));
 }
 
+TEST_F(CallbackTest, Move) {
+  // Moving should reset the callback.
+  ASSERT_FALSE(callback_a_.is_null());
+  ASSERT_FALSE(callback_a_.Equals(null_callback_));
+
+  auto tmp = std::move(callback_a_);
+
+  EXPECT_TRUE(callback_a_.is_null());
+  EXPECT_TRUE(callback_a_.Equals(null_callback_));
+}
+
 struct TestForReentrancy {
   TestForReentrancy()
       : cb_already_run(false),

cc/layers/surface_layer.cc

@@ -8,22 +8,22 @@
 
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
-#include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/trace_event.h"
 #include "cc/layers/surface_layer_impl.h"
 #include "cc/output/swap_promise.h"
 #include "cc/surfaces/surface_sequence_generator.h"
 #include "cc/trees/layer_tree_host.h"
 #include "cc/trees/swap_promise_manager.h"
+#include "cc/trees/task_runner_provider.h"
 
 namespace cc {
 
 class SatisfySwapPromise : public SwapPromise {
  public:
   SatisfySwapPromise(
-      std::unique_ptr<SurfaceReferenceBase> surface_ref,
+      base::Closure reference_returner,
       scoped_refptr<base::SingleThreadTaskRunner> main_task_runner)
-      : surface_ref_(std::move(surface_ref)),
+      : reference_returner_(reference_returner),
         main_task_runner_(std::move(main_task_runner)) {}
 
   ~SatisfySwapPromise() override {}
@@ -34,17 +34,17 @@ class SatisfySwapPromise : public SwapPromise {
   void WillSwap(CompositorFrameMetadata* metadata) override {}
 
   void DidSwap() override {
-    main_task_runner_->DeleteSoon(FROM_HERE, surface_ref_.release());
+    main_task_runner_->PostTask(FROM_HERE, reference_returner_);
   }
 
   DidNotSwapAction DidNotSwap(DidNotSwapReason reason) override {
-    main_task_runner_->DeleteSoon(FROM_HERE, surface_ref_.release());
+    main_task_runner_->PostTask(FROM_HERE, reference_returner_);
     return DidNotSwapAction::BREAK_PROMISE;
   }
 
   int64_t TraceId() const override { return 0; }
 
-  std::unique_ptr<SurfaceReferenceBase> surface_ref_;
+  base::Closure reference_returner_;
   scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(SatisfySwapPromise);
@@ -66,7 +66,7 @@ void SurfaceLayer::SetSurfaceInfo(const SurfaceInfo& surface_info) {
   RemoveCurrentReference();
   surface_info_ = surface_info;
   if (layer_tree_host()) {
-    current_ref_ =
+    reference_returner_ =
         ref_factory_->CreateReference(layer_tree_host(), surface_info_.id());
   }
   UpdateDrawsContent(HasDrawableContent());
@@ -96,7 +96,7 @@ void SurfaceLayer::SetLayerTreeHost(LayerTreeHost* host) {
   RemoveCurrentReference();
   Layer::SetLayerTreeHost(host);
   if (layer_tree_host()) {
-    current_ref_ =
+    reference_returner_ =
         ref_factory_->CreateReference(layer_tree_host(), surface_info_.id());
   }
 }
@@ -110,10 +110,11 @@ void SurfaceLayer::PushPropertiesTo(LayerImpl* layer) {
 }
 
 void SurfaceLayer::RemoveCurrentReference() {
-  if (!current_ref_)
+  if (!reference_returner_)
     return;
   auto swap_promise = base::MakeUnique<SatisfySwapPromise>(
-      std::move(current_ref_), base::ThreadTaskRunnerHandle::Get());
+      std::move(reference_returner_),
+      layer_tree_host()->GetTaskRunnerProvider()->MainThreadTaskRunner());
   layer_tree_host()->GetSwapPromiseManager()->QueueSwapPromise(
       std::move(swap_promise));
 }

cc/layers/surface_layer.h

@@ -8,11 +8,8 @@
 #include "base/macros.h"
 #include "cc/base/cc_export.h"
 #include "cc/layers/layer.h"
-#include "cc/surfaces/surface_id.h"
 #include "cc/surfaces/surface_info.h"
-#include "cc/surfaces/surface_reference_base.h"
 #include "cc/surfaces/surface_reference_factory.h"
-#include "cc/surfaces/surface_sequence.h"
 #include "ui/gfx/geometry/size.h"
 
 namespace cc {
@@ -50,7 +47,7 @@ class CC_EXPORT SurfaceLayer : public Layer {
 
   SurfaceInfo surface_info_;
   scoped_refptr<SurfaceReferenceFactory> ref_factory_;
-  std::unique_ptr<SurfaceReferenceBase> current_ref_;
+  base::Closure reference_returner_;
   bool stretch_content_to_fill_bounds_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(SurfaceLayer);

cc/surfaces/BUILD.gn

@@ -10,15 +10,11 @@ cc_source_set("surface_id") {
     "frame_sink_id.h",
     "local_frame_id.cc",
     "local_frame_id.h",
-    "sequence_surface_reference.cc",
-    "sequence_surface_reference.h",
     "surface_id.cc",
     "surface_id.h",
     "surface_info.h",
     "surface_reference.cc",
     "surface_reference.h",
-    "surface_reference_base.cc",
-    "surface_reference_base.h",
     "surface_reference_factory.h",
     "surface_reference_owner.h",
     "surface_sequence.h",

cc/surfaces/sequence_surface_reference.cc

-// 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.
-
-#include "cc/surfaces/sequence_surface_reference.h"
-
-namespace cc {
-
-SequenceSurfaceReference::SequenceSurfaceReference(
-    scoped_refptr<const SurfaceReferenceFactory> factory,
-    const SurfaceSequence& sequence)
-    : SurfaceReferenceBase(factory), sequence_(sequence) {}
-
-SequenceSurfaceReference::~SequenceSurfaceReference() {
-  Destroy();
-}
-
-}  // namespace cc

cc/surfaces/sequence_surface_reference.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_SEQUENCE_SURFACE_REFERENCE_H_
-#define CC_SURFACES_SEQUENCE_SURFACE_REFERENCE_H_
-
-#include "cc/surfaces/surface_reference_base.h"
-#include "cc/surfaces/surface_reference_factory.h"
-#include "cc/surfaces/surface_sequence.h"
-
-namespace cc {
-
-// A surface reference which is internally implemented using SurfaceSequence
-class SequenceSurfaceReference final : public SurfaceReferenceBase {
- public:
-  SequenceSurfaceReference(scoped_refptr<const SurfaceReferenceFactory> factory,
-                           const SurfaceSequence& sequence);
-
-  ~SequenceSurfaceReference() override;
-
-  const SurfaceSequence& sequence() const { return sequence_; }
-
- private:
-  const SurfaceSequence sequence_;
-
-  DISALLOW_COPY_AND_ASSIGN(SequenceSurfaceReference);
-};
-
-}  // namespace cc
-
-#endif  // CC_SURFACES_SEQUENCE_SURFACE_REFERENCE_H_

cc/surfaces/sequence_surface_reference_factory.cc

@@ -4,29 +4,20 @@
 
 #include "cc/surfaces/sequence_surface_reference_factory.h"
 
+#include "base/bind.h"
 #include "base/memory/ptr_util.h"
-#include "cc/surfaces/sequence_surface_reference.h"
+
+#include "cc/surfaces/surface_sequence.h"
 
 namespace cc {
 
-std::unique_ptr<SurfaceReferenceBase>
-SequenceSurfaceReferenceFactory::CreateReference(
+base::Closure SequenceSurfaceReferenceFactory::CreateReference(
     SurfaceReferenceOwner* owner,
     const SurfaceId& surface_id) const {
   auto seq = owner->GetSurfaceSequenceGenerator()->CreateSurfaceSequence();
   RequireSequence(surface_id, seq);
-  return base::MakeUnique<SequenceSurfaceReference>(make_scoped_refptr(this),
-                                                    seq);
-}
-
-void SequenceSurfaceReferenceFactory::DestroyReference(
-    SurfaceReferenceBase* surface_ref) const {
-  // This method can only be called by a SurfaceReferenceBase because it's
-  // private and only SurfaceReferenceBase is a friend. The reference only
-  // calls this method on the factory that created it. So it's safe to cast
-  // the passed reference to the type returned by CreateReference.
-  auto ref = static_cast<SequenceSurfaceReference*>(surface_ref);
-  SatisfySequence(ref->sequence());
+  return base::Bind(&SequenceSurfaceReferenceFactory::SatisfySequence, this,
+                    seq);
 }
 
 }  // namespace cc

cc/surfaces/sequence_surface_reference_factory.h

@@ -5,23 +5,21 @@
 #ifndef CC_SURFACES_SEQUENCE_SURFACE_REFERENCE_FACTORY_H_
 #define CC_SURFACES_SEQUENCE_SURFACE_REFERENCE_FACTORY_H_
 
-#include "cc/output/compositor_frame_metadata.h"
-#include "cc/surfaces/surface_reference_base.h"
 #include "cc/surfaces/surface_reference_factory.h"
+#include "cc/surfaces/surface_sequence.h"
 #include "cc/surfaces/surfaces_export.h"
 
 namespace cc {
 
-// A surface reference factory that generates references which internally
-// use SurfaceSequence.
+// A surface reference factory that uses SurfaceSequence.
 class CC_SURFACES_EXPORT SequenceSurfaceReferenceFactory
     : public NON_EXPORTED_BASE(SurfaceReferenceFactory) {
  public:
   SequenceSurfaceReferenceFactory() = default;
 
-  std::unique_ptr<SurfaceReferenceBase> CreateReference(
-      SurfaceReferenceOwner* owner,
-      const SurfaceId& surface_id) const override;
+  // SurfaceReferenceFactory implementation:
+  base::Closure CreateReference(SurfaceReferenceOwner* owner,
+                                const SurfaceId& surface_id) const override;
 
  protected:
   ~SequenceSurfaceReferenceFactory() override = default;
@@ -31,9 +29,6 @@ class CC_SURFACES_EXPORT SequenceSurfaceReferenceFactory
                                const SurfaceSequence& sequence) const = 0;
   virtual void SatisfySequence(const SurfaceSequence& sequence) const = 0;
 
-  // SurfaceReferenceFactory implementation:
-  void DestroyReference(SurfaceReferenceBase* surface_ref) const override;
-
   DISALLOW_COPY_AND_ASSIGN(SequenceSurfaceReferenceFactory);
 };
 

cc/surfaces/surface_reference_base.cc

-// 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.
-
-#include "cc/surfaces/surface_reference_base.h"
-
-#include "cc/surfaces/surface_reference_factory.h"
-
-namespace cc {
-
-SurfaceReferenceBase::SurfaceReferenceBase(
-    scoped_refptr<const SurfaceReferenceFactory> factory)
-    : factory_(std::move(factory)) {}
-
-SurfaceReferenceBase::~SurfaceReferenceBase() {
-  DCHECK(!factory_) << "Each leaf subclass must call Destroy in its destructor";
-}
-
-void SurfaceReferenceBase::Destroy() {
-  factory_->DestroyReference(this);
-  factory_ = nullptr;
-}
-
-}  // namespace cc

cc/surfaces/surface_reference_base.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_SURFACE_REFERENCE_BASE_H_
-#define CC_SURFACES_SURFACE_REFERENCE_BASE_H_
-
-#include <memory>
-
-#include "base/memory/ptr_util.h"
-#include "base/memory/ref_counted.h"
-#include "cc/surfaces/surface_sequence.h"
-
-namespace cc {
-
-class SurfaceReferenceFactory;
-
-// The base class for the references returned by SurfaceReferenceFactory and
-// its subclasses.
-// The objects of this class hold on their surface reference until they go
-// out of scope.
-// To keep things as lightweight as possible, the base class only keeps a
-// pointer to the factory and it's up to the subclasses to decide what other
-// information they need to keep.
-class SurfaceReferenceBase {
- public:
-  explicit SurfaceReferenceBase(
-      scoped_refptr<const SurfaceReferenceFactory> factory);
-
-  virtual ~SurfaceReferenceBase();
-
- protected:
-  void Destroy();
-
- private:
-  scoped_refptr<const SurfaceReferenceFactory> factory_;
-
-  DISALLOW_COPY_AND_ASSIGN(SurfaceReferenceBase);
-};
-
-}  // namespace cc
-
-#endif  // CC_SURFACES_SURFACE_REFERENCE_BASE_H_

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 must be called exactly once to remove the reference.
+  virtual base::Closure CreateReference(SurfaceReferenceOwner* owner,
+                                        const SurfaceId& surface_id) const = 0;
 
   SurfaceReferenceFactory() = default;
 
@@ -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);
 };
 

ui/aura/mus/client_surface_embedder.cc

@@ -12,35 +12,20 @@ namespace {
 
 // TODO(mfomitchev, samans): Remove these stub classes once the SurfaceReference
 // work is complete.
-class StubSurfaceReference : public cc::SurfaceReferenceBase {
- public:
-  StubSurfaceReference(scoped_refptr<const cc::SurfaceReferenceFactory> factory)
-      : cc::SurfaceReferenceBase(factory) {}
-
-  ~StubSurfaceReference() override { Destroy(); }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(StubSurfaceReference);
-};
-
 class StubSurfaceReferenceFactory : public cc::SurfaceReferenceFactory {
  public:
   StubSurfaceReferenceFactory() = default;
 
   // cc::SurfaceReferenceFactory:
-  std::unique_ptr<cc::SurfaceReferenceBase> CreateReference(
+  base::Closure CreateReference(
       cc::SurfaceReferenceOwner* owner,
       const cc::SurfaceId& surface_id) const override {
-    return base::MakeUnique<StubSurfaceReference>(make_scoped_refptr(this));
+    return base::Closure();
   }
 
  protected:
   ~StubSurfaceReferenceFactory() override = default;
 
- private:
-  // cc::SurfaceReferenceFactory:
-  void DestroyReference(cc::SurfaceReferenceBase* surface_ref) const override {}
-
   DISALLOW_COPY_AND_ASSIGN(StubSurfaceReferenceFactory);
 };
 }  // namespace