[webkit-reviews] review granted: [Bug 48509] [GTK] Implement WebView and WebKitWebView classes for WebKit2 : [Attachment 83285] Proposed patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Alejandro G. Castro
<alex at igalia.com>'s request for review:
Bug 48509: [GTK] Implement WebView and WebKitWebView classes for WebKit2
https://bugs.webkit.org/show_bug.cgi?id=48509

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

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

Looks good, but please make the following changes.

> Source/WebKit2/UIProcess/gtk/WebKitWebView.cpp:92
> +#ifdef GTK_API_VERSION_2
> +#if GTK_CHECK_VERSION(2, 20, 0)
> +    gtk_widget_style_attach(widget);
> +#else
> +    widget->style = gtk_style_attach(gtk_widget_get_style(widget), window);
> +#endif

A nice cleanup would be to make this handled by GtkVersioning.h in a followup
patch.

> Source/WebKit2/UIProcess/gtk/WebKitWebView.cpp:145
> +    GdkWindow* window = gtk_widget_get_window(widget);
> +    cairo_t* cr = gdk_cairo_create(window);
> +
> +    webView->paint(widget, clipRect, cr);
> +

This should be smarter eventually. It's probably a good idea to open a bug
tracking moving the smarts from WebKit1.

> Source/WebKit2/UIProcess/gtk/WebKitWebView.cpp:165
> +static void webViewOnSizeAllocate(GtkWidget* widget, GtkAllocation*
allocation)

Please rename this to webViewSizeAllocate for consistency.

> Source/WebKit2/UIProcess/gtk/WebKitWebView.cpp:349
> +    static volatile gsize gDefineTypeIdVolatile = 0;
> +    if (g_once_init_enter(&gDefineTypeIdVolatile)) {

Please turn this into an early return.

> Source/WebKit2/UIProcess/gtk/WebKitWebView.h:29
> +#ifndef WebKitWebView_h
> +#define WebKitWebView_h

Let's call this WebViewWidget for now,	just so that the separation of concerns
is clear. I think having a GtkWidget called WebKitWebView and a C++ class
called WebView is too confusing for the moment. Later on we can focus more
heavily on what kind of interface we want to expose to the end user. This will
be easier once we see how this looks for the released version of Mac and
Windows.