[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





--- Comment #16 from Ravi Phaneendra Kasibhatla <ravi.kasibhatla at motorola.com>  2011-01-14 09:02:08 PST ---
(In reply to comment #12)
> (From update of attachment 78233 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78233&action=review
> 
> Unofficial review
> 
> > WebKit2/ChangeLog:9
> > +        * UIProcess/gtk/WebView.cpp: Added. The native GtkWidget handle for each WKViewRef for GTK port.
> 
> Shouldn't this be in UIProcess/API/gtk ?
The public interfaces WKAPI files are part of patch (https://bugs.webkit.org/attachment.cgi?id=78234) in same bug. These files are internal to WebKit2, hence we have kept them in UIProcess/gtk instead of API/gtk. We have followed the win port pattern.
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.cpp:31
> > +static GObjectClass *parentClass = 0;
> 
> You can use G_DEFINE_TYPE() macro that already does this and other things for you.
As per discussion with mrobinson, we decided not to use G_DEFINE_TYPE so that we can be in sync with WebKit style rather than glib.
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.cpp:35
> > +    GTK_WIDGET_SET_FLAGS(widget, GTK_REALIZED);
> 
> GTK_WIDGET_SET_FLAGS is deprecated, use gtk_widget_set_realized() instead.
Done.
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.cpp:42
> > +    attributes.x = widget->allocation.x;
> > +    attributes.y = widget->allocation.y;
> > +    attributes.width = widget->allocation.width;
> > +    attributes.height = widget->allocation.height;
> 
> The widget inherits from GtkContainer so I think we should take into account the border width. Also GtkWidget::allocation is private now, you should use gtk_widget_get_allocation(). This should be something like:
> 
> borderWidth = gtk_container_get_border_width(GTK_CONTAINER(widget));
> gtk_widget_get_allocation(widget, &allocation);
> attributes.x = allocation.x + borderWidth;
> attributes.y = allocation.y + borderWidth;
> attributes.width = allocation.width - borderWidth * 2;
> attributes.height = allocation.height - borderWidth * 2;
Done.
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.cpp:52
> 
> 
> This is deprecated too because GtkWidget::style is private, use gtk_widget_style_attach() instead
Done.
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.cpp:53
> > +    gtk_style_set_background(widget->style, widget->window, GTK_STATE_NORMAL);
> 
> Use gtk_widget_get_style() here
Done.
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.cpp:62
> 
> 
> GTK_WIDGET_REALIZED() is deprecated, use gtk_widget_get_realized() instead
Done.
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.cpp:63
> 
> 
> Use gtk_widget_get_window() here
Done.
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.cpp:64
> > +    gtk_widget_set_parent(widget, GTK_WIDGET(container));
> 
> I think this is enough, why do you need to call set_parent_window too?
Done.
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.cpp:83
> > +static void
> > +webkitWidgetViewFinalize(GObject* obj)
> > +{
> > +    G_OBJECT_CLASS(parentClass)->finalize(obj);
> > +}
> 
> You don't need this, the parent finalize will be called anyway if you don't implement finalize()
Done. Since we have no other member to be freed in finalize, removed the callback itself.
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.cpp:85
> > +static void webkitWidgetViewClassInit(gpointer widgetViewParentClass, __attribute__((unused)) gpointer widgetViewClassData)
> 
> This can simply be webkitWidgetViewClassInit(WebKitWidgetViewClass* klass)
Done.
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.cpp:93
> > +    widgetClass->expose_event = WebKit::WebView::webViewOnExpose;
> 
> Expose is deprecated too, we should use #ifdefs here to use expose_event in gtk2 and draw in gtk3
Done. The draw is right now stubbed. We will try to put the properly tested draw() implementation ASAP (probably along with reworked UpdateChunk patch) 
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.cpp:114
> > +static void webkitWidgetViewInit(GTypeInstance* instance, __attribute__((unused)) gpointer widgetViewClass)
> 
> this can simply be static void webkitWidgetViewInit(WebKitWidgetView* webView)
Done.
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.cpp:126
> > +GType
> > +webkitWidgetViewGetType(void)
> 
> Use G_DEFINE_TYPE macro instead
As explained earlier, decided not to use this macro.
> 
> > WebKit2/UIProcess/gtk/WebView.cpp:36
> > +#include "WebViewInternal.h"
> 
> Why WebView and WebViewInternal?
Basically, WebView class implements interface PageClient and holds the native window handle. WebViewInternal is the gobject which captures the actual WebView (GtkWidget). I have now renamed WebViewInternal to WebKitWebView (as in WebKit1 since it does the same job as WebKitWebView).

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