Bug 136220 - [GTK] Add WaylandSurface
Summary: [GTK] Add WaylandSurface
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 04:11 PDT by Zan Dobersek
Modified: 2014-08-28 01:04 PDT (History)
8 users (show)

See Also:


Attachments
Patch (6.50 KB, patch)
2014-08-25 04:32 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (7.45 KB, patch)
2014-08-25 13:14 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (585.12 KB, application/zip)
2014-08-25 19:20 PDT, Build Bot
no flags Details
Patch (7.54 KB, patch)
2014-08-26 01:43 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 04:11:14 PDT
[GTK] Add WaylandSurface
Comment 1 Zan Dobersek 2014-08-25 04:32:22 PDT
Created attachment 237077 [details]
Patch
Comment 2 WebKit Commit Bot 2014-08-25 04:33:22 PDT
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.
Comment 3 Martin Robinson 2014-08-25 13:00:03 PDT
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?
Comment 4 Zan Dobersek 2014-08-25 13:14:23 PDT
Created attachment 237103 [details]
Patch
Comment 5 WebKit Commit Bot 2014-08-25 13:15:33 PDT
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.
Comment 6 Zan Dobersek 2014-08-25 13:25:04 PDT
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.
Comment 7 Martin Robinson 2014-08-25 13:32:11 PDT
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. :)
Comment 8 Build Bot 2014-08-25 19:20:42 PDT
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
Comment 9 Build Bot 2014-08-25 19:20:46 PDT
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
Comment 10 Zan Dobersek 2014-08-26 00:53:40 PDT
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.
Comment 11 Zan Dobersek 2014-08-26 01:43:17 PDT
Created attachment 237140 [details]
Patch
Comment 12 WebKit Commit Bot 2014-08-26 01:45:06 PDT
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.
Comment 13 Martin Robinson 2014-08-26 08:29:41 PDT
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.
Comment 14 Zan Dobersek 2014-08-27 23:09:37 PDT
Committed r173052: <http://trac.webkit.org/changeset/173052>