ImageCapture: ScopedResultCallback

media/capture/video/scoped_callback.h

Index: media/capture/video/scoped_callback.h
diff --git a/media/capture/video/scoped_callback.h b/media/capture/video/scoped_callback.h
new file mode 100644
index 0000000000000000000000000000000000000000..84a1a5a00d4902148624ec7244e80352aaf32b8d
--- /dev/null
+++ b/media/capture/video/scoped_callback.h
@@ -0,0 +1,52 @@
+// 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 MEDIA_CAPTURE_VIDEO_SCOPED_CALLBACK_H_
+#define MEDIA_CAPTURE_VIDEO_SCOPED_CALLBACK_H_
+
+#include "base/callback_forward.h"
+
+namespace media {
+
+// This class wraps a |callback_| and guarantees that it will be called on

[miu, 2016/05/28 01:29:56]

This is a little inaccurate. It will guarantee that either |callback_| has been
called, or will pass the callback to |on_error_callback_| at destruction time
for "default handling."

[mcasas, 2016/05/31 20:19:04]

Rewritten.

+// destruction and according to the provided OnErrorCallback if it hasn't been
+// retrieved before (via PassCallback()). Inspired by ScopedWebCallbacks
+// template.
+template <typename CallbackType>
+class ScopedCallback {

[miu, 2016/05/28 01:29:56]

For me, the name of this utility class was a bit confusing (e.g., I was
wondering if you had re-invented base::ScopedClosureRunner or something like
that). In fact, it's very close to "promises" in JavaScript. How about naming
this something like ResultPromise or ResultCallbackPair?

[mcasas, 2016/05/31 20:19:04]

If this class was to get traction, I think its base/
location could indeed be next to  ScopedClosureRunner
in base/callback_helpers.h.
It is, except Promises have explicit resolve() and reject() 
methods, and there are already nice wrappers for those,
e.g.ScopedWebCallbacks [1]

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
I added the Scoped part to make evident that this
class is move-only, as opposed to mojo::Callback
which is ref counted under the hood, making it hard 
to reason about its cancellation (who should cancel
it, on which thread etc). I think the key elements are: 
- this class is move-only 
- forces either the callback or the error callback to
be Run()
- either of those can only be called _once_ (whereas
{base,mojo}::Callback can be Run() several times).
- the OnErrorCallback operates on the same very 
callback that this class wraps!

ScopedErrorOrCallback ?
CallbackErrorPair?

[miu, 2016/06/01 00:53:36]

nit: ScopedResultCallback? (since this mechanism is for guaranteeing either a
result callback is run, or it is passed to an error handler when the operation
aborts/fails)

[mcasas, 2016/06/01 01:21:01]

ScopedResultCallback sounds great.

+  MOVE_ONLY_TYPE_FOR_CPP_03(ScopedCallback);
+
+ public:
+  using OnErrorCallback = base::Callback<void(const CallbackType&)>;
+  ScopedCallback(const CallbackType& callback,
+                 const OnErrorCallback& on_error_callback)
+      : callback_(callback), on_error_callback_(on_error_callback) {}
+
+  ~ScopedCallback() {
+    if (!callback_.is_null())
+      on_error_callback_.Run(callback_);

[miu, 2016/05/28 01:29:56]

Just in case that |callback_| has a ref-counted pointer in it:

  on_error_callback_.Run(base::ResetAndReturn(&callback_));

[mcasas, 2016/05/31 20:19:04]

Done. 
(Note that base::ResetAndReturn() doesn't work on mojo::Callback<>).

+  }
+
+  ScopedCallback(ScopedCallback&& other) { *this = std::move(other); }
+
+  ScopedCallback& operator=(ScopedCallback&& other) {
+    callback_ = other.PassCallback();
+    on_error_callback_ = other.on_error_callback_;
+    return *this;
+  }
+
+  CallbackType PassCallback() {

[miu, 2016/05/28 01:29:56]

A few things:

1. Instead of this method, would client code become a bit simpler if this were
like base::Callback<>::Run()?

2. Perhaps |on_error_callback_| should be reset before the success callback is
run.

3. nit: Use base::ResetAndReturn().

Putting it all together:

  template<typename... Args>
  void Success(Args... args) {
    on_error_callback_.Reset();
    base::ResetAndReturn(&callback_).Run(args);
  }

[mcasas, 2016/05/31 20:19:04]

Taken all in except the base::ResetAndReturn() for the
reason mentioned above. The glue logic had to be a bit
more complex to deal with move-only types.

[miu, 2016/06/01 00:53:36]

Understood.

+    CallbackType callback = callback_;
+    callback_.reset();
+    return callback;
+  }
+
+ private:
+  CallbackType callback_;
+  OnErrorCallback on_error_callback_;
+};
+
+}  // namespace media
+
+#endif  // MEDIA_CAPTURE_VIDEO_SCOPED_CALLBACK_H_