[Webkit-unassigned] [Bug 48512] [GTK] Implement sample browser app for WebKit2

[bugzilla-daemon at webkit.org]

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


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #86211|review?                     |review-
               Flag|                            |




--- Comment #12 from Martin Robinson <mrobinson at webkit.org>  2011-03-18 15:27:14 PST ---
(From update of attachment 86211)
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? :)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.