Bug 48509 - [GTK] Implement WebView and WebKitWebView classes for WebKit2
Summary: [GTK] Implement WebView and WebKitWebView classes for WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 52805
  Show dependency treegraph
 
Reported: 2010-10-28 04:43 PDT by Amruth Raj
Modified: 2011-02-25 03:46 PST (History)
10 users (show)

See Also:


Attachments
Implementation of all paint related classes ie UpdateChunk and ChunkedUpdateDrawingArea/Proxy classes (24.92 KB, patch)
2010-10-29 06:17 PDT, Amruth Raj
no flags Details | Formatted Diff | Diff
Contains Implementation of WebView Class and WKAPI exposing some API on top of it (36.55 KB, patch)
2010-10-29 06:21 PDT, Amruth Raj
no flags Details | Formatted Diff | Diff
UpdateChunk, ChunkedUpdateDrawingArea, WebView classes implementation for GTK port (50.86 KB, patch)
2010-11-02 09:41 PDT, Ravi Phaneendra Kasibhatla
no flags Details | Formatted Diff | Diff
Changes to WKAPI interfaces for GTK port (11.38 KB, patch)
2010-11-02 09:42 PDT, Ravi Phaneendra Kasibhatla
no flags Details | Formatted Diff | Diff
UpdateChunk, ChunkedUpdateDrawingArea implementation for GTK port (22.83 KB, patch)
2011-01-07 07:19 PST, Ravi Phaneendra Kasibhatla
mrobinson: review-
Details | Formatted Diff | Diff
WebView, WebViewInternal classes implementation for GTK port (29.53 KB, patch)
2011-01-07 07:19 PST, Ravi Phaneendra Kasibhatla
mrobinson: review-
Details | Formatted Diff | Diff
Changes to WKAPI interfaces for GTK port (11.42 KB, patch)
2011-01-07 07:21 PST, Ravi Phaneendra Kasibhatla
mrobinson: review-
Details | Formatted Diff | Diff
WebView & WebKitWebView implementation with review comments incorporation (30.54 KB, patch)
2011-01-14 08:51 PST, Ravi Phaneendra Kasibhatla
mrobinson: review-
Details | Formatted Diff | Diff
Proposed patch (30.28 KB, patch)
2011-02-09 06:30 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Proposed patch (30.15 KB, patch)
2011-02-10 05:50 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Proposed patch (31.75 KB, patch)
2011-02-11 11:03 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Proposed patch (31.92 KB, patch)
2011-02-15 08:54 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Proposed patch (35.04 KB, patch)
2011-02-22 01:38 PST, Alejandro G. Castro
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Amruth Raj 2010-10-28 04:43:41 PDT
Implementing UpdateChunk, ChunkedUpdateDrawingArea/Proxy, WebView classes for WebKit2 Gtk port and add WKAPI that are required for a sample app
Comment 1 Amruth Raj 2010-10-29 06:17:21 PDT
Created attachment 72322 [details]
Implementation of all paint related classes ie UpdateChunk and ChunkedUpdateDrawingArea/Proxy classes
Comment 2 Amruth Raj 2010-10-29 06:21:55 PDT
Created attachment 72323 [details]
Contains Implementation of WebView Class and WKAPI exposing some API on top of it

This patch has a compilation dependency on the other patch that is attached with this bug(attachment id: 72322).
Please do not push this until the 72322 is checked-in.
Comment 3 Ravi Phaneendra Kasibhatla 2010-10-29 07:15:09 PDT
Adding myself to the CC list for this bug.
Comment 4 Ravi Phaneendra Kasibhatla 2010-11-02 09:41:48 PDT
Created attachment 72682 [details]
UpdateChunk, ChunkedUpdateDrawingArea, WebView classes implementation for GTK port

UpdateChunk, ChunkedUpdateDrawingArea, WebView classes implementation for GTK port.
Comment 5 Ravi Phaneendra Kasibhatla 2010-11-02 09:42:52 PDT
Created attachment 72683 [details]
Changes to WKAPI interfaces for GTK port

Changes in WKBase and WKAPICast implementation for GTK port. Also, contains WKView implementation for GTK port.
Comment 6 Ravi Phaneendra Kasibhatla 2011-01-07 07:19:05 PST
Created attachment 78232 [details]
UpdateChunk, ChunkedUpdateDrawingArea implementation for GTK port
Comment 7 Ravi Phaneendra Kasibhatla 2011-01-07 07:19:54 PST
Created attachment 78233 [details]
WebView, WebViewInternal classes implementation for GTK port
Comment 8 Ravi Phaneendra Kasibhatla 2011-01-07 07:21:02 PST
Created attachment 78234 [details]
Changes to WKAPI interfaces for GTK port
Comment 9 Balazs Kelemen 2011-01-08 12:42:29 PST
Comment on attachment 78232 [details]
UpdateChunk, ChunkedUpdateDrawingArea implementation for GTK port

View in context: https://bugs.webkit.org/attachment.cgi?id=78232&action=review

Some thoughts from a non-reviewer.

> WebKit2/WebProcess/WebPage/ChunkedUpdateDrawingArea.h:74
> +#if PLATFORM(GTK)
> +    // to destroy the shared memory that is created in setSize

Please use full sentences with capital letter and dot at the end.

> WebKit2/WebProcess/WebPage/ChunkedUpdateDrawingArea.h:78
> +    HashMap <uint32_t, void*> xIDMap;

