Bug 206287 – [WPE] Expose the WebKitOptionMenu APIs

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[WPE] Expose the WebKitOptionMenu APIs

[Zan Dobersek]

Created attachment 387787 [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 http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Comment on attachment 387787 [details]
Patch

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

I think this will be easier to review if we first move the common code to glib. You can do it unreviewed, since there aren't code changes right?

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2162
> +        G_TYPE_BOOLEAN, 1,
> +        WEBKIT_TYPE_OPTION_MENU);

Would it be possible to include the event for WPE too? The element area is quite important too, so that the user can properly position the menu.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestOptionMenu.cpp:62
> +#endif
> +
> +#if PLATFORM(WPE)

elif since they are mutually exclusive and it's the impl of the same thing.

[Zan Dobersek]

Created attachment 387915 [details]
Patch

[Zan Dobersek]

Comment on attachment 387787 [details]
Patch

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

>> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2162
>> +        WEBKIT_TYPE_OPTION_MENU);
> 
> Would it be possible to include the event for WPE too? The element area is quite important too, so that the user can properly position the menu.

The event probably isn't possible cause WPE doesn't have ATM a single event structure.

I'd want to add the area support later, once a new API type is available for it.

[Zan Dobersek]

Created attachment 388015 [details]
Patch

[Carlos Garcia Campos]

(In reply to Zan Dobersek from comment #5)
> Comment on attachment 387787 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387787&action=review
> 
> >> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2162
> >> +        WEBKIT_TYPE_OPTION_MENU);
> > 
> > Would it be possible to include the event for WPE too? The element area is quite important too, so that the user can properly position the menu.
> 
> The event probably isn't possible cause WPE doesn't have ATM a single event
> structure.
> 
> I'd want to add the area support later, once a new API type is available for
> it.

When is later? Because adding a new parameter to a signal would be an api break.

[Zan Dobersek]

Comment on attachment 387787 [details]
Patch

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

>>>> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2162
>>>> +        WEBKIT_TYPE_OPTION_MENU);
>>> 
>>> Would it be possible to include the event for WPE too? The element area is quite important too, so that the user can properly position the menu.
>> 
>> The event probably isn't possible cause WPE doesn't have ATM a single event structure.
>> 
>> I'd want to add the area support later, once a new API type is available for it.
> 
> When is later? Because adding a new parameter to a signal would be an api break.

Before the branching in two weeks.

[Carlos Garcia Campos]

(In reply to Zan Dobersek from comment #8)
> Comment on attachment 387787 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387787&action=review
> 
> >>>> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2162
> >>>> +        WEBKIT_TYPE_OPTION_MENU);
> >>> 
> >>> Would it be possible to include the event for WPE too? The element area is quite important too, so that the user can properly position the menu.
> >> 
> >> The event probably isn't possible cause WPE doesn't have ATM a single event structure.
> >> 
> >> I'd want to add the area support later, once a new API type is available for it.
> > 
> > When is later? Because adding a new parameter to a signal would be an api break.
> 
> Before the branching in two weeks.

Ok, I guess we can add a boxed type WebKitRectangle only for WPE, like we did for  colors with WebKitColor

[Carlos Garcia Campos]

Comment on attachment 388015 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2145
> +     * 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.

There's no @rectangle yet, but I guess it's ok to leave it since we are going to add it soon.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2148
> +     * The default signal handler will pop up a #GtkMenu.

Remove this.

> Source/WebKit/UIProcess/API/wpe/PageClientImpl.cpp:259
> -    return nullptr;
> +    return WebKitPopupMenu::create(m_view.webView(), page);

We should return nullptr if m_view.webView() is nullptr, because WebKitPopup assumes the view is never null. I think we should pass m_view here though, instead of the view.

> Source/WebKit/UIProcess/API/wpe/WPEView.cpp:47
> -View::View(struct wpe_view_backend* backend, const API::PageConfiguration& baseConfiguration)
> +View::View(WebKitWebView* webView, struct wpe_view_backend* backend, const API::PageConfiguration& baseConfiguration)

We normally use ViewClient to avoid this. So, we add showPopupMenu() to View and ViewClient and the view one sjust forwards the request to the client.

> Source/WebKit/UIProcess/API/wpe/WebKitPopupMenu.cpp:29
> +WebKitPopupMenu::WebKitPopupMenu(WebKitWebView* webView, WebPopupMenuProxy::Client& client)

So, this would receive a WKWPE::View instead of the web view.

