Bug 191464

Created attachment 354328 [details] Patch

Konstantin, can you please have a look?

Attachment 354328 [details] did not pass style-queue: ERROR: Source/ThirdParty/WPE/QtWPE/QtViewBackend.h:46: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/ThirdParty/WPE/QtWPE/QtViewBackend.h:48: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/ThirdParty/WPE/QtWPE/wpeview.cpp:43: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/ThirdParty/WPE/QtWPE/qtwpe_plugin.cpp:43: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/ThirdParty/WPE/QtWPE/wpeviewloadrequest.cpp:43: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/ThirdParty/WPE/QtWPE/QtViewBackend.cpp:43: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 6 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.

I think it would be better to build it with cmake instead of qmake to avoid fragmentation of build system, or it should be independent project outside of the tree.

(In reply to Konstantin Tokarev from comment #4) > I think it would be better to build it with cmake instead of qmake to avoid > fragmentation of build system, or it should be independent project outside > of the tree. Yeah I can probably integrate this in the CMake build, but I would also like to keep the qmake stuff because 1) it's very small and not a burden 2) it can be useful for people who already have WebKit provided by distros and thus don't want to rebuild it from scratch :)

Created attachment 356506 [details] Patch

Attachment 356506 [details] did not pass style-queue: ERROR: Source/ThirdParty/WPE/QtWPE/QtViewBackend.h:46: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/ThirdParty/WPE/QtWPE/QtViewBackend.h:48: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/ThirdParty/WPE/QtWPE/wpeview.cpp:43: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/ThirdParty/WPE/QtWPE/qtwpe_plugin.cpp:43: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/ThirdParty/WPE/QtWPE/CMakeLists.txt:28: Alphabetical sorting problem. "Qt5::Core Qt5::Quick" should be before "WebKit". [list/order] [5] ERROR: Source/ThirdParty/WPE/QtWPE/wpeviewloadrequest.cpp:43: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/ThirdParty/WPE/QtWPE/QtViewBackend.cpp:43: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 7 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.

Again, if this is a third-party project with a separate source code repository, then I think it's very important to not add this to ThirdParty. It should just be a dependency. Where is the upstream repository? Minor: the source code files should have either a BSD license or an LGPL license, but not both for each file. If we own the copyright and it's not based on older code, we can pick whichever.

I don't understand this either. If this is a new port (even if it's based on public wpe API) it's should be in Source/ not in ThirdParty. If it's really third party, it's expected to have an upstream repo we are importing to WebKit, but it doesn't make sense in this case either, because nothing in WebKit is using the third-party project.

Created attachment 356602 [details] Patch

The code is now in Source/QtWPE and under LGPL2.

Much better. This might fit even better under Source/WebKit/UIProcess/API/QtWPE/, that way we don't have to add a new top-level directory under Source/.

Comment on attachment 356602 [details] Patch Attachment 356602 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/10278470 New failing tests: http/tests/misc/resource-timing-resolution.html

Created attachment 356609 [details] Archive of layout-test-results from ews205 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Comment on attachment 356602 [details] Patch Attachment 356602 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10278732 New failing tests: http/tests/misc/resource-timing-resolution.html

Created attachment 356610 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6

Created attachment 356723 [details] Patch

