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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 14 09:19:45 PST 2011 

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





--- Comment #17 from Ravi Phaneendra Kasibhatla <ravi.kasibhatla at motorola.com>  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.

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

More information about the webkit-unassigned mailing list