Bug 90085 - [GTK] Using a native window for the WebView breaks GtkOverlay
Summary: [GTK] Using a native window for the WebView breaks GtkOverlay
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords:
Depends on: 94678
Blocks: 94417
  Show dependency treegraph
 
Reported: 2012-06-27 11:07 PDT by Carlos Garcia Campos
Modified: 2012-10-09 10:10 PDT (History)
11 users (show)

See Also:


Attachments
Patch (44.41 KB, patch)
2012-07-06 11:12 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Add xcomposite pkg-config parsing to configuration, fix some assertions, and improve performance on the 2012 beercamp demo (47.40 KB, patch)
2012-07-13 14:59 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (47.38 KB, patch)
2012-08-10 10:21 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch fixing issues with scrolling (54.60 KB, patch)
2012-08-17 10:49 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (54.55 KB, patch)
2012-08-19 23:38 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (54.58 KB, patch)
2012-08-20 07:46 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (54.49 KB, patch)
2012-08-21 08:20 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (51.71 KB, patch)
2012-08-21 11:33 PDT, Martin Robinson
alex: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2012-06-27 11:07:44 PDT
Which is the case of epiphany, the child widgets of the GtkOverlay are not shown when webkit is built with AC enabled. The problem happens with WebKit1 and WebKit2, and I've checked that it doesn't happen when building without AC (I used --disable-accelerated-compositing --disable-webgl --with-acceleration-backend=none configure options, it needs a patch to build with those flags, though). 
I think we should fix the build with AC disabled, and disable it by default until the problem is fixed beause it breaks important basic features in epiphany (loading progress bar, hovered link messages and fullscreen message box)
Comment 1 Carlos Garcia Campos 2012-06-27 11:10:51 PDT
Btw, the OSD style class is relevant because it uses a transparent background for the GtkOverlay, so it's a problem with transparencies.
Comment 2 Martin Robinson 2012-06-27 17:55:19 PDT
A quick update debugging this issue. It seems that the root of the problem is that GtkOverlay does not react well when the background widget has its own native window. This feels like a bug in GTK+, but I'm still digging for the cause.

What we know so far is that GtkOverlay creates child windows for all its overlaid widgets and calls gtk_widget_set_parent_window on them. I'm going to try to explore whether or not the overlaid widget windows are just hiding behind the WebView window for some reason.
Comment 3 Carlos Garcia Campos 2012-06-27 23:08:43 PDT
I tried lowering and raising the child windows in the z-order and it didn't work. Note that it works even with AC enabled if the overlay uses an opaque background color. It seems the problem has to do with the way things are composed.
Comment 4 Martin Robinson 2012-07-01 16:32:06 PDT
Another update: I haven't spent any time looking at the GTK+ bug, but I've successfully eliminated the need to have a native window for accelerated compositing support using XComposite to redirect a window a pixmap. This approach has quite a few other benefits as well such as allowing us to do pixel tests with accelerated compositing and 3D CSS transforms.

I have the patch cooking locally, but I hope to post it early next week. If there's a release coming up, I'm fine with disabling accelerated compositing for the moment. I wanted to delay turning it off in the case that the fix was simple.

