V4 Debugger: Add command to change break points

The function to do so has been around for a long time. Finally expose
the functionality to the client. It doesn't make much sense to allow the
client to set the initial enabled/disabled state, but not to change it
later.

Task-number: QTCREATORBUG-20795
Change-Id: Ie2cb01ca3ca5578b6bc85650d7ee38d0aad9bbab
Reviewed-by: Simon Hausmann <simon.hausmann@qt.io>

2 files changed, 214 insertions, 49 deletions

diff --git a/src/plugins/qmltooling/qmldbg_debugger/qv4debugservice.cpp b/src/plugins/qmltooling/qmldbg_debugger/qv4debugservice.cpp
index aa05a62323..5866163ca6 100644
--- a/src/plugins/qmltooling/qmldbg_debugger/qv4debugservice.cpp
+++ b/src/plugins/qmltooling/qmldbg_debugger/qv4debugservice.cpp
@@ -174,91 +174,130 @@ public:
                     QLatin1String("this is not V8, this is V4 in Qt " QT_VERSION_STR));
         body.insert(QStringLiteral("UnpausedEvaluate"), true);
         body.insert(QStringLiteral("ContextEvaluate"), true);
+        body.insert(QStringLiteral("ChangeBreakpoint"), true);
         addBody(body);
     }
 };
 
-class V4SetBreakPointRequest: public V4CommandHandler
+class V4BreakPointRequest: public V4CommandHandler
 {
 public:
-    V4SetBreakPointRequest(): V4CommandHandler(QStringLiteral("setbreakpoint")) {}
+    V4BreakPointRequest(const QString &name): V4CommandHandler(name) {}
 
-    void handleRequest() override
+    void handleRequest() final
     {
+        // Other types are currently not supported
+        m_type = QStringLiteral("scriptRegExp");
+
         // decypher the payload:
-        QJsonObject args = req.value(QLatin1String("arguments")).toObject();
-        if (args.isEmpty())
+        m_args = req.value(QLatin1String("arguments")).toObject();
+        if (m_args.isEmpty()) {
+            createErrorResponse(QStringLiteral("breakpoint request with empty arguments object"));
             return;
+        }
+
+        const int id = handleBreakPointRequest();
+        if (id < 0) {
+            createErrorResponse(m_error);
+        } else {
+            // response:
+            addCommand();
+            addRequestSequence();
+            addSuccess(true);
+            addRunning();
+            QJsonObject body;
+            body.insert(QStringLiteral("type"), m_type);
+            body.insert(QStringLiteral("breakpoint"), id);
+            addBody(body);
+        }
+    }
 
-        QString type = args.value(QLatin1String("type")).toString();
+protected:
+    virtual int handleBreakPointRequest() = 0;
+
+    QJsonObject m_args;
+    QString m_type;
+    QString m_error;
+};
+
+class V4SetBreakPointRequest: public V4BreakPointRequest
+{
+public:
+    V4SetBreakPointRequest(): V4BreakPointRequest(QStringLiteral("setbreakpoint")) {}
+
+    int handleBreakPointRequest() final
+    {
+        // decypher the payload:
+        const QString type = m_args.value(QLatin1String("type")).toString();
         if (type != QLatin1String("scriptRegExp")) {
-            createErrorResponse(QStringLiteral("breakpoint type \"%1\" is not implemented").arg(type));
-            return;
+            m_error = QStringLiteral("breakpoint type \"%1\" is not implemented").arg(type);
+            return -1;
         }
 
-        QString fileName = args.value(QLatin1String("target")).toString();
+        const QString fileName = m_args.value(QLatin1String("target")).toString();
         if (fileName.isEmpty()) {
-            createErrorResponse(QStringLiteral("breakpoint has no file name"));
-            return;
+            m_error = QStringLiteral("breakpoint has no file name");
+            return -1;
         }
 
-        int line = args.value(QLatin1String("line")).toInt(-1);
+
+        const int line = m_args.value(QLatin1String("line")).toInt(-1);
         if (line < 0) {
-            createErrorResponse(QStringLiteral("breakpoint has an invalid line number"));
-            return;
+            m_error = QStringLiteral("breakpoint has an invalid line number");
+            return -1;
         }
 
-        bool enabled = args.value(QStringLiteral("enabled")).toBool(true);
-        QString condition = args.value(QStringLiteral("condition")).toString();
+        const bool enabled = m_args.value(QStringLiteral("enabled")).toBool(true);
+        const QString condition = m_args.value(QStringLiteral("condition")).toString();
 
         // set the break point:
-        int id = debugService->debuggerAgent.addBreakPoint(fileName, line + 1, enabled, condition);
+        return debugService->debuggerAgent.addBreakPoint(fileName, line + 1, enabled, condition);
 
-        // response:
-        addCommand();
-        addRequestSequence();
-        addSuccess(true);
-        addRunning();
-        QJsonObject body;
-        body.insert(QStringLiteral("type"), type);
-        body.insert(QStringLiteral("breakpoint"), id);
         // It's undocumented, but V8 sends back an actual_locations array too. However, our
         // Debugger currently doesn't tell us when it resolved a breakpoint, so we'll leave them
         // pending until the breakpoint is hit for the first time.
-        addBody(body);
     }
 };
 