Seems like simply owning the UpdateChunk would be more straightforward than the map.
The drawing area is acting on exactly one chunk at a time so the map would never has more than one item.
In didUpdate you can release the owned chunk.

> WebKit2/UIProcess/gtk/ChunkedUpdateDrawingAreaProxyGtk.cpp:67
> +    // BitBlt from the backing-store to the window.
> +    gdk_draw_drawable(window, gc, m_backingStoreBitmap, rect.x(), rect.y(), rect.x(), rect.y(), rect.width(), rect.height());
> +    g_object_unref(gc);

BitBlt is a Windows API function. I guess you wont refer to that here :)
Comment 10 Amruth Raj 2011-01-11 07:24:53 PST
(In reply to comment #9)
> (From update of attachment 78232 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78232&action=review
> 
> Some thoughts from a non-reviewer.
> 
> > WebKit2/WebProcess/WebPage/ChunkedUpdateDrawingArea.h:74
> > +#if PLATFORM(GTK)
> > +    // to destroy the shared memory that is created in setSize
> 
> Please use full sentences with capital letter and dot at the end.
Sure. Will address this.
> 
> > WebKit2/WebProcess/WebPage/ChunkedUpdateDrawingArea.h:78
> > +    HashMap <uint32_t, void*> xIDMap;
> 
> Seems like simply owning the UpdateChunk would be more straightforward than the map.
> The drawing area is acting on exactly one chunk at a time so the map would never has more than one item.
> In didUpdate you can release the owned chunk.
The map holds the UpdateChunk's created in setSize as well. There can be multiple setSize requests at a time or there can be a scenario where size-request and update-request happen simultaneously(for eg. resize a window when a gif image plays). In this case, there would be more than 1 UpdateChunks alive in the map.
> 
> > WebKit2/UIProcess/gtk/ChunkedUpdateDrawingAreaProxyGtk.cpp:67
> > +    // BitBlt from the backing-store to the window.
> > +    gdk_draw_drawable(window, gc, m_backingStoreBitmap, rect.x(), rect.y(), rect.x(), rect.y(), rect.width(), rect.height());
> > +    g_object_unref(gc);
> 
> BitBlt is a Windows API function. I guess you wont refer to that here :)
Sure. Will reword the comment.
Comment 11 Carlos Garcia Campos 2011-01-12 01:18:56 PST
Comment on attachment 78232 [details]
UpdateChunk, ChunkedUpdateDrawingArea implementation for GTK port

View in context: https://bugs.webkit.org/attachment.cgi?id=78232&action=review

I'm not reviewer, this is an unofficial review.

> WebKit2/Shared/gtk/UpdateChunk.h:47
> +    GdkPixmap* memory() const { return m_bitmapSharedMemory; }

GdkPixmap is deprecated, cairo surfaces should be used instead. Also the name memory() is confusing to me, since it doesn't return the memory data but the GdkPixmap.

> WebKit2/Shared/gtk/UpdateChunk.cpp:48
> +    m_bitmapSharedMemory = gdk_pixmap_new(0, rect.width(), rect.height(), gdk_visual_get_depth(gdk_visual_get_system()));
> +    m_xID = static_cast<uint32_t>(GDK_PIXMAP_XID(m_bitmapSharedMemory));

Since GdkPixmap is deprecated we could use a mmapped file and a cairo image surface to draw into.

> WebKit2/WebProcess/WebPage/ChunkedUpdateDrawingArea.cpp:94
> +#if PLATFORM(GTK)
> +    xIDMap.set(updateChunk.xID(), updateChunk.memory());
> +#endif

I think we should avoid having platform #ifdefs in common code

> WebKit2/UIProcess/gtk/ChunkedUpdateDrawingAreaProxyGtk.cpp:64
> +    GdkGC* gc = gdk_gc_new(m_backingStoreBitmap);

GdkGC is deprecated too, you can get a cairo context with gdk_cairo_create() and use cairo api for drawing.

> WebKit2/UIProcess/gtk/ChunkedUpdateDrawingAreaProxyGtk.cpp:85
> +    gdk_window_invalidate_rect(m_webView->window()->window, &gdkRect, false);

GtkWidget::window is private now in GTK+. Instead of having a method that returns the view window, I would add a method to the view to update a rectangle that calls gdk_window_invalidate_rect().

> WebKit2/UIProcess/ChunkedUpdateDrawingAreaProxy.cpp:124
> +#if PLATFORM(GTK)
> +    page->process()->send(DrawingAreaLegacyMessage::DidSetSizeDone, page->pageID(), CoreIPC::In(info().identifier, updateChunk->xID()));
> +#endif

Platform #ifdefs in common code again, I think we can avoid this.
Comment 12 Carlos Garcia Campos 2011-01-12 02:11:18 PST
Comment on attachment 78233 [details]
WebView, WebViewInternal classes implementation for GTK port

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 ?

> 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.

> 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.

> 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;

> WebKit2/UIProcess/gtk/WebViewInternal.cpp:52


This is deprecated too because GtkWidget::style is private, use gtk_widget_style_attach() instead

> WebKit2/UIProcess/gtk/WebViewInternal.cpp:53
> +    gtk_style_set_background(widget->style, widget->window, GTK_STATE_NORMAL);

Use gtk_widget_get_style() here

> WebKit2/UIProcess/gtk/WebViewInternal.cpp:62


GTK_WIDGET_REALIZED() is deprecated, use gtk_widget_get_realized() instead

> WebKit2/UIProcess/gtk/WebViewInternal.cpp:63


Use gtk_widget_get_window() here

> 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?

> 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()

> WebKit2/UIProcess/gtk/WebViewInternal.cpp:85
> +static void webkitWidgetViewClassInit(gpointer widgetViewParentClass, __attribute__((unused)) gpointer widgetViewClassData)

This can simply be webkitWidgetViewClassInit(WebKitWidgetViewClass* klass)

> 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

> WebKit2/UIProcess/gtk/WebViewInternal.cpp:114
> +static void webkitWidgetViewInit(GTypeInstance* instance, __attribute__((unused)) gpointer widgetViewClass)

this can simply be static void webkitWidgetViewInit(WebKitWidgetView* webView)

> WebKit2/UIProcess/gtk/WebViewInternal.cpp:126
> +GType
> +webkitWidgetViewGetType(void)

Use G_DEFINE_TYPE macro instead

> WebKit2/UIProcess/gtk/WebView.cpp:36
> +#include "WebViewInternal.h"

Why WebView and WebViewInternal?
Comment 13 Martin Robinson 2011-01-13 14:23:17 PST
Comment on attachment 78233 [details]
WebView, WebViewInternal classes implementation for GTK port

View in context: https://bugs.webkit.org/attachment.cgi?id=78233&action=review

Here are my first impressions. I have listed some specific issues below. I will do a more thorough review later though.

General: Is WebKitWidgetView going to be part of the public API? If so, it should go into WebKit2/UIProcess/gtk, and be organized like the other ports. I'm a little concered with the naming. The rest of the API follows the CF naming conventions WKEtcEtc and it seems that the other ports try to follow that as well. Maybe it makes sense to name this something like GTKWKView or GWKView. WebKitWidgetView implies this a view for holding widgets though. :)

If it allows the code to use WebKit naming conventions everywhere, I think we can avoid using G_DEFINE_TYPE.

>> 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.

Depending on how you decide about G_DEFINE_TYPE, this asterisk needs to go with the typename.

> WebKit2/UIProcess/gtk/WebViewInternal.cpp:68
> +static void
> +webkitWidgetViewDispose(GObject* obj)

This should be all one line. Please don't abbreviate the variable name.

> WebKit2/UIProcess/gtk/WebViewInternal.cpp:80
> +static void
> +webkitWidgetViewFinalize(GObject* obj)

Ditto.

> WebKit2/UIProcess/gtk/WebViewInternal.cpp:111
> +    g_type_class_add_private(klass, sizeof(WebKitWidgetViewPrivate));

Instead of doing this, it would be better to do it manually. This will allow you to have the private section that can hold C++ members and properly call their constructors and desctructors. Take a look at WebKit/gtk/webkit/webkitwebplugin.cpp for an example of how to do this. We'll be doign this all over the WebKit 1 API eventually.

>> WebKit2/UIProcess/gtk/WebViewInternal.cpp:126
>> +GType
>> +webkitWidgetViewGetType(void)
> 
> Use G_DEFINE_TYPE macro instead

This should just be one line. Leave out the 'void' for the empty argument list too, please.

> WebKit2/UIProcess/gtk/WebViewInternal.cpp:130
> +    static GType type = 0; 
> +
> +    if (!type) {

Use an early return here.

> WebKit2/UIProcess/gtk/WebViewInternal.cpp:144
> +        static const GTypeInfo info = {
> +            sizeof(WebKitWidgetViewClass),      /* class structure size */
> +            0,                                  /* base_init */
> +            0,                                  /* base_finalize */
> +            (GClassInitFunc)webkitWidgetViewClassInit, /* class initializer */
> +            0,                                  /* class_finalize */
> +            0,                                  /* class_data */
> +            sizeof(WebKitWidgetView),           /* instance structure size */
> +            0,                                  /* preallocated instances */
> +            (GInstanceInitFunc)webkitWidgetViewInit,   /* instance initializer */
> +            0                                   /* function table */
> +        };   
> +        type = g_type_register_static(GTK_TYPE_CONTAINER, "WebKitWidgetView", &info, (GTypeFlags)0);

Please use static_casts here (or reinterpret_casts where appropriate) instead of a c-style casts.

> WebKit2/UIProcess/gtk/WebView.cpp:47
> +    gpointer webViewInstance = widgetView->priv->webViewInstance;
> +    WebView* webView = reinterpret_cast<WebView*>(webViewInstance);

I think it's a bad idea to use reinterpret_cast here to get the WebView instance. Just store the pointer as a WebView* and make the private section actually private.

> WebKit2/UIProcess/gtk/WebView.cpp:50

Instead of getting the entire clipbox for the region, wouldn't it make sense to break it up into rectangles and paint them separately. Is that possible? Take a look at the expose event in webkitwebview.cpp. In any case, I think you should pass an IntRect to this method.

> WebKit2/UIProcess/gtk/WebView.cpp:59
> +    GObjectClass* parentClass = (GObjectClass*)g_type_class_peek_parent(WEBKIT_WIDGET_VIEW_GET_CLASS(widget));
> +    GTK_WIDGET_CLASS(parentClass)->size_allocate(widget, allocation);
> +    WebKitWidgetView* widgetView = WEBKIT_WIDGET_VIEW(widget);

Can't you just do widgetView->parentClass?

> WebKit2/UIProcess/gtk/WebView.cpp:61
> +    gpointer webViewInstance = widgetView->priv->webViewInstance;
> +    WebView* webView = reinterpret_cast<WebView*>(webViewInstance);

Same comment as above.

> WebKit2/UIProcess/gtk/WebView.cpp:66
> +gboolean WebView::webViewFocusInEvent(GtkWidget* widget, GdkEventFocus* event)

Same comments as above in this method.

> WebKit2/UIProcess/gtk/WebView.cpp:86
> +gboolean WebView::webViewFocusOutEvent(GtkWidget* widget, GdkEventFocus* event)

Ditto.

> WebKit2/UIProcess/gtk/WebView.cpp:102
> +gboolean WebView::webViewKeyPressEvent(GtkWidget* widget, GdkEventKey* event)

Ditto.

> WebKit2/UIProcess/gtk/WebView.cpp:113
> +gboolean WebView::webViewKeyReleaseEvent(GtkWidget* widget, GdkEventKey* event)

Ditto.

> WebKit2/UIProcess/gtk/WebView.cpp:127
> +gboolean WebView::webViewButtonPressEvent(GtkWidget* widget, GdkEventButton* const event)

Ditto.

> WebKit2/UIProcess/gtk/WebView.cpp:138
> +gboolean WebView::webViewButtonReleaseEvent(GtkWidget* widget, GdkEventButton* event)

Ditto.

> WebKit2/UIProcess/gtk/WebView.cpp:169
> +WebView::WebView(GdkRectangle rect, WebContext* context, WebPageGroup* pageGroup, GtkWidget*  hostWindow)

You should use a more descriptive parameter name than just "rect." I can't tell from this patch what it represents. There's an extra space after the final asterisk.

> WebKit2/UIProcess/gtk/WebView.cpp:179
> +    m_window = (GtkWidget*)g_object_new(WEBKIT_TYPE_WIDGET_VIEW, NULL); 

Use a static_cast here please.

> WebKit2/UIProcess/gtk/WebView.cpp:182
> +    WebKitWidgetView* webView = WEBKIT_WIDGET_VIEW(m_window);
> +    webView->priv->webViewInstance = reinterpret_cast<gpointer>(this);

Here you can just do WEBKIT_WIDGET_VIEW(m_window)->priv->webViewInstance = this;

> WebKit2/UIProcess/gtk/WebView.cpp:206
> +void WebView::onSetFocusEvent(GtkWidget*, bool)

What's the use of the second parameter? It's never used or named.

> WebKit2/UIProcess/gtk/WebView.cpp:211
> +void WebView::onKillFocusEvent(GtkWidget*, bool)

Ditto.

> WebKit2/UIProcess/gtk/WebView.cpp:224
> +    m_page->handleMouseEvent(mouseEvent);

This can just be:
m_page->handleMouseEvent(WebEventFactory::createWebMouseEvent(event));

> WebKit2/UIProcess/gtk/WebView.cpp:230
> +    WebWheelEvent wheelEvent = WebEventFactory::createWebWheelEvent(event);
> +    m_page->handleWheelEvent(wheelEvent);

Same comment here.

> WebKit2/UIProcess/gtk/WebViewInternal.h:46


Please don't store this anonymously.

> WebKit2/UIProcess/gtk/WebViewInternal.h:47
> +    GtkIMContext *imContext;

The asterisk is in the wrong position here.

> WebKit2/UIProcess/gtk/WebViewInternal.h:54
> +    WebKitWidgetViewPrivate *priv;

Ditto.

> WebKit2/UIProcess/gtk/WebViewInternal.h:62
> +GType
> +webkitWidgetViewGetType(void);

This should be one line.

> WebKit2/UIProcess/gtk/WebView.h:112
> +    GdkRectangle m_rect;

I'd really prefer a more descriptive name for this member. What does the rect actually represent?
Comment 14 Ravi Phaneendra Kasibhatla 2011-01-14 08:51:25 PST
Created attachment 78945 [details]
WebView & WebKitWebView implementation with review comments incorporation
Comment 15 WebKit Review Bot 2011-01-14 08:52:44 PST
Attachment 78945 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit2/ChangeLog', u'WebKit2/UIProcess/gt..." exit_code: 1

WebKit2/UIProcess/gtk/WebKitWebView.cpp:33:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Ravi Phaneendra Kasibhatla 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).
Comment 17 Ravi Phaneendra Kasibhatla 2011-01-14 09:19:45 PST
(In reply to comment #13)
> (From update of attachment 78233 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78233&action=review
> 
> Here are my first impressions. I have listed some specific issues below. I will do a more thorough review later though.
> 
> General: Is WebKitWidgetView going to be part of the public API? If so, it should go into WebKit2/UIProcess/gtk, and be organized like the other ports. I'm a little concered with the naming. The rest of the API follows the CF naming conventions WKEtcEtc and it seems that the other ports try to follow that as well. Maybe it makes sense to name this something like GTKWKView or GWKView. WebKitWidgetView implies this a view for holding widgets though. :)
> 
No WebKitWidgetView is always internal to WebView class. No other class should know even existence of the native widget handle. For all other classes, WebView is the actual handle. Hence, we kept the files in UIProcess/gtk folder. The actual public API files are WKView.[h/cpp] which are present in UIProcess/API/gtk folder (present in other patch of same bug). The API and file names of public API follow the CF pattern of WKxxx.

> If it allows the code to use WebKit naming conventions everywhere, I think we can avoid using G_DEFINE_TYPE.
I haven't used G_DEFINE_TYPE and instead following webkit style everywhere.
> 
> >> 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.
> 
> Depending on how you decide about G_DEFINE_TYPE, this asterisk needs to go with the typename.
Done.
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.cpp:68
> > +static void
> > +webkitWidgetViewDispose(GObject* obj)
> 
> This should be all one line. Please don't abbreviate the variable name.
Done.
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.cpp:80
> > +static void
> > +webkitWidgetViewFinalize(GObject* obj)
> 
> Ditto.
Done.
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.cpp:111
> > +    g_type_class_add_private(klass, sizeof(WebKitWidgetViewPrivate));
> 
> Instead of doing this, it would be better to do it manually. This will allow you to have the private section that can hold C++ members and properly call their constructors and desctructors. Take a look at WebKit/gtk/webkit/webkitwebplugin.cpp for an example of how to do this. We'll be doign this all over the WebKit 1 API eventually.
I haven't yet used the placement constructors for private section. This is because right now there are no C++ members in the private. There is only one GtkIMContext* only in private structure. My thinking was when we add any C++ members to private we can move to that placement constructor syntax (as in WebKit1).
> 
> >> WebKit2/UIProcess/gtk/WebViewInternal.cpp:126
> >> +GType
> >> +webkitWidgetViewGetType(void)
> > 
> > Use G_DEFINE_TYPE macro instead
> 
> This should just be one line. Leave out the 'void' for the empty argument list too, please.
Done.
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.cpp:130
> > +    static GType type = 0; 
> > +
> > +    if (!type) {
> 
> Use an early return here.
Done.
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.cpp:144
> > +        static const GTypeInfo info = {
> > +            sizeof(WebKitWidgetViewClass),      /* class structure size */
> > +            0,                                  /* base_init */
> > +            0,                                  /* base_finalize */
> > +            (GClassInitFunc)webkitWidgetViewClassInit, /* class initializer */
> > +            0,                                  /* class_finalize */
> > +            0,                                  /* class_data */
> > +            sizeof(WebKitWidgetView),           /* instance structure size */
> > +            0,                                  /* preallocated instances */
> > +            (GInstanceInitFunc)webkitWidgetViewInit,   /* instance initializer */
> > +            0                                   /* function table */
> > +        };   
> > +        type = g_type_register_static(GTK_TYPE_CONTAINER, "WebKitWidgetView", &info, (GTypeFlags)0);
> 
> Please use static_casts here (or reinterpret_casts where appropriate) instead of a c-style casts.
Done.
> 
> > WebKit2/UIProcess/gtk/WebView.cpp:47
> > +    gpointer webViewInstance = widgetView->priv->webViewInstance;
> > +    WebView* webView = reinterpret_cast<WebView*>(webViewInstance);
> 
> I think it's a bad idea to use reinterpret_cast here to get the WebView instance. Just store the pointer as a WebView* and make the private section actually private.
Done.
> 
> > WebKit2/UIProcess/gtk/WebView.cpp:50
> 
> Instead of getting the entire clipbox for the region, wouldn't it make sense to break it up into rectangles and paint them separately. Is that possible? Take a look at the expose event in webkitwebview.cpp. In any case, I think you should pass an IntRect to this method.
Done. Passing IntRect now. Regarding painting them separately, actually, WebProcess itself takes care of giving small rects for painting to UIProcess. If the rects needs to be coalesced, WebProcess itself coaletaes all such rects and sends the final rects to UIProcess to paint. UIProcess always just blindly paints the rects given to paint by WebProcess by calling gdk_window_invalidate_rect() on the rect given by WebProcess.
> 
> > WebKit2/UIProcess/gtk/WebView.cpp:59
> > +    GObjectClass* parentClass = (GObjectClass*)g_type_class_peek_parent(WEBKIT_WIDGET_VIEW_GET_CLASS(widget));
> > +    GTK_WIDGET_CLASS(parentClass)->size_allocate(widget, allocation);
> > +    WebKitWidgetView* widgetView = WEBKIT_WIDGET_VIEW(widget);
> 
> Can't you just do widgetView->parentClass?
Done.
> 
> > WebKit2/UIProcess/gtk/WebView.cpp:61
> > +    gpointer webViewInstance = widgetView->priv->webViewInstance;
> > +    WebView* webView = reinterpret_cast<WebView*>(webViewInstance);
> 
> Same comment as above.
Done.
> 
> > WebKit2/UIProcess/gtk/WebView.cpp:66
> > +gboolean WebView::webViewFocusInEvent(GtkWidget* widget, GdkEventFocus* event)
> 
> Same comments as above in this method.
Done.
> 
> > WebKit2/UIProcess/gtk/WebView.cpp:86
> > +gboolean WebView::webViewFocusOutEvent(GtkWidget* widget, GdkEventFocus* event)
> 
> Ditto.
Done.
> 
> > WebKit2/UIProcess/gtk/WebView.cpp:102
> > +gboolean WebView::webViewKeyPressEvent(GtkWidget* widget, GdkEventKey* event)
> 
> Ditto.
Done.
> 
> > WebKit2/UIProcess/gtk/WebView.cpp:113
> > +gboolean WebView::webViewKeyReleaseEvent(GtkWidget* widget, GdkEventKey* event)
> 
> Ditto.
Done.
> 
> > WebKit2/UIProcess/gtk/WebView.cpp:127
> > +gboolean WebView::webViewButtonPressEvent(GtkWidget* widget, GdkEventButton* const event)
> 
> Ditto.
Done.
> 
> > WebKit2/UIProcess/gtk/WebView.cpp:138
> > +gboolean WebView::webViewButtonReleaseEvent(GtkWidget* widget, GdkEventButton* event)
> 
> Ditto.
Done.
> 
> > WebKit2/UIProcess/gtk/WebView.cpp:169
> > +WebView::WebView(GdkRectangle rect, WebContext* context, WebPageGroup* pageGroup, GtkWidget*  hostWindow)
> 
> You should use a more descriptive parameter name than just "rect." I can't tell from this patch what it represents. There's an extra space after the final asterisk.
Done.
> 
> > WebKit2/UIProcess/gtk/WebView.cpp:179
> > +    m_window = (GtkWidget*)g_object_new(WEBKIT_TYPE_WIDGET_VIEW, NULL); 
> 
> Use a static_cast here please.
Done.
> 
> > WebKit2/UIProcess/gtk/WebView.cpp:182
> > +    WebKitWidgetView* webView = WEBKIT_WIDGET_VIEW(m_window);
> > +    webView->priv->webViewInstance = reinterpret_cast<gpointer>(this);
> 
> Here you can just do WEBKIT_WIDGET_VIEW(m_window)->priv->webViewInstance = this;
Done.
> 
> > WebKit2/UIProcess/gtk/WebView.cpp:206
> > +void WebView::onSetFocusEvent(GtkWidget*, bool)
> 
> What's the use of the second parameter? It's never used or named.
Done.
> 
> > WebKit2/UIProcess/gtk/WebView.cpp:211
> > +void WebView::onKillFocusEvent(GtkWidget*, bool)
> 
> Ditto.
Done.
> 
> > WebKit2/UIProcess/gtk/WebView.cpp:224
> > +    m_page->handleMouseEvent(mouseEvent);
> 
> This can just be:
> m_page->handleMouseEvent(WebEventFactory::createWebMouseEvent(event));
Done.
> 
> > WebKit2/UIProcess/gtk/WebView.cpp:230
> > +    WebWheelEvent wheelEvent = WebEventFactory::createWebWheelEvent(event);
> > +    m_page->handleWheelEvent(wheelEvent);
> 
> Same comment here.
Done.
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.h:46
> 
> 
> Please don't store this anonymously.
Done.
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.h:47
> > +    GtkIMContext *imContext;
> 
> The asterisk is in the wrong position here.
Done.
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.h:54
> > +    WebKitWidgetViewPrivate *priv;
> 
> Ditto.
Done.
> 
> > WebKit2/UIProcess/gtk/WebViewInternal.h:62
> > +GType
> > +webkitWidgetViewGetType(void);
> 
> This should be one line.
Done.
> 
> > WebKit2/UIProcess/gtk/WebView.h:112
> > +    GdkRectangle m_rect;
> 
> I'd really prefer a more descriptive name for this member. What does the rect actually represent?
Done.
Comment 18 Carlos Garcia Campos 2011-01-14 09:29:13 PST
(In reply to comment #16) 
> > 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.

I disagree, but at least you could use the current implementation of the G_DEFINE_TYPE macro, using the webkit coding style it would be something like:

GType webkitWebViewGetType()
{
    static volatile gsize gDefineTypeIdVolatile = 0;
    if (g_once_init_enter(&gDefineTypeIdVolatile)) {
        GType gDefineTypeId = g_type_register_static_simple(GTK_TYPE_CONTAINER,
                                                                                            g_intern_static_string("WebKitWebView"),
                                                                                            sizeof(WebKitWebViewClass),
                                                                                            reinterpret_cast<GClassInitFunc>(webkitWebViewClassInit),
                                                                                            sizeof(WebKitWebView),
                                                                                            reinterpret_cast<GInstanceInitFunc>(webkitWebViewInit),
                                                                                            static_cast<GTypeFlags>(0));
        g_once_init_leave(&gDefineTypeIdVolatile, gDefineTypeId);
    }
    return gDefineTypeIdVolatile;
}

> > 
> > > 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).
Comment 19 Martin Robinson 2011-01-14 09:30:49 PST
Comment on attachment 78945 [details]
WebView & WebKitWebView implementation with review comments incorporation

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.
Comment 20 Martin Robinson 2011-01-14 10:04:53 PST
Comment on attachment 78232 [details]
UpdateChunk, ChunkedUpdateDrawingArea implementation for GTK port

View in context: https://bugs.webkit.org/attachment.cgi?id=78232&action=review

r- because this should almost certainly use cairo surfaces instead of GdkPixmaps.

> WebKit2/UIProcess/ChunkedUpdateDrawingAreaProxy.h:28
> -#ifndef DrawingAreaProxyUpdateChunk_h
> -#define DrawingAreaProxyUpdateChunk_h
> +#ifndef ChunkedUpdateDrawingAreaProxy_h
> +#define ChunkedUpdateDrawingAreaProxy_h

This is a good change, but I think it can go in a separate cleanup patch.
Comment 21 Martin Robinson 2011-01-14 10:06:11 PST
Comment on attachment 78234 [details]
Changes to WKAPI interfaces for GTK port

View in context: https://bugs.webkit.org/attachment.cgi?id=78234&action=review

> WebKit2/UIProcess/API/C/gtk/WKView.h:37
> +WK_EXPORT WKViewRef WKViewCreate(GdkRectangle rect, WKContextRef context, WKPageGroupRef pageGroup, GtkWidget * hostWindow);

Since the GdkRectangle parameter is currently unused, I'd say just leave it out for now. You have an extra space after GtkWidget.
Comment 22 Martin Robinson 2011-01-14 10:17:23 PST
Comment on attachment 78945 [details]
WebView & WebKitWebView implementation with review comments incorporation

View in context: https://bugs.webkit.org/attachment.cgi?id=78945&action=review

> WebKit2/UIProcess/gtk/WebView.cpp:73
> +    GtkWidget* toplevel = gtk_widget_get_toplevel(widget);
> +    if (GTK_WIDGET_TOPLEVEL(toplevel) && gtk_window_has_toplevel_focus(GTK_WINDOW(toplevel))) {

Is it really necessary to check GTK_WIDGET_TOPLEVEL(...) on the return value of gtk_widget_get_toplevel(...)?

> WebKit2/UIProcess/gtk/WebView.cpp:79
> +        if (!(webView->m_isPageActive)) {
> +            webView->m_isPageActive = true;
> +            webView->updateActiveState();
> +        }
> +        webView->onSetFocusEvent(widget);

This logic is all in terms of internal WebView members. I think it makes sense to just move this into one method within WebView, perhaps WebView::handleFocusInEvent.

> WebKit2/UIProcess/gtk/WebView.cpp:92
> +    webView->onKillFocusEvent(widget);
> +    webView->m_isPageActive = false; 
> +    webView->updateActiveState();

This series of actions belongs inside one WebView method, perhaps WebView::handleFocusOutEvent.

> WebKit2/UIProcess/gtk/WebView.cpp:154
> +WebView::WebView(GdkRectangle windowRect, WebContext* context, WebPageGroup* pageGroup, GtkWidget* hostWindow)

I think you should remove the windowRect and hostWindow parameters here. They are both unused and it probably makes more sense to look them up when you need them. For instance, the host window of a widget may change, etc.

> WebKit2/UIProcess/gtk/WebView.cpp:229
> +void WebView::updateActiveState()
> +{
> +    m_page->setActive(isActive());
> +}

I'd prefer you to remove this method and just call ::setActive directly. It's clearer. I think it makes sense to make the onKillFocusEvent and onSetFocusEvent smarter rather than having all the logic in the GObject.

> WebKit2/UIProcess/gtk/WebView.h:84
> +    void onPaintEvent(GtkWidget*, WebCore::IntRect);
> +    void onSizeEvent(GtkWidget*, WebCore::IntSize);
> +    void onSetFocusEvent(GtkWidget*);
> +    void onKillFocusEvent(GtkWidget*);
> +    void onKeyEvent(GdkEventKey*);
> +    void onMouseEvent(GdkEventAny*);
> +    void onWheelEvent(GdkEventScroll*);

Sorry to bikeshed here, but I'd prefer these names to begin with verbs. For instance:
onPaintEvent(...) -> paint(...) or handlePaintEvent(...)
onSizeEvent(...) -> resisize(...) or handleSizeEvent(...)
etc.

>> WebKit2/UIProcess/gtk/WebView.h:118

> 
> 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.

I think this should be removed for  now, since it's unused.

> WebKit2/UIProcess/gtk/WebView.h:119


This might be more accurately called m_viewWidget.

> WebKit2/UIProcess/gtk/WebView.h:120
> +    GtkWidget* m_hostWindow;

I recommend removing this for now since it's unused.
Comment 23 Alejandro G. Castro 2011-02-09 01:32:15 PST
I'm going to create a bug per patch to simplify the review.
Comment 24 Xan Lopez 2011-02-09 02:48:51 PST
(In reply to comment #22)
> (From update of attachment 78945 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78945&action=review
> 
> > WebKit2/UIProcess/gtk/WebView.cpp:73
> > +    GtkWidget* toplevel = gtk_widget_get_toplevel(widget);
> > +    if (GTK_WIDGET_TOPLEVEL(toplevel) && gtk_window_has_toplevel_focus(GTK_WINDOW(toplevel))) {
> 
> Is it really necessary to check GTK_WIDGET_TOPLEVEL(...) on the return value of gtk_widget_get_toplevel(...)?
> 

It is recommended, yes, check the documentation of the function.
Comment 25 Alejandro G. Castro 2011-02-09 06:30:15 PST
Created attachment 81797 [details]
Proposed patch

We are not using currently the widget as proper API but if we want to create a gtk API eventually we will have to move it to the API/gtk directory and remove the webview dependency from its API. We can leave that for future patches though. I'm still reviewing criticals in the webprocess and the other patches but I think makes sense to upload for review so we can do both things at the same time.
Comment 26 Alejandro G. Castro 2011-02-10 05:50:12 PST
Created attachment 81961 [details]
Proposed patch

I've removed unuseful function pointed out by Carlos, and talked about the API decisions, we can start with this API for this initial patches and start later different work.

The criticals allow the rendering and browsing but they are things we have to implement different in webcore to avoid access to the widget, we will solve that in following bugs and patches.
Comment 27 Martin Robinson 2011-02-10 09:49:31 PST
Comment on attachment 81961 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81961&action=review

> Source/WebKit2/UIProcess/gtk/WebKitWebView.cpp:41
> +    gboolean disposeHasRun;

disposeRan?

> Source/WebKit2/UIProcess/gtk/WebKitWebView.cpp:50
> +    guint borderWidth = gtk_container_get_border_width(GTK_CONTAINER(widget));
> +    gtk_widget_get_allocation(widget, &allocation);

Does it make sense for a WebView to have a border, since it isn't a container in the traditional sense?

> Source/WebKit2/UIProcess/gtk/WebKitWebView.cpp:206
> +    webView->keyPress(event);

Please have method names that use verbs. handleKeyboardEvent. keyPress is a misnomer anyway, since this is a release event.

> Source/WebKit2/UIProcess/gtk/WebKitWebView.cpp:215
> +    webView->mouseActivate(reinterpret_cast<GdkEventAny*>(event));

Ditto. handleMouseEvent.

> Source/WebKit2/UIProcess/gtk/WebKitWebView.cpp:224
> +    webView->mouseActivate(reinterpret_cast<GdkEventAny*>(event));

Ditto. handleMouseEvent.

> Source/WebKit2/UIProcess/gtk/WebKitWebView.cpp:232
> +    webView->scroll(event);

Ditto. handleWheelEvent.

> Source/WebKit2/UIProcess/gtk/WebKitWebView.cpp:240
> +    webView->mouseActivate(reinterpret_cast<GdkEventAny*>(event));

Ditto. handleMouseEvent.

> Source/WebKit2/UIProcess/gtk/WebKitWebView.h:40
> +#define WEBKIT_WEB_VIEW(obj)                (G_TYPE_CHECK_INSTANCE_CAST((obj), WEBKIT_TYPE_WEB_VIEW, WebKitWebView))

Please don't abbreviate here since these are in WebKit style.

> Source/WebKit2/UIProcess/gtk/WebView.cpp:144
> +    return IntRect(m_viewRect).size();

Wouldn't it make sense to just ask the widget for its allocation here?
Comment 28 Carlos Garcia Campos 2011-02-10 10:25:33 PST
(In reply to comment #27)
> (From update of attachment 81961 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81961&action=review

> > Source/WebKit2/UIProcess/gtk/WebKitWebView.cpp:50
> > +    guint borderWidth = gtk_container_get_border_width(GTK_CONTAINER(widget));
> > +    gtk_widget_get_allocation(widget, &allocation);
> 
> Does it make sense for a WebView to have a border, since it isn't a container in the traditional sense?

you can do gtk_container_set_border_width (GTK_CONTAINER (web_view), 5);
Comment 29 Martin Robinson 2011-02-10 10:34:05 PST
(In reply to comment #28)

> you can do gtk_container_set_border_width (GTK_CONTAINER (web_view), 5);

It's true, but the WebView doesn't contain widgets that you pack into it manually. It's only a container because it contains flash plugin windows and embedded objects.
Comment 30 Alejandro G. Castro 2011-02-11 09:16:53 PST
Thanks for the review.

(In reply to comment #27)
> (From update of attachment 81961 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81961&action=review
> 
> > Source/WebKit2/UIProcess/gtk/WebKitWebView.cpp:41
> > +    gboolean disposeHasRun;
> 
> disposeRan?
> 

Actually now that you point this out, I think I'm going to change this to the usual check of each variable in the gtkwidgets for the case were dispose is called multiple times.

> > Source/WebKit2/UIProcess/gtk/WebKitWebView.cpp:50
> > +    guint borderWidth = gtk_container_get_border_width(GTK_CONTAINER(widget));
> > +    gtk_widget_get_allocation(widget, &allocation);
> 
> Does it make sense for a WebView to have a border, since it isn't a container in the traditional sense?
>

I agree, we do not need it.

> > Source/WebKit2/UIProcess/gtk/WebView.cpp:144
> > +    return IntRect(m_viewRect).size();
> 
> Wouldn't it make sense to just ask the widget for its allocation here?

Yep, I think you are right, I had this in my list but I forgot to check it because it was the only reason to keep the gdkrectangle.
Comment 31 Alejandro G. Castro 2011-02-11 11:03:20 PST
Created attachment 82145 [details]
Proposed patch

Fixed all the issues pointed out, and removed the gdkrectangle, we are not using it anymore. Webkit2 here we come! :)
Comment 32 Alejandro G. Castro 2011-02-15 08:54:54 PST
Created attachment 82466 [details]
Proposed patch

Updated the patch with new additions to the API.
Comment 33 Alejandro G. Castro 2011-02-22 01:38:50 PST
Created attachment 83285 [details]
Proposed patch

Patch updated, added the clickCount code similar to the code used in webkit1. I left one style issue, using NULL instead a 0 because we are calling a glib API and we had issues using 0 in these situation IIRC.
Comment 34 WebKit Review Bot 2011-02-22 01:39:58 PST
Attachment 83285 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/gtk/WebKitWebView.cpp:261:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 35 Martin Robinson 2011-02-24 10:32:23 PST
Comment on attachment 83285 [details]
Proposed patch

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.
Comment 36 Alejandro G. Castro 2011-02-25 03:15:26 PST
(In reply to comment #35)
> (From update of attachment 83285 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83285&action=review
> 
> Looks good, but please make the following changes.
> 

Thanks for the review :).

> > 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.
> 

Added bug:

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

> > 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.
>

I'm going to change it to use them.

> > 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.

All done, thanks again, I'll try to land it.
Comment 37 Alejandro G. Castro 2011-02-25 03:46:04 PST
Landed http://trac.webkit.org/changeset/79671.

Thanks everybody for the help :).