[webkit-reviews] review denied: [Bug 75543] [GTK] Add webkit_web_view_run_javascript() to WebKit2 GTK+ : [Attachment 126740] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 75543: [GTK] Add webkit_web_view_run_javascript() to WebKit2 GTK+
https://bugs.webkit.org/show_bug.cgi?id=75543

Attachment 126740: Patch
https://bugs.webkit.org/attachment.cgi?id=126740&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=126740&action=review


Looks pretty good, but it's an error to access JavaScript values with a
different global context.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1399
> +    JSGlobalContextRef scriptContext = JSGlobalContextCreate(0);
> +    JSValueRef scriptValue =
WKSerializedScriptValueDeserialize(wkSerializedScriptValue, scriptContext, 0);
> +    g_simple_async_result_set_op_res_gpointer(result.get(),
const_cast<OpaqueJSValue*>(scriptValue), 0);
> +    g_simple_async_result_complete(result.get());
> +    JSGlobalContextRelease(scriptContext);

Instead of creating a temporary JSGlobalContext, it would probably be better to
create one with the WebView, so that JavaScriptCore values can stay alive
longer.

> Source/WebKit2/UIProcess/API/gtk/tests/ScriptContext.cpp:45
> +    JSRetainPtr<JSStringRef> stringValue(Adopt,
JSValueToStringCopy(m_context, value, 0));
> +    g_assert(stringValue);
> +
> +    size_t cStringLength =
JSStringGetMaximumUTF8CStringSize(stringValue.get());
> +    char* cString = static_cast<char*>(g_malloc(cStringLength));
> +    JSStringGetUTF8CString(stringValue.get(), cString, cStringLength);

It's important to access the value with the same JSC context that created the
value. Thus it should probably be passed as property of the AsyncResult or kept
as a property of the WebKitWebView.