-class V4ClearBreakPointRequest: public V4CommandHandler
+class V4ClearBreakPointRequest: public V4BreakPointRequest
 {
 public:
-    V4ClearBreakPointRequest(): V4CommandHandler(QStringLiteral("clearbreakpoint")) {}
+    V4ClearBreakPointRequest(): V4BreakPointRequest(QStringLiteral("clearbreakpoint")) {}
 
-    void handleRequest() override
+    int handleBreakPointRequest() final
     {
-        // decypher the payload:
-        QJsonObject args = req.value(QLatin1String("arguments")).toObject();
-        if (args.isEmpty())
-            return;
+        const int id = m_args.value(QLatin1String("breakpoint")).toInt(-1);
+        if (id < 0)
+            m_error = QStringLiteral("breakpoint has an invalid number");
+        else // remove the break point:
+            debugService->debuggerAgent.removeBreakPoint(id);
+
+        return id;
+    }
+};
 
-        int id = args.value(QLatin1String("breakpoint")).toInt(-1);
+class V4ChangeBreakPointRequest: public V4BreakPointRequest
+{
+public:
+    V4ChangeBreakPointRequest(): V4BreakPointRequest(QStringLiteral("changebreakpoint")) {}
+
+    int handleBreakPointRequest() final
+    {
+        const int id = m_args.value(QLatin1String("breakpoint")).toInt(-1);
         if (id < 0) {
-            createErrorResponse(QStringLiteral("breakpoint has an invalid number"));
-            return;
+            m_error = QStringLiteral("breakpoint has an invalid number");
+            return id;
         }
 
-        // remove the break point:
-        debugService->debuggerAgent.removeBreakPoint(id);
+        const QJsonValue enabled = m_args.value(QLatin1String("enabled"));
+        if (!enabled.isBool()) {
+            m_error = QStringLiteral("missing bool \"enabled\" in breakpoint change request");
+            return -1;
+        }
 
-        // response:
-        addCommand();
-        addRequestSequence();
-        addSuccess(true);
-        addRunning();
-        QJsonObject body;
-        body.insert(QStringLiteral("type"), QStringLiteral("scriptRegExp"));
-        body.insert(QStringLiteral("breakpoint"), id);
-        addBody(body);
+        // enable or disable the break point:
+        debugService->debuggerAgent.enableBreakPoint(id, enabled.toBool());
+        return id;
     }
 };
 
@@ -659,6 +698,7 @@ QV4DebugServiceImpl::QV4DebugServiceImpl(QObject *parent) :
     addHandler(new V4VersionRequest);
     addHandler(new V4SetBreakPointRequest);
     addHandler(new V4ClearBreakPointRequest);
+    addHandler(new V4ChangeBreakPointRequest);
     addHandler(new V4BacktraceRequest);
     addHandler(new V4FrameRequest);
     addHandler(new V4ScopeRequest);
diff --git a/tests/auto/qml/debugger/qqmldebugjs/tst_qqmldebugjs.cpp b/tests/auto/qml/debugger/qqmldebugjs/tst_qqmldebugjs.cpp
index 59e1cd8160..065dddefed 100644
--- a/tests/auto/qml/debugger/qqmldebugjs/tst_qqmldebugjs.cpp
+++ b/tests/auto/qml/debugger/qqmldebugjs/tst_qqmldebugjs.cpp
@@ -36,6 +36,7 @@
 #include <private/qpacket_p.h>
 
 #include <QtTest/qtest.h>
+#include <QtTest/qtestsystem.h>
 #include <QtCore/qprocess.h>
 #include <QtCore/qtimer.h>
 #include <QtCore/qfileinfo.h>
