Bug 186345 – [WPE] Add a MiniBrowser and use it to run WebDriver tests

• Component: WPE WebKit
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Add a MiniBrowser for WPE and stop using dyz. Having a testing browser in tree is a lot more convenience.

[Carlos Garcia Campos]

Created attachment 342043 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

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

[Zan Dobersek]

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.

[Carlos Garcia Campos]

Created attachment 342246 [details]
Updated patch

[EWS Watchlist]

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.

[Carlos Garcia Campos]

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.

[EWS Watchlist]

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.

[Carlos Alberto Lopez Perez]

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)

[Carlos Garcia Campos]

It works here under wayland.

[Carlos Garcia Campos]

It doesn't work for me either in weston, could it be because we use the stable xdg_shell instead of v6?

[EWS Watchlist]

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

[EWS Watchlist]

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

[Carlos Alberto Lopez Perez]

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

[Carlos Garcia Campos]

Ok, I'll go back to v6 version then.

[Carlos Garcia Campos]

Created attachment 342254 [details]
Updated patch

Go back to v6 to make it work under weston.

[Carlos Alberto Lopez Perez]

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

[Carlos Alberto Lopez Perez]

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

[EWS Watchlist]

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.

[Zan Dobersek]

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

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Committed r232670: <https://trac.webkit.org/changeset/232670>