Bug 186345 - [WPE] Add a MiniBrowser and use it to run WebDriver tests
Summary: [WPE] Add a MiniBrowser and use it to run WebDriver tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WPE WebKit (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 186346
  Show dependency treegraph
 
Reported: 2018-06-06 04:58 PDT by Carlos Garcia Campos
Modified: 2018-06-10 23:43 PDT (History)
7 users (show)

See Also:


Attachments
Patch (60.17 KB, patch)
2018-06-06 05:06 PDT, Carlos Garcia Campos
ews: commit-queue-
Details | Formatted Diff | Diff
Updated patch (80.88 KB, patch)
2018-06-08 02:36 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix the build in EWS (81.04 KB, patch)
2018-06-08 03:57 PDT, Carlos Garcia Campos
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.83 MB, application/zip)
2018-06-08 05:46 PDT, Build Bot
no flags Details
Updated patch (81.26 KB, patch)
2018-06-08 06:38 PDT, Carlos Garcia Campos
zan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2018-06-06 04:58:02 PDT
Add a MiniBrowser for WPE and stop using dyz. Having a testing browser in tree is a lot more convenience.
Comment 1 Carlos Garcia Campos 2018-06-06 05:06:08 PDT
Created attachment 342043 [details]
Patch
Comment 2 Build Bot 2018-06-06 05:08:33 PDT
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 3 Build Bot 2018-06-06 06:22:41 PDT
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 4 Michael Catanzaro 2018-06-06 09:05:25 PDT
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.
Comment 5 Carlos Garcia Campos 2018-06-06 23:43:25 PDT
(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 6 Zan Dobersek 2018-06-07 00:04:07 PDT
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.
Comment 7 Carlos Garcia Campos 2018-06-08 02:36:32 PDT
Created attachment 342246 [details]
Updated patch
Comment 8 Build Bot 2018-06-08 02:39:05 PDT
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.
Comment 9 Carlos Garcia Campos 2018-06-08 03:57:59 PDT
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.
Comment 10 Build Bot 2018-06-08 04:02:21 PDT
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 11 Carlos Alberto Lopez Perez 2018-06-08 04:42:10 PDT
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)
Comment 12 Carlos Garcia Campos 2018-06-08 05:26:28 PDT
It works here under wayland.
Comment 13 Carlos Garcia Campos 2018-06-08 05:31:45 PDT
It doesn't work for me either in weston, could it be because we use the stable xdg_shell instead of v6?
Comment 14 Build Bot 2018-06-08 05:46:39 PDT
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
Comment 15 Build Bot 2018-06-08 05:46:50 PDT
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
Comment 16 Carlos Alberto Lopez Perez 2018-06-08 05:55:23 PDT
(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)
Comment 17 Carlos Garcia Campos 2018-06-08 06:25:42 PDT
Ok, I'll go back to v6 version then.
Comment 18 Carlos Garcia Campos 2018-06-08 06:38:29 PDT
Created attachment 342254 [details]
Updated patch

Go back to v6 to make it work under weston.
Comment 19 Carlos Alberto Lopez Perez 2018-06-08 07:06:22 PDT
(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 20 Carlos Alberto Lopez Perez 2018-06-08 07:18:07 PDT
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
Comment 21 Build Bot 2018-06-08 07:19:15 PDT
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.
Comment 22 Zan Dobersek 2018-06-08 08:23:34 PDT
(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?
Comment 23 Carlos Garcia Campos 2018-06-08 08:35:48 PDT
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.
Comment 24 Carlos Garcia Campos 2018-06-10 23:43:27 PDT
Committed r232670: <https://trac.webkit.org/changeset/232670>