[WPE] Expose the WebKitOptionMenu APIs
Created attachment 387787 [details] Patch
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
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.
Created attachment 387915 [details] Patch
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.
Created attachment 388015 [details] Patch
(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.
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.
(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
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.
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).
Created attachment 388218 [details] Patch
Comment on attachment 388218 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388218&action=review > Source/WebKit/ChangeLog:61 > +2020-01-19 Zan Dobersek <zdobersek@igalia.com> 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.
Created attachment 388224 [details] Patch for landing
Comment on attachment 388224 [details] Patch for landing Clearing flags on attachment: 388224 Committed r254819: <https://trac.webkit.org/changeset/254819>
All reviewed patches have been landed. Closing bug.
<rdar://problem/58735364>
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.
(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