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.
Created attachment 313397 [details] Patch
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.
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.
Created attachment 313407 [details] Patch
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>
(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.
(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.
Created attachment 313502 [details] Patch
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
Created attachment 313505 [details] Patch
Comment on attachment 313505 [details] Patch Clearing flags on attachment: 313505 Committed r218629: <http://trac.webkit.org/changeset/218629>
All reviewed patches have been landed. Closing bug.