[WPE][GTK] Warning: WebKit2: Couldn't find 'run_async_javascript_function_in_world_finish' for the corresponding async function: 'run_async_javascript_function_in_world'

[mcatanzaro]

910ab18

[WPE][GTK] Warning: WebKit2: Couldn't find 'run_async_javascript_function_in_world_finish' for the corresponding async function: 'run_async_javascript_function_in_world'
https://bugs.webkit.org/show_bug.cgi?id=269377

Reviewed by Adrian Perez de Castro.

We need to use the new finish-func annotation so that language bindings
can figure out how to complete the async call, due to our nonstandard
naming for the finish function. It seems trying to reuse the same finish
function for multiple async calls was not such a good idea.

Unfortunately, with older gobject-introspection, we cannot use this
new annotation or the build will fail due to the unrecognized
annotation. So we will need to conditionalize the entire doc comment.

Finally, I've also fixed the nullability of the world_name parameter,
which was broken due to a missing colon.

* Source/WebKit/PlatformGTK.cmake:
* Source/WebKit/PlatformWPE.cmake:
* Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:
* Source/WebKit/UIProcess/API/glib/WebKitWebView.h.in:

Canonical link: https://commits.webkit.org/276012@main

fd574d7

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt		⏳ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim		✅ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 39ac9c7)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	❌ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt		❌ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	❌ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

[aperezdc]

Nice catch 👌🏼

[mcatanzaro]

Oh look, we cannot do it because older gobject-introspection does not recognize it.

[aperezdc]

Oh look, we cannot do it because older gobject-introspection does not recognize it.

Dang. Any ideas?

[mcatanzaro]

Two ideas:

• Move the doc comment from the source file to the header template. Have two different copies of the doc comment, conditionalized on GI_CHECK_VERSION(). Hope that the gi-scanner version matches the introspection header version. This is probably the best solution.
• Add a new function webkit_web_view_run_async_javascript_function_in_world_finish() that just calls webkit_web_view_run_javascript_in_world_finish(). Deprecate it immediately. Probably overkill.

[aperezdc]

Two ideas:

• Move the doc comment from the source file to the header template. Have two different copies of the doc comment, conditionalized on GI_CHECK_VERSION(). Hope that the gi-scanner version matches the introspection header version. This is probably the best solution.

Let's try this. The version should match, but we could even check the actual version of the scanner using g-ir-scanner --version if we want to be sure, in case the version of the .pc module wouldn't match the one of the actual program which, while rare, might happen during cross-compilation... although cross-builds typically disable introspection 🙃

We already have a way in Source/cmake/FindGI.cmake for extracting the version from g-ir-scanner --version so something like this could be used to have add a macro to be used for guarding usage of (finish-func ...):

diff --git a/Source/cmake/FindGI.cmake b/Source/cmake/FindGI.cmake
index fdc56b21b148..dfdb33624fde 100644
--- a/Source/cmake/FindGI.cmake
+++ b/Source/cmake/FindGI.cmake
@@ -36,6 +36,9 @@ command. The following variables will also be set:
   Path to the ``g-ir-scanner`` program.
 ``GI_COMPILER_EXE``
   Path to the ``g-ir-compiler`` program.
+``GI_SCANNER_VERSION``
+  Version reported by ``g-ir-scanner --version``, typically the same as
+  ``GI_VERSION``.
 ``GI_GIRDIR``
   Path where to install ``.gir`` files in the target system.
 ``GI_TYPELIBDIR``
@@ -109,19 +112,21 @@ endif ()
 set(GI_GIRDIR "${_GI_GIRDIR}" CACHE PATH "Path to installed .gir files")
 set(GI_TYPELIBDIR "${_GI_TYPELIBDIR}" CACHE PATH "Path to installed .typelib files")
 
-if (NOT GI_VERSION AND GI_SCANNER_EXE)
-    execute_process(
-        COMMAND "${GI_SCANNER_EXE}" --version
-        OUTPUT_VARIABLE GI_VERSION
-        OUTPUT_STRIP_TRAILING_WHITESPACE
-        COMMAND_ERROR_IS_FATAL ANY
-        ERROR_QUIET
-    )
-    if (GI_VERSION MATCHES "^g-ir-scanner[[:space:]]+([0-9.]+)")
-        set(GI_VERSION ${CMAKE_MATCH_1})
-    else ()
-        unset(GI_VERSION)
-    endif ()
+execute_process(
+    COMMAND "${GI_SCANNER_EXE}" --version
+    OUTPUT_VARIABLE GI_SCANNER_VERSION
+    OUTPUT_STRIP_TRAILING_WHITESPACE
+    COMMAND_ERROR_IS_FATAL ANY
+    ERROR_QUIET
+)
+if (GI_SCANNER_VERSION MATCHES "^g-ir-scanner[[:space:]]+([0-9.]+)")
+    set(GI_SCANNER_VERSION ${CMAKE_MATCH_1})
+else ()
+    unset(GI_SCANNER_VERSION)
+endif ()
+
+if (NOT GI_VERSION AND GI_SCANNER_VERSION)
+    set(GI_VERSION ${GI_SCANNER_VERSION})
 endif ()
 
 if (GI_VERSION VERSION_GREATER_EQUAL 1.59.1)
