CopyOutputResult must have a working release callback when received over mojo

cc/resources/single_release_callback.h

 // Copyright 2013 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_RESOURCES_SINGLE_RELEASE_CALLBACK_H_
 #define CC_RESOURCES_SINGLE_RELEASE_CALLBACK_H_
 
 #include <memory>
 
 #include "base/memory/ptr_util.h"
 #include "cc/base/cc_export.h"
 #include "cc/resources/release_callback.h"
+#include "mojo/public/cpp/bindings/struct_traits.h"
+
+namespace mojo {
+template <class T>
+class InterfacePtr;
+}
 
 namespace cc {
 
+namespace mojom {
+class SingleReleaseCallbackDataView;
+class TextureMailboxReleaser;
+typedef mojo::InterfacePtr<TextureMailboxReleaser> TextureMailboxReleaserPtr;
+}
+
 class CC_EXPORT SingleReleaseCallback {
+  class PtrHolder {
+   public:
+    virtual mojom::TextureMailboxReleaserPtr TakePtr() = 0;
+    virtual void Release(const gpu::SyncToken& sync_token, bool is_lost) = 0;
+  };
+
+  template <class T>
+  class PtrHolderImpl : public PtrHolder {

[danakj, 2017/02/08 20:53:31]

If this is implemented right here, I'm not sure what the point of the virtuals
is. Can this be implemented inside 
StructTraits<cc::mojom::SingleReleaseCallbackDataView,
 std::unique_ptr<cc::SingleReleaseCallback>>? Then no friend relationships are
needed either.

[Saman Sami, 2017/02/08 21:44:21]

We use the virtuals because this template cannot be instantiated here due to
dependency problems. A class similar to this has to be implemented in not only
struct traits but also struct traits unit tests and GpuCompositorFrameSink (in a
future CL). Having this template here avoids duplicate code.

[danakj, 2017/02/08 23:24:17]

It's fine to have a test impl and a production impl. I don't think we should use
a template to avoid that. It also helps avoid test requirements leaking into
production.

[Saman Sami, 2017/02/09 16:30:00]

OK. I can avoid the template if having duplicate code is fine.

+   public:
+    explicit PtrHolderImpl(T ptr) : ptr_(std::move(ptr)) {}
+    T TakePtr() { return std::move(ptr_); }
+    void Release(const gpu::SyncToken& sync_token, bool is_lost) override {
+      return ptr_->Release(sync_token, is_lost);
+    }
+
+   private:
+    T ptr_;
+  };
+
  public:
   static std::unique_ptr<SingleReleaseCallback> Create(
       const ReleaseCallback& cb) {
     return base::WrapUnique(new SingleReleaseCallback(cb));
   }
 
+  template <class T>

[danakj, 2017/02/08 20:53:31]

Why template and not pass a unique_ptr<PtrHolder>?

[Saman Sami, 2017/02/08 21:44:21]

I think this looks better and more intuitive than
cc::SingleReleaseCallback::Create(cc::SingleReleaseCallback::PtrHolder(std::move(ptr))).
We also won't have to expose the internal classes at all.

+  static std::unique_ptr<SingleReleaseCallback> Create(T ptr) {
+    auto ptr_holder = base::MakeUnique<PtrHolderImpl<T>>(std::move(ptr));
+    return base::WrapUnique(new SingleReleaseCallback(std::move(ptr_holder)));
+  }
+
   ~SingleReleaseCallback();
 
   void Run(const gpu::SyncToken& sync_token, bool is_lost);
 
  private:
+  friend struct mojo::StructTraits<mojom::SingleReleaseCallbackDataView,
+                                   std::unique_ptr<SingleReleaseCallback>>;
+
   explicit SingleReleaseCallback(const ReleaseCallback& callback);
+  explicit SingleReleaseCallback(std::unique_ptr<PtrHolder> ptr_holder);
 
   ReleaseCallback callback_;
+  std::unique_ptr<PtrHolder> ptr_holder_;
+
+  DISALLOW_COPY_AND_ASSIGN(SingleReleaseCallback);
 };
 
 }  // namespace cc
 
 #endif  // CC_RESOURCES_SINGLE_RELEASE_CALLBACK_H_