Bug 136220

Created attachment 237077 [details] Patch

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 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?

Created attachment 237103 [details] Patch

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 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 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 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

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 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.

Created attachment 237140 [details] Patch

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 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.

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