Restore behavior of OpenURLFromTab if createWindow returns this

Instead of using QSharedPointer's reference count to communicate
adoption/non-adoption, change adoptNewWindow to return a adapter pointer, with
null meaning non-adoption. Then change QWebEnginePage's implementation to reuse
already existing adapters if possible, restoring previous behavior of
OpenURLFromTab when createWindow returns this.

Task-number: QTBUG-80596
Change-Id: I8ee7c31e4294aabd3207c504cba67d6171c66cb0
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

8 files changed, 42 insertions, 16 deletions

diff --git a/src/core/web_contents_adapter_client.h b/src/core/web_contents_adapter_client.h
index 250801f51..04df99f0e 100644
--- a/src/core/web_contents_adapter_client.h
+++ b/src/core/web_contents_adapter_client.h
@@ -469,7 +469,10 @@ public:
     virtual void loadFinished(bool success, const QUrl &url, bool isErrorPage = false, int errorCode = 0, const QString &errorDescription = QString()) = 0;
     virtual void focusContainer() = 0;
     virtual void unhandledKeyEvent(QKeyEvent *event) = 0;
-    virtual void adoptNewWindow(QSharedPointer<WebContentsAdapter> newWebContents, WindowOpenDisposition disposition, bool userGesture, const QRect & initialGeometry, const QUrl &targetUrl) = 0;
+    virtual QSharedPointer<WebContentsAdapter>
+    adoptNewWindow(QSharedPointer<WebContentsAdapter> newWebContents,
+                   WindowOpenDisposition disposition, bool userGesture,
+                   const QRect &initialGeometry, const QUrl &targetUrl) = 0;
     virtual bool isBeingAdopted() = 0;
     virtual void close() = 0;
     virtual void windowCloseRejected() = 0;
diff --git a/src/core/web_contents_delegate_qt.cpp b/src/core/web_contents_delegate_qt.cpp
index 3c37286f8..216e4faf1 100644
--- a/src/core/web_contents_delegate_qt.cpp
+++ b/src/core/web_contents_delegate_qt.cpp
@@ -663,14 +663,17 @@ void WebContentsDelegateQt::overrideWebPreferences(content::WebContents *webCont
     m_viewClient->webEngineSettings()->overrideWebPreferences(webContents, webPreferences);
 }
 
-QWeakPointer<WebContentsAdapter> WebContentsDelegateQt::createWindow(std::unique_ptr<content::WebContents> new_contents, WindowOpenDisposition disposition, const gfx::Rect& initial_pos, bool user_gesture)
+QSharedPointer<WebContentsAdapter>
+WebContentsDelegateQt::createWindow(std::unique_ptr<content::WebContents> new_contents,
+                                    WindowOpenDisposition disposition, const gfx::Rect &initial_pos,
+                                    bool user_gesture)
 {
     QSharedPointer<WebContentsAdapter> newAdapter = QSharedPointer<WebContentsAdapter>::create(std::move(new_contents));
 
-    m_viewClient->adoptNewWindow(newAdapter, static_cast<WebContentsAdapterClient::WindowOpenDisposition>(disposition), user_gesture, toQt(initial_pos), m_initialTargetUrl);
-
-    // If the client didn't reference the adapter, it will be deleted now, and the weak pointer zeroed.
-    return newAdapter;
+    return m_viewClient->adoptNewWindow(
+            std::move(newAdapter),
+            static_cast<WebContentsAdapterClient::WindowOpenDisposition>(disposition), user_gesture,
+            toQt(initial_pos), m_initialTargetUrl);
 }
 
 void WebContentsDelegateQt::allowCertificateError(const QSharedPointer<CertificateErrorController> &errorController)
diff --git a/src/core/web_contents_delegate_qt.h b/src/core/web_contents_delegate_qt.h
index a8c9561ea..bfef9a1df 100644
--- a/src/core/web_contents_delegate_qt.h
+++ b/src/core/web_contents_delegate_qt.h
@@ -201,7 +201,10 @@ public:
     base::WeakPtr<WebContentsDelegateQt> AsWeakPtr() { return m_weakPtrFactory.GetWeakPtr(); }
 
 private:
