[webkit-reviews] review granted: [Bug 69753] [GTK] Initial UI client implementation for WebKit2 GTK +API : [Attachment 113313] Updated patch to fix unit tests

Mon Dec 12 07:14:43 PST 2011

Martin Robinson <mrobinson at webkit.org> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 69753: [GTK] Initial UI client implementation for WebKit2 GTK +API
https://bugs.webkit.org/show_bug.cgi?id=69753

Attachment 113313: Updated patch to fix unit tests
https://bugs.webkit.org/attachment.cgi?id=113313&action=review

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

Looks good to me. I have a couple documentation nits below as well as a request
to use skip the soup server, if possible.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:163
> +static gboolean
webkitWebViewAccumulatorObjectHandled(GSignalInvocationHint*, GValue*
returnValue, const GValue* handlerReturn, gpointer)
> +{
> +    gpointer object = g_value_get_object(handlerReturn);
> +    if (object)
> +	   g_value_set_object(returnValue, object);
> +
> +    return !object;

I prefer void* to gpointer.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:236
> +	* The signal handlers should not try to deal with the reference count
for
> +	* the new #WebKitWebView. The widget to which the widget is added will
> +	* handle that.

I find this a bit confusing still. From what I understand, the user will add
the widget to a container. The container will handle the reference count. The
user is still responsible for adding the widget to the container. They can do
this at any time during the process of create -> ready-to-show -> close, right?
I think the documentation should just explain that the user should add the
WebKitWebView to a container as usual.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:258
> +	* size, position, whether the location, status and scrollbars
> +	* should be displayed, is already set.

Later we should come back and explain where it's set.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:276
> +	* delete the #WebKitWebView, if necessary.

delete -> destroy

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:99
> +    static const char* htmlOnLoadFormat = "<html><body
onLoad=\"%s\"></body></html>";
> +
> +    if (message->method != SOUP_METHOD_GET) {
> +	   soup_message_set_status(message, SOUP_STATUS_NOT_IMPLEMENTED);
> +	   return;
> +    }
> +
> +    soup_message_set_status(message, SOUP_STATUS_OK);
> +
> +    if (g_str_equal(path, "/create_new_view")) {
> +	   GOwnPtr<char> windowOpenString(g_strdup_printf("window.open('%s')",
kServer->getURIForPath("/new_window").data()));
> +	   char* htmlString = g_strdup_printf(htmlOnLoadFormat,
windowOpenString.get());
> +	   soup_message_body_append(message->response_body, SOUP_MEMORY_TAKE,
htmlString, strlen(htmlString));
> +    } else if (g_str_equal(path, "/new_window")) {
> +	   char* htmlString = g_strdup_printf(htmlOnLoadFormat,
"self.close();");
> +	   soup_message_body_append(message->response_body, SOUP_MEMORY_TAKE,
htmlString, strlen(htmlString));
> +    }

Can you load a string now instead of using Soup here?