[webkit-reviews] review granted: [Bug 226662] [GTK] Replace gtk-doc with gi-docgen : [Attachment 455242] The “only missing ChangeLogs and doc-comments in CMake modules” patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at gnome.org> has granted Adrian Perez
<aperez at igalia.com>'s request for review:
Bug 226662: [GTK] Replace gtk-doc with gi-docgen
https://bugs.webkit.org/show_bug.cgi?id=226662

Attachment 455242: The “only missing ChangeLogs and doc-comments in CMake
modules” patch

https://bugs.webkit.org/attachment.cgi?id=455242&action=review




--- Comment #18 from Michael Catanzaro <mcatanzaro at gnome.org> ---
Comment on attachment 455242
  --> https://bugs.webkit.org/attachment.cgi?id=455242
The “only missing ChangeLogs and doc-comments in CMake modules” patch

View in context: https://bugs.webkit.org/attachment.cgi?id=455242&action=review

The patch is too big to practically review everything closely, but in general
things look fine. I see plenty of nice mistakes fixed throughout, though.

BTW the existing gtk-doc for webkit2gtk-5.0 is not parallel-installable with
the docs for webkit2gtk-4.0. I hope your patch incidentally fixes this problem.

> Source/WebKit/PlatformWPE.cmake:486
> +# XXX: Using ${JavaScriptCore_INSTALLED_HEADERS} here expands to nothing.

What's up with this?

> Source/WebKit/UIProcess/API/gtk/WebKitDefines.h:53
> +/**
> + * WEBKIT_DEPRECATED_FOR: (skip)
> + * @f: replacement symbol name
> + *
> + * Marks a symbol as deprecated, indicating a replacement.
> + */

I'm not super fond that we have to document this just to be able to (skip) it,
but if that's the best way to handle this, then OK.

> Source/cmake/FindGIDocgen.cmake:18
> +# Allow dropping a copy of the tool source under gi-docgen/, as it can run
> +# uninstalled, and also search gi-docgen/bin/ to allow placing a virtualenv
> +# in that location.
> +find_program(GIDocgen_EXE
> +    NAMES gi-docgen gi-docgen.py
> +    HINTS "${CMAKE_SOURCE_DIR}/gi-docgen/bin"
> +	     "${CMAKE_SOURCE_DIR}/gi-docgen"
> +    DOC "Path to the gi-docgen program"
> +    REQUIRED
> +)

So if I understand correctly: gi-docgen can be a system dependency, OR it could
be bundled in the source directory, but if so it has to be copied there
manually prior to the build: it's not going to be provided by WebKit itself?

> Source/cmake/OptionsGTK.cmake:83
> +WEBKIT_OPTION_DEPEND(ENABLE_DOCUMENTATION ENABLE_INTROSPECTION)
>  WEBKIT_OPTION_DEPEND(ENABLE_3D_TRANSFORMS USE_OPENGL_OR_ES)
>  WEBKIT_OPTION_DEPEND(ENABLE_ASYNC_SCROLLING USE_OPENGL_OR_ES)

This should be alphabetized (under ENABLE_ASYNC_SCROLLING). Currently
USE_ANGLE_WEBGL is the only other option out of place here.