-    QWeakPointer<WebContentsAdapter> createWindow(std::unique_ptr<content::WebContents> new_contents, WindowOpenDisposition disposition, const gfx::Rect& initial_pos, bool user_gesture);
+    QSharedPointer<WebContentsAdapter>
+    createWindow(std::unique_ptr<content::WebContents> new_contents,
+                 WindowOpenDisposition disposition, const gfx::Rect &initial_pos,
+                 bool user_gesture);
     void EmitLoadStarted(const QUrl &url, bool isErrorPage = false);
     void EmitLoadFinished(bool success, const QUrl &url, bool isErrorPage = false, int errorCode = 0, const QString &errorDescription = QString());
     void EmitLoadCommitted();
diff --git a/src/webengine/api/qquickwebengineview.cpp b/src/webengine/api/qquickwebengineview.cpp
index 7052afe42..4096eb7f6 100644
--- a/src/webengine/api/qquickwebengineview.cpp
+++ b/src/webengine/api/qquickwebengineview.cpp
@@ -547,12 +547,13 @@ void QQuickWebEngineViewPrivate::unhandledKeyEvent(QKeyEvent *event)
         QCoreApplication::sendEvent(q->parentItem(), event);
 }
 
-void QQuickWebEngineViewPrivate::adoptNewWindow(QSharedPointer<WebContentsAdapter> newWebContents, WindowOpenDisposition disposition, bool userGesture, const QRect &, const QUrl &targetUrl)
+QSharedPointer<WebContentsAdapter>
+QQuickWebEngineViewPrivate::adoptNewWindow(QSharedPointer<WebContentsAdapter> newWebContents,
+                                           WindowOpenDisposition disposition, bool userGesture,
+                                           const QRect &, const QUrl &targetUrl)
 {
     Q_Q(QQuickWebEngineView);
     QQuickWebEngineNewViewRequest request;
-    // This increases the ref-count of newWebContents and will tell Chromium
-    // to start loading it and possibly return it to its parent page window.open().
     request.m_adapter = newWebContents;
     request.m_isUserInitiated = userGesture;
     request.m_requestedUrl = targetUrl;
@@ -575,6 +576,8 @@ void QQuickWebEngineViewPrivate::adoptNewWindow(QSharedPointer<WebContentsAdapte
     }
 
     Q_EMIT q->newViewRequested(&request);
+
+    return newWebContents;
 }
 
 bool QQuickWebEngineViewPrivate::isBeingAdopted()
diff --git a/src/webengine/api/qquickwebengineview_p_p.h b/src/webengine/api/qquickwebengineview_p_p.h
index 5c884e36e..e696f6a0c 100644
--- a/src/webengine/api/qquickwebengineview_p_p.h
+++ b/src/webengine/api/qquickwebengineview_p_p.h
@@ -121,7 +121,10 @@ public:
     void loadFinished(bool success, const QUrl &url, bool isErrorPage = false, int errorCode = 0, const QString &errorDescription = QString()) override;
     void focusContainer() override;
     void unhandledKeyEvent(QKeyEvent *event) override;
-    void adoptNewWindow(QSharedPointer<QtWebEngineCore::WebContentsAdapter> newWebContents, WindowOpenDisposition disposition, bool userGesture, const QRect &, const QUrl &targetUrl) override;
+    QSharedPointer<QtWebEngineCore::WebContentsAdapter>
+    adoptNewWindow(QSharedPointer<QtWebEngineCore::WebContentsAdapter> newWebContents,
+                   WindowOpenDisposition disposition, bool userGesture, const QRect &,
+                   const QUrl &targetUrl) override;
     bool isBeingAdopted() override;
     void close() override;
     void windowCloseRejected() override;
