Refactor the callback mechanism used by runJavaScript

This prepares the way for other API made async like toHtml and
toPlainText.

Use a callback class with an implicit templated constructor to carry
the functor across the API boundary and avoid the intermediate helper
method as the ABI that we have to maintain.

Also pass the callback result through WebContentsAdapterClient using
a bookkeeping ID instead of transferring the callback to
WebContentsAdapter. This will allow other calls, which might not
already allow passing a callback functor, to use a consisten way
of carrying back the result to the top API layer.

Change-Id: Ia923767b9c1021a108c26da17d4c41878ef7cb95
Reviewed-by: Pierre Rossi <pierre.rossi@gmail.com>

7 files changed, 59 insertions, 49 deletions

diff --git a/src/core/web_contents_adapter.cpp b/src/core/web_contents_adapter.cpp
index 108506b06..56b56ce33 100644
--- a/src/core/web_contents_adapter.cpp
+++ b/src/core/web_contents_adapter.cpp
@@ -143,10 +143,9 @@ static QVariant fromJSValue(const base::Value *result)
     return ret;
 }
 
-static void callbackOnEvaluateJS(JSCallbackBase *callback, const base::Value *result)
+static void callbackOnEvaluateJS(WebContentsAdapterClient *adapterClient, quint64 requestId, const base::Value *result)
 {
-    callback->call(fromJSValue(result));
-    delete callback;
+    adapterClient->didRunJavaScript(fromJSValue(result), requestId);
 }
 
 static QStringList listRecursively(const QDir& dir) {
@@ -169,11 +168,13 @@ public:
     scoped_ptr<content::WebContents> webContents;
     scoped_ptr<WebContentsDelegateQt> webContentsDelegate;
     WebContentsAdapterClient *adapterClient;
+    quint64 lastRequestId;
 };
 
 WebContentsAdapterPrivate::WebContentsAdapterPrivate(WebContentsAdapterClient::RenderingMode renderingMode)
     // This has to be the first thing we create, and the last we destroy.
     : engineContext(WebEngineContext::currentOrCreate(renderingMode))
+    , lastRequestId(0)
 {
 }
 
@@ -372,17 +373,22 @@ void WebContentsAdapter::enableInspector(bool enable)
     ContentBrowserClientQt::Get()->enableInspector(enable);
 }
 
-void WebContentsAdapter::runJavaScript(const QString &javaScript, const QString &xPath, JSCallbackBase *func)
+void WebContentsAdapter::runJavaScript(const QString &javaScript, const QString &xPath)
 {
     Q_D(WebContentsAdapter);
     content::RenderViewHost *rvh = d->webContents->GetRenderViewHost();
     Q_ASSERT(rvh);
-    if (!func)
-        rvh->ExecuteJavascriptInWebFrame(toString16(xPath), toString16(javaScript));
-    else {
-        content::RenderViewHost::JavascriptResultCallback callback = base::Bind(&callbackOnEvaluateJS, func);
-        rvh->ExecuteJavascriptInWebFrameCallbackResult(toString16(xPath), toString16(javaScript), callback);
-    }
+    rvh->ExecuteJavascriptInWebFrame(toString16(xPath), toString16(javaScript));
+}
+
+quint64 WebContentsAdapter::runJavaScriptCallbackResult(const QString &javaScript, const QString &xPath)
+{
+    Q_D(WebContentsAdapter);
+    content::RenderViewHost *rvh = d->webContents->GetRenderViewHost();
+    Q_ASSERT(rvh);
+    content::RenderViewHost::JavascriptResultCallback callback = base::Bind(&callbackOnEvaluateJS, d->adapterClient, ++d->lastRequestId);
+    rvh->ExecuteJavascriptInWebFrameCallbackResult(toString16(xPath), toString16(javaScript), callback);
+    return d->lastRequestId;
 }
 
 void WebContentsAdapter::dpiScaleChanged()
diff --git a/src/core/web_contents_adapter.h b/src/core/web_contents_adapter.h
index c4c25c8a5..49c1de4c0 100644
--- a/src/core/web_contents_adapter.h
+++ b/src/core/web_contents_adapter.h
@@ -49,17 +49,11 @@
 #include <QString>
 #include <QUrl>
 
-QT_FORWARD_DECLARE_CLASS(QVariant)
 namespace content {
 class WebContents;
 }
 class WebContentsAdapterPrivate;
 
