GTK port of WebKit2 should switch to new DrawingAreaImpl model

• Component: WebKit2
• Status: RESOLVED FIXED

[Sam Weinig]

The GTK port is the only port left using the ChunkedUpdate model for DrawingAreas.  I would like to remove the ChunkedUpdate model soon, but getting the GTK port off it is a blocker.  The new model is quite a bit simpler from a porting perspective, but has a lot more functionality.  The Qt guys made the change recently in http://trac.webkit.org/changeset/84613.

The main things you have to do in order to make the change are:
- Fill out an implementation of BackingStore.
- Update your native views painting routine to call the new DrawingArea's paint function.
- Add some stubs for accelerated compositing.

[Martin Robinson]

Created attachment 93211 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 93211 [details]
Patch

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

Great!

> Source/WebKit2/UIProcess/gtk/BackingStoreGtk.cpp:43
> +    cairo_set_operator(context, CAIRO_OPERATOR_SOURCE);

Why using the source operator?

> Source/WebKit2/UIProcess/gtk/BackingStoreGtk.cpp:82
> +#ifdef GTK_API_VERSION_2
> +    GdkRegion* moveRegion = gdk_region_rectangle(&moveRect);
> +#else
> +    cairo_region_t* moveRegion = cairo_region_create_rectangle(&moveRect);
> +#endif

Region is leaked. I don't think we have an OwnPtr for cairo_region_t nor GdkRegion, so we should either add them and use OwnPtr here or call cairo_region_destroy()/gdk_region_destroy() after gdk_window_move_region()

> Source/WebKit2/UIProcess/gtk/BackingStoreGtk.cpp:85
> +    gdk_window_move_region(gtk_widget_get_window(m_webPageProxy->viewWidget()), moveRegion,
> +                           scrollOffset.width(), scrollOffset.height());

Maybe we should call gdk_window_process_updates() here so that expose events are sent synchronously.

[Anders Carlsson]

Comment on attachment 93211 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:154
> +    WebKit::Region unpaintedRegion; // This is simply unused.
> +    static_cast<DrawingAreaProxyImpl*>(drawingArea)->paint(context, area, unpaintedRegion);

When you resize the widget and the web page doesn't have time to catch up, the unpainted region will contain the parts of the page that weren't painted from the backing store. On other platforms, we paint the unpainted region with white or transparent. Does GTK+ need to do this too?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1320
> -#if PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(QT)
> +#if PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(QT) || PLATFORM(GTK)

I think this covers all our supported platforms, so this #if can be removed.

> Source/WebKit2/WebProcess/WebPage/DrawingArea.cpp:48
> -#if PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(QT)
> +#if PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(QT) || PLATFORM(GTK)

I think this covers all our supported platforms, so this #if can be removed.

> Source/WebKit2/WebProcess/WebPage/DrawingArea.h:56
> -#if PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(QT)
> +#if PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(QT) || PLATFORM(GTK)

I think this covers all our supported platforms, so this #if can be removed.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1968
> -#if PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(QT)
> +#if PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(QT) || PLATFORM(GTK)

I think this covers all our supported platforms, so this #if can be removed.

[Martin Robinson]

