Bug 222985

Summary: [GTK] Duplicate WebKitWebView::show-option-menu confuses introspection, should use --warn-error when building gir
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, aperez, berto, bugs-noreply, cgarcia, ews-watchlist, gustavo, gyuyoung.kim, mcatanzaro, ryuan.choi, sergio
Priority: P2    
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
See Also: https://bugs.webkit.org/show_bug.cgi?id=222803
Bug Depends on:    
Bug Blocks: 178901, 226662    
Attachments:
Description Flags
Patch
none
Patch none

Description Michael Catanzaro 2021-03-09 10:00:34 PST
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.
Comment 1 Michael Catanzaro 2021-06-09 11:32:17 PDT
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.
Comment 2 Michael Catanzaro 2021-06-09 11:38:12 PDT
Created attachment 430983 [details]
Patch
Comment 3 EWS Watchlist 2021-06-09 11:39:15 PDT
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
Comment 4 Michael Catanzaro 2021-06-09 11:40:51 PDT
(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.
Comment 5 Michael Catanzaro 2021-06-09 11:44:39 PDT
Created attachment 430984 [details]
Patch
Comment 6 Michael Catanzaro 2021-06-09 11:47:55 PDT
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.
Comment 7 Adrian Perez 2021-06-09 12:27:39 PDT
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 ;-)
Comment 8 Michael Catanzaro 2021-06-09 12:31:47 PDT
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....
Comment 9 EWS 2021-06-09 14:28:55 PDT
Found 1 new test failure: imported/w3c/web-platform-tests/navigation-timing/nav2_test_attributes_values.html
Comment 10 EWS 2021-06-09 16:44:47 PDT
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].
Comment 11 Michael Catanzaro 2022-09-20 14:08:48 PDT
*** Bug 206794 has been marked as a duplicate of this bug. ***