[webkit-reviews] review denied: [Bug 48512] [GTK] Implement sample browser app for WebKit2 : [Attachment 86211] Proposed patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Alejandro G. Castro
<alex at igalia.com>'s request for review:
Bug 48512: [GTK] Implement sample browser app for WebKit2
https://bugs.webkit.org/show_bug.cgi?id=48512

Attachment 86211: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=86211&action=review

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

Looks good!. Just needs a little bit of cleanup.

> Tools/MiniBrowser/gtk/main.c:37
> +static void activateUriEntryCb(GtkWidget *entry, gpointer data)

Why not pass in the WebView as the data parameter?

> Tools/MiniBrowser/gtk/main.c:41
> +    g_assert(uri);

Is it possible for get_text to return NULL here? If so it should be handled. If
not, I think we can remove the assertion.

> Tools/MiniBrowser/gtk/main.c:65
> +static GtkWidget *createBrowser(GtkWidget *window, GtkWidget *uriEntry,
WKViewRef *webView)
> +{
> +    GtkWidget *webViewWidget = WKViewGetWindow(*webView);
> +
> +    return webViewWidget;
> +}

Just remove this entirely and call  WKViewGetWindow(...) at the callsite.

> Tools/MiniBrowser/gtk/main.c:111
> +    GtkWidget *vbox;
> +    GtkWidget *window;
> +    GtkWidget *uriEntry;
> +    GtkWidget *webViewWidget;

My preference is to declare these where you first use them. We should check to
see if this is the way that the Mac C code is written. If so, we should follow
the same style.

> Tools/MiniBrowser/gtk/main.c:119
> +    webViewWidget = WKViewGetWindow(*webView);

This seems to be unused?

> Tools/MiniBrowser/gtk/main.c:125
> +    gtk_box_pack_start(GTK_BOX(vbox), createBrowser(window, uriEntry,
webView), TRUE, TRUE, 0);

As stated above, simply use WKViewGetWindow above.

> Tools/MiniBrowser/gtk/main.c:149
> +    GtkWidget *main_window;

main_window ==> mainWindow.

> Tools/MiniBrowser/gtk/main.c:157
> +    gchar *uri =(gchar*)(argc > 1 ? argv[1] : "http://www.google.com/");

Missing a space here after =. Should we use "http://webkitgtk.org " as the
default page? :)