From 453d250e5b64078f24d149a81e3f1ca604515e9e Mon Sep 17 00:00:00 2001
From: Florian Bruhin <me@the-compiler.org>
Date: Mon, 15 Mar 2021 17:10:45 +0100
Subject: [PATCH] Revert "Stabilize load signals emitting"

This reverts commit b719da05c6429d72f4e0e0af44da0bf3f3e57984.
---
 src/core/web_contents_delegate_qt.cpp         |  53 ++-----
 src/core/web_contents_delegate_qt.h           |   4 +-
 tests/auto/quick/qmltests/BLACKLIST           |   2 +
 .../auto/quick/qmltests/data/tst_loadUrl.qml  |   2 +-
 .../quick/qmltests/data/tst_viewSource.qml    |  30 ++++
 .../quick/qmltests2/data/tst_viewSoure.qml    | 133 ------------------
 tests/auto/quick/qmltests2/qmltests2.pro      |   1 -
 tests/auto/shared/httpserver.cpp              |  10 +-
 tests/auto/shared/httpserver.h                |   1 -
 .../widgets/loadsignals/tst_loadsignals.cpp   |  40 ------
 .../tst_qwebenginehistory.cpp                 |   3 +-
 11 files changed, 50 insertions(+), 229 deletions(-)
 create mode 100644 tests/auto/quick/qmltests/BLACKLIST
 delete mode 100644 tests/auto/quick/qmltests2/data/tst_viewSoure.qml