> Source/WebKit/UIProcess/API/wpe/WebKitPopupMenu.cpp:42
> +void WebKitPopupMenu::showPopupMenu(const IntRect& rect, TextDirection direction, double pageScaleFactor, const Vector<WebPopupItem>& items, const PlatformPopupMenuData& platformData, int32_t selectedIndex)
> +{
> +    GRefPtr<WebKitOptionMenu> menu = adoptGRef(webkitOptionMenuCreate(*this, items, selectedIndex));

Here we would call m_view->showPopupMenu(rect, items, selectedIndex); and the WebKitWebView will build the WebKitOptionMenu and emit the signal. Since we need to keep the WebKitOptionMenu here we can make showPopupMenu() return the built menu or nullptr, using void* in the interface to avoid using glib api types in view client.

> Source/WebKit/UIProcess/API/wpe/WebKitWebView.h:252
>      void (*_webkit_reserved0) (void);

You have to remove one of the reserved.

[Zan Dobersek]

Comment on attachment 388015 [details]
Patch

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

>> Source/WebKit/UIProcess/API/wpe/WebKitPopupMenu.cpp:42
>> +    GRefPtr<WebKitOptionMenu> menu = adoptGRef(webkitOptionMenuCreate(*this, items, selectedIndex));
> 
> Here we would call m_view->showPopupMenu(rect, items, selectedIndex); and the WebKitWebView will build the WebKitOptionMenu and emit the signal. Since we need to keep the WebKitOptionMenu here we can make showPopupMenu() return the built menu or nullptr, using void* in the interface to avoid using glib api types in view client.

Would it make sense to pull the GLib-API-oriented WebViewClient implementation into a standalone header and implementation file, add necessary virtual methods to check for the type of the client at runtime, and then implement the popup menu functionality only for WebViewClient that has a usable WebKitWebView? This would also allow WebKitPopupMenu::create() to return null immediately if the WKView's client is not supported (i.e. not the WebViewClient implementation).

[Zan Dobersek]

Created attachment 388218 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 388218 [details]
Patch

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

> Source/WebKit/ChangeLog:61
> [phone]  Zan Dobersek  <[e-mail]>

double changelog

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2163
> +        G_TYPE_BOOLEAN, 1,
> +        WEBKIT_TYPE_OPTION_MENU);

Since we plan to add the rectangle later I would add the parameter here as a G_TYPE_POINTER. Then when adding the rectangle we just need to change the type.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2758
> +    g_signal_emit(webView, signals[SHOW_OPTION_MENU], 0, menu, &handled);

Here we would pass nullptr as the rectangle, and I would add a fixme comment too.

> Source/WebKit/UIProcess/API/wpe/WPEView.h:44
> +typedef struct _WebKitWebView WebKitWebView;

Do we need this here?

> Source/WebKit/UIProcess/API/wpe/WebKitOptionMenu.h:2
> + * Copyright (C) 2017 Igalia S.L.

2020

> Source/WebKit/UIProcess/API/wpe/WebKitOptionMenuItem.h:2
> + * Copyright (C) 2017 Igalia S.L.

2020

> Source/WebKit/UIProcess/API/wpe/WebKitPopupMenu.cpp:27
> +#include "WebKitWebViewPrivate.h"

Do we need this header here?

> Source/WebKit/UIProcess/API/wpe/WebKitPopupMenu.h:26
> +typedef struct _WebKitWebView WebKitWebView;

We don't need this.

> Source/WebKit/UIProcess/API/wpe/WebKitPopupMenu.h:50
> +    WebKitWebView* m_webView;

And this is now unused.

> Source/WebKit/UIProcess/API/wpe/WebKitWebView.h:250
> +    gboolean       (* show_option_menu)            (WebKitWebView               *web_view,
> +                                                    WebKitOptionMenu            *menu);

We would need to add a gpointer rectangle parameter here too.

[Zan Dobersek]

Created attachment 388224 [details]
Patch for landing

[Zan Dobersek]

Comment on attachment 388224 [details]
Patch for landing

Clearing flags on attachment: 388224

Committed r254819: <https://trac.webkit.org/changeset/254819>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58735364>

[Philippe Normand]

Since this landed I see this in my GTK build:

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

[Carlos Garcia Campos]

(In reply to Philippe Normand from comment #18)
> Since this landed I see this in my GTK build:
> 
> [66/89] Generating ../../WebKit2-4.0.gir
> ../../../../Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2137:
> Warning: WebKit2: multiple comment blocks documenting
> 'WebKitWebView::show-option-menu:' identifier (already seen at
> WebKitWebView.cpp:2101).
> ../../../../Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2137:
> Warning: WebKit2: incorrect number of parameters in comment block, parameter
> annotations will be ignored.

See bug #206794