<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Move the redirected XComposite window to the web process"
href="https://bugs.webkit.org/show_bug.cgi?id=160389#c5">Comment # 5</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Move the redirected XComposite window to the web process"
href="https://bugs.webkit.org/show_bug.cgi?id=160389">bug 160389</a>
from <span class="vcard"><a class="email" href="mailto:svillar@igalia.com" title="Sergio Villar Senin <svillar@igalia.com>"> <span class="fn">Sergio Villar Senin</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=284987&action=diff" name="attach_284987" title="Address review comments and try to fix EFL build">attachment 284987</a> <a href="attachment.cgi?id=284987&action=edit" title="Address review comments and try to fix EFL build">[details]</a></span>
Address review comments and try to fix EFL build
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=284987&action=review">https://bugs.webkit.org/attachment.cgi?id=284987&action=review</a>
Looking great to me. I will let Zan or Martin review it since there is stuff like what happens when there is no redirected window that is not completely clear to me.
<span class="quote">> Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:80
> +#if PLATFORM(GTK)</span >
Why this? It's just handling X stuff
<span class="quote">> Source/WebKit2/ChangeLog:40
> + all this also prevents several race conditions that were causing X errors and web process crashes.</span >
This is an amazing description, but too much for a single paragraph to my taste. It'd be awesome if you could split it a bit.
<span class="quote">> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:57
> + m_scene->setActive(!!m_nativeSurfaceHandle);</span >
Do we really need the !! ?
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:358
> + return;</span >
Is this even possible knowing that the whole method is guarded by REDIRECTED_XCOMPOSITE_WINDOW ?
<span class="quote">> Source/WebKit2/UIProcess/PageClient.h:-238
> - virtual void willEnterAcceleratedCompositingMode() = 0;</span >
Were we the unique users of this?
<span class="quote">> Source/WebKit2/UIProcess/WebPageProxy.h:-827
> - void willEnterAcceleratedCompositingMode();</span >
Ditto.
<span class="quote">> Source/WebKit2/UIProcess/efl/WebView.h:-224
> - void willEnterAcceleratedCompositingMode() override { }</span >
Ditto.
<span class="quote">> Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:40
> +static int s_damageEventBase = -1;</span >
Time to switch to Optional<int> ?
<span class="quote">> Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:57
> + if (s_damageEventBase == -1)</span >
...and change this
<span class="quote">> Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:67
> + if (s_damageEventBase == -1)</span >
and this
<span class="quote">> Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:77
> + ASSERT(s_damageEventBase != -1);</span >
and this
<span class="quote">> Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:82
> + XDamageNotifyEvent* damageEvent = reinterpret_cast<XDamageNotifyEvent*>(xEvent);</span >
You can use auto* there
<span class="quote">> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.h:114
> WebCore::IntPoint m_lastScrollPosition;</span >
I'm having a hard time to understand the difference between this two, perhaps we need better naming?</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>