From 3257ab9ce01b01b667237fd6ad33512739db5b01 Mon Sep 17 00:00:00 2001
From: Kirill Burtsev <kirill.burtsev@qt.io>
Date: Wed, 22 Aug 2018 09:53:21 +0200
Subject: [PATCH] Fix QWebEngineView changed properties emit on setPage
QWebEngineView now also emits url, title, icon and selection
changes on setPage in the save way when url changes. Before,
those updates were only forwarded from current page.
Fixes: QTBUG-69300
Change-Id: If827205094423bc00064a123ddb143b6002d2e7c
Reviewed-by: Kai Koehne <kai.koehne@qt.io>
---
src/webenginewidgets/api/qwebenginepage.cpp | 2 +-
src/webenginewidgets/api/qwebengineview.cpp | 76 +++++++++++----
src/webenginewidgets/api/qwebengineview_p.h | 5 +-
.../qwebengineview/tst_qwebengineview.cpp | 96 +++++++++++++++++++
4 files changed, 161 insertions(+), 18 deletions(-)
diff --git a/src/webenginewidgets/api/qwebenginepage.cpp b/src/webenginewidgets/api/qwebenginepage.cpp
index 187565a76..fbb7c1f9e 100644
--- a/src/webenginewidgets/api/qwebenginepage.cpp
+++ b/src/webenginewidgets/api/qwebenginepage.cpp
@@ -902,7 +902,7 @@ QWebEnginePage::~QWebEnginePage()
Q_D(QWebEnginePage);
setDevToolsPage(nullptr);
d->adapter->stopFinding();
- QWebEngineViewPrivate::bind(nullptr, this, true);
+ QWebEngineViewPrivate::removePageFromView(this);
}
QWebEngineHistory *QWebEnginePage::history() const
diff --git a/src/webenginewidgets/api/qwebengineview.cpp b/src/webenginewidgets/api/qwebengineview.cpp
index aa51e5b0e..e6f9fcb49 100644
--- a/src/webenginewidgets/api/qwebengineview.cpp
+++ b/src/webenginewidgets/api/qwebengineview.cpp
@@ -55,33 +55,77 @@
QT_BEGIN_NAMESPACE
-void QWebEngineViewPrivate::bind(QWebEngineView *view, QWebEnginePage *page, bool pageBeingDeleted)
+void QWebEngineViewPrivate::notify(QWebEngineView *view, QWebEnginePage *oldPage, QWebEnginePage *newPage)
+{
+ Q_ASSERT(view);
+
+ auto oldUrl = oldPage ? oldPage->url() : QUrl();
+ auto newUrl = newPage ? newPage->url() : QUrl();
+ if (oldUrl != newUrl)
+ Q_EMIT view->urlChanged(newUrl);
+
+ auto oldTitle = oldPage ? oldPage->title() : QString();
+ auto newTitle = newPage ? newPage->title() : QString();
+ if (oldTitle != newTitle)
+ Q_EMIT view->titleChanged(newTitle);
+
+ auto oldIcon = oldPage ? oldPage->iconUrl() : QUrl();
+ auto newIcon = newPage ? newPage->iconUrl() : QUrl();
+ if (oldIcon != newIcon) {
+ Q_EMIT view->iconUrlChanged(newIcon);
+ Q_EMIT view->iconChanged(newPage ? newPage->icon() : QIcon());
+ }
+
+ if ((oldPage && oldPage->hasSelection()) || (newPage && newPage->hasSelection()))
+ Q_EMIT view->selectionChanged();
+}
+
+QWebEnginePage* QWebEngineViewPrivate::removeViewFromPage(QWebEngineView *view)
+{
+ Q_ASSERT(view);
+ QWebEnginePage *oldPage = view->d_func()->page;
+
+ if (oldPage) {
+ oldPage->disconnect(view);
+ oldPage->d_func()->view = nullptr;
+ if (oldPage->parent() != view)
+ oldPage->d_func()->adapter->reattachRWHV();
+ }
+ return oldPage;
+}
+
+void QWebEngineViewPrivate::removePageFromView(QWebEnginePage *page)
+{
+ Q_ASSERT(page);
+ if (QWebEngineView *oldView = page->d_func()->view) {
+ page->disconnect(oldView);
+ page->d_func()->view = nullptr;
+ oldView->d_func()->page = nullptr;
+ notify(oldView, page, nullptr);
+ }
+}
+
+void QWebEngineViewPrivate::bind(QWebEngineView *view, QWebEnginePage *page)
{
if (view && page == view->d_func()->page)
return;
if (page) {
// Un-bind page from its current view.
- if (QWebEngineView *oldView = page->d_func()->view) {
- page->disconnect(oldView);
- oldView->d_func()->page = nullptr;
- }
+ removePageFromView(page);
page->d_func()->view = view;
- if (!pageBeingDeleted)
- page->d_func()->adapter->reattachRWHV();
+ page->d_func()->adapter->reattachRWHV();
}
if (view) {
// Un-bind view from its current page.
- if (QWebEnginePage *oldPage = view->d_func()->page) {
- oldPage->disconnect(view);
- oldPage->d_func()->view = nullptr;
- if (oldPage->parent() == view)
- delete oldPage;
- else
- oldPage->d_func()->adapter->reattachRWHV();
- }
+ QWebEnginePage *oldPage = removeViewFromPage(view);
+
view->d_func()->page = page;
+ notify(view, oldPage, page);
+
+ if (oldPage && oldPage->parent() == view)
+ delete oldPage;
}
if (view && page) {
@@ -149,7 +193,7 @@ QWebEngineView::QWebEngineView(QWidget *parent)
QWebEngineView::~QWebEngineView()
{
- QWebEngineViewPrivate::bind(this, nullptr);
+ QWebEngineViewPrivate::removeViewFromPage(this);
}
QWebEnginePage* QWebEngineView::page() const
diff --git a/src/webenginewidgets/api/qwebengineview_p.h b/src/webenginewidgets/api/qwebengineview_p.h
index bfb44bec5..1845bfb60 100644
--- a/src/webenginewidgets/api/qwebengineview_p.h
+++ b/src/webenginewidgets/api/qwebengineview_p.h
@@ -65,7 +65,10 @@ public:
Q_DECLARE_PUBLIC(QWebEngineView)
QWebEngineView *q_ptr;
- static void bind(QWebEngineView *view, QWebEnginePage *page, bool pageBeingDeleted = false);
+ static void notify(QWebEngineView *view, QWebEnginePage *oldPage, QWebEnginePage *newPage);
+ static QWebEnginePage* removeViewFromPage(QWebEngineView *view);
+ static void removePageFromView(QWebEnginePage *page);
+ static void bind(QWebEngineView *view, QWebEnginePage *page);
QWebEngineViewPrivate();
diff --git a/tests/auto/widgets/qwebengineview/tst_qwebengineview.cpp b/tests/auto/widgets/qwebengineview/tst_qwebengineview.cpp
index 57f5f6def..78677e459 100644
--- a/tests/auto/widgets/qwebengineview/tst_qwebengineview.cpp
+++ b/tests/auto/widgets/qwebengineview/tst_qwebengineview.cpp
@@ -134,6 +134,8 @@ private Q_SLOTS:
void renderHints();
void getWebKitVersion();
+ void changePage_data();
+ void changePage();
void reusePage_data();
void reusePage();
void microFocusCoordinates();
@@ -260,6 +262,100 @@ void tst_QWebEngineView::getWebKitVersion()
#endif
}
+void tst_QWebEngineView::changePage_data()
+{
+ QString html = "<html><head><title>%1</title>"
+ "<link rel='icon' href='file://" TESTS_SOURCE_DIR "/resources/image2.png'></head></html>";
+ QUrl urlFrom("data:text/html," + html.arg("TitleFrom"));
+ QUrl urlTo("data:text/html," + html.arg("TitleTo"));
+ QUrl nullPage("data:text/html,<html/>");
+ QTest::addColumn<QUrl>("urlFrom");
+ QTest::addColumn<QUrl>("urlTo");
+ QTest::addColumn<bool>("fromIsNullPage");
+ QTest::addColumn<bool>("toIsNullPage");
+ QTest::newRow("From empty page to url") << nullPage << urlTo << true << false;
+ QTest::newRow("From url to empty content page") << urlFrom << nullPage << false << true;
+ QTest::newRow("From one content to another") << urlFrom << urlTo << false << false;
+}
+
+void tst_QWebEngineView::changePage()
+{
+ QScopedPointer<QWebEngineView> view(new QWebEngineView); view->resize(640, 480); view->show();
+
+ QFETCH(QUrl, urlFrom);
+ QFETCH(QUrl, urlTo);
+ QFETCH(bool, fromIsNullPage);
+ QFETCH(bool, toIsNullPage);
+
+ QSignalSpy spyUrl(view.get(), &QWebEngineView::urlChanged);
+ QSignalSpy spyTitle(view.get(), &QWebEngineView::titleChanged);
+ QSignalSpy spyIconUrl(view.get(), &QWebEngineView::iconUrlChanged);
+ QSignalSpy spyIcon(view.get(), &QWebEngineView::iconChanged);
+
+ QScopedPointer<QWebEnginePage> pageFrom(new QWebEnginePage);
+ QSignalSpy pageFromLoadSpy(pageFrom.get(), &QWebEnginePage::loadFinished);
+ QSignalSpy pageFromIconLoadSpy(pageFrom.get(), &QWebEnginePage::iconChanged);
+ pageFrom->load(urlFrom);
+ QTRY_COMPARE(pageFromLoadSpy.count(), 1);
+ QCOMPARE(pageFromLoadSpy.last().value(0).toBool(), true);
+ if (!fromIsNullPage) {
+ QTRY_COMPARE(pageFromIconLoadSpy.count(), 1);
+ QVERIFY(!pageFromIconLoadSpy.last().value(0).isNull());
+ }
+
+ view->setPage(pageFrom.get());
+
+ QTRY_COMPARE(spyUrl.count(), 1);
+ QCOMPARE(spyUrl.last().value(0).toUrl(), pageFrom->url());
+ QTRY_COMPARE(spyTitle.count(), 1);
+ QCOMPARE(spyTitle.last().value(0).toString(), pageFrom->title());
+
+ QTRY_COMPARE(spyIconUrl.count(), fromIsNullPage ? 0 : 1);
+ QTRY_COMPARE(spyIcon.count(), fromIsNullPage ? 0 : 1);
+ if (!fromIsNullPage) {
+ QVERIFY(!pageFrom->iconUrl().isEmpty());
+ QCOMPARE(spyIconUrl.last().value(0).toUrl(), pageFrom->iconUrl());
+ QCOMPARE(spyIcon.last().value(0), QVariant::fromValue(pageFrom->icon()));
+ }
+
+ QScopedPointer<QWebEnginePage> pageTo(new QWebEnginePage);
+ QSignalSpy pageToLoadSpy(pageTo.get(), &QWebEnginePage::loadFinished);
+ QSignalSpy pageToIconLoadSpy(pageTo.get(), &QWebEnginePage::iconChanged);
+ pageTo->load(urlTo);
+ QTRY_COMPARE(pageToLoadSpy.count(), 1);
+ QCOMPARE(pageToLoadSpy.last().value(0).toBool(), true);
+ if (!toIsNullPage) {
+ QTRY_COMPARE(pageToIconLoadSpy.count(), 1);
+ QVERIFY(!pageToIconLoadSpy.last().value(0).isNull());
+ }
+
+ view->setPage(pageTo.get());
+
+ QTRY_COMPARE(spyUrl.count(), 2);
+ QCOMPARE(spyUrl.last().value(0).toUrl(), pageTo->url());
+ QTRY_COMPARE(spyTitle.count(), 2);
+ QCOMPARE(spyTitle.last().value(0).toString(), pageTo->title());
+
+ bool iconIsSame = fromIsNullPage == toIsNullPage;
+ int iconChangeNotifyCount = fromIsNullPage ? (iconIsSame ? 0 : 1) : (iconIsSame ? 1 : 2);
+
+ QTRY_COMPARE(spyIconUrl.count(), iconChangeNotifyCount);
+ QTRY_COMPARE(spyIcon.count(), iconChangeNotifyCount);
+ QCOMPARE(pageFrom->iconUrl() == pageTo->iconUrl(), iconIsSame);
+ if (!iconIsSame) {
+ QCOMPARE(spyIconUrl.last().value(0).toUrl(), pageTo->iconUrl());
+ QCOMPARE(spyIcon.last().value(0), QVariant::fromValue(pageTo->icon()));
+ }
+
+ // verify no emits on destroy with the same number of signals in spy
+ view.reset();
+ qApp->processEvents();
+ QTRY_COMPARE(spyUrl.count(), 2);
+ QTRY_COMPARE(spyTitle.count(), 2);
+ QTRY_COMPARE(spyIconUrl.count(), iconChangeNotifyCount);
+ QTRY_COMPARE(spyIcon.count(), iconChangeNotifyCount);
+}
+
void tst_QWebEngineView::reusePage_data()
{
QTest::addColumn<QString>("html");
--
GitLab