[Webkit-unassigned] [Bug 160389] [GTK] Move the redirected XComposite window to the web process

[bugzilla-daemon at webkit.org]

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

--- Comment #5 from Sergio Villar Senin <svillar at igalia.com> ---
Comment on attachment 284987
  --> https://bugs.webkit.org/attachment.cgi?id=284987
Address review comments and try to fix EFL build

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

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.

> Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:80
> +#if PLATFORM(GTK)

Why this? It's just handling X stuff

> Source/WebKit2/ChangeLog:40
> +        all this also prevents several race conditions that were causing X errors and web process crashes.

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.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:57
> +        m_scene->setActive(!!m_nativeSurfaceHandle);

Do we really need the !! ?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:358
> +        return;

Is this even possible knowing that the whole method is guarded by REDIRECTED_XCOMPOSITE_WINDOW ?

> Source/WebKit2/UIProcess/PageClient.h:-238
> -    virtual void willEnterAcceleratedCompositingMode() = 0;

Were we the unique users of this?

> Source/WebKit2/UIProcess/WebPageProxy.h:-827
> -    void willEnterAcceleratedCompositingMode();

Ditto.

> Source/WebKit2/UIProcess/efl/WebView.h:-224
> -    void willEnterAcceleratedCompositingMode() override { }

Ditto.

> Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:40
> +static int s_damageEventBase = -1;

Time to switch to Optional<int> ?

> Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:57
> +    if (s_damageEventBase == -1)

...and change this

> Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:67
> +    if (s_damageEventBase == -1)

and this

> Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:77
> +    ASSERT(s_damageEventBase != -1);

and this

> Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:82
> +    XDamageNotifyEvent* damageEvent = reinterpret_cast<XDamageNotifyEvent*>(xEvent);

You can use auto* there

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.h:114
>      WebCore::IntPoint m_lastScrollPosition;

I'm having a hard time to understand the difference between this two, perhaps we need better naming?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160801/168bddf2/attachment.html>