Bug 136216 - [GTK] Add WaylandDisplay
Summary: [GTK] Add WaylandDisplay
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks: 115803
  Show dependency treegraph
 
Reported: 2014-08-25 03:23 PDT by Zan Dobersek
Modified: 2015-12-07 09:30 PST (History)
7 users (show)

See Also:


Attachments
Patch (10.97 KB, patch)
2014-08-25 03:54 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (11.02 KB, patch)
2014-08-25 12:56 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (11.29 KB, patch)
2014-08-26 01:47 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (11.42 KB, patch)
2014-08-28 00:58 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (13.07 KB, patch)
2014-08-30 12:16 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (13.22 KB, patch)
2014-08-31 23:49 PDT, Zan Dobersek
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2014-08-25 03:23:32 PDT
[GTK] Add WaylandDisplay
Comment 1 Zan Dobersek 2014-08-25 03:54:04 PDT
Created attachment 237074 [details]
Patch
Comment 2 WebKit Commit Bot 2014-08-25 03:56:27 PDT
Attachment 237074 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/wayland/WaylandDisplay.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/wayland/WaylandDisplay.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Zan Dobersek 2014-08-25 06:06:43 PDT
Comment on attachment 237074 [details]
Patch

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

> Source/WebCore/platform/graphics/wayland/WaylandDisplay.cpp:131
> +void WaylandDisplay::destroySurface(WaylandSurface* surface)
> +{
> +    if (surface) {
> +        eglMakeCurrent(m_eglDisplay, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT);
> +
> +        wl_egl_window_destroy(surface->nativeWindowHandle());
> +        wl_surface_destroy(surface->surface());
> +    }
> +}

Looking at this as I was writing the changelog, I think all this could be just executed in the WaylandSurface destructor.

I'll try it out and report back, either with a new patch or an explanation on why it can't be done.
Comment 4 Zan Dobersek 2014-08-25 12:56:41 PDT
Created attachment 237101 [details]
Patch
Comment 5 WebKit Commit Bot 2014-08-25 12:58:05 PDT
Attachment 237101 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/wayland/WaylandDisplay.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/wayland/WaylandDisplay.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Zan Dobersek 2014-08-26 01:47:18 PDT
Created attachment 237141 [details]
Patch
Comment 7 WebKit Commit Bot 2014-08-26 01:49:46 PDT
Attachment 237141 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/wayland/WaylandDisplay.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/wayland/WaylandDisplay.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Zan Dobersek 2014-08-28 00:58:13 PDT
Created attachment 237299 [details]
Patch
Comment 9 WebKit Commit Bot 2014-08-28 01:00:47 PDT
Attachment 237299 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/wayland/WaylandDisplay.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/wayland/WaylandDisplay.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Martin Robinson 2014-08-28 10:22:57 PDT
Comment on attachment 237299 [details]
Patch

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

> Source/WebCore/platform/graphics/wayland/WaylandDisplay.cpp:78
> +    m_registry = wl_display_get_registry(m_display);

This can be moved the initializer list.

> Source/WebCore/platform/graphics/wayland/WaylandDisplay.cpp:94
> +        g_warning("eglGetDisplay EGL_NO_DISPLAY");

It's probably better to issue a more descriptive warning like: "WaylandDisplay initialization failed to get EGL display."

> Source/WebCore/platform/graphics/wayland/WaylandDisplay.cpp:99
> +        g_warning("eglInitialize EGL_FALSE");

Ditto.

> Source/WebCore/platform/graphics/wayland/WaylandDisplay.cpp:104
> +        g_warning("eglBindAPI EGL_FALSE");

Ditto. When these fail how does the owner of this instance detect the failure?

> Source/WebCore/platform/graphics/wayland/WaylandDisplay.cpp:110
> +    EGLint n;
> +    if (!eglChooseConfig(m_eglDisplay, configAttributes, &m_eglConfig, 1, &n) || n != 1)
> +        g_warning("eglChooseConfig failed");

n -> numberOfConfigs

> Source/WebCore/platform/graphics/wayland/WaylandDisplay.cpp:116
> +    EGLNativeWindowType nativeWindow = wl_egl_window_create(wlSurface, std::max(1, width), std::max(1, height));

Perhaps better to assert(width > 0 && height > 0)?

> Source/WebCore/platform/graphics/wayland/WaylandDisplay.h:58
> +    std::unique_ptr<WaylandSurface> createSurface(int, int, int);
> +
> +    PassOwnPtr<GLContextEGL> createSharingGLContext();
> +
> +private:
> +    static const struct wl_registry_listener m_registryListener;
> +    static void globalCallback(void*, struct wl_registry*, uint32_t, const char*, uint32_t);
> +    static void globalRemoveCallback(void*, struct wl_registry*, uint32_t);

Arguments should have a name here unless the name is simply a repetition of the type. That way one can read the header and know how to call the method. For instance, "struct wl_registry* registry" doesn't need a name, but "int height" does.
Comment 11 Zan Dobersek 2014-08-30 12:16:22 PDT
Created attachment 237417 [details]
Patch

Still have to check the possibility of asserting the non-zero size parameters in WaylandDisplay::createSurface() (and if IntSize could be used) and where in LayerTreeHostGtk do 0x0 surfaces come from in the first place.
Comment 12 WebKit Commit Bot 2014-08-30 12:18:36 PDT
Attachment 237417 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/wayland/WaylandDisplay.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/wayland/WaylandDisplay.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Zan Dobersek 2014-08-31 23:49:16 PDT
Created attachment 237441 [details]
Patch
Comment 14 WebKit Commit Bot 2014-08-31 23:51:20 PDT
Attachment 237441 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/wayland/WaylandDisplay.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/wayland/WaylandDisplay.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Zan Dobersek 2014-09-01 00:08:44 PDT
Comment on attachment 237441 [details]
Patch

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

> Source/WebCore/platform/graphics/wayland/WaylandDisplay.cpp:126
> +    // We keep the minimum size at 1x1px since Mesa returns null values in wl_egl_window_create() for zero width or height.
> +    EGLNativeWindowType nativeWindow = wl_egl_window_create(wlSurface, std::max(1, size.width()), std::max(1, size.height()));

The 0x0px surfaces are created in LayerTreeHostGtk::initialize() using the size of the WebPage object which at that point still doesn't have the proper size.

On the other hand Mesa returns a null pointer from wl_egl_window_create() if either width or height is zero. That's what's being avoided here by falling back to a 1x1px surface that is eventually resized through WaylandSurface::resize(), from LayerTreeHostGtk::sizeDidChange().
Comment 16 Zan Dobersek 2014-09-15 23:53:51 PDT
Committed r173651: <http://trac.webkit.org/changeset/173651>
Comment 17 Michael Catanzaro 2015-12-07 09:30:26 PST
It's very confusing to have an unused Wayland protocol in the source tree. I think we should get rid of this code until such time as we are ready to land patches that use it.