Bug 145645

Created attachment 254276 [details] Patch

BTW, if you run weston inside X, you have to unset the DISPLAY environment value before executing the MiniBrowser inside the weston terminal. Otherwise another crash happens (I still didn't checked that one, but I think is unrelated with the one on this bug)

Comment on attachment 254276 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254276&action=review Commenting only about minor stuff, I leave the actual review for Zan > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:60 > + struct wl_display* wlDisplay = wl_display_connect(0); 0 -> nullptr > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:62 > + g_warning("PlatformDisplayWayland initialization: failed to connect to the Wayland server socket. Check your WAYLAND_DISPLAY or WAYLAND_SOCKET environment variables."); Use WTFLogAlways instead of g_warning > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:68 > + g_warning("PlatformDisplayWayland initialization: failed to complete the initialization of the display."); Ditto.

Created attachment 254351 [details] Patch

Created attachment 254353 [details] Patch Only a trivial change to add a link on the changelog to the actual revision/line referenced.

Comment on attachment 254353 [details] Patch Perhaps Zan can confirm, but I wonder if this is simply because the UIProcess-embedded Wayland subcompositer is not implemented yet. If so, we can always change the strings back when it's in place.

(In reply to comment #6) > Comment on attachment 254353 [details] > Patch > > Perhaps Zan can confirm, but I wonder if this is simply because the > UIProcess-embedded Wayland subcompositer is not implemented yet. If so, we > can always change the strings back when it's in place. That's pretty much the reason why a specific string is used in the wl_display_connect() call, and why that leads to a failure when invoked from the UIProcess. I think there should be possible to connect the WebProcess and the nested compositor in the UIProcess some other way, so the socket name can be removed.

Comment on attachment 254353 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254353&action=review > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:50 > - else if (!std::strcmp(interface, "wl_webkitgtk")) > + else if (!std::strcmp(interface, "wl_shell")) > display->m_webkitgtk = static_cast<struct wl_webkitgtk*>(wl_registry_bind(registry, name, &wl_webkitgtk_interface, 1)); This else-if branch should be removed completely at this point.

Comment on attachment 254353 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254353&action=review > Source/WebCore/ChangeLog:18 > + (WebCore::PlatformDisplayWayland::globalCallback): We should check for the > + string "wl_shell" in order to get the notification that the shell surface is > + available because this is the name of this object in the Wayland protocol. wl_webkitgtk and wl_shell interfaces are not interchangeable, so this is not correct. wl_webkitgtk interface is specific to the nested compositor implementation, used by the WebProcess which behaves as a client to that compositor. wl_shell is a more generic interface with greater capabilities, exposed by the parent compositor.

(In reply to comment #8) > Comment on attachment 254353 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254353&action=review > > > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:50 > > - else if (!std::strcmp(interface, "wl_webkitgtk")) > > + else if (!std::strcmp(interface, "wl_shell")) > > display->m_webkitgtk = static_cast<struct wl_webkitgtk*>(wl_registry_bind(registry, name, &wl_webkitgtk_interface, 1)); > > This else-if branch should be removed completely at this point. If I remove that branch then I have to also to apply this patch: --- a/Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h +++ b/Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h @@ -58,7 +58,7 @@ private: static void globalRemoveCallback(void* data, struct wl_registry*, uint32_t name); PlatformDisplayWayland(struct wl_display*); - bool isInitialized() { return m_compositor && m_webkitgtk && m_eglDisplay != EGL_NO_DISPLAY && m_eglConfigChosen; } + bool isInitialized() { return m_compositor && m_eglDisplay != EGL_NO_DISPLAY && m_eglConfigChosen; } Type type() const override { return PlatformDisplay::Type::Wayland; } In order to isInitialized return true. So, I can let that else-if branch without touching it because we not longer check m_webkitgtk in isInitialized

Created attachment 254358 [details] Patch Updated to not touch the else-if branch, but instead removed the check for the nested subcompositor in isInitialized()

Comment on attachment 254358 [details] Patch OK, thanks.

Comment on attachment 254358 [details] Patch Clearing flags on attachment: 254358 Committed r185252: <http://trac.webkit.org/changeset/185252>

All reviewed patches have been landed. Closing bug.