Summary: | [WPE] Add a MiniBrowser and use it to run WebDriver tests | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||||||
Component: | WPE WebKit | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | aperez, bugs-noreply, calvaris, clopez, ews-watchlist, mcatanzaro, zan | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 186346 | ||||||||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2018-06-06 04:58:02 PDT
Created attachment 342043 [details]
Patch
Attachment 342043 [details] did not pass style-queue:
ERROR: Tools/MiniBrowser/wpe/main.cpp:27: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Tools/MiniBrowser/wpe/main.cpp:33: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Tools/MiniBrowser/wpe/WlGlueHost.h:30: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Tools/MiniBrowser/wpe/WlGlueHost.h:35: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Tools/MiniBrowser/wpe/WlGlueHost.cpp:26: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
ERROR: Tools/MiniBrowser/wpe/WlGlueHost.cpp:27: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
ERROR: Tools/MiniBrowser/wpe/WlGlueWindow.cpp:26: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
ERROR: Tools/MiniBrowser/wpe/WlGlueWindow.cpp:27: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
Total errors found: 8 in 19 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 342043 [details] Patch Attachment 342043 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/8028792 New failing tests: stress/ftl-put-by-id-setter-exception-interesting-live-state.js.ftl-eager-no-cjit Comment on attachment 342043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342043&action=review This seems like a great idea. I've reviewed the CMake bits and the scripts, not the code. > Source/cmake/FindWaylandProtocols.cmake:48 > +macro(_WP_GET_PKGCONFIG_VAR _outvar _varname) > + execute_process( > + COMMAND ${PKG_CONFIG_EXECUTABLE} --variable=${_varname} wayland-protocols > + OUTPUT_VARIABLE _result > + RESULT_VARIABLE _null > + ) > + > + if (_null) > + else () > + string(REGEX REPLACE "[\r\n]" " " _result "${_result}") > + string(REGEX REPLACE " +$" "" _result "${_result}") > + separate_arguments(_result) > + set(${_outvar} ${_result} CACHE INTERNAL "") > + endif () > +endmacro(_WP_GET_PKGCONFIG_VAR) Did you write this yourself? Use pkg_get_variable() instead. See /usr/share/cmake/Modules/FindPkgConfig.cmake. > Source/cmake/FindWaylandProtocols.cmake:52 > + _wp_get_pkgconfig_var(WAYLAND_PROTOCOLS_DATADIR "pkgdatadir") > + find_program(WAYLAND_SCANNER NAMES wayland-scanner) I think you might need to call mark_as_advanced() to hide WAYLAND_PROTOCOLS_DATADIR and WAYLAND_SCANNER_NAMES from the list of build options... not sure. Run 'cmake -LH' in your build directory and scan the list of variables. If they show up, then it's needed. > Source/cmake/OptionsWPE.cmake:115 > + find_package(EGL) > + find_package(OpenGLES2) I don't understand, why are these not required? Surely it's not going to link successfully without them? I would expect some code to handle the case where they're missing if not marked as REQUIRED. > Tools/MiniBrowser/wpe/CMakeLists.txt:7 > + ${DERIVED_SOURCES_MINIBROWSER_DIR}/xdg-shell-unstable-v6-protocol.c Hm, xdg-shell is stable now, do you know how hard it would be to switch to the stable version of the protocol? Probably not much? This will start crashing once compositors drop support for v6. (In reply to Michael Catanzaro from comment #4) > Comment on attachment 342043 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=342043&action=review > > This seems like a great idea. I've reviewed the CMake bits and the scripts, > not the code. Thanks! > > Source/cmake/FindWaylandProtocols.cmake:48 > > +macro(_WP_GET_PKGCONFIG_VAR _outvar _varname) > > + execute_process( > > + COMMAND ${PKG_CONFIG_EXECUTABLE} --variable=${_varname} wayland-protocols > > + OUTPUT_VARIABLE _result > > + RESULT_VARIABLE _null > > + ) > > + > > + if (_null) > > + else () > > + string(REGEX REPLACE "[\r\n]" " " _result "${_result}") > > + string(REGEX REPLACE " +$" "" _result "${_result}") > > + separate_arguments(_result) > > + set(${_outvar} ${_result} CACHE INTERNAL "") > > + endif () > > +endmacro(_WP_GET_PKGCONFIG_VAR) > > Did you write this yourself? Use pkg_get_variable() instead. See > /usr/share/cmake/Modules/FindPkgConfig.cmake. Of course not, I copy-pasted like a monkey, which is the only way I know how to deal with cmake. > > Source/cmake/FindWaylandProtocols.cmake:52 > > + _wp_get_pkgconfig_var(WAYLAND_PROTOCOLS_DATADIR "pkgdatadir") > > + find_program(WAYLAND_SCANNER NAMES wayland-scanner) > > I think you might need to call mark_as_advanced() to hide > WAYLAND_PROTOCOLS_DATADIR and WAYLAND_SCANNER_NAMES from the list of build > options... not sure. Run 'cmake -LH' in your build directory and scan the > list of variables. If they show up, then it's needed. Ok, I'll check. > > Source/cmake/OptionsWPE.cmake:115 > > + find_package(EGL) > > + find_package(OpenGLES2) > > I don't understand, why are these not required? Surely it's not going to > link successfully without them? I would expect some code to handle the case > where they're missing if not marked as REQUIRED. Me neither, but I got a lot of linking errors without them. I'll mark them as REQUIRED. > > Tools/MiniBrowser/wpe/CMakeLists.txt:7 > > + ${DERIVED_SOURCES_MINIBROWSER_DIR}/xdg-shell-unstable-v6-protocol.c > > Hm, xdg-shell is stable now, do you know how hard it would be to switch to > the stable version of the protocol? Probably not much? This will start > crashing once compositors drop support for v6. No idea, this is what dyz uses, I'll try to use the stable version then. Comment on attachment 342043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342043&action=review >>> Source/cmake/OptionsWPE.cmake:115 >>> + find_package(EGL) >>> + find_package(OpenGLES2) >> >> I don't understand, why are these not required? Surely it's not going to link successfully without them? I would expect some code to handle the case where they're missing if not marked as REQUIRED. > > Me neither, but I got a lot of linking errors without them. I'll mark them as REQUIRED. Just use epoxy. > Source/cmake/OptionsWPE.cmake:122 > +if (ENABLE_MINIBROWSER) > + find_package(EGL) > + find_package(OpenGLES2) > + find_package(Wayland REQUIRED) > + find_package(WaylandProtocols 1.12 REQUIRED) > + if (NOT WAYLAND_PROTOCOLS_FOUND) > + message(FATAL_ERROR "WaylandProtocols is required to enabled WPE MiniBrowser.") > + endif () > +endif () > + You can search for all this in Tools/MiniBrowser/wpe/CMakeLists.txt. >>> Tools/MiniBrowser/wpe/CMakeLists.txt:7 >>> + ${DERIVED_SOURCES_MINIBROWSER_DIR}/xdg-shell-unstable-v6-protocol.c >> >> Hm, xdg-shell is stable now, do you know how hard it would be to switch to the stable version of the protocol? Probably not much? This will start crashing once compositors drop support for v6. > > No idea, this is what dyz uses, I'll try to use the stable version then. Go with the stable version, but I think it's simpler to copy in the protocol implementation file and the client header, instead of having to wrangle with wayland-protocols to get those two generated on-the-fly. > Tools/MiniBrowser/wpe/main.cpp:59 > +class FdoBackend final : public WlGlue::WindowClient { This, along with the WlGlue logic, would IMO fit better under Tools/wpe, alongside HeadlessViewBackend. Created attachment 342246 [details]
Updated patch
Attachment 342246 [details] did not pass style-queue:
ERROR: Tools/wpe/backends/WindowViewBackend.h:32: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Tools/wpe/backends/HeadlessViewBackend.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
ERROR: Tools/wpe/backends/HeadlessViewBackend.cpp:32: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Tools/wpe/backends/HeadlessViewBackend.cpp:33: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Tools/wpe/backends/ViewBackend.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
ERROR: Tools/wpe/backends/ViewBackend.cpp:108: Use 'WTFMove()' instead of 'std::move()'. [runtime/wtf_move] [4]
ERROR: Tools/MiniBrowser/wpe/main.cpp:27: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Tools/wpe/backends/WindowViewBackend.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
ERROR: Tools/wpe/backends/WindowViewBackend.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 9 in 30 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 342249 [details]
Try to fix the build in EWS
Builds for me locally, but it seems EGL_CAST is not defined in the bots.
Attachment 342249 [details] did not pass style-queue:
ERROR: Tools/wpe/backends/WindowViewBackend.h:32: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Tools/wpe/backends/HeadlessViewBackend.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
ERROR: Tools/wpe/backends/HeadlessViewBackend.cpp:32: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Tools/wpe/backends/HeadlessViewBackend.cpp:33: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Tools/wpe/backends/ViewBackend.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
ERROR: Tools/wpe/backends/ViewBackend.cpp:108: Use 'WTFMove()' instead of 'std::move()'. [runtime/wtf_move] [4]
ERROR: Tools/MiniBrowser/wpe/main.cpp:27: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Tools/wpe/backends/WindowViewBackend.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
ERROR: Tools/wpe/backends/WindowViewBackend.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 9 in 30 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 342249 [details] Try to fix the build in EWS I have tested it, and unfortunately is not working for me :( After running "Tools/Scripts/run-minibrowser --wpe http://google.com" the MB window never appears on the screen. I don't see any error logged, and the WebProcess is not crashed (its alive even when I see nothing) I'm trying to run it inside weston inside x11 (xfce) (which is something that currently works with dyz/-fdo) It works here under wayland. It doesn't work for me either in weston, could it be because we use the stable xdg_shell instead of v6? Comment on attachment 342249 [details] Try to fix the build in EWS Attachment 342249 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/8079598 New failing tests: http/tests/security/contentSecurityPolicy/video-redirect-blocked.html Created attachment 342250 [details]
Archive of layout-test-results from ews202 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
(In reply to Carlos Garcia Campos from comment #13) > It doesn't work for me either in weston, could it be because we use the > stable xdg_shell instead of v6? It seems that is the cause. Previous patch <https://bugs.webkit.org/attachment.cgi?id=342043> works fine for me (weston) Ok, I'll go back to v6 version then. Created attachment 342254 [details]
Updated patch
Go back to v6 to make it work under weston.
(In reply to Carlos Garcia Campos from comment #18) > Created attachment 342254 [details] > Updated patch > > Go back to v6 to make it work under weston. It works fine now for me :) Thanks! Comment on attachment 342254 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=342254&action=review > Tools/MiniBrowser/wpe/main.cpp:13 > + * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY <nitpick> This should be: THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND ANY Attachment 342254 [details] did not pass style-queue:
ERROR: Tools/wpe/backends/WindowViewBackend.h:32: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Tools/wpe/backends/HeadlessViewBackend.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
ERROR: Tools/wpe/backends/HeadlessViewBackend.cpp:32: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Tools/wpe/backends/HeadlessViewBackend.cpp:33: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Tools/wpe/backends/ViewBackend.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
ERROR: Tools/wpe/backends/ViewBackend.cpp:108: Use 'WTFMove()' instead of 'std::move()'. [runtime/wtf_move] [4]
ERROR: Tools/MiniBrowser/wpe/main.cpp:27: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Tools/wpe/backends/WindowViewBackend.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
ERROR: Tools/wpe/backends/WindowViewBackend.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 9 in 30 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Zan Dobersek from comment #6) > >>> Tools/MiniBrowser/wpe/CMakeLists.txt:7 > >>> + ${DERIVED_SOURCES_MINIBROWSER_DIR}/xdg-shell-unstable-v6-protocol.c > >> > >> Hm, xdg-shell is stable now, do you know how hard it would be to switch to the stable version of the protocol? Probably not much? This will start crashing once compositors drop support for v6. > > > > No idea, this is what dyz uses, I'll try to use the stable version then. > > Go with the stable version, but I think it's simpler to copy in the protocol > implementation file and the client header, instead of having to wrangle with > wayland-protocols to get those two generated on-the-fly. > Any specific reason why you don't pre-generate the XDG protocol sources? I don't want to add generated code to the repo. I think it's better to generate it as part of the build in DerivedSources like all other generated code. Committed r232670: <https://trac.webkit.org/changeset/232670> |