(In reply to comment #2)

Thanks for the comments.

> (From update of attachment 93211 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93211&action=review

> > Source/WebKit2/UIProcess/gtk/BackingStoreGtk.cpp:43
> > +    cairo_set_operator(context, CAIRO_OPERATOR_SOURCE);
> Why using the source operator?

Certainly the over operator is incorrect when painting parts of the backing store?

> > Source/WebKit2/UIProcess/gtk/BackingStoreGtk.cpp:82
> > +#ifdef GTK_API_VERSION_2
> > +    GdkRegion* moveRegion = gdk_region_rectangle(&moveRect);
> > +#else
> > +    cairo_region_t* moveRegion = cairo_region_create_rectangle(&moveRect);
> > +#endif
> 
> Region is leaked. I don't think we have an OwnPtr for cairo_region_t nor GdkRegion, so we should either add them and use OwnPtr here or call cairo_region_destroy()/gdk_region_destroy() after gdk_window_move_region()

Nice catch. I'll just add an #ifdef'd free here and then fix the entire situation in a followup patch. I have an idea that I've been putting off for months.

> > Source/WebKit2/UIProcess/gtk/BackingStoreGtk.cpp:85
> > +    gdk_window_move_region(gtk_widget_get_window(m_webPageProxy->viewWidget()), moveRegion,
> > +                           scrollOffset.width(), scrollOffset.height());
> 
> Maybe we should call gdk_window_process_updates() here so that expose events are sent synchronously.

I'd like to keep the behavior asynchronous here to match other ports. If we run into trouble in the future, I'll be happy to explore doing the redraw synchronously.

[Martin Robinson]

(In reply to comment #3)
> (From update of attachment 93211 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93211&action=review

Thanks for the comments.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:154
> > +    WebKit::Region unpaintedRegion; // This is simply unused.
> > +    static_cast<DrawingAreaProxyImpl*>(drawingArea)->paint(context, area, unpaintedRegion);

> When you resize the widget and the web page doesn't have time to catch up, the unpainted region will contain the parts of the page that weren't painted from the backing store. On other platforms, we paint the unpainted region with white or transparent. Does GTK+ need to do this too?

Right now it seems the unpainted areas are painted black. Would it make sense to do a white repaint in a followup patch? The logic deciding whether or not the window is transparent might be tricky.

> > Source/WebKit2/UIProcess/WebPageProxy.cpp:1320
> > -#if PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(QT)
> > +#if PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(QT) || PLATFORM(GTK)
> 
> I think this covers all our supported platforms, so this #if can be removed.
> 
> > Source/WebKit2/WebProcess/WebPage/DrawingArea.cpp:48
> > -#if PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(QT)
> > +#if PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(QT) || PLATFORM(GTK)
> 
> I think this covers all our supported platforms, so this #if can be removed.
> 
> > Source/WebKit2/WebProcess/WebPage/DrawingArea.h:56
> > -#if PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(QT)
> > +#if PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(QT) || PLATFORM(GTK)
> 
> I think this covers all our supported platforms, so this #if can be removed.
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1968
> > -#if PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(QT)
> > +#if PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(QT) || PLATFORM(GTK)
> 
> I think this covers all our supported platforms, so this #if can be removed.

Fixed all of these!

[Martin Robinson]

Created attachment 93301 [details]
Patch

[Martin Robinson]

Comment on attachment 93301 [details]
Patch

I have discovered that with this version of my patch, it is possible for the backing store and the window to get out of sync leading to rendering errors. :( Will post a new patch soon.

[Martin Robinson]

Created attachment 93458 [details]
Patch without scrolling artifacts

[Anders Carlsson]

Comment on attachment 93458 [details]
Patch without scrolling artifacts

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

> Source/WebKit2/UIProcess/gtk/BackingStoreGtk.cpp:102
> +    GdkRectangle invalidatedRect = targetRect;
> +    gdk_window_invalidate_rect(gtk_widget_get_window(m_webPageProxy->viewWidget()),
> +                               &invalidatedRect, FALSE);
> +}

This is not needed. Instead, you should implement PageClientImpl::scrollView which will be called automatically after the backing store has been updated.

[Martin Robinson]

Created attachment 93481 [details]
Patch which also implements PageClientImpl::scrollView

[Martin Robinson]

(In reply to comment #9)
> (From update of attachment 93458 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93458&action=review
> 
> > Source/WebKit2/UIProcess/gtk/BackingStoreGtk.cpp:102
> > +    GdkRectangle invalidatedRect = targetRect;
> > +    gdk_window_invalidate_rect(gtk_widget_get_window(m_webPageProxy->viewWidget()),
> > +                               &invalidatedRect, FALSE);
> > +}
> 
> This is not needed. Instead, you should implement PageClientImpl::scrollView which will be called automatically after the backing store has been updated.

Thanks for the quick review. I've updated the patch to implement a version of PageClientImpl::scrollView that just calls into PageClientImpl::setViewNeedsDisplay like the Windows port.

[Martin Robinson]

Committed r86612: <http://trac.webkit.org/changeset/86612>