[webkit-reviews] review denied: [Bug 48512] [GTK] Implement sample browser app for WebKit2 : [Attachment 86627] 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 86627: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=86627&action=review

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

Sorry. I missed a few things. :/ My only issues here are minor style issues,
but I feel like the change to createWindow will need another round.

> Tools/MiniBrowser/gtk/main.c:29
> +// FIXME: replace bool type in the C API
> +// https://bugs.webkit.org/show_bug.cgi?id=56668

I guess we may eventually need to add the stdbool.h to the headers ourselvses.
:/

> Tools/MiniBrowser/gtk/main.c:33
> +#include <WebKit2/WebKit2.h>
> +#include <gtk/gtk.h>

You shoud reorder these includes.

> Tools/MiniBrowser/gtk/main.c:44
> +static void destroyCb(GtkWidget *widget, GtkWidget *window)

Please change Cb to Callback everywhere.

> Tools/MiniBrowser/gtk/main.c:49
> +static void goBackCb(GtkWidget *widget, WKViewRef *webView)

Ditto.

> Tools/MiniBrowser/gtk/main.c:54
> +static void goForwardCb(GtkWidget *widget, WKViewRef *webView)

Ditto.

> Tools/MiniBrowser/gtk/main.c:120
> +    GtkWidget *window;
> +    window = gtk_window_new(GTK_WINDOW_TOPLEVEL);
> +    gtk_window_set_default_size(GTK_WINDOW(window), 800, 600);
> +    gtk_widget_set_name(window, "MiniBrowser");
> +
> +    WKContextRef context = WKContextGetSharedProcessContext();
> +    *webView = WKViewCreate(context, 0);
> +
> +    GtkWidget *uriEntry;
> +    uriEntry = gtk_entry_new();
> +
> +    GtkWidget *vbox;
> +    vbox = gtk_vbox_new(FALSE, 0);
> +    gtk_box_pack_start(GTK_BOX(vbox), createToolbar(uriEntry, webView),
FALSE, FALSE, 0);
> +    gtk_box_pack_start(GTK_BOX(vbox), WKViewGetWindow(*webView), TRUE, TRUE,
0);
> +
> +    gtk_container_add(GTK_CONTAINER(window), vbox);
> +
> +    g_signal_connect(window, "destroy", G_CALLBACK(destroyCb), NULL);
> +
> +    return window;

Oh, sorry. I guess I wasn't totally clear. You should do:

GtkWindow* window = gtk_window_new(GTK_WINDOW_TOPLEVEL); 

and the same with context, uriEntry, vbox, etc.

> Tools/MiniBrowser/gtk/main.c:144
> +    WKViewRef webView;
> +    GtkWidget *mainWindow;
> +    mainWindow = createWindow(&webView);
> +

I think I just realized what's confusing me about this bit. Here's my suggested
change:

1. Create the WKWebView externally with a helper: createWebView.
2. Pass the WebView widget to createWindow.

In the end it will look like this:

WkViewRef webView = createWebView();
GtkWidget *mainWindow = createWindow(&webView);