@@ -88,6 +89,7 @@ const char *SCRIPTS = "scripts";
 const char *SOURCE = "source";
 const char *SETBREAKPOINT = "setbreakpoint";
 const char *CLEARBREAKPOINT = "clearbreakpoint";
+const char *CHANGEBREAKPOINT = "changebreakpoint";
 const char *SETEXCEPTIONBREAK = "setexceptionbreak";
 const char *VERSION = "version";
 const char *DISCONNECT = "disconnect";
@@ -188,6 +190,9 @@ private slots:
     void clearBreakpoint_data() { targetData(); }
     void clearBreakpoint();
 
+    void changeBreakpoint_data() { targetData(); }
+    void changeBreakpoint();
+
     void setExceptionBreak_data() { targetData(); }
     void setExceptionBreak();
 
@@ -228,6 +233,7 @@ private:
 
     void targetData();
     bool waitForClientSignal(const char *signal, int timeout = 30000);
+    void checkVersionParameters();
 
     QTime t;
 };
@@ -273,6 +279,7 @@ public:
     void setBreakpoint(QString target, int line = -1, int column = -1, bool enabled = true,
                        QString condition = QString(), int ignoreCount = -1);
     void clearBreakpoint(int breakpoint);
+    void changeBreakpoint(int breakpoint, bool enabled);
     void setExceptionBreak(Exception type, bool enabled = false);
     void version();
     void disconnect();
@@ -609,6 +616,28 @@ void QJSDebugClient::clearBreakpoint(int breakpoint)
     sendMessage(packMessage(V8REQUEST, json.toString().toUtf8()));
 }
 
+void QJSDebugClient::changeBreakpoint(int breakpoint, bool enabled)
+{
+    //    { "seq"       : <number>,
+    //      "type"      : "request",
+    //      "command"   : "changebreakpoint",
+    //      "arguments" : { "breakpoint" : <number of the break point to change>
+    //                      "enabled" : <bool: enables the break type if true, disables if false>
+    //                    }
+    //    }
+    VARIANTMAPINIT;
+    jsonVal.setProperty(QLatin1String(COMMAND), QLatin1String(CHANGEBREAKPOINT));
+
+    QJSValue args = parser.call(QJSValueList() << obj);
+
+    args.setProperty(QLatin1String(BREAKPOINT), breakpoint);
+    args.setProperty(QLatin1String(ENABLED), enabled);
+    jsonVal.setProperty(QLatin1String(ARGUMENTS), args);
+
+    const QJSValue json = stringify.call(QJSValueList() << jsonVal);
+    sendMessage(packMessage(V8REQUEST, json.toString().toUtf8()));
+}
+
 void QJSDebugClient::setExceptionBreak(Exception type, bool enabled)
 {
     //    { "seq"       : <number>,
@@ -696,7 +725,8 @@ void QJSDebugClient::messageReceived(const QByteArray &data)
 
                 if (!value.value("success").toBool()) {
                     emit failure();
-                    qDebug() << "Received success == false response from application";
+                    qDebug() << "Received success == false response from application:"
+                             << value.value("message").toString();
                     return;
                 }
 
@@ -718,7 +748,6 @@ void QJSDebugClient::messageReceived(const QByteArray &data)
                         debugCommand == "setexceptionbreak" /*||
                         debugCommand == "profile"*/) {
                     emit result();
-
                 } else {
                     // DO NOTHING
                 }
@@ -825,6 +854,7 @@ void tst_QQmlDebugJS::getVersion()
 
     m_client->version();
     QVERIFY(waitForClientSignal(SIGNAL(result())));
+    checkVersionParameters();
 }
 
 void tst_QQmlDebugJS::getVersionWhenAttaching()
@@ -837,6 +867,7 @@ void tst_QQmlDebugJS::getVersionWhenAttaching()
 
     m_client->version();
     QVERIFY(waitForClientSignal(SIGNAL(result())));
+    checkVersionParameters();
 }
 
 void tst_QQmlDebugJS::disconnect()
@@ -1131,6 +1162,89 @@ void tst_QQmlDebugJS::clearBreakpoint()
     QCOMPARE(body.value("sourceLine").toInt(), sourceLine2);
 }
 
