CopyOutputResult must have a working release callback when received over mojo

cc/ipc/copy_output_result_struct_traits.cc

Index: cc/ipc/copy_output_result_struct_traits.cc
diff --git a/cc/ipc/copy_output_result_struct_traits.cc b/cc/ipc/copy_output_result_struct_traits.cc
index 0398fe115cd52463b56fbf2a1a1540d3d96b2935..32dc9b40443ec5578902ee0f456dbef06674a6d5 100644
--- a/cc/ipc/copy_output_result_struct_traits.cc
+++ b/cc/ipc/copy_output_result_struct_traits.cc
@@ -4,9 +4,62 @@
 
 #include "cc/ipc/copy_output_result_struct_traits.h"
 
+#include "mojo/public/cpp/bindings/strong_binding.h"
+
+namespace {
+
+class TextureMailboxReleaserImpl : public cc::mojom::TextureMailboxReleaser {
+ public:
+  TextureMailboxReleaserImpl(
+      std::unique_ptr<cc::SingleReleaseCallback> release_callback)
+      : release_callback_(std::move(release_callback)) {}
+
+  // mojom::TextureMailboxReleaser implementation:
+  void Release(const gpu::SyncToken& sync_token, bool is_lost) override {
+    release_callback_->Run(sync_token, is_lost);
+  }
+
+ private:
+  std::unique_ptr<cc::SingleReleaseCallback> release_callback_;
+};
+
+void Release(cc::mojom::TextureMailboxReleaserPtr ptr,
+             const gpu::SyncToken& sync_token,
+             bool is_lost) {
+  ptr->Release(sync_token, is_lost);
+}
+
+}  // namespace
+
 namespace mojo {
 
 // static
+const SkBitmap& StructTraits<cc::mojom::CopyOutputResultDataView,
+                             std::unique_ptr<cc::CopyOutputResult>>::
+    bitmap(const std::unique_ptr<cc::CopyOutputResult>& result) {
+  static SkBitmap* null_bitmap = new SkBitmap();

[danakj, 2017/02/10 18:32:51]

curious why do we need this, given that bitmap() is an optional field? the
return type here seems to imply its not optional

[Saman Sami, 2017/02/10 19:10:07]

SkBitmap is nullable (The struct traits has IsNull and SetToNull). That's all is
needed to use the ? notation. See
https://cs.chromium.org/chromium/src/skia/public/interfaces/bitmap_skbitmap_s...

[danakj, 2017/02/10 22:51:03]

If there is IsNull() would this ever be called when result->bitmap_ is not
present? Could we DCHECK instead?

[danakj, 2017/02/13 20:50:16]

Still wondering this

[Saman Sami, 2017/02/13 21:03:01]

Sorry I missed this one. I don't think IsNull replaces the getter. After all,
mojo needs to first get the element and then check if it's null or not.

[danakj, 2017/02/13 22:43:17]

Oh... it's rather unfortunate you have to deserialize before asking if the field
is present. That's a lot of wasted overhead for optionally-missing fields.

[Saman Sami, 2017/02/13 23:23:43]

This method doesn't do any deserialization. It just returns the bitmap from the
CopyOutputResult. (And returns a const ref actually. Pretty efficient.)

[danakj, 2017/02/14 17:49:10]

I guess so, I just keep looking at this malloc :p

+  if (!result->bitmap_)
+    return *null_bitmap;
+  return *result->bitmap_;
+}
+
+// static
+cc::mojom::TextureMailboxReleaserPtr
+StructTraits<cc::mojom::CopyOutputResultDataView,
+             std::unique_ptr<cc::CopyOutputResult>>::
+    releaser(const std::unique_ptr<cc::CopyOutputResult>& result) {
+  if (result->release_callback_) {

[danakj, 2017/02/10 18:32:51]

Prefer early outs to if/else. Here:

if (!release callback)
  return {};

<!-- don't have to indent the rest of this method -->

[Saman Sami, 2017/02/10 19:10:07]

Done.

+    cc::mojom::TextureMailboxReleaserPtr releaser;
+    auto impl = base::MakeUnique<TextureMailboxReleaserImpl>(
+        std::move(result->release_callback_));
+    MakeStrongBinding(std::move(impl), MakeRequest(&releaser));
+    return releaser;
+  } else {
+    return cc::mojom::TextureMailboxReleaserPtr();

[danakj, 2017/02/10 18:32:51]

nit: you could "return {};" here

[Saman Sami, 2017/02/10 19:10:07]

Done.

+  }
+}
+
+// static
 bool StructTraits<cc::mojom::CopyOutputResultDataView,
                   std::unique_ptr<cc::CopyOutputResult>>::
     Read(cc::mojom::CopyOutputResultDataView data,
@@ -23,19 +76,15 @@ bool StructTraits<cc::mojom::CopyOutputResultDataView,
   if (!data.ReadTextureMailbox(&result->texture_mailbox_))
     return false;
 
+  auto releaser = data.TakeReleaser<cc::mojom::TextureMailboxReleaserPtr>();
+  if (releaser) {
+    result->release_callback_ = cc::SingleReleaseCallback::Create(
+        base::Bind(Release, base::Passed(&releaser)));

[danakj, 2017/02/10 18:32:51]

you can bind directly to TextureMailboxReleaserPtr::Release here, you don't need
the intermediate Release static method?

[Saman Sami, 2017/02/10 19:10:07]

TextureMailboxReleaserPtr does not have a Release method. It has the -> operator
which returns the TextureMailboxReleaserProxy that it owns.

[danakj, 2017/02/10 22:52:51]

Ohh, wild. ok :)

+  }
+
   *out_p = std::move(result);
 
   return true;
 }
 
-// static
-const SkBitmap& StructTraits<cc::mojom::CopyOutputResultDataView,
-                             std::unique_ptr<cc::CopyOutputResult>>::
-    bitmap(const std::unique_ptr<cc::CopyOutputResult>& result) {
-  static SkBitmap* null_bitmap = new SkBitmap();
-  if (!result->bitmap_)
-    return *null_bitmap;
-  return *result->bitmap_;
-}
-
 }  // namespace mojo