CallbackPromiseAdapter types should be more compatible with WebCallbacks (1/3).

Source/bindings/core/v8/CallbackPromiseAdapter.h

Index: Source/bindings/core/v8/CallbackPromiseAdapter.h
diff --git a/Source/bindings/core/v8/CallbackPromiseAdapter.h b/Source/bindings/core/v8/CallbackPromiseAdapter.h
index 8f26b5c07451677a0735b2392e3c2dae058699fe..742ba94783f28f8ea12deefad207d3dd8f6d8248 100644
--- a/Source/bindings/core/v8/CallbackPromiseAdapter.h
+++ b/Source/bindings/core/v8/CallbackPromiseAdapter.h
@@ -33,47 +33,55 @@
 
 #include "bindings/core/v8/ScriptPromiseResolver.h"
 #include "public/platform/WebCallbacks.h"
+#include "public/platform/WebPrivateOwnPtr.h"
+#include "wtf/OwnPtr.h"
+#include "wtf/PassOwnPtr.h"
 
 namespace blink {
 
-// This class provides an easy way to convert from a Script-exposed
-// class (i.e. a class that has a toV8() overload) that uses Promises
-// to a WebKit API class that uses WebCallbacks. You can define
-// separate Success and Error classes, but this example just uses one
-// object for both.
-//
-// To use:
-//
-// class MyClass ... {
-//    typedef blink::WebMyClass WebType;
-//
-//    // Takes ownership of |webInstance|.
-//    static PassRefPtr<MyClass> take(ScriptPromiseResolver* resolver,
-//                                    PassOwnPtr<blink::WebMyClass> webInstance) {
-//        // convert/create as appropriate, but often it's just:
-//        return MyClass::create(webInstance);
-//
-//        // Since promise resolving is done as an async task, it's not
-//        // guaranteed that the script context has seen the promise resolve
-//        // immediately after calling onSuccess/onError. You can use the
-//        // ScriptPromise from the resolver to schedule a task that executes
-//        // after resolving:
-//        ScriptState::Scope scope(resolver->scriptState());
-//        resolver->promise().then(...);
-//    }
-//
-// Now when calling into a WebKit API that requires a WebCallbacks<blink::WebMyClass, blink::WebMyClass>*:
-//
-//    // call signature: callSomeMethod(WebCallbacks<MyClass, MyClass>* callbacks)
-//    webObject->callSomeMethod(new CallbackPromiseAdapter<MyClass, MyClass>(resolver, scriptExecutionContext));
-//
-// Note:
-// - This class does not manage its own lifetime. In this example that ownership
-//   of the WebCallbacks instance is being passed in and it is up to the callee
-//   to free the WebCallbacks instance.
-// - onSuccess and onError take ownership of the given WebType instance.
+class CallbackPromiseAdapterBase {

[kinuko, 2015/07/15 06:19:23]

nit: This could probably be done without inheritance? Is this one introduced
mainly for namespace isolation?

[yhirano, 2015/07/15 08:49:45]

Yes. Do you like PS2?
By the way, I'm not very interested in cleaning up this file in this CL because
I will change it drastically in https://codereview.chromium.org/1240763002/

[kinuko, 2015/07/21 11:27:06]

Ok, understood.

+public:
+    template <typename T>
+    struct PassTypeImpl {
+        using Type = T;
+    };
+    template <typename T>
+    struct PassTypeImpl<OwnPtr<T>> {
+        using Type = PassOwnPtr<T>;
+    };
+
+    template <typename T>
+    struct WebPassTypeImpl {
+        using Type = T;
+    };
+    template <typename T>
+    struct WebPassTypeImpl<OwnPtr<T>> {
+        using Type = WebPrivateOwnPtr<T>&&;

[kinuko, 2015/07/15 06:19:24]

Rvalue reference is currently not allowed. We could try asking for exceptions
but exposing these types at public/platform feels a bit too visible. Should we
try introducing some PassOwnPtr-like machinery?

[yhirano, 2015/07/15 08:49:45]

I see. I uploaded a separate CL https://codereview.chromium.org/1232033004/ and
made this CL use it.

+    };
+
+    template <typename T>
+    using PassType = typename PassTypeImpl<T>::Type;
+    template <typename T>
+    using WebPassType = typename WebPassTypeImpl<T>::Type;
+
+    // TODO(yhirano): Use universal reference and std::forward.

[kinuko, 2015/07/15 06:19:23]

nit: do you mean this TODO to be resolved after we support c++11 library
features in chromium? Might be nice to link to
https://chromium-cpp.appspot.com/.

[yhirano, 2015/07/15 08:49:45]

Done.

+    template <typename T>
+    static T& adopt(T& x) { return x; }
+    template <typename T>
+    static PassOwnPtr<T> adopt(WebPrivateOwnPtr<T>& x) { return x.release(); }
+
+    // TODO(yhirano): Use rvalue reference and std::move.
+    template <typename T>
+    static PassType<T> pass(T& x) { return x; }
+    template <typename T>
+    static PassOwnPtr<T> pass(OwnPtr<T>& x) { return x.release(); }
+};
+
+
+// TODO(yhirano): Add comments.
 template<typename S, typename T>
-class CallbackPromiseAdapter final : public WebCallbacks<typename S::WebType*, typename T::WebType*> {
+class CallbackPromiseAdapter final
+    : public WebCallbacks<typename CallbackPromiseAdapterBase::WebPassType<typename S::WebType>, typename CallbackPromiseAdapterBase::WebPassType<typename T::WebType>>, public CallbackPromiseAdapterBase {
     WTF_MAKE_NONCOPYABLE(CallbackPromiseAdapter);
 public:
     explicit CallbackPromiseAdapter(PassRefPtrWillBeRawPtr<ScriptPromiseResolver> resolver)
@@ -83,22 +91,20 @@ public:
     }
     ~CallbackPromiseAdapter() override { }
 
-    // Takes ownership of |result|.
-    void onSuccess(typename S::WebType* result) override
+    void onSuccess(WebPassType<typename S::WebType> r) override
     {
-        OwnPtr<typename S::WebType> ownPtr = adoptPtr(result);
+        typename S::WebType result(adopt(r));
         if (!m_resolver->executionContext() || m_resolver->executionContext()->activeDOMObjectsAreStopped())
             return;
-        m_resolver->resolve(S::take(m_resolver.get(), ownPtr.release()));
+        m_resolver->resolve(S::take(m_resolver.get(), pass(result)));
     }
 
-    // Takes ownership of |error|.
-    void onError(typename T::WebType* error) override
+    void onError(WebPassType<typename T::WebType> e) override
     {
-        OwnPtr<typename T::WebType> ownPtr = adoptPtr(error);
+        typename T::WebType error(adopt(e));
         if (!m_resolver->executionContext() || m_resolver->executionContext()->activeDOMObjectsAreStopped())
             return;
-        m_resolver->reject(T::take(m_resolver.get(), ownPtr.release()));
+        m_resolver->reject(T::take(m_resolver.get(), pass(error)));
     }
 
 private:
@@ -106,7 +112,7 @@ private:
 };
 
 template<typename T>
-class CallbackPromiseAdapter<void, T> final : public WebCallbacks<void, typename T::WebType*> {
+class CallbackPromiseAdapter<void, T> final : public WebCallbacks<void, typename CallbackPromiseAdapterBase::WebPassType<typename T::WebType>>, public CallbackPromiseAdapterBase {
     WTF_MAKE_NONCOPYABLE(CallbackPromiseAdapter);
 public:
     explicit CallbackPromiseAdapter(PassRefPtrWillBeRawPtr<ScriptPromiseResolver> resolver)
@@ -123,13 +129,12 @@ public:
         m_resolver->resolve();
     }
 
-    // Takes ownership of |error|.
-    void onError(typename T::WebType* error) override
+    void onError(WebPassType<typename T::WebType> e) override
     {
-        OwnPtr<typename T::WebType> ownPtr = adoptPtr(error);
+        typename T::WebType error(adopt(e));
         if (!m_resolver->executionContext() || m_resolver->executionContext()->activeDOMObjectsAreStopped())
             return;
-        m_resolver->reject(T::take(m_resolver.get(), ownPtr.release()));
+        m_resolver->reject(T::take(m_resolver.get(), pass(error)));
     }
 
 private:
@@ -137,7 +142,7 @@ private:
 };
 
 template<typename S>
-class CallbackPromiseAdapter<S, void> final : public WebCallbacks<typename S::WebType*, void> {
+class CallbackPromiseAdapter<S, void> final : public WebCallbacks<typename CallbackPromiseAdapterBase::WebPassType<typename S::WebType>, void>, public CallbackPromiseAdapterBase {
     WTF_MAKE_NONCOPYABLE(CallbackPromiseAdapter);
 public:
     explicit CallbackPromiseAdapter(PassRefPtrWillBeRawPtr<ScriptPromiseResolver> resolver)
@@ -147,13 +152,12 @@ public:
     }
     ~CallbackPromiseAdapter() override { }
 
-    // Takes ownership of |result|.
-    void onSuccess(typename S::WebType* result) override
+    void onSuccess(WebPassType<typename S::WebType> r) override
     {
-        OwnPtr<typename S::WebType> ownPtr = adoptPtr(result);
+        typename S::WebType result(adopt(r));
         if (!m_resolver->executionContext() || m_resolver->executionContext()->activeDOMObjectsAreStopped())
             return;
-        m_resolver->resolve(S::take(m_resolver.get(), ownPtr.release()));
+        m_resolver->resolve(S::take(m_resolver.get(), pass(result)));
     }
 
     void onError() override
@@ -168,7 +172,7 @@ private:
 };
 
 template<typename T>
-class CallbackPromiseAdapter<bool, T> final : public WebCallbacks<bool*, typename T::WebType*> {
+class CallbackPromiseAdapter<bool, T> final : public WebCallbacks<bool*, typename CallbackPromiseAdapterBase::WebPassType<typename T::WebType>>, public CallbackPromiseAdapterBase {
     WTF_MAKE_NONCOPYABLE(CallbackPromiseAdapter);
 public:
     explicit CallbackPromiseAdapter(PassRefPtrWillBeRawPtr<ScriptPromiseResolver> resolver)
@@ -187,13 +191,12 @@ public:
         m_resolver->resolve(*result);
     }
 
-    // Takes ownership of |error|.
-    void onError(typename T::WebType* error) override
+    void onError(WebPassType<typename T::WebType> e) override
     {
-        OwnPtr<typename T::WebType> ownPtr = adoptPtr(error);
+        typename T::WebType error(adopt(e));
         if (!m_resolver->executionContext() || m_resolver->executionContext()->activeDOMObjectsAreStopped())
             return;
-        m_resolver->reject(T::take(m_resolver.get(), ownPtr.release()));
+        m_resolver->reject(T::take(m_resolver.get(), pass(error)));
     }
 
 private:
@@ -201,7 +204,7 @@ private:
 };
 
 template<>
-class CallbackPromiseAdapter<void, void> final : public WebCallbacks<void, void> {
+class CallbackPromiseAdapter<void, void> final : public WebCallbacks<void, void>, public CallbackPromiseAdapterBase {
     WTF_MAKE_NONCOPYABLE(CallbackPromiseAdapter);
 public:
     explicit CallbackPromiseAdapter(PassRefPtrWillBeRawPtr<ScriptPromiseResolver> resolver)
@@ -230,7 +233,7 @@ private:
 };
 
 template<>
-class CallbackPromiseAdapter<bool, void> final : public WebCallbacks<bool*, void> {
+class CallbackPromiseAdapter<bool, void> final : public WebCallbacks<bool*, void>, public CallbackPromiseAdapterBase {
     WTF_MAKE_NONCOPYABLE(CallbackPromiseAdapter);
 public:
     explicit CallbackPromiseAdapter(PassRefPtrWillBeRawPtr<ScriptPromiseResolver> resolver)