-struct JSCallbackBase {
-    virtual ~JSCallbackBase() {}
-    virtual void call(const QVariant&) = 0;
-};
-
 class QWEBENGINE_EXPORT WebContentsAdapter : public QSharedData {
 public:
     // Takes ownership of the WebContents.
@@ -89,8 +83,9 @@ public:
     void setZoomFactor(qreal);
     qreal currentZoomFactor() const;
     void enableInspector(bool);
-    void runJavaScript(const QString &javaScript, const QString &xPath = QString(), JSCallbackBase * = 0);
     void filesSelectedInChooser(const QStringList &fileList, WebContentsAdapterClient::FileChooserMode);
+    void runJavaScript(const QString &javaScript, const QString &xPath);
+    quint64 runJavaScriptCallbackResult(const QString &javaScript, const QString &xPath);
 
     void dpiScaleChanged();
 
diff --git a/src/core/web_contents_adapter_client.h b/src/core/web_contents_adapter_client.h
index 28cd3c4d8..e3e88e110 100644
--- a/src/core/web_contents_adapter_client.h
+++ b/src/core/web_contents_adapter_client.h
@@ -49,6 +49,8 @@
 #include <QStringList>
 #include <QUrl>
 
+QT_FORWARD_DECLARE_CLASS(QVariant)
+
 class JavaScriptDialogController;
 class RenderWidgetHostViewQt;
 class RenderWidgetHostViewQtDelegate;
@@ -127,6 +129,7 @@ public:
     virtual bool contextMenuRequested(const WebEngineContextMenuData&) = 0;
     virtual void javascriptDialog(QSharedPointer<JavaScriptDialogController>) = 0;
     virtual void runFileChooser(FileChooserMode, const QString &defaultFileName, const QStringList &acceptedMimeTypes) = 0;
+    virtual void didRunJavaScript(const QVariant& result, quint64 requestId) = 0;
 };
 
 #endif // WEB_CONTENTS_ADAPTER_CLIENT_H
diff --git a/src/webengine/api/qquickwebengineview_p_p.h b/src/webengine/api/qquickwebengineview_p_p.h
index f0bc0a9fb..96632904c 100644
--- a/src/webengine/api/qquickwebengineview_p_p.h
+++ b/src/webengine/api/qquickwebengineview_p_p.h
@@ -138,6 +138,7 @@ public:
     virtual bool contextMenuRequested(const WebEngineContextMenuData &) Q_DECL_OVERRIDE;
     virtual void javascriptDialog(QSharedPointer<JavaScriptDialogController>) Q_DECL_OVERRIDE;
     virtual void runFileChooser(FileChooserMode, const QString &defaultFileName, const QStringList &acceptedMimeTypes) Q_DECL_OVERRIDE;
+    virtual void didRunJavaScript(const QVariant&, quint64) Q_DECL_OVERRIDE { }
 
     void setDevicePixelRatio(qreal);
 
diff --git a/src/webenginewidgets/api/qwebenginepage.cpp b/src/webenginewidgets/api/qwebenginepage.cpp
index 54abb4384..8192f38ed 100644
--- a/src/webenginewidgets/api/qwebenginepage.cpp
+++ b/src/webenginewidgets/api/qwebenginepage.cpp
@@ -146,6 +146,11 @@ void QWebEnginePagePrivate::close()
     Q_EMIT q->windowCloseRequested();
 }
 
