Bug 173598 – [GTK][WAYLAND] Create WaylandCompositorDisplay unconditionally when initializing the WebProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Miguel Gomez]

Currently the WaylandCompositorDisplay is created when entering AC mode, and at this points it sets itself as the sharedDisplayForCompositing to use. But components that render gl, like the MediaPlayer or the WebGLRenderingContext can be created before this happens, which causes them to get a wrong sharedDisplayForcompositing (and sharingGLContext), which breaks rendering as the TextureMapper cannot access their content.

[Miguel Gomez]

Created attachment 313397 [details]
Patch

[Zan Dobersek]

Comment on attachment 313397 [details]
Patch

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

> Source/WebKit2/WebProcess/WebProcess.cpp:318
>  #if PLATFORM(WAYLAND)
> -    m_waylandCompositorDisplayName = parameters.waylandCompositorDisplayName;
> +    m_waylandCompositorDisplay = WaylandCompositorDisplay::create(parameters.waylandCompositorDisplayName);
>  #endif

Can this be done in platformInitializeWebProcess()? Currently the GTK+ and WPE ports are building WebProcessSoup.cpp, where that method is defined, and it's soup-specific. But IMO the implementation file should be GLib-specific, with m_waylandCompositorDisplay initialization guarded with PLATFORM(WAYLAND).

> Source/WebKit2/WebProcess/WebProcess.h:35
> +#include "WaylandCompositorDisplay.h"

This needs to be guarded with PLATFORM(WAYLAND). Furthermore, it should be possible to just forward-declare the WaylandCompositorDisplay class, and include the header only in the implementation file.

[Carlos Garcia Campos]

Comment on attachment 313397 [details]
Patch

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

> Source/WebKit2/ChangeLog:8
> +        Move WaylandCompositorDisplay code to its own files to it can be used from other clases. Then, instead of

to it -> so it
clases -> classes

>> Source/WebKit2/WebProcess/WebProcess.cpp:318
>>  #endif
> 
> Can this be done in platformInitializeWebProcess()? Currently the GTK+ and WPE ports are building WebProcessSoup.cpp, where that method is defined, and it's soup-specific. But IMO the implementation file should be GLib-specific, with m_waylandCompositorDisplay initialization guarded with PLATFORM(WAYLAND).

I guess PLATFORM(WAYLAND) is not enabled in WPE

>> Source/WebKit2/WebProcess/WebProcess.h:35
>> +#include "WaylandCompositorDisplay.h"
> 
> This needs to be guarded with PLATFORM(WAYLAND). Furthermore, it should be possible to just forward-declare the WaylandCompositorDisplay class, and include the header only in the implementation file.

Yes, better forward declare it.

> Source/WebKit2/WebProcess/gtk/WaylandCompositorDisplay.cpp:37
> +std::unique_ptr<WaylandCompositorDisplay> WaylandCompositorDisplay::create(String displayName)

const String&

> Source/WebKit2/WebProcess/gtk/WaylandCompositorDisplay.h:30
> +#include "WebPage.h"

You can probably forward declare this.

> Source/WebKit2/WebProcess/gtk/WaylandCompositorDisplay.h:39
> +    static std::unique_ptr<WaylandCompositorDisplay> create(String);

I'm not sure if you also have to add a destructor for the smart pointers, even if it's just = default.

[Miguel Gomez]

Created attachment 313407 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 313407 [details]
Patch

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

> Source/WebKit2/WebProcess/WebProcess.cpp:120
> +#if PLATFORM(WAYLAND) && PLATFORM(GTK)
> +#include "WaylandCompositorDisplay.h"
> +#endif

WaylandCompositorDisplay.h already has the PLATFORM(WAYLAND), we don't guard headers when they are already guarded.

> Source/WebKit2/WebProcess/WebProcess.cpp:320
> +#if PLATFORM(WAYLAND) && PLATFORM(GTK)

I don't think PLATFORM(WAYLAND) and PLATFORM(WPE are compatible), only GTK+ port exposes Wayland platform, so the check was probably enough.

