Bug 59655 - GTK port of WebKit2 should switch to new DrawingAreaImpl model
Summary: GTK port of WebKit2 should switch to new DrawingAreaImpl model
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords:
Depends on: 60293
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-27 16:40 PDT by Sam Weinig
Modified: 2011-05-16 15:08 PDT (History)
4 users (show)

See Also:


Attachments
Patch (26.55 KB, patch)
2011-05-11 16:37 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (27.15 KB, patch)
2011-05-12 10:32 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch without scrolling artifacts (27.75 KB, patch)
2011-05-13 09:06 PDT, Martin Robinson
andersca: review+
Details | Formatted Diff | Diff
Patch which also implements PageClientImpl::scrollView (27.94 KB, patch)
2011-05-13 10:44 PDT, Martin Robinson
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2011-04-27 16:40:13 PDT
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.
Comment 1 Martin Robinson 2011-05-11 16:37:26 PDT
Created attachment 93211 [details]
Patch
Comment 2 Carlos Garcia Campos 2011-05-11 23:47:51 PDT
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.
Comment 3 Anders Carlsson 2011-05-12 08:30:07 PDT
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.
Comment 4 Martin Robinson 2011-05-12 10:26:59 PDT
(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.
Comment 5 Martin Robinson 2011-05-12 10:28:11 PDT
(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!
Comment 6 Martin Robinson 2011-05-12 10:32:13 PDT
Created attachment 93301 [details]
Patch
Comment 7 Martin Robinson 2011-05-12 12:41:16 PDT
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.
Comment 8 Martin Robinson 2011-05-13 09:06:33 PDT
Created attachment 93458 [details]
Patch without scrolling artifacts
Comment 9 Anders Carlsson 2011-05-13 10:12:15 PDT
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.
Comment 10 Martin Robinson 2011-05-13 10:44:50 PDT
Created attachment 93481 [details]
Patch which also implements PageClientImpl::scrollView
Comment 11 Martin Robinson 2011-05-13 10:47:21 PDT
(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.
Comment 12 Martin Robinson 2011-05-16 15:08:06 PDT
Committed r86612: <http://trac.webkit.org/changeset/86612>