[Webkit-unassigned] [Bug 48509] [GTK] Implement UpdateChunk, ChunkedUpdateDrawingArea/Proxy, WebView classes for WebKit2 and add WKAPI

[bugzilla-daemon at webkit.org]

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


Martin Robinson <mrobinson at webkit.org> changed:

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




--- Comment #19 from Martin Robinson <mrobinson at webkit.org>  2011-01-14 09:30:49 PST ---
(From update of attachment 78945)
View in context: https://bugs.webkit.org/attachment.cgi?id=78945&action=review

Looking better! I'm slightly concerned with the name of the GTK+ widget because it collides with the WebKit1 naming. I'm not sure if that would cause linking issues or not (some older libraries might link against WebKit1 for JavaScriptCore. I know it's unusual, but maybe we should consider the WKView naming convention to match other ports.

> WebKit2/UIProcess/gtk/WebKitWebView.cpp:51
> +#if GTK_CHECK_VERSION(2, 18, 0)
> +    gtk_widget_get_allocation(widget, &allocation);
> +#else
> +    allocation = widget->allocation;
> +#endif

Here you can just include GtkVersioning.h from WebCore and only call gtk_widget_get_allocation(...)

> WebKit2/UIProcess/gtk/WebKitWebView.cpp:168
> +            sizeof(WebKitWebViewClass),          /* class structure size */
> +            0,                                  /* base_init */
> +            0,                                  /* base_finalize */
> +            reinterpret_cast<GClassInitFunc>(webkitWebViewClassInit), /* class initializer */
> +            0,                                  /* class_finalize */
> +            0,                                  /* class_data */
> +            sizeof(WebKitWebView),               /* instance structure size */
> +            0,                                  /* preallocated instances */
> +            reinterpret_cast<GInstanceInitFunc>(webkitWebViewInit),   /* instance initializer */
> +            0                                   /* function table */
> +    };   

This should be a four space indent. Nit!: do you mind lining up the comments? :)

> WebKit2/UIProcess/gtk/WebView.h:61
> +#ifdef GTK_API_VERSION_2
> +    static gboolean webViewOnExpose(GtkWidget*, GdkEventExpose*);
> +#else
> +    static gboolean webViewOnDraw(GtkWidget*, cairo_t*);
> +#endif
> +    static void webViewOnSizeAllocate(GtkWidget*, GtkAllocation*);
> +    static gboolean webViewFocusInEvent(GtkWidget*, GdkEventFocus*);
> +    static gboolean webViewFocusOutEvent(GtkWidget*, GdkEventFocus*);
> +    static gboolean webViewKeyPressEvent(GtkWidget*, GdkEventKey*);
> +    static gboolean webViewKeyReleaseEvent(GtkWidget*, GdkEventKey*);
> +    static gboolean webViewButtonPressEvent(GtkWidget*, GdkEventButton*);
> +    static gboolean webViewButtonReleaseEvent(GtkWidget*, GdkEventButton*);
> +    static gboolean webViewScrollEvent(GtkWidget*, GdkEventScroll*);
> +    static gboolean webViewMotionNotifyEvent(GtkWidget*, GdkEventMotion*);

These seem to be GObject methods that are really only used in WebKitWebView.cpp. I think it might make more sense to declare them statically inside the GObject implementation file.

Ah, I see you have them here, because the methods on WebView are private. Perhaps a better approach would be to make these methods on the private data section of the GObject and make the private data struct a friend class of WebView. Either that or make the WebView:onWhatever methods public. Technically you are calling them from another class.

> WebKit2/UIProcess/gtk/WebView.h:118
> +    GdkRectangle m_windowRect;

Is this the rectangle that represents the entire window or just the viewport? If it's the viewport, I think m_viewRect or m_viewportRect makes more sense.

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