<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - WebKit2 Gtk+ JavaScriptCore bindings doesn't work properly"
href="https://bugs.webkit.org/show_bug.cgi?id=136989#c4">Comment # 4</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - WebKit2 Gtk+ JavaScriptCore bindings doesn't work properly"
href="https://bugs.webkit.org/show_bug.cgi?id=136989">bug 136989</a>
from <span class="vcard"><a class="email" href="mailto:mcatanzaro@igalia.com" title="Michael Catanzaro <mcatanzaro@igalia.com>"> <span class="fn">Michael Catanzaro</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=260684&action=diff" name="attach_260684" title="patch that adds function get_value_as_string to WebKitJavaScriptResult">attachment 260684</a> <a href="attachment.cgi?id=260684&action=edit" title="patch that adds function get_value_as_string to WebKitJavaScriptResult">[details]</a></span>
patch that adds function get_value_as_string to WebKitJavaScriptResult
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=260684&action=review">https://bugs.webkit.org/attachment.cgi?id=260684&action=review</a>
This patch looks good to me; all my comments are just regarding code style. It looks like you're already familiar with GNOME coding style, which is great, but WebKit style is rather different [1]. We only use the GNOME style in our public header files.
Carlos Lopez mentioned on IRC that the patch needs to include a changelog entry [2], which can be created with the prepare-ChangeLog script.
Since this patch adds new API, it needs to be discussed on the mailing list [3] and then approved by two GTK+ reviewers. Since it's a small addition I think it should be easy: just write a mail introducing your proposal; signing up for the list will make this easier so that your messages aren't queued for moderation, but isn't mandatory.
[1] <a href="http://www.webkit.org/coding/coding-style.html">http://www.webkit.org/coding/coding-style.html</a>
[2] <a href="http://www.webkit.org/coding/contributing.html">http://www.webkit.org/coding/contributing.html</a>
[3] <a href="https://lists.webkit.org/mailman/listinfo/webkit-gtk">https://lists.webkit.org/mailman/listinfo/webkit-gtk</a>
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:118
> + * gets converted to a string, if possible. In any case where this isn't possible NULL is returned. </span >
Write %NULL so that it appears pretty in the documentation
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:121
> + * string; free with g_free()</span >
I don't think you need to mention how to free it; that should be apparent due to the (transfer full) annotation.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:129
> + gsize str_length;</span >
In WebKit we don't use this extra space between the type and the variable name, and the * always goes on the type, not the variable name. Also, we declare variables at the point of first use whenever possible, instead of at the top of functions. You can use 'check-webkit-style' (which will also be run during 'webkit-patch upload' if you use that) to find most style issues; I think it is smart enough to automatically find most of the style issues I found here.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:131
> + js_str_value = JSValueToStringCopy (context, javascriptResult->value, NULL);</span >
Declare JSStringRef on this line, omit the space before the opening parenthesis in the function call, and use nullptr rather than NULL.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:136
> + return NULL;</span >
Omit the braces around one-line statements, and use nullptr instead of NULL. I would also omit the comment here, since the behavior is clear from the doc comment up above.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:139
> + str_length = JSStringGetMaximumUTF8CStringSize (js_str_value);</span >
Declare str_length on this line, and again omit the extra space before the opening parenthesis in the function call (and for the other function calls too).
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:140
> + str_value = (gchar *)g_malloc (str_length);</span >
Declare str_value on this line, use static_cast<gchar*> instead of the C cast.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:142
> + JSStringRelease (js_str_value);</span >
Also, in all these cases, omit the extra space before the opening parenthesis in the function call.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>