Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unload event issue in QtWebkit #519

Open
neel5481 opened this issue Apr 21, 2017 · 28 comments
Open

unload event issue in QtWebkit #519

neel5481 opened this issue Apr 21, 2017 · 28 comments
Labels

Comments

@neel5481
Copy link

Hi,

As we are using this webkit in Qt application.

We have used QTabWidget. Once we get the javascript call "window.open" then we are getting Qt signal "createWindow" and we are able to create the new tab which contains custom close button with WebView and WebPage.

Now Everything works fine till now except when we close the tab then "unload" function should be called. Even though we have registered unload event but we are not getting call for "unload" function.

//below register the unload event for window.
window.onbeforeunload = function(ev) {
// unload event action....
};

Can you please suggest how to call unload event that through WebKit ?

Thanks in Advance.

@annulen
Copy link
Member

annulen commented Apr 21, 2017

Here is one of possible implementations: https://0x0.st/xIz.txt
It adds closeEvent handlers to QWebView and QGraphicsWebView, that invoke beforeunload handler and mark QCloseEvent as ignored in case close is cancelled.

It still does not work transparently when QWebView is not a top-level widget, because Qt events are not propagated from parent to children. You'll have to handle close event yourself and check return value of close method

I'll change the patch to use QWebPage::RequestClose action instead of close event for consistency with QtWebEngine

@annulen
Copy link
Member

annulen commented Apr 21, 2017

BTW, there is an existing issue: https://bugreports.qt.io/browse/QTBUG-36155

@annulen
Copy link
Member

annulen commented Apr 21, 2017

New version of patch. Usage: trigger QWebPage::RequestClose, signal windowCloseRequested will be emitted when user confirms close

diff --git a/Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp b/Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp
index fb85535fcbe..9b6d18ecdfd 100644
--- a/Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp
+++ b/Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp
@@ -490,6 +490,11 @@ void QWebPageAdapter::adjustPointForClicking(QMouseEvent* ev)
 #endif
 }
 
+bool QWebPageAdapter::tryClosePage()
+{
+    return mainFrameAdapter().frame->loader().shouldClose();
+}
+
 void QWebPageAdapter::mouseMoveEvent(QMouseEvent* ev)
 {
     WebCore::Frame* frame = mainFrameAdapter().frame;
diff --git a/Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.h b/Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.h
index 48bc0afe03b..33e9bcad803 100644
--- a/Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.h
+++ b/Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.h
@@ -315,6 +315,7 @@ public:
 
     void adjustPointForClicking(QMouseEvent*);
 
+    bool tryClosePage();
     void mouseMoveEvent(QMouseEvent*);
     void mousePressEvent(QMouseEvent*);
     void mouseDoubleClickEvent(QMouseEvent*);
diff --git a/Source/WebKit/qt/WidgetApi/qwebpage.cpp b/Source/WebKit/qt/WidgetApi/qwebpage.cpp
index c2bc712e80d..dea15773ca0 100644
--- a/Source/WebKit/qt/WidgetApi/qwebpage.cpp
+++ b/Source/WebKit/qt/WidgetApi/qwebpage.cpp
@@ -1208,6 +1208,7 @@ QWebInspector* QWebPagePrivate::getOrCreateInspector()
     \value ToggleMediaPlayPause Toggles the play/pause state of the hovered audio or video element. (Added in Qt 5.2)
     \value ToggleMediaMute Mutes or unmutes the hovered audio or video element. (Added in Qt 5.2)
     \value ToggleVideoFullscreen Switches the hovered video element into or out of fullscreen mode. (Added in Qt 5.2)
+    \value RequestClose Request to close the web page. If defined, the window.onbeforeunload handler is run, and the user can confirm or reject to close the page. If the close request is confirmed, windowCloseRequested is emitted. (Added in ?)
 
     \omitvalue WebActionCount
 
@@ -1835,6 +1836,11 @@ void QWebPage::triggerAction(WebAction action, bool)
             it.next()->d->cancelLoad();
         break;
     }
+    case RequestClose: {
+        if (d->tryClosePage())
+            emit windowCloseRequested();
+        break;
+    }
     default:
         command = QWebPagePrivate::editorCommandForWebActions(action);
         break;
diff --git a/Source/WebKit/qt/WidgetApi/qwebpage.h b/Source/WebKit/qt/WidgetApi/qwebpage.h
index 50849494a39..c67830c6c69 100644
--- a/Source/WebKit/qt/WidgetApi/qwebpage.h
+++ b/Source/WebKit/qt/WidgetApi/qwebpage.h
@@ -193,6 +193,8 @@ public:
         ToggleMediaMute,
         ToggleVideoFullscreen,
 
+        RequestClose,
+
         WebActionCount
     };
 
