[Webkit-unassigned] [Bug 186345] [WPE] Add a MiniBrowser and use it to run WebDriver tests

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=186345

--- Comment #5 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180607/a3251a84/attachment-0001.html>