REGRESSION(r177075): Flickering when the WebView is realized

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

This is caused by the XReparentWindow instroduced in r177075. It's very easy to reproduce in Epiphany, when creating a new tab, you see like if the view was rendered in black for a while, causing a flickering effect. I think we should just use the web view visual and depth when creating the redirected x window instead.

[Carlos Garcia Campos]

Created attachment 244437 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Martin Robinson]

Comment on attachment 244437 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:323
> +        GdkVisual* visual = gdk_window_get_visual(parentWindow);
>          priv->redirectedWindow = RedirectedXCompositeWindow::create(
>              GDK_DISPLAY_XDISPLAY(display),
> -            parentWindow ? GDK_WINDOW_XID(parentWindow) : 0,
> -            IntSize(1, 1),
> +            GDK_VISUAL_XVISUAL(visual),
> +            gdk_visual_get_depth(visual),

You are no longer handling the null parentWindow case here?

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:131
> -std::unique_ptr<RedirectedXCompositeWindow> RedirectedXCompositeWindow::create(Display* display, Window parent, const IntSize& size, std::function<void()> damageNotify)
> +std::unique_ptr<RedirectedXCompositeWindow> RedirectedXCompositeWindow::create(Display* display, Visual* visual, int depth, std::function<void()> damageNotify)
>  {
> -    return supportsXDamageAndXComposite(display) ? std::unique_ptr<RedirectedXCompositeWindow>(new RedirectedXCompositeWindow(display, parent, size, damageNotify)) : nullptr;
> +    return supportsXDamageAndXComposite(display) ? std::unique_ptr<RedirectedXCompositeWindow>(new RedirectedXCompositeWindow(display, visual, depth, damageNotify)) : nullptr;

I think we don't need to change the arguments here. You can get a window's visual and depth using XGetWindowAttributes.

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:152
> +    // CWBorderPixel must be present when the depth doesn't match the parent's one.
> +    // See http://cgit.freedesktop.org/xorg/xserver/tree/dix/window.c?id=xorg-server-1.16.0#n703.
> +    windowAttributes.border_pixel = 0;

Why does the depth not match the parent's depth?

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:155
> -        parent ? parent : RootWindowOfScreen(screen),
> +        RootWindowOfScreen(screen),

Is there really no way to do this without properly parenting the window?

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 244437 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244437&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:323
> > +        GdkVisual* visual = gdk_window_get_visual(parentWindow);
> >          priv->redirectedWindow = RedirectedXCompositeWindow::create(
> >              GDK_DISPLAY_XDISPLAY(display),
> > -            parentWindow ? GDK_WINDOW_XID(parentWindow) : 0,
> > -            IntSize(1, 1),
> > +            GDK_VISUAL_XVISUAL(visual),
> > +            gdk_visual_get_depth(visual),
> 
> You are no longer handling the null parentWindow case here?

hmm, widgets must be inside a toplevel window (or be a toplevel) before being realized. I'll check if this causes problems with uwing a web view inside a popup window, for example.

> > Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:131
> > -std::unique_ptr<RedirectedXCompositeWindow> RedirectedXCompositeWindow::create(Display* display, Window parent, const IntSize& size, std::function<void()> damageNotify)
> > +std::unique_ptr<RedirectedXCompositeWindow> RedirectedXCompositeWindow::create(Display* display, Visual* visual, int depth, std::function<void()> damageNotify)
> >  {
> > -    return supportsXDamageAndXComposite(display) ? std::unique_ptr<RedirectedXCompositeWindow>(new RedirectedXCompositeWindow(display, parent, size, damageNotify)) : nullptr;
> > +    return supportsXDamageAndXComposite(display) ? std::unique_ptr<RedirectedXCompositeWindow>(new RedirectedXCompositeWindow(display, visual, depth, damageNotify)) : nullptr;
> 
> I think we don't need to change the arguments here. You can get a window's
> visual and depth using XGetWindowAttributes.

Yes, but GTK+ keeps a internal list of GdkVisuals, so gdk_window_get_visual() and gdk_visual_get_depth() are just a simple return, we don't need to query the xwindow again.

> > Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:152
> > +    // CWBorderPixel must be present when the depth doesn't match the parent's one.
> > +    // See http://cgit.freedesktop.org/xorg/xserver/tree/dix/window.c?id=xorg-server-1.16.0#n703.
> > +    windowAttributes.border_pixel = 0;
> 
> Why does the depth not match the parent's depth?

The root window might have a 24 (which I think it's the default). When rgba visual has been explicitly set in the widget (to use transparencies), the depth is 32 and doesn't match the root window (24).

> > Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:155
> > -        parent ? parent : RootWindowOfScreen(screen),
> > +        RootWindowOfScreen(screen),
> 
> Is there really no way to do this without properly parenting the window?

This is properly parenting the window. We want a window whose parent is the root window. We are using the web view parent window as parent only to inherit the visual, colormap and depth values, and then reparenting to the root window. This patch uses the proper parent directly, passing the visual, colormap and depth desired, instead of using another window just to inherit the values and then having to reparent. The reparent is causing the flash effect.

[Carlos Garcia Campos]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 244437 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=244437&action=review
> > 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:323
> > > +        GdkVisual* visual = gdk_window_get_visual(parentWindow);
> > >          priv->redirectedWindow = RedirectedXCompositeWindow::create(
> > >              GDK_DISPLAY_XDISPLAY(display),
> > > -            parentWindow ? GDK_WINDOW_XID(parentWindow) : 0,
> > > -            IntSize(1, 1),
> > > +            GDK_VISUAL_XVISUAL(visual),
> > > +            gdk_visual_get_depth(visual),
> > 
> > You are no longer handling the null parentWindow case here?
> 
> hmm, widgets must be inside a toplevel window (or be a toplevel) before
> being realized. I'll check if this causes problems with uwing a web view
> inside a popup window, for example.
> 

I've checked this and the window shoulnd't be NULL at this point, there are a lot of widgets in GTK that call gtk_widget_get_parent_window() in the realize method without checking the return value too. I don't think we need an ASSERT(), because gdk_window_get_visual() has a g_return macro if the passed in window is not a GdkWindow, so I wouldn't change anything in the patch.

[Julien Isorce]

Comment on attachment 244437 [details]
Patch

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

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:148
> +    windowAttributes.colormap = XCreateColormap(display, RootWindowOfScreen(screen), visual, AllocNone);

Don't you need to call XFreeColormap once the window is released in ::~RedirectedXCompositeWindow ?

[Carlos Garcia Campos]

(In reply to comment #6)
> Comment on attachment 244437 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244437&action=review
> 
> > Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:148
> > +    windowAttributes.colormap = XCreateColormap(display, RootWindowOfScreen(screen), visual, AllocNone);
> 
> Don't you need to call XFreeColormap once the window is released in
> ::~RedirectedXCompositeWindow ?

Good point! But I don't think we need to wait until the RedirectedXCompositeWindow is destroyed, I think we can free the colormap after creating the second window.

[Carlos Garcia Campos]

Created attachment 244521 [details]
Updated patch

Added XFreeColormap

[Martin Robinson]

(In reply to comment #3)

> > Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:152
> > +    // CWBorderPixel must be present when the depth doesn't match the parent's one.
> > +    // See http://cgit.freedesktop.org/xorg/xserver/tree/dix/window.c?id=xorg-server-1.16.0#n703.
> > +    windowAttributes.border_pixel = 0;
> 
> Why does the depth not match the parent's depth?
> 
> > Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:155
> > -        parent ? parent : RootWindowOfScreen(screen),
> > +        RootWindowOfScreen(screen),
> 
> Is there really no way to do this without properly parenting the window?

Okay. I see how it's nice for the visual, but the depth is a property of the visual. Getting the depth from the visual in RedirectedXCompositeWindow will prevent silly mistakes like passing a depth that doesn't match the visual. I would really prefer using XGetVisualInfo in RedirectedXCompositeWindow or to simply use GDK there.

[Carlos Garcia Campos]

(In reply to comment #9)
> (In reply to comment #3)
> 
> > > Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:152
> > > +    // CWBorderPixel must be present when the depth doesn't match the parent's one.
> > > +    // See http://cgit.freedesktop.org/xorg/xserver/tree/dix/window.c?id=xorg-server-1.16.0#n703.
> > > +    windowAttributes.border_pixel = 0;
> > 
> > Why does the depth not match the parent's depth?
> > 
> > > Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:155
> > > -        parent ? parent : RootWindowOfScreen(screen),
> > > +        RootWindowOfScreen(screen),
> > 
> > Is there really no way to do this without properly parenting the window?
> 
> Okay. I see how it's nice for the visual, but the depth is a property of the
> visual. Getting the depth from the visual in RedirectedXCompositeWindow will
> prevent silly mistakes like passing a depth that doesn't match the visual. I
> would really prefer using XGetVisualInfo in RedirectedXCompositeWindow or to
> simply use GDK there.

XGetVisualInfo returns the list of all available visuals, that's already done by GTK+, and gdk_visual_get_depth returns the already cached value for the current visual, so we don't need to iterate the visuals again just ot get the depth. We already removed the GDK dependency in RedirectedXCompositeWindow. I don't think passing the depth is such a big deal.

[Carlos Garcia Campos]

Created attachment 244522 [details]
New patch

I had forgotten that RedirectedXCompositeWindow already depended on GDK, so passing the parent Gdkwindow to the constructor makes everything simpler.

[Martin Robinson]

Comment on attachment 244522 [details]
New patch

Nice. Thank you.

[Carlos Garcia Campos]

Committed r178413: <http://trac.webkit.org/changeset/178413>

[Michael Catanzaro]

(In reply to comment #0)
> This is caused by the XReparentWindow instroduced in r177075. It's very easy
> to reproduce in Epiphany, when creating a new tab, you see like if the view
> was rendered in black for a while, causing a flickering effect.

We have this bug or a bug with identical symptoms in Fedora with 2.6.5, which does not include r177075. It's a regression from 2.6.3; I don't have an easy way to check 2.6.4.