[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 #22 from Martin Robinson <mrobinson at webkit.org>  2011-01-14 10:17:23 PST ---
(From update of attachment 78945)
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.

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