[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 #6 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #5)
> Comment on attachment 284987 [details]
> 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.

Thanks for looking at it. The situation when the there's no redirected window is that nothing will be rendered (like with current code). It's not expected to happen, it would mean that we are running in a very very old version of the xserver. In that case nothing will be rendered, but hopefully it won't crash either. Ideally we would not allow to build with that unsupported configuration, and we indeed fail if XComposite extension is not available, but the actual version needs to be checked at runtime. So, I wouldn't worry about such situation.

> > Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:80
> > +#if PLATFORM(GTK)
> 
> Why this? It's just handling X stuff

Yes, but only the GTK+ port depends on it. That's what we do in all other places where XDamage is used. I agree we could add a specific build option for that instead, but that would be a separate bug, since we are already using GTK ifdefs form Xdamage everywhere.

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

No, but I think it's a clearer way to convert to bool.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:358
> > +        return;
> 
> Is this even possible knowing that the whole method is guarded by
> REDIRECTED_XCOMPOSITE_WINDOW ?

Please, add more lines to the context, I only see a return here. Ok, that's the display check. Yes, it's needed, because the REDIRECTED_XCOMPOSITE_WINDOW guard is at compile time, but we build wayland and X11 support at the same time, we need to check the actual display at runtime to took one path or the other.

> > Source/WebKit2/UIProcess/PageClient.h:-238
> > -    virtual void willEnterAcceleratedCompositingMode() = 0;
> 
> Were we the unique users of this?

Yes, I added this to fix one of the issues of having the redirected window in the UI process.

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

Not sure it's worth it for a global static variable like this. And I'm not sure I can pass the mem address to the query method to get the value.

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

Yes.

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

Again, single line in the context, let check which two you mean. Ah, you mena prev, and last. I have no idea, I only re-ordered to initialize the redirected window first in the constructor, those have nothing to do with this patch.

-- 
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/20c7a9d3/attachment-0001.html>