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

Source/bindings/core/v8/CallbackPromiseAdapter.h

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 18 matching lines @@
  */
 
 #ifndef CallbackPromiseAdapter_h
 #define CallbackPromiseAdapter_h
 
 #include "bindings/core/v8/ScriptPromiseResolver.h"
 #include "public/platform/WebCallbacks.h"
 #include "public/platform/WebPassOwnPtr.h"
 #include "wtf/OwnPtr.h"
 #include "wtf/PassOwnPtr.h"
+#include "wtf/TypeTraits.h"
 
 namespace blink {
 

[kinuko, 2015/07/26 15:15:08]

nit: for files that heavily use templates I prefer having overview comment at
the beginning of file and have internal templates in a nested namespace like
'internal', so that it becomes clearer what all these templates are about. I'd
probably move the comment at line 163 here and also add a (very) brief
description about how it's implemented.

[yhirano, 2015/07/27 05:30:02]

Done.

CallbackPromiseAdapterInternal in internal is a bit messy, though. It is needed
because blink::internal is shared by many classes.

[kinuko, 2015/07/27 05:59:00]

Thanks, I still prefer having them in internal (but let's see what owners say).

-class CallbackPromiseAdapterBase {
-public:
+// This template is placed outside of CallbackPromiseAdapterInternal because
+// explicit specialization is forbidden in a class scope.
+template <typename T>
+struct CallbackPromiseAdapterTrivialWebTypeHolder {
+    using WebType = T;
+    // TODO(yhirano): Use universal reference and std::forward once allowed.
+    // https://chromium-cpp.appspot.com/
+    static T take(ScriptPromiseResolver*, const T& x) { return x; }
+};
+template <>
+struct CallbackPromiseAdapterTrivialWebTypeHolder<void> {
+    using WebType = void;
+};
+
+class CallbackPromiseAdapterInternal {
+private:
+    template <typename T> static T webTypeHolderMatcher(typename WTF::RemoveReference<typename T::WebType>::Type*);
+    template <typename T> static CallbackPromiseAdapterTrivialWebTypeHolder<T> webTypeHolderMatcher(...);
+    template <typename T> using WebTypeHolder = decltype(webTypeHolderMatcher<T>(nullptr));
+
+    // TODO(yhirano): Use scoped_ptr.
     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 = WebPassOwnPtr<T>;
     };
-
-    template <typename T>
-    using PassType = typename PassTypeImpl<T>::Type;
-    template <typename T>
-    using WebPassType = typename WebPassTypeImpl<T>::Type;
+    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 once they are
     // allowed. https://chromium-cpp.appspot.com/
-    template <typename T>
-    static T& adopt(T& x) { return x; }
-    template <typename T>
-    static PassOwnPtr<T> adopt(WebPassOwnPtr<T>& x) { return x.release(); }
+    template <typename T> static T& adopt(T& x) { return x; }
+    template <typename T> static PassOwnPtr<T> adopt(WebPassOwnPtr<T>& x) { return x.release(); }
 
     // TODO(yhirano): Use rvalue reference and std::move once they are allowed.
     // https://chromium-cpp.appspot.com/
+    template <typename T> static PassType<T> pass(T& x) { return x; }
+    template <typename T> static PassOwnPtr<T> pass(OwnPtr<T>& x) { return x.release(); }
+
+    template <typename S, typename T>
+    class Base : public WebCallbacks<WebPassType<typename S::WebType>, WebPassType<typename T::WebType>> {
+    public:
+        explicit Base(PassRefPtrWillBeRawPtr<ScriptPromiseResolver> resolver) : m_resolver(resolver) {}
+        ScriptPromiseResolver* resolver() { return m_resolver.get(); }
+
+    private:
+        RefPtrWillBePersistent<ScriptPromiseResolver> m_resolver;
+    };
+
+    template <typename S, typename T>
+    class OnSuccess : public Base<S, T> {

[kinuko, 2015/07/26 15:15:08]

Um... would it be possible / make sense to define onSuccess / onError behavior
as separate templates and define CallbackPromiseAdapter template as composite of
these?  I feel using composite would make this more straightforward but I might
be wrong.

[yhirano, 2015/07/27 05:30:01]

I wanted to do so but I couldn't. I think the proposed implementation is better
than the existing one that has duplicated onSuccess / onError implementations.

[kinuko, 2015/07/27 05:59:00]

Hm, ok...

+    public:
+        explicit OnSuccess(PassRefPtrWillBeRawPtr<ScriptPromiseResolver> resolver) : Base<S, T>(resolver) {}
+        void onSuccess(WebPassType<typename S::WebType> r) override
+        {
+            typename S::WebType result(adopt(r));
+            ScriptPromiseResolver* resolver = this->resolver();
+            if (!resolver->executionContext() || resolver->executionContext()->activeDOMObjectsAreStopped())
+                return;
+            resolver->resolve(S::take(resolver, pass(result)));
+        }
+    };
     template <typename T>
-    static PassType<T> pass(T& x) { return x; }
-    template <typename T>
-    static PassOwnPtr<T> pass(OwnPtr<T>& x) { return x.release(); }
+    class OnSuccess<WebTypeHolder<void>, T> : public Base<WebTypeHolder<void>, T> {
+    public:
+        explicit OnSuccess(PassRefPtrWillBeRawPtr<ScriptPromiseResolver> resolver) : Base<WebTypeHolder<void>, T>(resolver) {}
+        void onSuccess() override
+        {
+            ScriptPromiseResolver* resolver = this->resolver();
+            if (!resolver->executionContext() || resolver->executionContext()->activeDOMObjectsAreStopped())
+                return;
+            resolver->resolve();
+        }
+    };
+    template <typename S, typename T>
+    class OnError : public OnSuccess<S, T> {
+    public:
+        explicit OnError(PassRefPtrWillBeRawPtr<ScriptPromiseResolver> resolver) : OnSuccess<S, T>(resolver) {}
+        void onError(WebPassType<typename T::WebType> e) override
+        {
+            typename T::WebType result(adopt(e));
+            ScriptPromiseResolver* resolver = this->resolver();
+            if (!resolver->executionContext() || resolver->executionContext()->activeDOMObjectsAreStopped())
+                return;
+            resolver->reject(T::take(resolver, pass(result)));
+        }
+    };
+    template <typename S>
+    class OnError<S, WebTypeHolder<void>> : public OnSuccess<S, WebTypeHolder<void>> {
+    public:
+        explicit OnError(PassRefPtrWillBeRawPtr<ScriptPromiseResolver> resolver) : OnSuccess<S, WebTypeHolder<void>>(resolver) {}
+        void onError() override
+        {
+            ScriptPromiseResolver* resolver = this->resolver();
+            if (!resolver->executionContext() || resolver->executionContext()->activeDOMObjectsAreStopped())
+                return;
+            resolver->reject();
+        }
+    };
+
+public:
+    template <typename S, typename T>
+    class CallbackPromiseAdapter final : public OnError<WebTypeHolder<S>, WebTypeHolder<T>> {
+        WTF_MAKE_NONCOPYABLE(CallbackPromiseAdapter);
+    public:
+        explicit CallbackPromiseAdapter(PassRefPtrWillBeRawPtr<ScriptPromiseResolver> resolver) : OnError<WebTypeHolder<S>, WebTypeHolder<T>>(resolver) {}
+    };
 };
 
-
-// TODO(yhirano): Add comments.
-template<typename S, typename T>
-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)
-        : m_resolver(resolver)
-    {
-        ASSERT(m_resolver);
-    }
-    ~CallbackPromiseAdapter() override { }
-
-    void onSuccess(WebPassType<typename S::WebType> r) override
-    {
-        typename S::WebType result(adopt(r));
-        if (!m_resolver->executionContext() || m_resolver->executionContext()->activeDOMObjectsAreStopped())
-            return;
-        m_resolver->resolve(S::take(m_resolver.get(), pass(result)));
-    }
-
-    void onError(WebPassType<typename T::WebType> e) override
-    {
-        typename T::WebType error(adopt(e));
-        if (!m_resolver->executionContext() || m_resolver->executionContext()->activeDOMObjectsAreStopped())
-            return;
-        m_resolver->reject(T::take(m_resolver.get(), pass(error)));
-    }
-
-private:
-    RefPtrWillBePersistent<ScriptPromiseResolver> m_resolver;
-};
-
-template<typename T>
-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)
-        : m_resolver(resolver)
-    {
-        ASSERT(m_resolver);
-    }
-    ~CallbackPromiseAdapter() override { }
-
-    void onSuccess() override
-    {
-        if (!m_resolver->executionContext() || m_resolver->executionContext()->activeDOMObjectsAreStopped())
-            return;
-        m_resolver->resolve();
-    }
-
-    void onError(WebPassType<typename T::WebType> e) override
-    {
-        typename T::WebType error(adopt(e));
-        if (!m_resolver->executionContext() || m_resolver->executionContext()->activeDOMObjectsAreStopped())
-            return;
-        m_resolver->reject(T::take(m_resolver.get(), pass(error)));
-    }
-
-private:
-    RefPtrWillBePersistent<ScriptPromiseResolver> m_resolver;
-};
-
-template<typename S>
-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)
-        : m_resolver(resolver)
-    {
-        ASSERT(m_resolver);
-    }
-    ~CallbackPromiseAdapter() override { }
-
-    void onSuccess(WebPassType<typename S::WebType> r) override
-    {
-        typename S::WebType result(adopt(r));
-        if (!m_resolver->executionContext() || m_resolver->executionContext()->activeDOMObjectsAreStopped())
-            return;
-        m_resolver->resolve(S::take(m_resolver.get(), pass(result)));
-    }
-
-    void onError() override
-    {
-        if (!m_resolver->executionContext() || m_resolver->executionContext()->activeDOMObjectsAreStopped())
-            return;
-        m_resolver->reject();
-    }
-
-private:
-    RefPtrWillBePersistent<ScriptPromiseResolver> m_resolver;
-};
-
-template<typename T>
-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)
-        : m_resolver(resolver)
-    {
-        ASSERT(m_resolver);
-    }
-    ~CallbackPromiseAdapter() override { }
-
-    // TODO(nhiroki): onSuccess should take ownership of a bool object for
-    // consistency. (http://crbug.com/493531)
-    void onSuccess(bool* result) override
-    {
-        if (!m_resolver->executionContext() || m_resolver->executionContext()->activeDOMObjectsAreStopped())
-            return;
-        m_resolver->resolve(*result);
-    }
-
-    void onError(WebPassType<typename T::WebType> e) override
-    {
-        typename T::WebType error(adopt(e));
-        if (!m_resolver->executionContext() || m_resolver->executionContext()->activeDOMObjectsAreStopped())
-            return;
-        m_resolver->reject(T::take(m_resolver.get(), pass(error)));
-    }
-
-private:
-    RefPtrWillBePersistent<ScriptPromiseResolver> m_resolver;
-};
-
-template<>
-class CallbackPromiseAdapter<void, void> final : public WebCallbacks<void, void>, public CallbackPromiseAdapterBase {
-    WTF_MAKE_NONCOPYABLE(CallbackPromiseAdapter);
-public:
-    explicit CallbackPromiseAdapter(PassRefPtrWillBeRawPtr<ScriptPromiseResolver> resolver)
-        : m_resolver(resolver)
-    {
-        ASSERT(m_resolver);
-    }
-    ~CallbackPromiseAdapter() override { }
-
-    void onSuccess() override
-    {
-        if (!m_resolver->executionContext() || m_resolver->executionContext()->activeDOMObjectsAreStopped())
-            return;
-        m_resolver->resolve();
-    }
-
-    void onError() override
-    {
-        if (!m_resolver->executionContext() || m_resolver->executionContext()->activeDOMObjectsAreStopped())
-            return;
-        m_resolver->reject();
-    }
-
-private:
-    RefPtrWillBePersistent<ScriptPromiseResolver> m_resolver;
-};
-
-template<>
-class CallbackPromiseAdapter<bool, void> final : public WebCallbacks<bool*, void>, public CallbackPromiseAdapterBase {
-    WTF_MAKE_NONCOPYABLE(CallbackPromiseAdapter);
-public:
-    explicit CallbackPromiseAdapter(PassRefPtrWillBeRawPtr<ScriptPromiseResolver> resolver)
-        : m_resolver(resolver)
-    {
-        ASSERT(m_resolver);
-    }
-    ~CallbackPromiseAdapter() override { }
-
-    // TODO(nhiroki): onSuccess should take ownership of a bool object for
-    // consistency. (http://crbug.com/493531)
-    void onSuccess(bool* result) override
-    {
-        if (!m_resolver->executionContext() || m_resolver->executionContext()->activeDOMObjectsAreStopped())
-            return;
-        m_resolver->resolve(*result);
-    }
-
-    void onError() override
-    {
-        if (!m_resolver->executionContext() || m_resolver->executionContext()->activeDOMObjectsAreStopped())
-            return;
-        m_resolver->reject();
-    }
-
-private:
-    RefPtrWillBePersistent<ScriptPromiseResolver> m_resolver;
-};
+// CallbackPromiseAdapter is a WebCallbacks subclass and resolves / rejects the
+// stored resolver when onSuccess / onError is called, respectively.
+//
+// Basically CallbackPromiseAdapter<S, T> is a subclass of
+// WebCallbacks<S::WebType, T::WebType>. There are some exceptions:
+//  - If S or T don't have WebType (e.g. S = bool), the trivial WebType holder
+//    is used. For example, CallbackPromiseAdapter<bool, void> is a subclass of
+//    WebCallbacks<bool, void>.
+//  - If a WebType is OwnPtr<T>, its corresponding type parameter on
+//    WebCallbacks is WebPassOwnPtr<T>, because WebCallbacks is exposed to
+//    Chromium.
+//
+// When onSuccess is called with a S::WebType value, the value is passed to
+// S::take and the resolver is resolved with its return value. Ditto for
+// onError.
+//
+// ScriptPromiseResolver::resolve / reject will not be called when the execution
+// context is stopped.
+template <typename S, typename T>
+using CallbackPromiseAdapter = CallbackPromiseAdapterInternal::CallbackPromiseAdapter<S, T>;
 
 } // namespace blink
 
 #endif