Bug 191464 – [WPE] Add Qt extension

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Philippe Normand]

This new extension is a QML plugin embedding a WPE ViewBackend implementation. It provides a public API very similar to Qt's WebView module. It comes with a simple mini-browser implemented in QML.

[Philippe Normand]

Created attachment 354328 [details]
Patch

[Philippe Normand]

Konstantin, can you please have a look?

[EWS Watchlist]

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.

[Konstantin Tokarev]

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.

[Philippe Normand]

(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 :)

[Philippe Normand]

Created attachment 356506 [details]
Patch

[EWS Watchlist]

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.

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

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.

[Philippe Normand]

Created attachment 356602 [details]
Patch

[Philippe Normand]

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

[Michael Catanzaro]

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/.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Philippe Normand]

Created attachment 356723 [details]
Patch

[Philippe Normand]

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

Ping ;)

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

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?

[Michael Catanzaro]

(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!

[Philippe Normand]

Created attachment 358693 [details]
Patch

[EWS Watchlist]

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.

[Carlos Garcia Campos]

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

[Philippe Normand]

Created attachment 359152 [details]
Patch

[EWS Watchlist]

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.

[Carlos Garcia Campos]

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.

[Philippe Normand]

(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.

[Philippe Normand]

Created attachment 359380 [details]
Patch

[EWS Watchlist]

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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

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

[Philippe Normand]

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