[webkit-reviews] review denied: [Bug 69753] [GTK] Initial UI client implementation for WebKit2 GTK +API : [Attachment 110349] Patch updated to fix coding style issues

[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 69753: [GTK] Initial UI client implementation for WebKit2 GTK +API
https://bugs.webkit.org/show_bug.cgi?id=69753

Attachment 110349: Patch updated to fix coding style issues
https://bugs.webkit.org/attachment.cgi?id=110349&action=review

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


I'd prefer us to separate these into a Client class. Is there a reason it's
impossible? I'm not opposed to having these three signals on the WebView, but
they can be fired via an exposed private API from the WebView if you don't want
to use g_signal_emit_by_name.

I believe that the ::ready signal should be called ::show to match the
underlying API.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:66
> +static GtkWidget* webkitWebViewCreate(WebKitWebView*)
> +{
> +    return 0;
> +}

This seems to be dead code?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:76
> +static WKPageRef createNewPage(WKPageRef page, WKURLRequestRef request,
WKDictionaryRef features, WKEventModifiers modifiers, WKEventMouseButton
button, const void* clientInfo)
> +{
> +    WebKitWebView* newWebView;
> +    g_signal_emit(WEBKIT_WEB_VIEW(clientInfo), signals[CREATE], 0,
&newWebView);
> +    if (!newWebView)
> +	   return 0;
> +
> +    return
static_cast<WKPageRef>(WKRetain(toAPI(webkitWebViewBaseGetPage(WEBKIT_WEB_VIEW_
BASE(newWebView)))));
> +}

It's important to send all those arguments along. A hash table of features is
probably fine here, but if you like you can create a WebKitWebViewFeatures
object ala the WK1 API. This is one thing that the WK1 port didn't do quite
right.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:86
> +static void showPage(WKPageRef page, const void *clientInfo)
> +{
> +    g_signal_emit(WEBKIT_WEB_VIEW(clientInfo), signals[READY], 0, NULL);
> +}
> +
> +static void closePage(WKPageRef page, const void *clientInfo)
> +{
> +    g_signal_emit(WEBKIT_WEB_VIEW(clientInfo), signals[CLOSE], 0, NULL);
> +}

The asterisks on clientInfo are in the wrong place.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:108
> +	   webView,/* clientInfo */
> +	   0,	   /* createNewPage_deprecatedForUseWithV0 */
> +	   showPage,
> +	   closePage,
> +	   0,	   /* takeFocus */
> +	   0,	   /* focus */
> +	   0,	   /* unfocus */
> +	   0,	   /* runJavaScriptAlert */
> +	   0,	   /* runJavaScriptConfirm */
> +	   0,	   /* runJavaScriptPrompt */
> +	   0,	   /* setStatusText */
> +	   0,	   /* mouseDidMoveOverElement_deprecatedForUseWithV0 */
> +	   0,	   /* missingPluginButtonClicked */
> +	   0,	   /* didNotHandleKeyEvent */
> +	   0,	   /* didNotHandleWheelEvent */
> +	   0,	   /* toolbarsAreVisible */
> +	   0,	   /* setToolbarsAreVisible */

You should use C++ style comments here and they should be one space away from
the 0,.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:136
> +    WebPageProxy* page =
webkitWebViewBaseGetPage(WEBKIT_WEB_VIEW_BASE(webView));
> +    WKPageSetPageUIClient(toAPI(page), &uiClient);

This can be one line.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:148
>      priv->loaderClient =
adoptGRef(WEBKIT_WEB_LOADER_CLIENT(g_object_new(WEBKIT_TYPE_WEB_LOADER_CLIENT,
"web-view", webView, NULL)));
> +
> +    webkitWebViewUIClientInit(webView);

Again, I think a WebKitUIClient would be a lot cleaner here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:229
> +	* Emitted when the creation of a new view is requested.

We should probably say #WebKitWebView here instead of "view" as it's less
ambiguous.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:239
> +	* 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.
> +	*

Should the user wait until ::ready to add the widget to a parent widget? If so
you should probably mention that explicitly.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:243
> +    signals[CREATE] =
> +	   g_signal_new("create",

This can be one line.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:259
> +	* Emitted after #WebKitWebView::create on the newly created
#WebKitWebView
> +	* when it should be displayed to the user. When this signal is emitted
> +	* all the information about how the window should look, including
> +	* size, position, whether the location, status and scroll bars
> +	* should be displayed, is already set.

scroll bars -> scrollbars

I'm not sure I understand exactly how you mean the information is already set.
All that information should be passed as signal arguments to ::create.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:262
> +    signals[READY] =
> +	   g_signal_new("ready",

This can be one line.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:277
> +	* delete the web view, if necessary.

web view -> #WebKitWebView.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:280
> +    signals[CLOSE] =
> +	   g_signal_new("close",

Ditto.