[webkit-reviews] review granted: [Bug 191464] [WPE] Add Qt extension : [Attachment 359380] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 191464: [WPE] Add Qt extension
https://bugs.webkit.org/show_bug.cgi?id=191464

Attachment 359380: Patch

https://bugs.webkit.org/attachment.cgi?id=359380&action=review




--- Comment #32 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 359380
  --> https://bugs.webkit.org/attachment.cgi?id=359380
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<JavascriptCallbackDat
a*>(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