[webkit-reviews] review denied: [Bug 191464] [WPE] Add Qt extension : [Attachment 358693] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied 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 358693: Patch

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




--- Comment #24 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 358693
  --> https://bugs.webkit.org/attachment.cgi?id=358693
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