Bug 136220 – [GTK] Add WaylandSurface

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

[GTK] Add WaylandSurface

[Zan Dobersek]

Created attachment 237077 [details]
Patch

[WebKit Commit Bot]

Attachment 237077 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/wayland/WaylandSurface.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/wayland/WaylandSurface.h:34:  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.

[Martin Robinson]

Comment on attachment 237077 [details]
Patch

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

> Source/WebCore/platform/graphics/wayland/WaylandSurface.cpp:43
> +const struct wl_callback_listener WaylandSurface::m_frameListener = {
> +    // frame_callback
> +    [](void*, struct wl_callback* callback, uint32_t)
> +    {
> +        if (callback)
> +            wl_callback_destroy(callback);
> +    }
> +};

Why make this a lambda instead of simply a static method or function?

[Zan Dobersek]

Created attachment 237103 [details]
Patch

[WebKit Commit Bot]

Attachment 237103 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/wayland/WaylandSurface.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/wayland/WaylandSurface.h:34:  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.

[Zan Dobersek]

Comment on attachment 237077 [details]
Patch

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

>> Source/WebCore/platform/graphics/wayland/WaylandSurface.cpp:43
>> +};
> 
> Why make this a lambda instead of simply a static method or function?

It's more C++-like, and the functions are short enough to fit into a lambda. I find them prettier as they don't pollute class declarations like static methods would, and they don't feel unrelated to the class like standalone functions would, but that is a matter of personal taste.

This still generates a separate function, just like a static method or a standalone function would.

[Martin Robinson]

Comment on attachment 237077 [details]
Patch

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

>>> Source/WebCore/platform/graphics/wayland/WaylandSurface.cpp:43
>>> +};
>> 
>> Why make this a lambda instead of simply a static method or function?
> 
> It's more C++-like, and the functions are short enough to fit into a lambda. I find them prettier as they don't pollute class declarations like static methods would, and they don't feel unrelated to the class like standalone functions would, but that is a matter of personal taste.
> 
> This still generates a separate function, just like a static method or a standalone function would.

I'm not sure it makes sense to do this here, since you don't need to use it as a lambda. Making a function with a name is less confusing. The new features are cool, but we don't *have* to use them if it doesn't fit. :)

[Build Bot]

Comment on attachment 237103 [details]
Patch

Attachment 237103 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4532765248716800

New failing tests:
accessibility/accessibility-node-memory-management.html
compositing/absolute-inside-out-of-view-fixed.html
animations/added-while-suspended.html
canvas/philip/tests/2d.canvas.readonly.html
http/tests/appcache/404-manifest.html
animations/3d/change-transform-in-end-event.html

[Build Bot]

Created attachment 237125 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Zan Dobersek]

Comment on attachment 237077 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/wayland/WaylandSurface.cpp:43
>>>> +};
>>> 
>>> Why make this a lambda instead of simply a static method or function?
>> 
>> It's more C++-like, and the functions are short enough to fit into a lambda. I find them prettier as they don't pollute class declarations like static methods would, and they don't feel unrelated to the class like standalone functions would, but that is a matter of personal taste.
>> 
>> This still generates a separate function, just like a static method or a standalone function would.
> 
> I'm not sure it makes sense to do this here, since you don't need to use it as a lambda. Making a function with a name is less confusing. The new features are cool, but we don't *have* to use them if it doesn't fit. :)

I'd argue that lambdas are generally used because they can be used -- they weren't available before C++11 and in this specific use case static methods or functions were used instead, working equally well.

The biggest disadvantage of lambdas as used here is that you don't have a named function and have to use an additional comment to make it clear what function this lambda represents. What I do like about this approach is that lambdas in this static private member initialization can directly access private members on an object of this class. I find this more attractive than having some number of static private methods on a class and using those, but only if the actual function body is short enough so that everything remains readable.

I'll switch back to static methods to push this patch forward.

[Zan Dobersek]

Created attachment 237140 [details]
Patch

[WebKit Commit Bot]

Attachment 237140 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/wayland/WaylandSurface.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/wayland/WaylandSurface.h:34:  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.

[Martin Robinson]

Comment on attachment 237140 [details]
Patch

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

Looks good. Glad to see this moving forward. Please make these few small changes before landing.

> Source/WebCore/PlatformGTK.cmake:438
> +        platform/graphics/wayland/WaylandSurface.cpp
>          ${DERIVED_SOURCES_WEBCORE_DIR}/WebKitGtkWaylandClientProtocol.c

The WebKit CMake style specifies that there should be a blank line between files in different directories.

> Source/WebCore/platform/graphics/wayland/WaylandSurface.h:49
> +    void resize(int, int);

You should specify the variable names here or pass IntSize.

> Source/WebCore/platform/graphics/wayland/WaylandSurface.h:56
> +    static const struct wl_callback_listener m_frameListener;

This doesn't need to be a static member of the class. It's enough to make it static in the source file.

> Source/WebCore/platform/graphics/wayland/WaylandSurface.h:57
> +    static void frameCallback(void*, struct wl_callback*, uint32_t);

Ditto.

[Zan Dobersek]

Committed r173052: <http://trac.webkit.org/changeset/173052>