Fix JS interrupt

fix_js_interrupt.patch

From 96832b2ef7376b20598c3ae372b490f52c80f28d Mon Sep 17 00:00:00 2001
From: Vitaliy Slobodin <vitaliy.slobodin@gmail.com>
Date: Wed, 3 Apr 2013 22:06:38 +0400
Subject: [PATCH] Define the new page callback for interrupting a long-running
 JavaScript

---
 src/modules/webpage.js |  3 +++
 src/webpage.cpp        | 37 +++++++++++++++++++++++++++++++++++--
 src/webpage.h          |  2 ++
 3 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/src/modules/webpage.js b/src/modules/webpage.js
index e2f52c6..e8e6369 100644
--- a/src/modules/webpage.js
+++ b/src/modules/webpage.js
@@ -468,6 +468,9 @@ function decorateNewPage(opts, page) {
     // @see https://developer.mozilla.org/en/DOM/window.prompt
     definePageCallbackHandler(page, handlers, "onPrompt", "_getJsPromptCallback");
 
+    // Calls from within the page when some javascript code running to long
+    definePageCallbackHandler(page, handlers, "onShouldInterruptJs", "_getJsInterruptCallback");
+
     page.event = {};
     page.event.modifier = {
         shift:  0x02000000,
diff --git a/src/webpage.cpp b/src/webpage.cpp
index c76a4b8..3ba71cf 100644
--- a/src/webpage.cpp
+++ b/src/webpage.cpp
@@ -112,8 +112,8 @@ public:
 
 public slots:
     bool shouldInterruptJavaScript() {
-        QApplication::processEvents(QEventLoop::AllEvents, 42);
-        return false;
+        bool willInterrupt = m_webPage->javascriptInterrupt();
+        return willInterrupt;
     }
 
 protected:
@@ -251,6 +251,7 @@ public:
         , m_filePickerCallback(NULL)
         , m_jsConfirmCallback(NULL)
         , m_jsPromptCallback(NULL)
+        , m_jsInterruptCallback(NULL)
     {
     }
 
@@ -289,6 +290,15 @@ public:
         }
         return m_jsPromptCallback;
     }
+    
+    QObject *getJsInterruptCallback() {
+        qDebug() << "WebpageCallbacks - getJsInterruptCallback";
+
+        if (!m_jsInterruptCallback) {
+            m_jsInterruptCallback = new Callback(this);
+        }
+        return m_jsInterruptCallback;
+    }
 
 public slots:
     QVariant call(const QVariantList &arguments) {
@@ -303,6 +313,7 @@ private:
     Callback *m_filePickerCallback;
     Callback *m_jsConfirmCallback;
     Callback *m_jsPromptCallback;
+    Callback *m_jsInterruptCallback;
 
     friend class WebPage;
 };
@@ -729,6 +740,19 @@ bool WebPage::javaScriptPrompt(const QString &msg, const QString &defaultValue,
     return false;
 }
 
+bool WebPage::javascriptInterrupt()
+{
+    if (m_callbacks->m_jsInterruptCallback) {
+        QVariant res = m_callbacks->m_jsInterruptCallback->call(QVariantList());
+
+        if (res.canConvert<bool>()) {
+            return res.toBool();
+        }
+    }
+
+    return false;
+}
+
 void WebPage::finish(bool ok)
 {
     QString status = ok ? "success" : "fail";
@@ -1293,6 +1317,15 @@ QObject *WebPage::_getJsPromptCallback()
     return m_callbacks->getJsPromptCallback();
 }
 
+QObject *WebPage::_getJsInterruptCallback()
+{
+    if (!m_callbacks) {
+        m_callbacks = new WebpageCallbacks(this);
+    }
+
+    return m_callbacks->getJsInterruptCallback();
+}
+
 void WebPage::sendEvent(const QString &type, const QVariant &arg1, const QVariant &arg2, const QString &mouseButton, const QVariant &modifierArg)
 {
     Qt::KeyboardModifiers keyboardModifiers(modifierArg.toInt());
diff --git a/src/webpage.h b/src/webpage.h
index e065bb4..f8d022b 100644
--- a/src/webpage.h
+++ b/src/webpage.h
@@ -258,6 +258,7 @@ public slots:
     QObject *_getFilePickerCallback();
     QObject *_getJsConfirmCallback();
     QObject *_getJsPromptCallback();
+    QObject *_getJsInterruptCallback();
     void _uploadFile(const QString &selector, const QStringList &fileNames);
     void sendEvent(const QString &type, const QVariant &arg1 = QVariant(), const QVariant &arg2 = QVariant(), const QString &mouseButton = QString(), const QVariant &modifierArg = QVariant());
 
@@ -497,6 +498,7 @@ private:
     QString filePicker(const QString &oldFile);
     bool javaScriptConfirm(const QString &msg);
     bool javaScriptPrompt(const QString &msg, const QString &defaultValue, QString *result);
+    bool javascriptInterrupt();
 
 private:
     CustomPage *m_customWebPage;
-- 
1.8.1.msysgit.1
 

L133 looks like a typo:

return extension == ChooseMultipleFilesExtension || ShouldInterruptJavaScript;

I'm thinking it should be:

return extension == ChooseMultipleFilesExtension || extension == ShouldInterruptJavaScript;

Showing off my lack of WebKit knowledge as usual:
Why is all of this Extension stuff necessary? Shouldn't we just be able to override the shouldInterruptJavaScript function (as we are already doing) to just use a callback of our own creation?

@JamesMGreene: This is exactly what I was thinking when I reviewed the patch last night.

@JamesMGreene, @terinjokes because interrupting a long-running JavaScript was implemented using a hack in Qt 4.8:
https://github.com/ariya/phantomjs/blob/master/src/qt/src/3rdparty/webkit/Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp#L374

Ideally solution was in creating a new virtual function, which might be overriden. But, in that case, this will break the binary compatibility. And they decide to implement this functionality using QMetaMethod.
Yes, extension is overkill for that.

But, we can forget about this terrible extension! :)
I've implemented the 'normal' solution for this issue (without patching our copy of Webkit). I'll update my gist soon.

@vitallium - I've been testing with this patch for a while and I have not found any issues. I notice that you also have a branch with this code in. I would like to see this pushed if possible.