[Webkit-unassigned] [Bug 139028] [GTK] Enable depth 32 for the RedirectedXCompositeWindow

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=139028

Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #242197|review?                     |review-
              Flags|                            |

--- Comment #8 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 242197
  --> https://bugs.webkit.org/attachment.cgi?id=242197
gtk: support RGBA end-to-end compositing when using RedirectedXCompositeWindow

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

> Source/WebKit2/ChangeLog:3
> +        gtk: support RGBA end-to-end compositing when using RedirectedXCompositeWindow

Please, use the bug title here and move the bug url, see other changelog entries.

> Source/WebKit2/ChangeLog:12
> +        This allows an end-to-end RGBA solution when the application
> +        wants to interact with the alpha channel at compositing time.

Do you have a test case or is there any website that are not rendering correctly because of this?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:264
> +        GdkVisual* visual = gtk_widget_get_visual(parent);
> +        gint depth = gdk_visual_get_depth(visual);
> +        bool supportAlpha = depth == 32 && (!priv->redirectedWindow || !priv->redirectedWindow->hasAlpha());

I think here you should check if the widget visual is the screen rgba visual.

visual == gdk_screen_get_rgba_visual(gdk_display_get_default_screen(display)));

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:269
> +            if (GDK_IS_X11_DISPLAY(display)) {
> +                priv->redirectedWindow = RedirectedXCompositeWindow::create(GDK_DISPLAY_XDISPLAY(display), IntSize(1, 1), [widget] { gtk_widget_queue_draw(widget); },
> +                    supportAlpha);

Why are we creating this here again? If it's too early in constructed, we can create the redirected window when realized, but not in both places, I would say.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:271
> +                    priv->pageProxy->setAcceleratedCompositingWindowId(priv->redirectedWindow->windowID());

This is also done when the page is created

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:154
> +        XVisualInfo vi_in;

Use WebKit coding style here, visualInfo instead of vi_in

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:159
> +        int nvi = 0;

nvi -> visualInfoCount, for example

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:160
> +        XVisualInfo* xvi = XGetVisualInfo(display, VisualScreenMask | VisualDepthMask | VisualClassMask, &vi_in, &nvi);

xvi -> xVisualInfo

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:161
> +        Visual* vis = 0;

vis -> visual, use nullptr instead of 0

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:164
> +            if ((fmt->type == PictTypeDirect) && (fmt->direct.alphaMask)) {

I don't think you need the extra parentheses

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:203
> +    } else {
> +        windowAttributes.override_redirect = True;
> +        m_parentWindow = XCreateWindow(display,
> +            RootWindowOfScreen(screen),
> +            WidthOfScreen(screen) + 1, 0, 1, 1,
> +            0,
> +            CopyFromParent,
> +            InputOutput,
> +            CopyFromParent,
> +            CWOverrideRedirect,
> +            &windowAttributes);
> +        XMapWindow(display, m_parentWindow);

Do we really need two paths here? or can we set the window attrs, the mask, etc depending on whether alpha is supported or not and then create the window and call XMapWindow using the same code?

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:245
> +    if (m_hasAlpha)
> +        XFreeColormap(m_display, m_colormap);

Could we check m_colormap instead of m_hasAlpha and then we don't need to keep m_hasAlpha at all?

> Tools/ChangeLog:3
> +        gtk: support RGBA end-to-end compositing when using RedirectedXCompositeWindow

Same here about changelog formatting.

> Tools/MiniBrowser/gtk/main.c:69
> +    const gchar *alpha = g_getenv("MINIBROWSER_ALPHA");
> +    if (alpha && *alpha) {
> +        GdkScreen *screen = gtk_widget_get_screen(GTK_WIDGET(mainWindow));
> +        GdkVisual *visual = gdk_screen_get_rgba_visual(screen);
> +        gtk_widget_set_visual(GTK_WIDGET(mainWindow), visual);
> +    }

I don't understand why this is a user decision, doesn't it depend on whether it's supported or not?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141126/3614aae5/attachment-0002.html>