> Source/WebKit2/WebProcess/WebProcess.cpp:321
> +    m_waylandCompositorDisplay = WaylandCompositorDisplay::create(parameters.waylandCompositorDisplayName);

Weren't we going to move this to platformInitializeWebProcess?

> Source/WebKit2/WebProcess/WebProcess.h:104
> +#if PLATFORM(WAYLAND) && PLATFORM(GTK)
> +class WaylandCompositorDisplay;
> +#endif

Forward declarations don't need to be guarded.

> Source/WebKit2/WebProcess/gtk/WaylandCompositorDisplay.h:32
> +#include <wtf/text/AtomicString.h>

What do we need this for? If it's for String it should be <wtf/text/WTFString.h> or simply <wtf/Forward.h>

[Miguel Gomez]

(In reply to Carlos Garcia Campos from comment #5)
> Comment on attachment 313407 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313407&action=review
> 
> > Source/WebKit2/WebProcess/WebProcess.cpp:120
> > +#if PLATFORM(WAYLAND) && PLATFORM(GTK)
> > +#include "WaylandCompositorDisplay.h"
> > +#endif
> 
> WaylandCompositorDisplay.h already has the PLATFORM(WAYLAND), we don't guard
> headers when they are already guarded.

But WaylandCompositorDisplay.h is stored inside a GTK only directory, so it doesn't exist for WPE builds and fails.

> > Source/WebKit2/WebProcess/WebProcess.cpp:320
> > +#if PLATFORM(WAYLAND) && PLATFORM(GTK)
> 
> I don't think PLATFORM(WAYLAND) and PLATFORM(WPE are compatible), only GTK+
> port exposes Wayland platform, so the check was probably enough.

Yep, I checked and seems that a check for WAYLAND is enough.
 
> > Source/WebKit2/WebProcess/WebProcess.cpp:321
> > +    m_waylandCompositorDisplay = WaylandCompositorDisplay::create(parameters.waylandCompositorDisplayName);
> 
> Weren't we going to move this to platformInitializeWebProcess?

Ok.

[Carlos Garcia Campos]

(In reply to Miguel Gomez from comment #6)
> (In reply to Carlos Garcia Campos from comment #5)
> > Comment on attachment 313407 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=313407&action=review
> > 
> > > Source/WebKit2/WebProcess/WebProcess.cpp:120
> > > +#if PLATFORM(WAYLAND) && PLATFORM(GTK)
> > > +#include "WaylandCompositorDisplay.h"
> > > +#endif
> > 
> > WaylandCompositorDisplay.h already has the PLATFORM(WAYLAND), we don't guard
> > headers when they are already guarded.
> 
> But WaylandCompositorDisplay.h is stored inside a GTK only directory, so it
> doesn't exist for WPE builds and fails.

Ah, right the include dirs.

> > > Source/WebKit2/WebProcess/WebProcess.cpp:320
> > > +#if PLATFORM(WAYLAND) && PLATFORM(GTK)
> > 
> > I don't think PLATFORM(WAYLAND) and PLATFORM(WPE are compatible), only GTK+
> > port exposes Wayland platform, so the check was probably enough.
> 
> Yep, I checked and seems that a check for WAYLAND is enough.
>  
> > > Source/WebKit2/WebProcess/WebProcess.cpp:321
> > > +    m_waylandCompositorDisplay = WaylandCompositorDisplay::create(parameters.waylandCompositorDisplayName);
> > 
> > Weren't we going to move this to platformInitializeWebProcess?
> 
> Ok.

[Miguel Gomez]

Created attachment 313502 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 313502 [details]
Patch

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

LGTM, check why it doesn't apply.

> Source/WebKit2/ChangeLog:8
> +        Move WaylandCompositorDisplay code to its own files to it can be used from other classes. Then, instead of

to it -> so it

[Miguel Gomez]

Created attachment 313505 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 313505 [details]
Patch

Clearing flags on attachment: 313505

Committed r218629: <http://trac.webkit.org/changeset/218629>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.