Bug 136897 - [GTK] Add WaylandCompositor
Summary: [GTK] Add WaylandCompositor
Status: RESOLVED DUPLICATE of bug 115803
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:
 
Reported: 2014-09-17 14:26 PDT by Zan Dobersek
Modified: 2016-08-17 00:31 PDT (History)
7 users (show)

See Also:


Attachments
Patch (38.31 KB, patch)
2014-09-17 14:42 PDT, Zan Dobersek
no flags 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-09-17 14:26:59 PDT
[GTK] Add WaylandCompositor
Comment 1 Zan Dobersek 2014-09-17 14:42:20 PDT
Created attachment 238266 [details]
Patch
Comment 2 WebKit Commit Bot 2014-09-17 14:43:02 PDT
Attachment 238266 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/wayland/WaylandCompositorEGL.cpp:86:  int32_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/graphics/wayland/WaylandCompositorEGL.cpp:86:  int32_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/graphics/wayland/WaylandCompositorEGL.cpp:174:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/wayland/WaylandCompositor.cpp:246:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/graphics/wayland/WaylandCompositorEGL.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/wayland/WaylandCompositor.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/wayland/WaylandCompositor.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/wayland/WaylandCompositor.h:108:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 8 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Pekka Paalanen 2014-12-17 02:32:45 PST
Comment on attachment 238266 [details]
Patch

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

Kov asked me to take a look.

comments on the design... well, I can read through that, but not sure about the design, since your protocol extension specifications are not there.

Nothing I can complain about the design, really, from what I can see here.

> Source/WebCore/platform/graphics/wayland/WaylandCompositor.cpp:301
> +    if (wl_display_add_socket(m_display.childDisplay, "webkitgtk-wayland-compositor-socket")) {

shouldn't the wl_display_add_socket() be somehow browser-instance specific?
do you even need a socket to begin with?

> Source/WebCore/platform/graphics/wayland/WaylandCompositorEGL.cpp:164
> +        surface->buffer = nullptr;

assumes there cannot be a commit without attach

> Source/WebCore/platform/graphics/wayland/WaylandCompositorEGL.cpp:176
> +        wl_callback_send_done(nc->resource, 0);
> +        wl_resource_destroy(nc->resource);

sending frame callbacks from the commitSurface seems too early, you haven't drawn yet. You are hoping that the clients throttle by some other means.

> Source/WebCore/platform/graphics/wayland/WaylandCompositorEGL.cpp:214
> +        EGL_STENCIL_SIZE, 8,

what is the stencil buffer needed for?

> Source/WebCore/platform/graphics/wayland/WaylandCompositorEGL.cpp:251
> +    if (eglChooseConfig(m_display.eglDisplay, configAttributes, &m_eglConfig, 1, &numberOfConfigs) == EGL_FALSE
> +        || numberOfConfigs != 1)

The eglChooseConfig call seems naive, but that's the majority of apps anyway.

> Source/WebCore/platform/graphics/wayland/WaylandCompositorEGL.cpp:258
> +    if (eglMakeCurrent(m_display.eglDisplay, EGL_NO_SURFACE, EGL_NO_SURFACE, m_eglContext) == EGL_FALSE)

seems to be relying on the surfaceless extension
Comment 4 Carlos Garcia Campos 2016-08-17 00:31:37 PDT
Let's track this in bug #115803 instead.

*** This bug has been marked as a duplicate of bug 115803 ***