diff --git a/Tools/QtTestBrowser/launcherwindow.cpp b/Tools/QtTestBrowser/launcherwindow.cpp
index 1321760f824..ddabb7ad96b 100644
--- a/Tools/QtTestBrowser/launcherwindow.cpp
+++ b/Tools/QtTestBrowser/launcherwindow.cpp
@@ -244,7 +244,7 @@ void LauncherWindow::createChrome()
     fileMenu->addAction("New Window", this, SLOT(newWindow()), QKeySequence::New);
     fileMenu->addAction(tr("Open File..."), this, SLOT(openFile()), QKeySequence::Open);
     fileMenu->addAction(tr("Open Location..."), this, SLOT(openLocation()), QKeySequence(Qt::CTRL | Qt::Key_L));
-    fileMenu->addAction("Close Window", this, SLOT(close()), QKeySequence::Close);
+    fileMenu->addAction("Close Window", this, SLOT(requestCloseWindow()), QKeySequence::Close);
     fileMenu->addSeparator();
     fileMenu->addAction("Take Screen Shot...", this, SLOT(screenshot()));
 #if !defined(QT_NO_PRINTER) && HAVE(QTPRINTSUPPORT)
@@ -1168,6 +1168,11 @@ void LauncherWindow::clearMemoryCaches()
     qDebug() << "Memory caches were cleared";
 }
 