+void tst_QQmlDebugJS::changeBreakpoint()
+{
+    //void clearBreakpoint(int breakpoint);
+    QFETCH(bool, qmlscene);
+
+    int sourceLine2 = 37;
+    int sourceLine1 = 38;
+    QCOMPARE(init(qmlscene, CHANGEBREAKPOINT_QMLFILE), ConnectSuccess);
+
+    m_client->connect();
+
+    auto extractBody = [&]() {
+        const QVariantMap value = m_client->parser.call(
+                    QJSValueList() << QJSValue(QString(m_client->response))).toVariant().toMap();
+        return value.value("body").toMap();
+    };
+
+    auto extractBreakPointId = [&](const QVariantMap &body) {
+        const QList<QVariant> breakpointsHit = body.value("breakpoints").toList();
+        if (breakpointsHit.size() != 1)
+            return -1;
+        return breakpointsHit[0].toInt();
+    };
+
+    auto setBreakPoint = [&](int sourceLine, bool enabled) {
+        int id = -1;
+        auto connection = QObject::connect(m_client, &QJSDebugClient::result, [&]() {
+            id = extractBody().value("breakpoint").toInt();
+        });
+
+        m_client->setBreakpoint(QLatin1String(CHANGEBREAKPOINT_QMLFILE), sourceLine, -1, enabled);
+        bool success = QTest::qWaitFor([&]() { return id >= 0; });
+        Q_UNUSED(success);
+
+        QObject::disconnect(connection);
+        return id;
+    };
+
+    //The breakpoints are in a timer loop so we can set them after connect().
+    //Furthermore the breakpoints should be hit in the right order because setting of breakpoints
+    //can only occur in the QML event loop. (see QCOMPARE for sourceLine2 below)
+    const int breakpoint1 = setBreakPoint(sourceLine1, false);
+    QVERIFY(breakpoint1 >= 0);
+
+    const int breakpoint2 = setBreakPoint(sourceLine2, true);
+    QVERIFY(breakpoint2 >= 0);
+
+    auto verifyBreakpoint = [&](int sourceLine, int breakpointId) {
+        QVERIFY(waitForClientSignal(SIGNAL(stopped())));
+        const QVariantMap body = extractBody();
+        QCOMPARE(body.value("sourceLine").toInt(), sourceLine);
+        QCOMPARE(extractBreakPointId(body), breakpointId);
+    };
+
+    verifyBreakpoint(sourceLine2, breakpoint2);
+
+    m_client->continueDebugging(QJSDebugClient::Continue);
+    verifyBreakpoint(sourceLine2, breakpoint2);
+
+    m_client->changeBreakpoint(breakpoint2, false);
+    QVERIFY(waitForClientSignal(SIGNAL(result())));
+
+    m_client->changeBreakpoint(breakpoint1, true);
+    QVERIFY(waitForClientSignal(SIGNAL(result())));
+
+    m_client->continueDebugging(QJSDebugClient::Continue);
+    verifyBreakpoint(sourceLine1, breakpoint1);
+
+    m_client->continueDebugging(QJSDebugClient::Continue);
+    verifyBreakpoint(sourceLine1, breakpoint1);
+
+    m_client->changeBreakpoint(breakpoint2, true);
+    QVERIFY(waitForClientSignal(SIGNAL(result())));
+
+    m_client->changeBreakpoint(breakpoint1, false);
+    QVERIFY(waitForClientSignal(SIGNAL(result())));
+
+    for (int i = 0; i < 3; ++i) {
+        m_client->continueDebugging(QJSDebugClient::Continue);
+        verifyBreakpoint(sourceLine2, breakpoint2);
+    }
+}
+
 void tst_QQmlDebugJS::setExceptionBreak()
 {
     //void setExceptionBreak(QString type, bool enabled = false);
@@ -1498,6 +1612,17 @@ bool tst_QQmlDebugJS::waitForClientSignal(const char *signal, int timeout)
     return QQmlDebugTest::waitForSignal(m_client.data(), signal, timeout);
 }
 
+void tst_QQmlDebugJS::checkVersionParameters()
+{
+    const QVariantMap value = m_client->parser.call(
+                QJSValueList() << QJSValue(QString(m_client->response))).toVariant().toMap();
+    QCOMPARE(value.value("command").toString(), QString("version"));
+    const QVariantMap body = value.value("body").toMap();
+    QCOMPARE(body.value("UnpausedEvaluate").toBool(), true);
+    QCOMPARE(body.value("ContextEvaluate").toBool(), true);
+    QCOMPARE(body.value("ChangeBreakpoint").toBool(), true);
+}
+
 QTEST_MAIN(tst_QQmlDebugJS)
 
 #include "tst_qqmldebugjs.moc"