Summary: | [GTK3] Bring back usage of GtkMenu for context menus | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adrian Perez <aperez> | ||||||||||
Component: | WebKitGTK | Assignee: | Adrian Perez <aperez> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | berto, bugs-noreply, cgarcia, ews-watchlist, gustavo, jmason, mcatanzaro | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Local Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=211241 https://bugs.webkit.org/show_bug.cgi?id=211294 |
||||||||||||
Attachments: |
|
Description
Adrian Perez
2020-05-07 01:56:36 PDT
Created attachment 398817 [details]
WIP 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 398817 [details]
WIP Patch
LGTM
(In reply to Carlos Garcia Campos from comment #3) > Comment on attachment 398817 [details] > WIP Patch > > LGTM Thanks! I'll make the needed changes on the API tests so we can land this. IMO having a few helper functions at the top of the “WebContextMenuProxyGtk.cpp” does not look too bad :D Created attachment 399294 [details]
Patch
Comment on attachment 399294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399294&action=review > Source/WebKit/ChangeLog:8 > + WIP: Bug #211557 I guess this is no longer WIP. > Source/WebKit/UIProcess/gtk/WebContextMenuProxyGtk.cpp:39 > +#include "WebKitWebViewPrivate.h" Ideally this should only know about WebKitWebViewBase, I'm not sure if this code is ever reached by the C api, though. (In reply to Carlos Garcia Campos from comment #6) > Comment on attachment 399294 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=399294&action=review > > > Source/WebKit/ChangeLog:8 > > + WIP: Bug #211557 > > I guess this is no longer WIP. Indeed 😅️ > > Source/WebKit/UIProcess/gtk/WebContextMenuProxyGtk.cpp:39 > > +#include "WebKitWebViewPrivate.h" > > Ideally this should only know about WebKitWebViewBase, I'm not sure if this > code is ever reached by the C api, though. Yes, it's reached. Do you have any suggestion to avoid letting WebContextMenuProxyGtk know about WebKitWebView? I'll also try to come up with something. (In reply to Adrian Perez from comment #7) > (In reply to Carlos Garcia Campos from comment #6) > > Comment on attachment 399294 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=399294&action=review > > > > > Source/WebKit/ChangeLog:8 > > > + WIP: Bug #211557 > > > > I guess this is no longer WIP. > > Indeed 😅️ > > > > Source/WebKit/UIProcess/gtk/WebContextMenuProxyGtk.cpp:39 > > > +#include "WebKitWebViewPrivate.h" > > > > Ideally this should only know about WebKitWebViewBase, I'm not sure if this > > code is ever reached by the C api, though. > > Yes, it's reached. Do you have any suggestion to avoid letting > WebContextMenuProxyGtk know about WebKitWebView? I'll also try to > come up with something. Connect to close/deactive from the API instead like we did before the popover rework. (In reply to Carlos Garcia Campos from comment #8) > (In reply to Adrian Perez from comment #7) > > (In reply to Carlos Garcia Campos from comment #6) > > > Comment on attachment 399294 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=399294&action=review > > > > > > > Source/WebKit/ChangeLog:8 > > > > + WIP: Bug #211557 > > > > > > I guess this is no longer WIP. > > > > Indeed 😅️ > > > > > > Source/WebKit/UIProcess/gtk/WebContextMenuProxyGtk.cpp:39 > > > > +#include "WebKitWebViewPrivate.h" > > > > > > Ideally this should only know about WebKitWebViewBase, I'm not sure if this > > > code is ever reached by the C api, though. > > > > Yes, it's reached. Do you have any suggestion to avoid letting > > WebContextMenuProxyGtk know about WebKitWebView? I'll also try to > > come up with something. > > Connect to close/deactive from the API instead like we did before the > popover rework. That's precisely what I was trying to avoid, as it's ugly. How about adding a “WebContextMenuProxyGtk::connectDismissed(WTF::Function&&)” method? (In reply to Adrian Perez from comment #9) > (In reply to Carlos Garcia Campos from comment #8) > > (In reply to Adrian Perez from comment #7) > > > (In reply to Carlos Garcia Campos from comment #6) > > > > Comment on attachment 399294 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=399294&action=review > > > > > > > > > Source/WebKit/ChangeLog:8 > > > > > + WIP: Bug #211557 > > > > > > > > I guess this is no longer WIP. > > > > > > Indeed 😅️ > > > > > > > > Source/WebKit/UIProcess/gtk/WebContextMenuProxyGtk.cpp:39 > > > > > +#include "WebKitWebViewPrivate.h" > > > > > > > > Ideally this should only know about WebKitWebViewBase, I'm not sure if this > > > > code is ever reached by the C api, though. > > > > > > Yes, it's reached. Do you have any suggestion to avoid letting > > > WebContextMenuProxyGtk know about WebKitWebView? I'll also try to > > > come up with something. > > > > Connect to close/deactive from the API instead like we did before the > > popover rework. > > That's precisely what I was trying to avoid, as it's ugly. How about adding > a “WebContextMenuProxyGtk::connectDismissed(WTF::Function&&)” method? I don't see what's ugly there, I think it's the easiest solution. If you mean it's ugly because of the ifdefs, add a helper to connect to the signal and move the ifdef to the helper. (In reply to Carlos Garcia Campos from comment #10) > (In reply to Adrian Perez from comment #9) > > (In reply to Carlos Garcia Campos from comment #8) > > > (In reply to Adrian Perez from comment #7) > > > > (In reply to Carlos Garcia Campos from comment #6) > > > > > Comment on attachment 399294 [details] > > > > > Patch > > > > > > > > > > View in context: > > > > > https://bugs.webkit.org/attachment.cgi?id=399294&action=review > > > > > > > > > > > Source/WebKit/ChangeLog:8 > > > > > > + WIP: Bug #211557 > > > > > > > > > > I guess this is no longer WIP. > > > > > > > > Indeed 😅️ > > > > > > > > > > Source/WebKit/UIProcess/gtk/WebContextMenuProxyGtk.cpp:39 > > > > > > +#include "WebKitWebViewPrivate.h" > > > > > > > > > > Ideally this should only know about WebKitWebViewBase, I'm not sure if this > > > > > code is ever reached by the C api, though. > > > > > > > > Yes, it's reached. Do you have any suggestion to avoid letting > > > > WebContextMenuProxyGtk know about WebKitWebView? I'll also try to > > > > come up with something. > > > > > > Connect to close/deactive from the API instead like we did before the > > > popover rework. > > > > That's precisely what I was trying to avoid, as it's ugly. How about adding > > a “WebContextMenuProxyGtk::connectDismissed(WTF::Function&&)” method? > > I don't see what's ugly there, I think it's the easiest solution. If you > mean it's ugly because of the ifdefs, add a helper to connect to the signal > and move the ifdef to the helper. It's partly the ifdefs, but having a WebContextMenuProxyGtk::gtkWidget() accessor is something I don't like either. Created attachment 399367 [details]
Patch v2
This moves back connecting to the context menu widget signals to
WebKitWebView.cpp and WebKitWebViewBase.cpp, and uses a static
constant defined in WebContextMenuProxyGtk.cpp to avoid needing
more #ifdefs.
Comment on attachment 399367 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=399367&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2616 > + g_signal_emit(webView, signals[CONTEXT_MENU_DISMISSED], 0, NULL); NULL -> nullptr Created attachment 399461 [details]
Patch for landing
Committed r261735: <https://trac.webkit.org/changeset/261735> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399461 [details]. Looks great, thanks gents! *** Bug 211294 has been marked as a duplicate of this bug. *** |