diff --git a/src/core/web_contents_delegate_qt.cpp b/src/core/web_contents_delegate_qt.cpp
index e594f648..1f1f7f36 100644
--- a/src/core/web_contents_delegate_qt.cpp
+++ b/src/core/web_contents_delegate_qt.cpp
@@ -105,6 +105,7 @@ WebContentsDelegateQt::WebContentsDelegateQt(content::WebContents *webContents,
     : m_viewClient(adapterClient)
     , m_faviconManager(new FaviconManager(webContents, adapterClient))
     , m_findTextHelper(new FindTextHelper(webContents, adapterClient))
+    , m_lastLoadProgress(-1)
     , m_loadingState(determineLoadingState(webContents))
     , m_didStartLoadingSeen(m_loadingState == LoadingState::Loading)
     , m_frameFocusedObserver(adapterClient)
@@ -259,14 +260,14 @@ void WebContentsDelegateQt::CloseContents(content::WebContents *source)
 
 void WebContentsDelegateQt::LoadProgressChanged(double progress)
 {
-    QUrl current_url(m_viewClient->webContentsAdapter()->getNavigationEntryOriginalUrl(m_viewClient->webContentsAdapter()->currentNavigationEntryIndex()));
-    int p = qMin(qRound(progress * 100), 100);
-
-    if (!m_loadingErrorFrameList.isEmpty() || !m_loadProgressMap.contains(current_url) || m_loadProgressMap[current_url] == 100 || p ==  100)
+    if (!m_loadingErrorFrameList.isEmpty())
+        return;
+    if (m_lastLoadProgress < 0) // suppress signals that aren't between loadStarted and loadFinished
         return;
 
-    if (p > m_loadProgressMap[current_url]) { // ensure strict monotonic increase
-        m_loadProgressMap[current_url] = p;
+    int p = qMin(qRound(progress * 100), 100);
+    if (p > m_lastLoadProgress) { // ensure strict monotonic increase
+        m_lastLoadProgress = p;
         m_viewClient->loadProgressChanged(p);
     }
 }
@@ -341,32 +342,12 @@ void WebContentsDelegateQt::RenderViewHostChanged(content::RenderViewHost *, con
 
 void WebContentsDelegateQt::EmitLoadStarted(const QUrl &url, bool isErrorPage)
 {
+    if (m_lastLoadProgress >= 0 && m_lastLoadProgress < 100) // already running
+        return;
     m_viewClient->loadStarted(url, isErrorPage);
     m_viewClient->updateNavigationActions();
-
-    if ((url.hasFragment() || m_lastLoadedUrl.hasFragment())
-        && url.adjusted(QUrl::RemoveFragment) == m_lastLoadedUrl.adjusted(QUrl::RemoveFragment)
-        && !m_isNavigationCommitted) {
-        m_loadProgressMap.insert(url, 100);
-        m_lastLoadedUrl = url;
-        m_viewClient->loadProgressChanged(100);
-        return;
-    }
-
-    if (!m_loadProgressMap.isEmpty()) {
-        QMap<QUrl, int>::iterator it = m_loadProgressMap.begin();
-        while (it != m_loadProgressMap.end()) {
-            if (it.value() == 100) {
-                it = m_loadProgressMap.erase(it);
-                continue;
-            }
-            ++it;
-        }
-    }
-
-    m_lastLoadedUrl = url;
-    m_loadProgressMap.insert(url, 0);
     m_viewClient->loadProgressChanged(0);
+    m_lastLoadProgress = 0;
 }
 
 void WebContentsDelegateQt::DidStartNavigation(content::NavigationHandle *navigation_handle)
@@ -387,15 +368,11 @@ void WebContentsDelegateQt::EmitLoadFinished(bool success, const QUrl &url, bool
 {
     Q_ASSERT(!isErrorPage || webEngineSettings()->testAttribute(WebEngineSettings::ErrorPageEnabled));
 
-    // When error page enabled we don't need to send the error page load finished signal
-    if (m_loadProgressMap[url] == 100)
+    if (m_lastLoadProgress < 0) // not currently running
         return;
-
-    m_lastLoadedUrl = url;
-    m_loadProgressMap[url] = 100;
-    m_isNavigationCommitted = false;
-    m_viewClient->loadProgressChanged(100);
-
+    if (m_lastLoadProgress < 100)
+        m_viewClient->loadProgressChanged(100);
+    m_lastLoadProgress = -1;
     m_viewClient->loadFinished(success, url, isErrorPage, errorCode, errorDescription);
     m_viewClient->updateNavigationActions();
 }
@@ -420,10 +397,8 @@ void WebContentsDelegateQt::DidFinishNavigation(content::NavigationHandle *navig
                 profileAdapter->visitedLinksManager()->addUrl(url);
         }
 
-        m_isNavigationCommitted = true;
         EmitLoadCommitted();
     }
-
     // Success is reported by DidFinishLoad, but DidFailLoad is now dead code and needs to be handled below
     if (navigation_handle->GetNetErrorCode() == net::OK)
         return;
diff --git a/src/core/web_contents_delegate_qt.h b/src/core/web_contents_delegate_qt.h
index cd6d901e..c088d2cf 100644
--- a/src/core/web_contents_delegate_qt.h
+++ b/src/core/web_contents_delegate_qt.h
@@ -228,6 +228,7 @@ class WebContentsDelegateQt : public content::WebContentsDelegate
     QScopedPointer<FindTextHelper> m_findTextHelper;
     SavePageInfo m_savePageInfo;
     QSharedPointer<FilePickerController> m_filePickerController;
+    int m_lastLoadProgress;
     LoadingState m_loadingState;
     bool m_didStartLoadingSeen;
     FrameFocusedObserver m_frameFocusedObserver;
@@ -239,9 +240,6 @@ class WebContentsDelegateQt : public content::WebContentsDelegate
     int m_desktopStreamCount = 0;
     mutable bool m_pendingUrlUpdate = false;
 
-    QMap<QUrl, int> m_loadProgressMap;
-    QUrl m_lastLoadedUrl;
-    bool m_isNavigationCommitted = false;
     base::WeakPtrFactory<WebContentsDelegateQt> m_weakPtrFactory { this };
 };
 
diff --git a/tests/auto/quick/qmltests/BLACKLIST b/tests/auto/quick/qmltests/BLACKLIST
new file mode 100644
index 00000000..46bc6592
--- /dev/null
+++ b/tests/auto/quick/qmltests/BLACKLIST
@@ -0,0 +1,2 @@
+[WebEngineViewSource::test_viewSourceURL]
+*
diff --git a/tests/auto/quick/qmltests/data/tst_loadUrl.qml b/tests/auto/quick/qmltests/data/tst_loadUrl.qml
index 47dbbc08..872c4664 100644
--- a/tests/auto/quick/qmltests/data/tst_loadUrl.qml
+++ b/tests/auto/quick/qmltests/data/tst_loadUrl.qml
@@ -301,8 +301,8 @@ TestWebEngineView {
             // In-page navigation.
             webEngineView.url = Qt.resolvedUrl("test4.html#content");
             // In-page navigation doesn't trigger load succeeded, wait for load progress instead.
-            tryCompare(loadRequestArray, "length", 3);
             tryCompare(webEngineView, "loadProgress", 100);
+            compare(loadRequestArray.length, 3);
             compare(loadRequestArray[2].status, WebEngineView.LoadStartedStatus);
 
             // Load after in-page navigation.
diff --git a/tests/auto/quick/qmltests/data/tst_viewSource.qml b/tests/auto/quick/qmltests/data/tst_viewSource.qml
index 22c340c2..4966a052 100644
--- a/tests/auto/quick/qmltests/data/tst_viewSource.qml
+++ b/tests/auto/quick/qmltests/data/tst_viewSource.qml
@@ -94,6 +94,36 @@ TestWebEngineView {
             compare(webEngineView.url, "view-source:" + Qt.resolvedUrl("test1.html"));
         }
 
+        function test_viewSourceURL_data() {
+            var testLocalUrl = "view-source:" + Qt.resolvedUrl("test1.html");
+            var testLocalUrlWithoutScheme = "view-source:" + Qt.resolvedUrl("test1.html").substring(7);
+
+            return [
+                   { tag: "view-source:", userInputUrl: "view-source:", loadSucceed: true, url: "view-source:", title: "view-source:" },
+                   { tag: "view-source:about:blank", userInputUrl: "view-source:about:blank", loadSucceed: true, url: "view-source:about:blank", title: "view-source:about:blank" },
+                   { tag: testLocalUrl, userInputUrl: testLocalUrl, loadSucceed: true, url: testLocalUrl, title: "test1.html" },
+                   { tag: testLocalUrlWithoutScheme, userInputUrl: testLocalUrlWithoutScheme, loadSucceed: true, url: testLocalUrl, title: "test1.html" },
+                   { tag: "view-source:http://non.existent", userInputUrl: "view-source:http://non.existent", loadSucceed: false, url: "http://non.existent/", title: "non.existent" },
+                   { tag: "view-source:non.existent", userInputUrl: "view-source:non.existent", loadSucceed: false, url: "http://non.existent/", title: "non.existent" },
+            ];
+        }
+
+        function test_viewSourceURL(row) {
+            WebEngine.settings.errorPageEnabled = true
+            webEngineView.url = row.userInputUrl;
+
+            if (row.loadSucceed) {
+                tryCompare(webEngineView, "loadStatus", WebEngineView.LoadSucceededStatus);
+            } else {
+                tryCompare(webEngineView, "loadStatus", WebEngineView.LoadFailedStatus, 15000);
+            }
+            tryVerify(function() { return titleChangedSpy.count == 1; });
+
+            compare(webEngineView.url, row.url);
+            tryCompare(webEngineView, "title", row.title);
+            verify(!webEngineView.action(WebEngineView.ViewSource).enabled);
+        }
+
         function test_viewSourceCredentials() {
             var url = "http://user:passwd@httpbin.org/basic-auth/user/passwd";
 
diff --git a/tests/auto/quick/qmltests2/data/tst_viewSoure.qml b/tests/auto/quick/qmltests2/data/tst_viewSoure.qml
deleted file mode 100644
index 04b40f54..00000000
--- a/tests/auto/quick/qmltests2/data/tst_viewSoure.qml
+++ /dev/null
@@ -1,133 +0,0 @@
-/****************************************************************************
-**
-** Copyright (C) 2020 The Qt Company Ltd.
-** Contact: https://www.qt.io/licensing/
-**
-** This file is part of the QtWebEngine module of the Qt Toolkit.
-**
-** $QT_BEGIN_LICENSE:GPL-EXCEPT$
-** Commercial License Usage
-** Licensees holding valid commercial Qt licenses may use this file in
-** accordance with the commercial license agreement provided with the
-** Software or, alternatively, in accordance with the terms contained in
-** a written agreement between you and The Qt Company. For licensing terms
-** and conditions see https://www.qt.io/terms-conditions. For further
-** information use the contact form at https://www.qt.io/contact-us.
-**
-** GNU General Public License Usage
-** Alternatively, this file may be used under the terms of the GNU
-** General Public License version 3 as published by the Free Software
-** Foundation with exceptions as appearing in the file LICENSE.GPL3-EXCEPT
-** included in the packaging of this file. Please review the following
-** information to ensure the GNU General Public License requirements will
-** be met: https://www.gnu.org/licenses/gpl-3.0.html.
-**
-** $QT_END_LICENSE$
-**
-****************************************************************************/
-
-import QtQuick 2.0
-import QtTest 1.0
-import QtWebEngine 1.4
-import QtWebEngine.testsupport 1.0
-import "../../qmltests/data" 1.0
-
-TestWebEngineView {
-    id: webEngineView
-    width: 200
-    height: 400
-
-    property var viewRequest: null
-    property var loadRequestArray: []
-
-    testSupport: WebEngineTestSupport {
-        errorPage.onLoadingChanged: {
-            loadRequestArray.push({
-               "status": loadRequest.status,
-            })
-        }
-    }
-
-    onLoadingChanged: {
-        loadRequestArray.push({
-            "status": loadRequest.status,
-        });
-    }
-
-    SignalSpy {
-        id: newViewRequestedSpy
-        target: webEngineView
-        signalName: "newViewRequested"
-    }
-
-    SignalSpy {
-        id: titleChangedSpy
-        target: webEngineView
-        signalName: "titleChanged"
-    }
-
-    onNewViewRequested: {
-        viewRequest = {
-            "destination": request.destination,
-            "userInitiated": request.userInitiated
-        };
-
-        request.openIn(webEngineView);
-    }
-
-    TestCase {
-        id: test
-        name: "WebEngineViewSource"
-
-        function init() {
-            webEngineView.loadStatus = null;
-            webEngineView.url = Qt.resolvedUrl("test1.html");
-            tryCompare(webEngineView, "loadStatus", WebEngineView.LoadSucceededStatus);
-            webEngineView.loadStatus = null;
-
-            newViewRequestedSpy.clear();
-            titleChangedSpy.clear();
-            viewRequest = null;
-        }
-
-        function test_viewSourceURL_data() {
-            var testLocalUrl = "view-source:" + Qt.resolvedUrl("test1.html");
-            var testLocalUrlWithoutScheme = "view-source:" + Qt.resolvedUrl("test1.html").substring(7);
-
-            return [
-                   { tag: "view-source:", userInputUrl: "view-source:", loadSucceed: true, url: "view-source:", title: "view-source:" },
-                   { tag: "view-source:about:blank", userInputUrl: "view-source:about:blank", loadSucceed: true, url: "view-source:about:blank", title: "view-source:about:blank" },
-                   { tag: testLocalUrl, userInputUrl: testLocalUrl, loadSucceed: true, url: testLocalUrl, title: "test1.html" },
-                   { tag: testLocalUrlWithoutScheme, userInputUrl: testLocalUrlWithoutScheme, loadSucceed: true, url: testLocalUrl, title: "test1.html" },
-                   { tag: "view-source:http://non.existent", userInputUrl: "view-source:http://non.existent", loadSucceed: false, url: "http://non.existent/", title: "non.existent" },
-                   { tag: "view-source:non.existent", userInputUrl: "view-source:non.existent", loadSucceed: false, url: "http://non.existent/", title: "non.existent" },
-            ];
-        }
-
-        function test_viewSourceURL(row) {
-            loadRequestArray = [];
-            WebEngine.settings.errorPageEnabled = true
-            webEngineView.url = row.userInputUrl;
-
-            if (row.loadSucceed) {
-                tryVerify(function() { return loadRequestArray.length >= 2 });
-                compare(loadRequestArray[1].status, WebEngineView.LoadSucceededStatus);
-            } else {
-                tryVerify(function() { return loadRequestArray.length >= 2 });
-                compare(loadRequestArray[1].status, WebEngineView.LoadFailedStatus);
-                tryVerify(function() { return loadRequestArray.length == 4 });
-                compare(loadRequestArray[3].status, WebEngineView.LoadSucceededStatus);
-            }
-            tryVerify(function() { return titleChangedSpy.count == 1; });
-
-            compare(webEngineView.url, row.url);
-            tryCompare(webEngineView, "title", row.title);
-            if (row.loadSucceed) {
-                verify(!webEngineView.action(WebEngineView.ViewSource).enabled);
-            } else {
-                verify(webEngineView.action(WebEngineView.ViewSource).enabled);
-            }
-        }
-    }
-}
-
diff --git a/tests/auto/quick/qmltests2/qmltests2.pro b/tests/auto/quick/qmltests2/qmltests2.pro
index 98e33972..4c7a8dc7 100644
--- a/tests/auto/quick/qmltests2/qmltests2.pro
+++ b/tests/auto/quick/qmltests2/qmltests2.pro
@@ -33,7 +33,6 @@ OTHER_FILES += \
     $$PWD/data/tst_linkHovered.qml \
     $$PWD/data/tst_loadFail.qml \
     $$PWD/data/tst_mouseClick.qml \
-    $$PWD/data/tst_viewSource.qml \
     $$PWD/data/icons/favicon.png \
     $$PWD/data/icons/gray128.png \
     $$PWD/data/icons/gray16.png \
diff --git a/tests/auto/shared/httpserver.cpp b/tests/auto/shared/httpserver.cpp
index 69e8cb6c..67f491fa 100644
--- a/tests/auto/shared/httpserver.cpp
+++ b/tests/auto/shared/httpserver.cpp
@@ -54,7 +54,6 @@ bool HttpServer::start()
 {
     m_error = false;
     m_expectingError = false;
-    m_ignoreNewConnection = false;
 
     if (!m_tcpServer->listen()) {
         qCWarning(gHttpServerLog).noquote() << m_tcpServer->errorString();
@@ -85,9 +84,6 @@ QUrl HttpServer::url(const QString &path) const
 
 void HttpServer::handleNewConnection()
 {
-    if (m_ignoreNewConnection)
-        return;
-
     auto rr = new HttpReqRep(m_tcpServer->nextPendingConnection(), this);
     connect(rr, &HttpReqRep::requestReceived, [this, rr]() {
         Q_EMIT newRequest(rr);
@@ -126,9 +122,5 @@ void HttpServer::handleNewConnection()
                                             << error;
         m_error = true;
     });
-
-    if (!m_tcpServer->isListening()) {
-        m_ignoreNewConnection = true;
-        connect(rr, &HttpReqRep::closed, rr, &QObject::deleteLater);
-    }
+    connect(rr, &HttpReqRep::closed, rr, &QObject::deleteLater);
 }
diff --git a/tests/auto/shared/httpserver.h b/tests/auto/shared/httpserver.h
index 952ead22..9764852d 100644
--- a/tests/auto/shared/httpserver.h
+++ b/tests/auto/shared/httpserver.h
@@ -90,7 +90,6 @@ private Q_SLOTS:
     QUrl m_url;
     QStringList m_dirs;
     bool m_error = false;
-    bool m_ignoreNewConnection = false;
     bool m_expectingError = false;
 };
 
diff --git a/tests/auto/widgets/loadsignals/tst_loadsignals.cpp b/tests/auto/widgets/loadsignals/tst_loadsignals.cpp
index b4170587..32bf5393 100644
--- a/tests/auto/widgets/loadsignals/tst_loadsignals.cpp
+++ b/tests/auto/widgets/loadsignals/tst_loadsignals.cpp
@@ -52,7 +52,6 @@ private Q_SLOTS:
     void secondLoadForError_WhenErrorPageEnabled();
     void loadAfterInPageNavigation_qtbug66869();
     void fileDownloadDoesNotTriggerLoadSignals_qtbug66661();
-    void numberOfStartedAndFinishedSignalsIsSame();
 
 private:
     QWebEngineProfile profile;
@@ -244,44 +243,5 @@ void tst_LoadSignals::fileDownloadDoesNotTriggerLoadSignals_qtbug66661()
     QCOMPARE(loadFinishedSpy.size(), 1);
 }
 
-void tst_LoadSignals::numberOfStartedAndFinishedSignalsIsSame() {
-
-    HttpServer server;
-    server.setResourceDirs({ TESTS_SOURCE_DIR "/qwebengineprofile/resources" });
-    connect(&server, &HttpServer::newRequest, [] (HttpReqRep *) {
-         QTest::qWait(250); // just add delay to trigger some progress for every sub resource
-    });
-    QVERIFY(server.start());
-
-    view.load(server.url("/hedgehog.png"));
-    QTRY_COMPARE(loadFinishedSpy.size(), 1);
-    QVERIFY(loadFinishedSpy[0][0].toBool());
-
-    loadStartedSpy.clear();
-    loadFinishedSpy.clear();
-    loadProgressSpy.clear();
-
-    view.page()->setHtml("<html><body>"
-                         "<img src=\"" + server.url("/hedgehog.png").toEncoded() + "\">"
-                         "<form method='GET' name='hiddenform' action='qrc:///resources/page1.html' />"
-                         "<script language='javascript'>document.forms[0].submit();</script>"
-                         "</body></html>");
-
-    QTRY_COMPARE(loadStartedSpy.size(), 2);
-    QTRY_COMPARE(loadFinishedSpy.size(), 2);
-
-    QTRY_VERIFY(!loadFinishedSpy[0][0].toBool());
-    QTRY_VERIFY(loadFinishedSpy[1][0].toBool());
-
-    view.page()->setHtml("<html><body>"
-                         "<form method='GET' name='hiddenform' action='qrc:///resources/page1.html' />"
-                         "<script language='javascript'>document.forms[0].submit();</script>"
-                         "</body></html>");
-    QTRY_COMPARE(loadStartedSpy.size(), 4);
-    QTRY_COMPARE(loadFinishedSpy.size(), 4);
-    QVERIFY(loadFinishedSpy[2][0].toBool());
-    QVERIFY(loadFinishedSpy[3][0].toBool());
-}
-
 QTEST_MAIN(tst_LoadSignals)
 #include "tst_loadsignals.moc"
diff --git a/tests/auto/widgets/qwebenginehistory/tst_qwebenginehistory.cpp b/tests/auto/widgets/qwebenginehistory/tst_qwebenginehistory.cpp
index 72a45379..bdb48679 100644
--- a/tests/auto/widgets/qwebenginehistory/tst_qwebenginehistory.cpp
+++ b/tests/auto/widgets/qwebenginehistory/tst_qwebenginehistory.cpp
@@ -320,8 +320,7 @@ void tst_QWebEngineHistory::serialize_2()
     hist->forward();
     QTRY_COMPARE(loadFinishedSpy->count(), 5);
     hist->forward();
-    // In-page navigation, the last url was the page5.html
-    QTRY_COMPARE(loadFinishedSpy->count(), 5);
+    QTRY_COMPARE(loadFinishedSpy->count(), 6);
     QTRY_COMPARE(hist->currentItemIndex(), initialCurrentIndex);
 }
 
-- 
2.30.2