@@ -129,6 +134,11 @@ if (GI_VERSION VERSION_GREATER_EQUAL 1.59.1)
 else ()
     set(GI_HAVE_SOURCES_TOP_DIRS FALSE)
 endif ()
+if (GI_SCANNER_VERSION VERSION_GREATER 1.78.1)
+    set(GI_SCANNER_HAVE_FINISH_FUNC 1)
+else ()
+    set(GI_SCANNER_HAVE_FINISH_FUNC 0)
+endif ()
 
 include(FindPackageHandleStandardArgs)
 find_package_handle_standard_args(GI
@@ -354,6 +364,7 @@ function(GI_INTROSPECT namespace nsversion header)
             "--pkg-export=${opt_PACKAGE}-${nsversion}"
             "$<$<BOOL:${target_def}>:-D$<JOIN:${target_def},;-D>>"
             "$<$<BOOL:${target_inc}>:-I$<JOIN:${target_inc},;-I>>"
+            "-DGI_SCANNER_HAVE_FINISH_FUNC=${GI_SCANNER_HAVE_FINISH_FUNC}"
             ${scanner_flags}
             ${opt_OPTIONS}
             ${gir_srcs}

WDYT?

[mcatanzaro]

I suppose that's even better.

At this point, do you want to prepare a pull request (since you've already done the hard part) or you want me to copy that into this one?

[aperezdc]

I suppose that's even better.

At this point, do you want to prepare a pull request (since you've already done the hard part) or you want me to copy that into this one?

Please go ahead, pick the diff I shared and incorporate it in this patch 👍🏼

[mcatanzaro]

 "-DGI_SCANNER_HAVE_FINISH_FUNC=${GI_SCANNER_HAVE_FINISH_FUNC}"

I'm not sure about this. Will gi-scanner be smart enough to respect the preprocessor definitions when parsing the source file? I'm skeptical. I had been thinking we'd move the doc comment to the header template and preprocess it with unifdef; that's how we've handled other cases where the doc comment needs to be conditionalized thus far.

[mcatanzaro]

OK, big brain moment: let's have only one doc comment annotated for newer gobject-introspection, and use GI_SCANNER_VERSION to decide whether the warnings should be fatal.

[mcatanzaro]

I think I have something, but I want to test it downstream first to be confident that it actually works.

[mcatanzaro]

if (GI_SCANNER_VERSION MATCHES "^g-ir-scanner[[:space:]]+([0-9.]+)")

This doesn't work.

[mcatanzaro]

OK, I've spent more time than planned on this. I'm just going to use GI_CHECK_VERSION().

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 9df9f33)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	❌ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt		❌ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	❌ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

[mcatanzaro]

Please review again.

[mcatanzaro]

Please review again.

Just kidding. This didn't work at all. I have yet another idea...

[mcatanzaro]

Sorry for the delay. This should be ready again now.

[webkit-early-warning-system]

EWS run on current version of this PR (hash fd574d7)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt		⏳ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim		✅ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

[aperezdc]

The repeated paragraph isn't great, but it seems this the reasonable approach with the current tooling we have 👌🏼

[webkit-commit-queue]

Committed 276012@main (910ab18): https://commits.webkit.org/276012@main

Reviewed commits have been landed. Closing PR #24427 and removing active labels.

[philn]

This broke builds with GI disabled...

CMake Error at Source/WebKit/PlatformWPE.cmake:279 (if):                                                                                                                                                           
  if given arguments:                                                                                                                                                                                              
                                                                                                                                                                                                                   
    "VERSION_GREATER_EQUAL" "1.79.2"                                                                                                                                                                               
                                                                                                                                                                                                                   
  Unknown arguments specified                                                                                                                                                                                      
Call Stack (most recent call first):                                                                                                                                                                               
  Source/cmake/WebKitMacros.cmake:74 (include)                                                                                                                                                                     
  Source/WebKit/CMakeLists.txt:727 (WEBKIT_INCLUDE_CONFIG_FILES_IF_EXISTS)

[aperezdc]

This broke builds with GI disabled...

CMake Error at Source/WebKit/PlatformWPE.cmake:279 (if):                                                                                                                                                           
  if given arguments:                                                                                                                                                                                              
                                                                                                                                                                                                                   
    "VERSION_GREATER_EQUAL" "1.79.2"                                                                                                                                                                               
                                                                                                                                                                                                                   
  Unknown arguments specified                                                                                                                                                                                      
Call Stack (most recent call first):                                                                                                                                                                               
  Source/cmake/WebKitMacros.cmake:74 (include)                                                                                                                                                                     
  Source/WebKit/CMakeLists.txt:727 (WEBKIT_INCLUDE_CONFIG_FILES_IF_EXISTS)

I'll fix it momentarily, thanks for the heads up.

[aperezdc]

Follow-up to fix the issue with GObject-Introspection disabled: #25811