Bug 222985 – [GTK] Duplicate WebKitWebView::show-option-menu confuses introspection, should use --warn-error when building gir

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

Since bug #222803 we use --warn-error when generating the JavaScriptCore gir. We should do this for the WebKit gir as well, to ensure we don't release broken gir. Problem is we in fact currently releasing a broken gir, and must fix that first:

[29/72] Generating ../../WebKit2-4.0.gir
../../../../Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2207: Warning: WebKit2: multiple comment blocks documenting 'WebKitWebView::show-option-menu:' identifier (already seen at WebKitWebView.cpp:2171).
../../../../Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2207: Warning: WebKit2: incorrect number of parameters in comment block, parameter annotations will be ignored.

[Michael Catanzaro]

Looks like the WPE version of our introspection is winning:

      <glib:signal name="show-option-menu" when="last" version="2.28">
        <doc xml:space="preserve"
             filename="../Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp"
             line="2207">This signal is emitted when a select element in @web_view needs to display a
dropdown menu. This signal can be used to show a custom menu, using @menu to get
the details of all items that should be displayed. The area of the element in the
#WebKitWebView is given as @rectangle parameter, it can be used to position the
menu.
To handle this signal asynchronously you should keep a ref of the @menu.</doc>
        <return-value transfer-ownership="none">
          <doc xml:space="preserve"
               filename="../Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp"
               line="2220">%TRUE to stop other handlers from being invoked for the event.
  %FALSE to propagate the event further.</doc>
          <type name="gboolean" c:type="gboolean"/>
        </return-value>
        <parameters>
          <parameter name="object" transfer-ownership="none">
            <type name="OptionMenu"/>
          </parameter>
          <parameter name="p0" transfer-ownership="none">
            <type name="Gdk.Event"/>
          </parameter>
          <parameter name="p1" transfer-ownership="none">
            <type name="Gdk.Rectangle"/>
          </parameter>
        </parameters>
      </glib:signal>

So... that's bad.

I've tried various ways of passing -DPLATFORM_GTK to the introspection scanner, but I'm not having much luck. I got it to work by splitting the signal declaration out into the GTK and WPE files.

[Michael Catanzaro]

Created attachment 430983 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #1)
>           </parameter>
>           <parameter name="p0" transfer-ownership="none">
>             <type name="Gdk.Event"/>
>           </parameter>
>           <parameter name="p1" transfer-ownership="none">
>             <type name="Gdk.Rectangle"/>
>           </parameter>

Heh, it took the *documentation* from the WPE version of the signal, but it took the *parameter list* from the GTK version of the signal. So that could have been a lot worse. Let me revise the commit message.

[Michael Catanzaro]

Created attachment 430984 [details]
Patch

[Michael Catanzaro]

With this patch, the generated signal looks much better:

      <glib:signal name="show-option-menu" when="last" version="2.18">
        <doc xml:space="preserve"
             filename="../../Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp"
             line="471">This signal is emitted when a select element in @web_view needs to display a
dropdown menu. This signal can be used to show a custom menu, using @menu to get
the details of all items that should be displayed. The area of the element in the
#WebKitWebView is given as @rectangle parameter, it can be used to position the
menu. If this was triggered by a user interaction, like a mouse click,
@event parameter provides the #GdkEvent.
To handle this signal asynchronously you should keep a ref of the @menu.

The default signal handler will pop up a #GtkMenu.</doc>
        <return-value transfer-ownership="none">
          <doc xml:space="preserve"
               filename="../../Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp"
               line="488">%TRUE to stop other handlers from being invoked for the event.
  %FALSE to propagate the event further.</doc>
          <type name="gboolean" c:type="gboolean"/>
        </return-value>
        <parameters>
          <parameter name="menu" transfer-ownership="none">
            <doc xml:space="preserve"
                 filename="../../Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp"
                 line="474">the #WebKitOptionMenu</doc>
            <type name="OptionMenu"/>
          </parameter>
          <parameter name="event" transfer-ownership="none">
            <doc xml:space="preserve"
                 filename="../../Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp"
                 line="475">the #GdkEvent that triggered the menu, or %NULL</doc>
            <type name="Gdk.Event"/>
          </parameter>
          <parameter name="rectangle" transfer-ownership="none">
            <doc xml:space="preserve"
                 filename="../../Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp"
                 line="476">the option element area</doc>
            <type name="Gdk.Rectangle"/>
          </parameter>
        </parameters>
      </glib:signal>

It will break any code that relied on positional parameters with the broken names p0 and p1, but those are obviously-broken names that a reasonable developer should not rely on, and it's very unlikely to be a problem.

[Adrian Perez]

Comment on attachment 430984 [details]
Patch

Nice one, thanks for the patch!

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

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2177
> +    signals[SHOW_OPTION_MENU] = createShowOptionMenuSignal(webViewClass);

This looks a bit awkward, but makes sense. I suppose that's only because
it's the first and only place where this is being done ;-)

[Michael Catanzaro]

Comment on attachment 430984 [details]
Patch

I think we'll need this all over the place for GTK 3 vs. GTK 4, unless we can come up with something better....

[EWS]

Found 1 new test failure: imported/w3c/web-platform-tests/navigation-timing/nav2_test_attributes_values.html

[EWS]

Committed r278682 (238659@main): <https://commits.webkit.org/238659@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430984 [details].

[Michael Catanzaro]

*** Bug 206794 has been marked as a duplicate of this bug. ***