[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

Martin Robinson <mrobinson at webkit.org> changed:

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

--- Comment #11 from Martin Robinson <mrobinson at webkit.org> ---
Comment on attachment 242267
  --> https://bugs.webkit.org/attachment.cgi?id=242267
[GTK] Enable depth 32 for the RedirectedXCompositeWindow

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

This looks like a good change, though I have a few questions.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:260
> +    if (GtkWidget* parent = gtk_widget_get_parent(widget)) {

Why not fall back if the parent doesn't exist. If the parent always must exist, why not use an assertion instead of a conditional?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1045
> +    bool isEnable = false;
> +
>  #if USE(TEXTURE_MAPPER_GL) && PLATFORM(X11)
> -    if (priv->redirectedWindow)
> -        return;
> +    isEnable = !!priv->redirectedWindow;
>  #endif
>  
> -    priv->pageProxy->preferences().setAcceleratedCompositingEnabled(false);
> +    priv->pageProxy->preferences().setAcceleratedCompositingEnabled(isEnable);

Doesn't this do the same thing as before? It looks like the default was true? Regardless,

isEnabled should be called acceleratedCompositingEnabled and I do not believe you need the double !.

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:162
> +    // The top parent is only necessary to inherit visual and depth at creation time.
> +    if (parent)
> +        XReparentWindow(display, m_parentWindow, RootWindowOfScreen(screen), WidthOfScreen(screen) + 1, 0);

Would it be possible to pass the visual and depth from the parent instead of inheriting?

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:173
> +        CWEventMask | CWOverrideRedirect,

Why is this change necessary? You should include this information in the ChangeLog.

> Tools/MiniBrowser/gtk/main.c:69
> +    // Prefer RGBA visual if available.
> +    GdkScreen *screen = gtk_widget_get_screen(GTK_WIDGET(mainWindow));
> +    GdkVisual *visual = gdk_screen_get_rgba_visual(screen);
> +    if (visual)
> +        gtk_widget_set_visual(GTK_WIDGET(mainWindow), visual);
> +

Does this make the WebView transparent with a transparent HTML background. I'm not sure if we want that behavior in the MiniBrowser.

-- 
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/20141207/0f8ece11/attachment-0002.html>