(In reply to Philippe Normand from comment #2) > Konstantin, can you please have a look? Ping ;)

Comment on attachment 356723 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356723&action=review Nice.... > Source/WebKit/ChangeLog:19 > + For commodity purposes a Flatpak build manifest is available but > + nothing prevents distributors to ship this extension as a > + standalone module because it uses a regular QMake build-system > + along with WebKit's CMake build-system. Is there any value in doing this still, now that it's shipped as part of WebKit? > Source/WebKit/UIProcess/API/QtWPE/CMakeLists.txt:63 > +else () > + message(WARNING "Qt5 development libraries not found. QtWPE will not be built") > +endif () This isn't good because it results in the feature silently getting disabled or enabled depending on what devel libraries you have installed on your host system, which is almost never desired. What I would do is: add a real build option in OptionsWPE.cmake, ENABLE_QT_WPE or ENABLE_QT_API, something like that. Probably ON by default. If it's ON and Qt libraries are missing, then fail the build to distributors know to install it and try again to avoid distributing WPE without Qt. And if it's ON by default, then probably there's no longer any reason to have the separate qmake stuff, right? It would just always be built as part of WebKit. Not sure why there's reason to allow different ways to build it? > Source/WebKit/UIProcess/API/QtWPE/COPYING.LGPL2:2 > + GNU LIBRARY GENERAL PUBLIC LICENSE > + Version 2, June 1991 Since it's part of WebKit now, you no longer need the separate license file. > Tools/wpe/qt-wpe-mini-browser/COPYING.LGPL2:2 > + GNU LIBRARY GENERAL PUBLIC LICENSE > + Version 2, June 1991 Again, we know the license for everything in WebKit is BSD or LGPL, no need for separate COPYING files.

Comment on attachment 356723 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356723&action=review > Source/WebKit/ChangeLog:22 > + * UIProcess/API/QtWPE/CMakeLists.txt: Added. hmm, this isn't actually a new port, but a qt API on top of wpe glib api. I think I would add the code in UIProcess/API/wpe/qt instead. What do you think?

(In reply to Carlos Garcia Campos from comment #20) > hmm, this isn't actually a new port, but a qt API on top of wpe glib api. I > think I would add the code in UIProcess/API/wpe/qt instead. What do you > think? That's even better!

Created attachment 358693 [details] Patch

Attachment 358693 [details] did not pass style-queue: ERROR: Tools/glib/api_test_runner.py:169: missing whitespace after ',' [pep8/E231] [5] ERROR: Tools/glib/api_test_runner.py:177: [TestRunner._run_test_qt] Module 'subprocess' has no 'TimeoutExpired' member (but some types could not be inferred) [pylint/E1103] [5] ERROR: Tools/TestWebKitAPI/Tests/WPEQt/TestLoadHtml.cpp:40: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/TestWebKitAPI/Tests/WPEQt/WPEQtTest.cpp:38: Found header this file implements after a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/TestWebKitAPI/Tests/WPEQt/TestLoad.cpp:61: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/TestWebKitAPI/Tests/WPEQt/TestRunJavaScript.cpp:49: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/TestWebKitAPI/Tests/WPEQt/WPEQtTest.h:25: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/TestWebKitAPI/Tests/WPEQt/TestLoadRequest.cpp:66: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebKit/UIProcess/API/wpe/qt/WPEQtViewBackend.h:24: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit/UIProcess/API/wpe/qt/WPEQtViewBackend.h:26: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 10 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 358693 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358693&action=review > Source/WebKit/PlatformWPE.cmake:365 > +set(qtwpe_SOURCES > + ${WEBKIT_DIR}/UIProcess/API/wpe/qt/WPEQtViewBackend.cpp > + ${WEBKIT_DIR}/UIProcess/API/wpe/qt/WPEQmlExtensionPlugin.cpp > + ${WEBKIT_DIR}/UIProcess/API/wpe/qt/WPEQtView.cpp > + ${WEBKIT_DIR}/UIProcess/API/wpe/qt/WPEQtViewLoadRequest.cpp > +) Why is this out of if (ENABLE_WPE_QT_API) ? > Source/WebKit/UIProcess/API/wpe/qt/WPEQmlExtensionPlugin.cpp:26 > + Remove this empty line. > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:26 > + Remove this empty line > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:62 > + if (m_webView) > + g_object_unref(m_webView); Could we use GRefPtr? > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:90 > + auto* settings = webkit_settings_new_with_settings("enable-developer-extras", TRUE, GRefPtr > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:137 > + loadStatus = WPEQtView::LoadStatus::LoadSucceededStatus; > + statusSet = true; Note that finished doesn't imply success. it's called also when load fails. > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:175 > + QSGSimpleTextureNode* n = static_cast<QSGSimpleTextureNode*>(node); auto* textureNode > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:193 > + return uri ? QUrl(QString(uri)) : m_url; Is QString expecting utf8 too? > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:231 > + gdouble estimatedProgress; > + g_object_get(m_webView, "estimated-load-progress", &estimatedProgress, nullptr); > + return estimatedProgress * 100; It's simpler and more efficient to use webkit_web_view_get_estimated_load_progress(). > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:245 > + return webkit_web_view_get_title(m_webView); Ditto. > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:292 > + gboolean isLoading = false; > + g_object_get(m_webView, "is-loading", &isLoading, nullptr); > + return isLoading; It's simpler and more efficient to use webkit_web_view_is_loading() > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:382 > + WebKitJavascriptResult* jsResult; Move this below where jsResult is set > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:383 > + GError* error = nullptr; GUniqueOutPtr > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:392 > + WPEQtView* view = reinterpret_cast<WPEQtView*>(userData); This will get the wrong pointer if the view is destroyed before the async operation completes. > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:393 > + view->handleJsResult(jsResult); It's a bit weird that the ownership is transferred, I would unref here and you don't need to unref in every early return in handleJsResult. > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:411 > + gchar* str_value = jsc_value_to_string(value); GUniquePtr str_value -> strValue > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:413 > + exception = jsc_context_get_exception(context); It's probably easier to use jsc_context_push_exception_handler() and not throw the exception in the handler. > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:439 > + m_jsCallback = callback; This is not correct, you can't call runJavaScript twice, the second one will override the callback, so both scripts will be invoking the second one on completion. > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:440 > + webkit_web_view_run_javascript(m_webView, script.toUtf8().constData(), nullptr, jsAsyncReadyCallback, this); You should save the callback in a struct (together with this) and pass it to the jsAsyncReadyCallback as user data. I don't know if WPEQtView is refcounted, but we should either keep a ref until the async operation finishes, or we should use a cancellable to cancell all pending async operation when the view is destroyed. > Source/WebKit/UIProcess/API/wpe/qt/WPEQtViewBackend.cpp:39 > + RELEASE_ASSERT(configData->backend); This could be non-null but still invalid if the WPEQtViewBackend is destroyed before the callback is dispatched by the main loop. You should take a reference, use a weak pointer or cancel the idle in the backend destructor. > Source/WebKit/UIProcess/API/wpe/qt/WPEQtViewBackend.cpp:46 > +WPEQtViewBackend::WPEQtViewBackend(WPEQtViewBackendClient* client) This should receive a reference instead of a pointer > Source/WebKit/UIProcess/API/wpe/qt/WPEQtViewBackend.cpp:48 > + , m_client(client) And m_client should also be a reference > Source/WebKit/UIProcess/API/wpe/qt/WPEQtViewBackend.cpp:60 > + m_exportable = nullptr; No need to set to nullptr in the destructor > Source/WebKit/UIProcess/API/wpe/qt/WPEQtViewBackend.cpp:65 > + m_eglContext = nullptr; Ditto. > Source/WebKit/UIProcess/API/wpe/qt/WPEQtViewBackend.cpp:71 > + EGLConfigurationData* configData = g_new0(EGLConfigurationData, 1); fastMalloc? or std::make_unique? > Source/WebKit/UIProcess/API/wpe/qt/WPEQtViewBackend.cpp:75 > + g_idle_add_full(G_PRIORITY_DEFAULT_IDLE, configureCallback, configData, g_free); RunLoop::main().dispatch() ? > Source/WebKit/UIProcess/API/wpe/qt/WPEQtViewBackend.cpp:105 > + QOpenGLFunctions* f = context->functions(); Use at least funcs instead of just f > Source/WebKit/UIProcess/API/wpe/qt/WPEQtViewBackend.cpp:155 > + EGLConfig* configs = g_new0(EGLConfig, count); GUniquePtr > Source/WebKit/UIProcess/API/wpe/qt/WPEQtViewBackend.cpp:206 > +GLuint WPEQtViewBackend::getTexture(QOpenGLContext* context) getTexture -> texture

Created attachment 359152 [details] Patch

Attachment 359152 [details] did not pass style-queue: ERROR: Tools/glib/api_test_runner.py:178: [TestRunner._run_test_qt] Module 'subprocess' has no 'TimeoutExpired' member (but some types could not be inferred) [pylint/E1103] [5] ERROR: Tools/TestWebKitAPI/Tests/WPEQt/TestLoadHtml.cpp:40: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/TestWebKitAPI/Tests/WPEQt/WPEQtTest.cpp:38: Found header this file implements after a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/TestWebKitAPI/Tests/WPEQt/TestLoad.cpp:61: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/TestWebKitAPI/Tests/WPEQt/TestRunJavaScript.cpp:49: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/TestWebKitAPI/Tests/WPEQt/WPEQtTest.h:25: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/TestWebKitAPI/Tests/WPEQt/TestLoadRequest.cpp:66: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebKit/UIProcess/API/wpe/qt/WPEQtViewBackend.h:24: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit/UIProcess/API/wpe/qt/WPEQtViewBackend.h:26: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.h:23: Header file should not contain WebCore config.h. Should be: alphabetically sorted. [build/include_order] [4] Total errors found: 10 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 359152 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359152&action=review > Source/WebKit/PlatformWPE.cmake:384 > + list(APPEND WPE_API_INSTALLED_HEADERS > + ${WEBKIT_DIR}/UIProcess/API/wpe/qt/WPEQtView.h > + ${WEBKIT_DIR}/UIProcess/API/wpe/qt/WPEQtViewLoadRequest.h > + ) I thought we didn't have to install headers. > Source/WebKit/PlatformWPE.cmake:407 > + Extra line here. > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:54 > + // Make sure WebKit2 is initialized because the ViewBackend needs to > + // initialize from the main loop. > + WebKit::InitializeWebKit2(); Maybe it would be better to init wk2 in registerTypes, that I'm assuming it's called earlier and only once? > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:93 > + "backend", webkit_web_view_backend_new(wpeBackend, [](gpointer) { }, this), Wow, I didn't consider this case when we add webkit_web_view_backend_new(). > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:136 > + case WEBKIT_LOAD_FINISHED: > + loadStatus = WPEQtView::LoadStatus::LoadSucceededStatus; > + statusSet = true; Note that finished doesn't imply success. it's called also when load fails. You should check if notifyLoadFailedCallback was called and not set statusSetr to true in that case, I guess. > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:379 > + virtual ~JavascriptCallbackData() = default; Do you really need this? This is allocated with fastMalloc and the constructor/destructor aren't called at all. > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:408 > + // TODO: Handle more value types? Use FIXME instead of TODO. > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:413 > + JSCException* exception = jsc_context_get_exception(context); > + if (exception) { It's probably easier to use jsc_context_push_exception_handler() and not throw the exception in the handler. > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:439 > + auto* data = static_cast<JavascriptCallbackData*>(fastMalloc(sizeof(JavascriptCallbackData))); You can use std::make_unique here, using release() when passing data to webkit_web_view_run_javascript() and then adopt the pointer in the callback with std::unique_ptr<>. That way you don't need all the fastFree in the callback.

(In reply to Carlos Garcia Campos from comment #27) > Comment on attachment 359152 [details] > Patch > > I thought we didn't have to install headers. > Now we do! > > > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:93 > > + "backend", webkit_web_view_backend_new(wpeBackend, [](gpointer) { }, this), > > Wow, I didn't consider this case when we add webkit_web_view_backend_new(). > Is this a good Wow or a bad Wow? > > > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:413 > > + JSCException* exception = jsc_context_get_exception(context); > > + if (exception) { > > It's probably easier to use jsc_context_push_exception_handler() and not > throw the exception in the handler. > How simpler? I've just reused the code example from the run_javascript() documentation FWIW.

Created attachment 359380 [details] Patch

Attachment 359380 [details] did not pass style-queue: ERROR: Tools/glib/api_test_runner.py:178: [TestRunner._run_test_qt] Module 'subprocess' has no 'TimeoutExpired' member (but some types could not be inferred) [pylint/E1103] [5] ERROR: Tools/TestWebKitAPI/Tests/WPEQt/TestLoadHtml.cpp:40: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/TestWebKitAPI/Tests/WPEQt/WPEQtTest.cpp:38: Found header this file implements after a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/TestWebKitAPI/Tests/WPEQt/TestLoad.cpp:61: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/TestWebKitAPI/Tests/WPEQt/TestRunJavaScript.cpp:49: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/TestWebKitAPI/Tests/WPEQt/WPEQtTest.h:25: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/TestWebKitAPI/Tests/WPEQt/TestLoadRequest.cpp:66: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebKit/UIProcess/API/wpe/qt/WPEQtViewBackend.h:24: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit/UIProcess/API/wpe/qt/WPEQtViewBackend.h:26: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.h:23: Header file should not contain WebCore config.h. Should be: alphabetically sorted. [build/include_order] [4] Total errors found: 10 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to Philippe Normand from comment #28) > (In reply to Carlos Garcia Campos from comment #27) > > Comment on attachment 359152 [details] > > Patch > > > > > I thought we didn't have to install headers. > > > > Now we do! How are they used? We are not changing the pkg-config file to add the new include path. > > > > > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:93 > > > + "backend", webkit_web_view_backend_new(wpeBackend, [](gpointer) { }, this), > > > > Wow, I didn't consider this case when we add webkit_web_view_backend_new(). > > > > Is this a good Wow or a bad Wow? Bad because I didn't consider this case, and good because it's possible to workaround it. > > > > > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:413 > > > + JSCException* exception = jsc_context_get_exception(context); > > > + if (exception) { > > > > It's probably easier to use jsc_context_push_exception_handler() and not > > throw the exception in the handler. > > > > How simpler? I've just reused the code example from the run_javascript() > documentation FWIW. One way or another depends on how you handle the exception. If you are just going to show a warning message it's probably easier to push an exception handler.

Comment on attachment 359380 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359380&action=review > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:106 > + case WEBKIT_LOAD_STARTED: > + loadStatus = WPEQtView::LoadStatus::LoadStartedStatus; > + statusSet = true; You should set error occurred to false here. > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:111 > + break; Or even better, since error occurred is only used for this, I would set it to false here to make sure it won't affect future loads. > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:149 > + "backend", webkit_web_view_backend_new(m_backend->backend(), [](gpointer) { }, this), Now we can actually use the destroy notify as it was designed for and pass the WPEQtViewBackend to attach it to the life of WebKitWebView. > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:156 > + g_signal_connect_swapped(m_webView.get(), "notify::uri", G_CALLBACK(notifyUrlChangedCallback), this); > + g_signal_connect_swapped(m_webView.get(), "notify::title", G_CALLBACK(notifyTitleChangedCallback), this); > + g_signal_connect_swapped(m_webView.get(), "notify::estimated-load-progress", G_CALLBACK(notifyLoadProgressCallback), this); > + g_signal_connect(m_webView.get(), "load-changed", G_CALLBACK(notifyLoadChangedCallback), this); > + g_signal_connect(m_webView.get(), "load-failed", G_CALLBACK(notifyLoadFailedCallback), this); I didn't notice it before, but you should disconnect all these signals in the destructor. Note that the web view can be alive after "this" is destroyed, due to an async operation (run_js for example). > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:171 > + if (!m_backend) { > + QMetaObject::invokeMethod(this, "initializeBackend", Qt::QueuedConnection, Q_ARG(QPointer<QOpenGLContext>, window()->openglContext())); > + return node; > + } So, if backend initialization fails we enter an infinite loop here. Maybe we should just assert if egl can't be initialized? > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:386 > + JavascriptCallbackData(JavascriptCallbackData* other) > + : callback(other->callback) > + , object(other->object) { } You shouldn't need this. > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:395 > + std::unique_ptr<JavascriptCallbackData> data = std::make_unique<JavascriptCallbackData>(reinterpret_cast<JavascriptCallbackData*>(userData)); Don't use make_unique here, just adopt the pointer. std::unique_ptr<JavascriptCallbackData> data(reinterpret_cast<JavascriptCallbackData*>(userData)); > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:422 > + variant.setValue(QString(g_strdup(strValue.get()))); We set the value depending on the exception, so it's indeed easier this way, sorry for the noise. > Source/WebKit/UIProcess/API/wpe/qt/WPEQtViewBackend.cpp:82 > +#if defined(WPE_BACKEND_CHECK_VERSION) && WPE_BACKEND_CHECK_VERSION(0, 2, 0) > + wpe_loader_init("libWPEBackend-fdo-0.1.so"); > +#endif Shouldn't we do that before? At least before calling wpe_fdo_initialize_for_egl_display, no? > Source/WebKit/UIProcess/API/wpe/qt/WPEQtViewBackend.cpp:221 > + m_lockedImage = image; We might be leaking the image if previous frame was not handled. If that should never happen, then add an assert here to ensure m_lockedImage == EGL_NO_IMAGE_KHR

Committed r240141: <https://trac.webkit.org/changeset/240141>