Bug 59655

Created attachment 93211 [details] Patch

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

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

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

Created attachment 93301 [details] Patch

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.

Created attachment 93458 [details] Patch without scrolling artifacts

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.

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

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

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