From 13416b2db167dc1849e4af94391d3c6ae602ec76 Mon Sep 17 00:00:00 2001 From: Jocelyn Turcotte Date: Wed, 8 Jan 2014 14:37:40 +0100 Subject: Fix QWebEngineView to page reattachment. This fixes the crash in tst_QWebEngineView::reusePage. Also add a test to check the case where show() would react incorrectly. Change-Id: I40247c7c225d74b26675b6a7fa5ff1f06d3bb3e6 Reviewed-by: Pierre Rossi --- src/core/web_contents_adapter.cpp | 7 +++++++ src/core/web_contents_adapter.h | 1 + src/webenginewidgets/api/qwebenginepage.cpp | 2 ++ src/webenginewidgets/api/qwebengineview.cpp | 4 ++++ .../render_widget_host_view_qt_delegate_widget.cpp | 10 +++++++--- .../widgets/qwebenginepage/tst_qwebenginepage.cpp | 23 ++++++++++++++++++++++ 6 files changed, 44 insertions(+), 3 deletions(-) diff --git a/src/core/web_contents_adapter.cpp b/src/core/web_contents_adapter.cpp index bb2ac36f4..ce9dccd90 100644 --- a/src/core/web_contents_adapter.cpp +++ b/src/core/web_contents_adapter.cpp @@ -218,6 +218,13 @@ void WebContentsAdapter::initialize(WebContentsAdapterClient *adapterClient) contentsView->initialize(adapterClient); } +void WebContentsAdapter::reattachRWHV() +{ + Q_D(WebContentsAdapter); + if (content::RenderWidgetHostView *rwhv = d->webContents->GetRenderWidgetHostView()) + rwhv->InitAsChild(0); +} + bool WebContentsAdapter::canGoBack() const { Q_D(const WebContentsAdapter); diff --git a/src/core/web_contents_adapter.h b/src/core/web_contents_adapter.h index 5379afb70..1f00b4b12 100644 --- a/src/core/web_contents_adapter.h +++ b/src/core/web_contents_adapter.h @@ -66,6 +66,7 @@ public: WebContentsAdapter(WebContentsAdapterClient::RenderingMode renderingMode, content::WebContents *webContents = 0); ~WebContentsAdapter(); void initialize(WebContentsAdapterClient *adapterClient); + void reattachRWHV(); bool canGoBack() const; bool canGoForward() const; diff --git a/src/webenginewidgets/api/qwebenginepage.cpp b/src/webenginewidgets/api/qwebenginepage.cpp index 0178c1118..74a1692b2 100644 --- a/src/webenginewidgets/api/qwebenginepage.cpp +++ b/src/webenginewidgets/api/qwebenginepage.cpp @@ -207,6 +207,8 @@ QWebEnginePage::QWebEnginePage(QObject* parent) QWebEnginePage::~QWebEnginePage() { + Q_D(QWebEnginePage); + QWebEngineViewPrivate::bind(d->view, 0); } QWebEngineHistory *QWebEnginePage::history() const diff --git a/src/webenginewidgets/api/qwebengineview.cpp b/src/webenginewidgets/api/qwebengineview.cpp index 4dea036b1..054925e83 100644 --- a/src/webenginewidgets/api/qwebengineview.cpp +++ b/src/webenginewidgets/api/qwebengineview.cpp @@ -43,6 +43,7 @@ #include "qwebengineview_p.h" #include "qwebenginepage_p.h" +#include "web_contents_adapter.h" #include #include @@ -63,6 +64,7 @@ void QWebEngineViewPrivate::bind(QWebEngineView *view, QWebEnginePage *page) oldView->d_func()->page = 0; } page->d_func()->view = view; + page->d_func()->adapter->reattachRWHV(); } if (view) { @@ -98,6 +100,8 @@ QWebEngineView::QWebEngineView(QWidget *parent) QWebEngineView::~QWebEngineView() { + Q_D(QWebEngineView); + QWebEngineViewPrivate::bind(0, d->page); } QWebEnginePage* QWebEngineView::page() const diff --git a/src/webenginewidgets/render_widget_host_view_qt_delegate_widget.cpp b/src/webenginewidgets/render_widget_host_view_qt_delegate_widget.cpp index 0d84a0f46..c7b831853 100644 --- a/src/webenginewidgets/render_widget_host_view_qt_delegate_widget.cpp +++ b/src/webenginewidgets/render_widget_host_view_qt_delegate_widget.cpp @@ -64,9 +64,11 @@ RenderWidgetHostViewQtDelegateWidget::RenderWidgetHostViewQtDelegateWidget(Rende void RenderWidgetHostViewQtDelegateWidget::initAsChild(WebContentsAdapterClient* container) { QWebEnginePagePrivate *pagePrivate = static_cast(container); - // FIXME: What is going to trigger this if the page is attached later to the view? - if (pagePrivate->view) + if (pagePrivate->view) { pagePrivate->view->layout()->addWidget(this); + QWidget::show(); + } else + setParent(0); } void RenderWidgetHostViewQtDelegateWidget::initAsPopup(const QRect& rect) @@ -94,7 +96,9 @@ bool RenderWidgetHostViewQtDelegateWidget::hasKeyboardFocus() void RenderWidgetHostViewQtDelegateWidget::show() { - QWidget::show(); + // Check if we're attached to a QWebEngineView, we don't want to show as top-level. + if (parent()) + QWidget::show(); } void RenderWidgetHostViewQtDelegateWidget::hide() diff --git a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp index 6ca8e0d8e..a670903fe 100644 --- a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp +++ b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp @@ -152,6 +152,7 @@ private Q_SLOTS: void userAgentApplicationName(); void userAgentNewlineStripping(); void undoActionHaveCustomText(); + void renderWidgetHostViewNotShowTopLevel(); void viewModes(); @@ -3534,6 +3535,28 @@ void tst_QWebEnginePage::undoActionHaveCustomText() #endif } +void tst_QWebEnginePage::renderWidgetHostViewNotShowTopLevel() +{ + QWebEnginePage page; + QSignalSpy spyLoadFinished(&page, SIGNAL(loadFinished(bool))); + + page.load(QUrl("http://qt-project.org")); + if (!spyLoadFinished.wait(10000) || !spyLoadFinished.at(0).at(0).toBool()) + QSKIP("Couldn't load page from network, skipping test."); + spyLoadFinished.clear(); + + // Loading a different domain will force the creation of a separate render + // process and should therefore create a new RenderWidgetHostViewQtDelegateWidget. + page.load(QUrl("http://www.wikipedia.org/")); + if (!spyLoadFinished.wait(10000) || !spyLoadFinished.at(0).at(0).toBool()) + QSKIP("Couldn't load page from network, skipping test."); + + // Make sure that RenderWidgetHostViewQtDelegateWidgets are not shown as top-level. + // They should only be made visible when parented to a QWebEngineView. + foreach (QWidget *widget, QApplication::topLevelWidgets()) + QCOMPARE(widget->isVisible(), false); +} + void tst_QWebEnginePage::openWindowDefaultSize() { #if !defined(QWEBENGINEPAGE_EVALUATEJAVASCRIPT) -- cgit v1.2.3