diff --git a/src/webenginewidgets/api/qwebenginepage.cpp b/src/webenginewidgets/api/qwebenginepage.cpp
index 6f0009183..8e6f402ff 100644
--- a/src/webenginewidgets/api/qwebenginepage.cpp
+++ b/src/webenginewidgets/api/qwebenginepage.cpp
@@ -348,7 +348,10 @@ void QWebEnginePagePrivate::unhandledKeyEvent(QKeyEvent *event)
         QGuiApplication::sendEvent(view->parentWidget(), event);
 }
 
-void QWebEnginePagePrivate::adoptNewWindow(QSharedPointer<WebContentsAdapter> newWebContents, WindowOpenDisposition disposition, bool userGesture, const QRect &initialGeometry, const QUrl &targetUrl)
+QSharedPointer<WebContentsAdapter>
+QWebEnginePagePrivate::adoptNewWindow(QSharedPointer<WebContentsAdapter> newWebContents,
+                                      WindowOpenDisposition disposition, bool userGesture,
+                                      const QRect &initialGeometry, const QUrl &targetUrl)
 {
     Q_Q(QWebEnginePage);
     Q_UNUSED(userGesture);
@@ -356,7 +359,10 @@ void QWebEnginePagePrivate::adoptNewWindow(QSharedPointer<WebContentsAdapter> ne
 
     QWebEnginePage *newPage = q->createWindow(toWindowType(disposition));
     if (!newPage)
-        return;
+        return nullptr;
+
+    if (!newWebContents->webContents())
+        return newPage->d_func()->adapter; // Reuse existing adapter
 
     // Mark the new page as being in the process of being adopted, so that a second mouse move event
     // sent by newWebContents->initialize() gets filtered in RenderWidgetHostViewQt::forwardEvent.
@@ -375,6 +381,8 @@ void QWebEnginePagePrivate::adoptNewWindow(QSharedPointer<WebContentsAdapter> ne
 
     if (!initialGeometry.isEmpty())
         emit newPage->geometryChangeRequested(initialGeometry);
+
+    return newWebContents;
 }
 
 bool QWebEnginePagePrivate::isBeingAdopted()
diff --git a/src/webenginewidgets/api/qwebenginepage_p.h b/src/webenginewidgets/api/qwebenginepage_p.h
index ae845c530..b4424ec4b 100644
--- a/src/webenginewidgets/api/qwebenginepage_p.h
+++ b/src/webenginewidgets/api/qwebenginepage_p.h
@@ -112,7 +112,10 @@ public:
     void loadFinished(bool success, const QUrl &url, bool isErrorPage = false, int errorCode = 0, const QString &errorDescription = QString()) override;
     void focusContainer() override;
     void unhandledKeyEvent(QKeyEvent *event) override;
-    void adoptNewWindow(QSharedPointer<QtWebEngineCore::WebContentsAdapter> newWebContents, WindowOpenDisposition disposition, bool userGesture, const QRect &initialGeometry, const QUrl &targetUrl) override;
+    QSharedPointer<QtWebEngineCore::WebContentsAdapter>
+    adoptNewWindow(QSharedPointer<QtWebEngineCore::WebContentsAdapter> newWebContents,
+                   WindowOpenDisposition disposition, bool userGesture,
+                   const QRect &initialGeometry, const QUrl &targetUrl) override;
     bool isBeingAdopted() override;
     void close() override;
     void windowCloseRejected() override;
diff --git a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp
index a638b7183..d2d8ade91 100644
--- a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp
+++ b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp
@@ -3520,8 +3520,8 @@ void tst_QWebEnginePage::openLinkInNewPage()
         QTRY_COMPARE(page1.spy.count(), 1);
         QVERIFY(page1.spy.takeFirst().value(0).toBool());
         QCOMPARE(page2.spy.count(), 0);
-        if (decision == Decision::ReturnSelf)
-            // History was discarded
+        if (decision == Decision::ReturnSelf && cause == Cause::TargetBlank)
+            // History was discarded due to AddNewContents
             QCOMPARE(page1.history()->count(), 1);
         else
             QCOMPARE(page1.history()->count(), 2);