Again I hope to post the patch within a few days though.
Comment 5 Carlos Garcia Campos 2012-07-01 23:58:54 PDT
(In reply to comment #4)
> Another update: I haven't spent any time looking at the GTK+ bug, but I've successfully eliminated the need to have a native window for accelerated compositing support using XComposite to redirect a window a pixmap. This approach has quite a few other benefits as well such as allowing us to do pixel tests with accelerated compositing and 3D CSS transforms.

Great news.

> I have the patch cooking locally, but I hope to post it early next week. If there's a release coming up, I'm fine with disabling accelerated compositing for the moment. I wanted to delay turning it off in the case that the fix was simple.

Sure.

> Again I hope to post the patch within a few days though.
Comment 6 Martin Robinson 2012-07-06 11:12:58 PDT
Created attachment 151106 [details]
Patch
Comment 7 Martin Robinson 2012-07-13 14:59:37 PDT
Created attachment 152344 [details]
Add xcomposite pkg-config parsing to configuration, fix some assertions, and improve performance on the 2012 beercamp demo
Comment 8 Martin Robinson 2012-08-10 10:21:06 PDT
Created attachment 157755 [details]
Patch
Comment 9 Martin Robinson 2012-08-17 10:49:45 PDT
Created attachment 159158 [details]
Patch fixing issues with scrolling
Comment 10 Carlos Garcia Campos 2012-08-19 03:32:43 PDT
Comment on attachment 159158 [details]
Patch fixing issues with scrolling

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

> Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:103
> +    cleanupPixmapAndPixmapSurface();

You should probably remove the source if m_pendingResizeSourceId > 0 to avoid the callback could be called on a destroyed object.

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:115
> +bool AcceleratedCompositingContext::renderLayersToWindow(cairo_t* cr, const IntRect& clipRect)

clipRect is passed but it doesn't seem to be used, is it really needed?

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:135
> +
> +

Extra line here?

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:228
> +    if (m_layerFlushTimerCallbackId) {
> +        g_source_remove(m_layerFlushTimerCallbackId);
> +        m_layerFlushTimerCallbackId = 0;
> +    }

This is done 3 times, maybe you could add a helper clearLayerFlushTimerSource() or seomthing similar.

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:321
> +    {
> +        Frame* frame = core(m_webView)->mainFrame();
> +        if (!frame || !frame->contentRenderer() || !frame->view())
> +            return;
> +        frame->view()->updateLayoutAndStyleIfNeededRecursive();
> +    }

why the braces?

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:1010
> +    IntRect webViewRect = getWebViewRect(m_webView);
> +    if (turningOnCompositing) {
> +        m_displayTimer.stop();
> +        m_webView->priv->backingStore = WebCore::WidgetBackingStore::create(GTK_WIDGET(m_webView), IntSize(1, 1));
> +    }
> +
> +    if (turningOffCompositing) {
> +        m_webView->priv->backingStore = WebCore::WidgetBackingStore::create(GTK_WIDGET(m_webView), webViewRect.size());
> +        RefPtr<cairo_t> cr = adoptRef(cairo_create(m_webView->priv->backingStore->cairoSurface()));
> +        clearEverywhereInBackingStore(m_webView, cr.get());
> +    }

webViewRect is only used when turning compositing off, so it could be moved here and maybe don't use a local variable:

m_webView->priv->backingStore = WebCore::WidgetBackingStore::create(GTK_WIDGET(m_webView), getWebViewRect().size());
Comment 11 Martin Robinson 2012-08-19 23:34:41 PDT
(In reply to comment #10)

Thanks for the review!

> (From update of attachment 159158 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159158&action=review
> 
> > Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:103
> > +    cleanupPixmapAndPixmapSurface();
> 
> You should probably remove the source if m_pendingResizeSourceId > 0 to avoid the callback could be called on a destroyed object.

Okay.

> 
> > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:115
> > +bool AcceleratedCompositingContext::renderLayersToWindow(cairo_t* cr, const IntRect& clipRect)
> 
> clipRect is passed but it doesn't seem to be used, is it really needed?

It can't be removed, because it might be used by the other implementations. In fact, I'll use it here to speed up painting AC results after damage events.
> 
> > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:135
> > +
> > +
> 
> Extra line here?

Removed.


> > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:228
> > +    if (m_layerFlushTimerCallbackId) {
> > +        g_source_remove(m_layerFlushTimerCallbackId);
> > +        m_layerFlushTimerCallbackId = 0;
> > +    }
> 
> This is done 3 times, maybe you could add a helper clearLayerFlushTimerSource() or seomthing similar.

Okay.

> > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:321
> > +    {
> > +        Frame* frame = core(m_webView)->mainFrame();
> > +        if (!frame || !frame->contentRenderer() || !frame->view())
> > +            return;
> > +        frame->view()->updateLayoutAndStyleIfNeededRecursive();
> > +    }
> 
> why the braces?

There was a smart pointer in an older implementation. I've removed them.

> webViewRect is only used when turning compositing off, so it could be moved here and maybe don't use a local variable:

Sounds good.
Comment 12 Martin Robinson 2012-08-19 23:38:20 PDT
Created attachment 159341 [details]
Patch
Comment 13 Carlos Garcia Campos 2012-08-20 00:20:16 PDT
Comment on attachment 159341 [details]
Patch

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

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:-60
> -    if (m_syncTimerCallbackId)
> -        g_source_remove(m_syncTimerCallbackId);

This should be now stopAnyPendingLayerFlush();
Comment 14 Martin Robinson 2012-08-20 07:46:45 PDT
Created attachment 159426 [details]
Patch
Comment 15 Carlos Garcia Campos 2012-08-21 06:50:30 PDT
Comment on attachment 159426 [details]
Patch

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

> Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:108
> +    static int framebufferAttributes[] = {
> +        GLX_DRAWABLE_TYPE, GLX_WINDOW_BIT,
> +        GLX_DOUBLEBUFFER, bufferingType == DoubleBuffer ? True : False,
> +        GLX_RENDER_TYPE, GLX_RGBA_BIT,
> +        GLX_RED_SIZE, 1,
> +        GLX_GREEN_SIZE, 1,
> +        GLX_BLUE_SIZE, 1,
> +        GLX_STENCIL_SIZE, 1,
> +        None
> +    };

if this is static, GLX_DOUBLEBUFFER would only be initialized the first time with whatever bufferingType is, no?

> Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:114
> +    if (!numberReturned || !configs)
> +        return nullptr;

is configs leaked here if !numberReturned? or glXChooseFBConfig never sets numberReturned=0?
Comment 16 Martin Robinson 2012-08-21 07:44:33 PDT
(In reply to comment #15)
> (From update of attachment 159426 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159426&action=review
> 
> > Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:108
> > +    static int framebufferAttributes[] = {
> > +        GLX_DRAWABLE_TYPE, GLX_WINDOW_BIT,
> > +        GLX_DOUBLEBUFFER, bufferingType == DoubleBuffer ? True : False,
> > +        GLX_RENDER_TYPE, GLX_RGBA_BIT,
> > +        GLX_RED_SIZE, 1,
> > +        GLX_GREEN_SIZE, 1,
> > +        GLX_BLUE_SIZE, 1,
> > +        GLX_STENCIL_SIZE, 1,
> > +        None
> > +    };
> 
> if this is static, GLX_DOUBLEBUFFER would only be initialized the first time with whatever bufferingType is, no?

Nice catch! This isn't a huge deal since there's no double-buffered GLContexts at the moment, but I've fixed this.

> > Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:114
> > +    if (!numberReturned || !configs)
> > +        return nullptr;
> 
> is configs leaked here if !numberReturned? or glXChooseFBConfig never sets numberReturned=0?

I believe that when numberReturned == 0, configs will always be null.
Comment 17 Martin Robinson 2012-08-21 08:20:40 PDT
Created attachment 159694 [details]
Patch
Comment 18 Martin Robinson 2012-08-21 11:33:08 PDT
Created attachment 159728 [details]
Patch
Comment 19 Alejandro G. Castro 2012-08-21 12:45:38 PDT
Comment on attachment 159728 [details]
Patch

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

Great patch, LGTM. This seems the way to go.

> Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:66
> +    m_parentWindow = XCreateWindow(display,
> +                                   RootWindow(display, DefaultScreen(display)),
> +                                    -100, -100, 1, 1,
> +                                    0,
> +                                    CopyFromParent,
> +                                    InputOutput,
> +                                    CopyFromParent,
> +                                    CWOverrideRedirect,
> +                                    &windowAttributes);

Indent typo.

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContext.h:-81
> -#if USE(TEXTURE_MAPPER_GL)
>      WebCore::GLContext* glContext();
>      OwnPtr<WebCore::GLContext> m_context;
>  #endif
> -#endif

Why do you remove this #if?

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:193
> +    // called. For non-animation situations we use this terrible hack until we can get to the

Trailing whitespace in horrible hack comment ;).

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:216
> +    // to update. This isn't a problem during animations, because swapBuffer is continuously 

Ditto.

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:348
> +    //    return;

I guess this is a leftover.
Comment 20 Martin Robinson 2012-08-21 13:28:48 PDT
Committed r126182: <http://trac.webkit.org/changeset/126182>
Comment 21 Martin Robinson 2012-08-21 13:30:37 PDT
(In reply to comment #19)

Thanks for the review!

> > Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:66
> > +    m_parentWindow = XCreateWindow(display,
> > +                                   RootWindow(display, DefaultScreen(display)),
> > +                                    -100, -100, 1, 1,
> > +                                    0,
> > +                                    CopyFromParent,
> > +                                    InputOutput,
> > +                                    CopyFromParent,
> > +                                    CWOverrideRedirect,
> > +                                    &windowAttributes);
> 
> Indent typo.

Fixed.

> > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContext.h:-81
> > -#if USE(TEXTURE_MAPPER_GL)
> >      WebCore::GLContext* glContext();
> >      OwnPtr<WebCore::GLContext> m_context;
> >  #endif
> > -#endif
> 
> Why do you remove this #if?

I actually meant to remove these entirely. I've done that.

> > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:193
> > +    // called. For non-animation situations we use this terrible hack until we can get to the
> 
> Trailing whitespace in horrible hack comment ;).

Fixed.


> > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:348
> > +    //    return;
> 
> I guess this is a leftover.

I meant this to be

if (!flush....)
   return;
Comment 22 WebKit Review Bot 2012-08-22 01:21:53 PDT
Re-opened since this is blocked by 94678
Comment 23 Daniel Drake 2012-10-09 10:10:58 PDT
r126182 a regression in plugin display, filed as bug #98789.