+void LauncherWindow::requestCloseWindow()
+{
+    page()->triggerAction(QWebPage::RequestClose);
+}
+
 void LauncherWindow::updateFPS(int fps)
 {
     QString fpsStatusText = QString("Current FPS: %1").arg(fps);
diff --git a/Tools/QtTestBrowser/launcherwindow.h b/Tools/QtTestBrowser/launcherwindow.h
index c9388da8592..26e489e6b9b 100644
--- a/Tools/QtTestBrowser/launcherwindow.h
+++ b/Tools/QtTestBrowser/launcherwindow.h
@@ -176,6 +176,7 @@ protected Q_SLOTS:
 #endif
 
     void clearMemoryCaches();
+    void requestCloseWindow();
 
 public Q_SLOTS:
     LauncherWindow* newWindow();

@neel5481
Copy link
Author

So Do you integrate the fix in this webkit ? If yes, when it will be available. What else should we do to make it working ?
Let me know how can i help you.

@annulen
Copy link
Member

annulen commented Apr 21, 2017

You can apply patch above and start using it right now. If you are asking for binary builds - there will be new release pretty soon. Usage instruction is given in previous message, see also documentation line in the patch and LauncherWindow changes as an example

@neel5481
Copy link
Author

neel5481 commented Apr 21, 2017

Thank you for your quick reply.
Yes, i am looking for binary builds with Qt 5.8. It will be available as same path as "https://github.com/annulen/webkit/releases" right ?.

Ok. I will make necessary changes and compile it with new binary build once it will be available and let you know the result.

Conclusion:
On tab close event I have to trigger action of "RequestClose" and at the time of web page creation i also need to listen on "windowCloseRequested". So once i get this signal it should call unload function.

Let me know if i misunderstood your suggestion.

@annulen
Copy link
Member

annulen commented Apr 21, 2017

On tab close event I have to trigger action of "RequestClose"

Yes

and at the time of web page creation i also need to listen on "windowCloseRequested"

Yes. BTW, you need to listen to the same signal so that windows created by JS could be closd automatically.

So once i get this signal it should call unload function.

Yes

@neel5481
Copy link
Author

Thank you for your reply.
BTW binary build will be available "https://github.com/annulen/webkit/releases". Is that correct ? If no let me provide the link so that i can use that binary with application.

Is that binary build available in 2-3 days ?

@annulen
Copy link
Member

annulen commented Apr 21, 2017

Yes binaries will be available in https://github.com/annulen/webkit/releases, maybe with mirror on http://download.qt.io/ this time. No, it will take about a week probably

@neel5481
Copy link
Author

Ok. Thank you.
I will take the build once available, compile & test the application with latest build and update this mail thread.

@annulen annulen added the API label Apr 22, 2017
@annulen
Copy link
Member

annulen commented Apr 26, 2017

What platforms are you interested in?

@neel5481
Copy link
Author

neel5481 commented Apr 28, 2017

As we are using all 3 platforms Mac OS X, Linux and Windows but for testing, you can share Linux Build. Is that patch committed ? In which branch it will be committed ?

@annulen
Copy link
Member

annulen commented Apr 28, 2017

What if I provide you macOS build instead?

@neel5481
Copy link
Author

It is ok for me.

Can you please suggest how to build webkit from source ? I have tried compiling in linux but i am getting below error.

############
[ 20%] Linking CXX executable ../../../bin/testb3
cd /home/neel/Projects/webkit/release/Source/JavaScriptCore/shell && /usr/bin/cmake -E cmake_link_script CMakeFiles/testb3.dir/link.txt --verbose=1
/usr/bin/g++ -fvisibility=hidden -fvisibility-inlines-hidden -fno-exceptions -fno-strict-aliasing -fno-rtti -std=c++11 -O3 -DNDEBUG -ffunction-sections -fdata-sections -fuse-ld=gold -Wl,--disable-new-dtags CMakeFiles/testb3.dir/__/b3/testb3.cpp.o CMakeFiles/testb3.dir/testb3_automoc.cpp.o -o ../../../bin/testb3 -rdynamic -ldl ../../../lib/libJavaScriptCore.a ../../../lib/libWTF.a ../../../lib/libbmalloc.a -ldl /opt/Qt5.8.0/5.8/gcc_64/lib/libicuuc.so -lpthread -lgobject-2.0 -lglib-2.0 /opt/Qt5.8.0/5.8/gcc_64/lib/libicui18n.so /opt/Qt5.8.0/5.8/gcc_64/lib/libQt5Core.so.5.8.0 -Wl,-rpath,/opt/Qt5.8.0/5.8/gcc_64/lib
../../../lib/libJavaScriptCore.a(../../../lib/../Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/parser/Lexer.cpp.o):Lexer.cpp:function JSC::isNonLatin1IdentStart(unsigned short): error: undefined reference to 'u_charType_55'
../../../lib/libJavaScriptCore.a(../../../lib/../Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/parser/Lexer.cpp.o):Lexer.cpp:function JSC::isNonLatin1IdentPart(int): error: undefined reference to 'u_charType_55'
../../../lib/libJavaScriptCore.a(../../../lib/../Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/parser/Lexer.cpp.o):Lexer.cpp:function JSC::Lexer::nextTokenIsColon(): error: undefined reference to 'u_charType_55'
../../../lib/libJavaScriptCore.a(../../../lib/../Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/parser/Lexer.cpp.o):Lexer.cpp:function JSC::Lexer::skipWhitespace(): error: undefined reference to 'u_charType_55'
##############

Any idea how to solve.

@annulen
Copy link
Member

annulen commented Apr 28, 2017

Remove all libicu* symlinks in /opt/Qt5.8.0/5.8/gcc_64/lib that don't have numeric suffix, e.g. libicui18n.so and not libicui18n.so.1

then rebuild

@neel5481
Copy link
Author

Thanks for the reply. I change as per your suggestion and share that compilation result. It took some time to build.
Meanwhile can you please share Mac build ?

@annulen
Copy link
Member

annulen commented Apr 28, 2017

Yep, I'll share it when it's ready

@neel5481
Copy link
Author

Ok. Thanks.

@neel5481
Copy link
Author

Hi,

I am able to build the webkit from source code in Linux. Thanks for your input as it resolve the issue.
Now new folder "release" has created. Now you are giving the releases for all platforms as per below link.
https://github.com/annulen/webkit/releases

Which files we should copy from "release" and source folder so that it should be same folder structure as above link as you have created.

@annulen
Copy link
Member

annulen commented Apr 28, 2017

You should run make install, it will install all necessary files into Qt

@annulen
Copy link
Member

annulen commented Apr 29, 2017

Please download Mac version from http://86.110.182.234:8000/qtwebkit-darwin-58.zip

@neel5481
Copy link
Author

neel5481 commented May 2, 2017

Not able to access above URL to download Mac version of webkit.

@annulen
Copy link
Member

annulen commented May 2, 2017

Please use https://0x0.st/353.zip

@neel5481
Copy link
Author

neel5481 commented May 8, 2017

Thank you for your support. Tested with Linux and it is working now.

@neel5481
Copy link
Author

neel5481 commented May 9, 2017

One more question. How to make source code compatible with older version of webkit.
Here, you have provided patch file and added "RequestClose" action but this action was not present in earlier version of webkit. How to detect in application that new "RequestClose" action is present or not ? If user use older version of webkit then this action was not present and it gives compilation error.

Kindly Suggest.

@annulen
Copy link
Member

annulen commented May 9, 2017

You can just use numeric value of enum element

@neel5481
Copy link
Author

neel5481 commented May 9, 2017

I have tried. The total numeric value for WebAction enum is 78. But how it will be useful ?
It gives compilation error. If we put below condition in application then also it gives error.

if (QWebPage::WebActionCount == 78)
page->triggerAction(QWebPage::RequestClose);

Let me know if i misunderstood.

@annulen
Copy link
Member

annulen commented May 9, 2017

I meant

page->triggerAction(static_cast<QWebPage::WebAction>(ToggleVideoFullscreen + 1))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants