Bug 121165 – [GTK][WK1] Block accelerated compositing under non-X11 displays

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[GTK][WK1] Only utilize the RedirectedXCompositeWindow under X11 displays

[Zan Dobersek]

Created attachment 211325 [details]
Patch

[Martin Robinson]

Wouldn't it make more sense to simply disable accelerated compositing entirely when running on Wayland?

[Zan Dobersek]

The accelerated compositing setting is disabled by default in WK1, but I guess it would still make sense.

The setting is also overridable, so perhaps there should be some warning about the feature being unsupported when someone tries to enable it?

[Martin Robinson]

(In reply to comment #3)
> The accelerated compositing setting is disabled by default in WK1, but I guess it would still make sense.
> 
> The setting is also overridable, so perhaps there should be some warning about the feature being unsupported when someone tries to enable it?

I guess the WebKit1 code does not have a mode where the AcceleratedCompositingContext is null, so perhaps this is the simplest way.

[Zan Dobersek]

OK if I still include the AcceleratedCompositingContextGL changes? The RedirectedXCompositeWindow is still specific to X11 displays.

[Martin Robinson]

Comment on attachment 211325 [details]
Patch

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

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:45
> +#if defined(GDK_WINDOWING_X11)
> +#include <gdk/gdkx.h>
> +#endif

Is it possible to move this past the end of the first block of includes? We typically do that if possible.

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:93
> +#if PLATFORM(X11) && defined(GDK_WINDOWING_X11)
> +        if (GDK_IS_X11_DISPLAY(display)) {
> +            if (m_redirectedWindow = RedirectedXCompositeWindow::create(pageSize))
> +                m_redirectedWindow->setDamageNotifyCallback(redirectedWindowDamagedCallback, m_webView);
> +        }
> +#endif

I think maybe it would be cleaner to simply put:

#if !defined(GDK_WINDOWING_X11)
    return;
#endif
    if (!GDK_IS_X11_DISPLAY(display))
        return;

at the beginning of the function. Is this

[Zan Dobersek]

Comment on attachment 211325 [details]
Patch

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

>> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:45
>> +#endif
> 
> Is it possible to move this past the end of the first block of includes? We typically do that if possible.

OK.

>> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:93
>> +#endif
> 
> I think maybe it would be cleaner to simply put:
> 
> #if !defined(GDK_WINDOWING_X11)
>     return;
> #endif
>     if (!GDK_IS_X11_DISPLAY(display))
>         return;
> 
> at the beginning of the function. Is this

I'd put the GDK_IS_X11_DISPLAY() check inside the GDK_WINDOWING_X11 guard:

#if !defined(GDK_WINDOWING_X11)
    if (!GDK_IS_X11_DISPLAY(display))
        return;
#endif

I don't like the outstanding return statement guarded by GDK_WINDOWING_X11, it's kind of odd.

Also, GDK_IS_X11_DISPLAY() would be missing if not guarded with GDK_WINDOWING_X11 and building with a GTK+ dependency that doesn't provide the X11 backend.

[Martin Robinson]

(In reply to comment #7)

> #if !defined(GDK_WINDOWING_X11)
>     if (!GDK_IS_X11_DISPLAY(display))
>         return;
> #endif


I assume you meant #if defined(GDK_WINDOWING_X11) here? This doesn't handle the case where GDK doesn't have support for X11. Perhaps this makes more sense?

#if defined(GDK_WINDOWING_X11)
     if (!GDK_IS_X11_DISPLAY(display))
         return;
#else
    return;
#endif

[Zan Dobersek]

(In reply to comment #8)
> (In reply to comment #7)
> 
> > #if !defined(GDK_WINDOWING_X11)
> >     if (!GDK_IS_X11_DISPLAY(display))
> >         return;
> > #endif
> 
> 
> I assume you meant #if defined(GDK_WINDOWING_X11) here? This doesn't handle the case where GDK doesn't have support for X11.

Yes, I didn't mean to use the negation, but it's still wrong.

> Perhaps this makes more sense?
> 
> #if defined(GDK_WINDOWING_X11)
>      if (!GDK_IS_X11_DISPLAY(display))
>          return;
> #else
>     return;
> #endif

That's the one.

[Zan Dobersek]

Created attachment 211406 [details]
Patch

[Zan Dobersek]

Comment on attachment 211406 [details]
Patch

Clearing flags on attachment: 211406

Committed r155619: <http://trac.webkit.org/changeset/155619>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.