[Webkit-unassigned] [Bug 136989] WebKit2 Gtk+ JavaScriptCore bindings doesn't work properly

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=136989

--- Comment #4 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 260684
  --> https://bugs.webkit.org/attachment.cgi?id=260684
patch that adds function get_value_as_string to WebKitJavaScriptResult

View in context: https://bugs.webkit.org/attachment.cgi?id=260684&action=review

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] http://www.webkit.org/coding/coding-style.html
[2] http://www.webkit.org/coding/contributing.html
[3] https://lists.webkit.org/mailman/listinfo/webkit-gtk

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

Write %NULL so that it appears pretty in the documentation

> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:121
> + * string; free with g_free()

I don't think you need to mention how to free it; that should be apparent due to the (transfer full) annotation.

> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:129
> +    gsize       str_length;

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.

> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:131
> +    js_str_value = JSValueToStringCopy (context, javascriptResult->value, NULL);

Declare JSStringRef on this line, omit the space before the opening parenthesis in the function call, and use nullptr rather than NULL.

> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:136
> +        return NULL;

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.

> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:139
> +    str_length = JSStringGetMaximumUTF8CStringSize (js_str_value);

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

> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:140
> +    str_value = (gchar *)g_malloc (str_length);

Declare str_value on this line, use static_cast<gchar*> instead of the C cast.

> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:142
> +    JSStringRelease (js_str_value);

Also, in all these cases, omit the extra space before the opening parenthesis in the function call.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150905/8a70a779/attachment-0001.html>