+void QWebEnginePagePrivate::didRunJavaScript(const QVariant& result, quint64 requestId)
+{
+    (*m_variantCallbacks.take(requestId))(result);
+}
+
 void QWebEnginePagePrivate::updateAction(QWebEnginePage::WebAction action) const
 {
 #ifdef QT_NO_ACTION
@@ -490,20 +495,11 @@ void QWebEnginePage::runJavaScript(const QString &scriptSource, const QString &x
     d->adapter->runJavaScript(scriptSource, xPath);
 }
 
-namespace {
-struct JSCallbackFunctor : public JSCallbackBase {
-    JSCallbackFunctor(QtWebEnginePrivate::FunctorBase *functor) : m_func(functor) { }
-    ~JSCallbackFunctor() { delete m_func; }
-    void call(const QVariant &value) { (*m_func)(value); }
-private:
-    QtWebEnginePrivate::FunctorBase *m_func;
-};
-}
-
-void QWebEnginePage::runJavaScriptHelper(const QString &source, QtWebEnginePrivate::FunctorBase *functor, const QString &xPath)
+void QWebEnginePage::runJavaScript(const QString& scriptSource, const QWebEngineCallback<const QVariant &> &resultCallback, const QString &xPath)
 {
     Q_D(QWebEnginePage);
-    d->adapter->runJavaScript(source, xPath, new JSCallbackFunctor(functor));
+    quint64 requestId = d->adapter->runJavaScriptCallbackResult(scriptSource, xPath);
+    d->m_variantCallbacks.insert(requestId, resultCallback.d);
 }
 
 QWebEnginePage *QWebEnginePage::createWindow(WebWindowType type)
diff --git a/src/webenginewidgets/api/qwebenginepage.h b/src/webenginewidgets/api/qwebenginepage.h
index 4d6a72fc4..c33119571 100644
--- a/src/webenginewidgets/api/qwebenginepage.h
+++ b/src/webenginewidgets/api/qwebenginepage.h
@@ -44,6 +44,7 @@ class QWebEngineElementCollection;
 
 class QWebEngineHistory;
 class QWebEngineHistoryItem;
+class QWebEnginePage;
 class QWebEnginePagePrivate;
 class QWebEnginePluginFactory;
 class QWebEngineSecurityOrigin;
@@ -52,19 +53,33 @@ class QWebEngineHitTestResultPrivate;
 
 namespace QtWebEnginePrivate {
 
-struct FunctorBase {
-    virtual ~FunctorBase() {}
-    virtual void operator()(const QVariant &) = 0;
+template <typename T>
+class QWebEngineCallbackPrivateBase : public QSharedData {
+public:
+    virtual ~QWebEngineCallbackPrivateBase() {}
+    virtual void operator()(T) = 0;
+};
+
+template <typename T, typename F>
+class QWebEngineCallbackPrivate : public QWebEngineCallbackPrivateBase<T> {
+public:
+    QWebEngineCallbackPrivate(F callable) : m_callable(callable) {}
+    virtual void operator()(T value) Q_DECL_OVERRIDE { m_callable(value); }
+private:
+    F m_callable;
 };
 
-template <typename F>
-struct FunctorCallback : public FunctorBase {
-    FunctorCallback(F callback) : m_callback(callback) {}
-    virtual void operator()(const QVariant &value) Q_DECL_OVERRIDE { m_callback(value); }
+} // namespace QtWebEnginePrivate
+
+template <typename T>
+class QWebEngineCallback {
+public:
+    template <typename F>
+    QWebEngineCallback(F f) : d(new QtWebEnginePrivate::QWebEngineCallbackPrivate<T, F>(f)) { }
 private:
-    F m_callback;
+    QExplicitlySharedDataPointer<QtWebEnginePrivate::QWebEngineCallbackPrivateBase<T> > d;
+    friend class QWebEnginePage;
 };
-}
 
 class QWEBENGINEWIDGETS_EXPORT QWebEngineHitTestResult {
 public:
@@ -484,9 +499,7 @@ public:
     QWebEngineSecurityOrigin securityOrigin() const;
 
     void runJavaScript(const QString& scriptSource, const QString &xPath = QString());
-
-    template <typename F>
-    void runJavaScript(const QString& scriptSource, F func, const QString &xPath = QString());
+    void runJavaScript(const QString& scriptSource, const QWebEngineCallback<const QVariant &> &resultCallback, const QString &xPath = QString());
 
 public Q_SLOTS:
     // Ex-QWebFrame slot
@@ -564,7 +577,6 @@ private:
 #ifndef QT_NO_ACTION
     Q_PRIVATE_SLOT(d_func(), void _q_webActionTriggered(bool checked))
 #endif
-    void runJavaScriptHelper(const QString &source, QtWebEnginePrivate::FunctorBase *, const QString &xPath);
 
     friend class QWebEngineView;
     friend class QWebEngineViewPrivate;
@@ -573,13 +585,6 @@ private:
 Q_DECLARE_OPERATORS_FOR_FLAGS(QWebEnginePage::FindFlags);
 Q_DECLARE_OPERATORS_FOR_FLAGS(QWebEnginePage::RenderLayers);
 
-
-template <typename F>
-inline void QWebEnginePage::runJavaScript(const QString &scriptSource, F func, const QString &xPath)
-{
-    runJavaScriptHelper(scriptSource, new QtWebEnginePrivate::FunctorCallback<F>(func), xPath);
-}
-
 QT_END_NAMESPACE
 
 #endif // QWEBENGINEPAGE_H
diff --git a/src/webenginewidgets/api/qwebenginepage_p.h b/src/webenginewidgets/api/qwebenginepage_p.h
index c8dd63daa..656eb4788 100644
--- a/src/webenginewidgets/api/qwebenginepage_p.h
+++ b/src/webenginewidgets/api/qwebenginepage_p.h
@@ -80,6 +80,7 @@ public:
     virtual bool contextMenuRequested(const WebEngineContextMenuData &data) Q_DECL_OVERRIDE;
     virtual void javascriptDialog(QSharedPointer<JavaScriptDialogController>) Q_DECL_OVERRIDE;
     virtual void runFileChooser(FileChooserMode, const QString &defaultFileName, const QStringList &acceptedMimeTypes) Q_DECL_OVERRIDE;
+    virtual void didRunJavaScript(const QVariant& result, quint64 requestId) Q_DECL_OVERRIDE;
 
     void updateAction(QWebEnginePage::WebAction) const;
     void updateNavigationActions();
@@ -91,6 +92,9 @@ public:
     mutable QAction *actions[QWebEnginePage::WebActionCount];
     bool m_isLoading;
     WebEngineContextMenuData m_menuData;
+
+    typedef QtWebEnginePrivate::QWebEngineCallbackPrivateBase<const QVariant&> VariantCallback;
+    mutable QHash<quint64, QExplicitlySharedDataPointer<VariantCallback> > m_variantCallbacks;
 };
 
 QT_END_NAMESPACE