[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

--- Comment #12 from Julien Isorce <j.isorce at samsung.com> ---
Comment on attachment 242267
  --> https://bugs.webkit.org/attachment.cgi?id=242267
[GTK] Enable depth 32 for the RedirectedXCompositeWindow

Thx for the third review. I'll address the changes in the next patch.


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

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

Good point. Indeed it should fallback to passing 0 to RedirectedXCompositeWindow::create. I'll address the change.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1045
>> +    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 !.

The default is false but it gets enable if GL and redirectedWindow exists.
Without the double !, it fails to build with ../../Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1042:14: error: cannot convert 'std::unique_ptr<WebKit::RedirectedXCompositeWindow>' to 'bool' in assignment
I'll rewrite this part more clearly.

>> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:162
>> +        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?

Ok I'll have a try and let you know in the next patch.

>> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:173
>> +        CWEventMask | CWOverrideRedirect,
> 
> Why is this change necessary? You should include this information in the ChangeLog.

Indeed this is not necessary. This change should go in a separate patch. It is not necessary for my patch to work. Was just a consistency change.

>> Tools/MiniBrowser/gtk/main.c:69
>> +
> 
> Does this make the WebView transparent with a transparent HTML background. I'm not sure if we want that behavior in the MiniBrowser.

With only the patch, the background does not get transparent if the html page contains "body { background-color: transparent; }".
Indeed to have a transparent background it requires more changes in Minibrowser, like using gtk_widget_override_background_color(...transparent...).
So I'll let this change.

-- 
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/20141209/6adb65d0/attachment-0002.html>