Bug 181423 – Merge sync and async code paths for getting context menus

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Merge sync and async code paths for getting context menus

[Alex Christensen]

Created attachment 330777 [details]
Patch

[Alex Christensen]

Could someone double check that context menus on linux work as before?

[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

[Michael Catanzaro]

(In reply to Alex Christensen from comment #2)
> Could someone double check that context menus on linux work as before?

I can look at it tomorrow, sure.

Note:

lib/libwebkit2gtk-4.0.so.37.26.1: error: undefined reference to 'WebKit::WebContextMenuListenerProxy::useContextMenuItems(WTF::Vector<WTF::Ref<WebKit::WebContextMenuItem, WTF::DumbPtrTraits<WebKit::WebContextMenuItem> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&)'
lib/libwebkit2gtk-4.0.so.37.26.1: error: undefined reference to 'WebKit::WebContextMenuListenerProxy::WebContextMenuListenerProxy(WebKit::WebContextMenuProxy*)'

[Alex Christensen]

Created attachment 330794 [details]
Patch

[Michael Catanzaro]

(In reply to Alex Christensen from comment #2)
> Could someone double check that context menus on linux work as before?

Yes, they're working fine!

[Joseph Pecoraro]

Comment on attachment 330794 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/API/glib/WebKitContextMenuClient.cpp:51
>          webkitWebViewPopulateContextMenu(m_webView, menuItems, hitTestResultData, variant.get());
> -        return true;
> +        contextMenuListener.useContextMenuItems({ });

So Glib does something special in `webkitWebViewPopulateContextMenu` instead of passing the new items to `contextMenuListener.useContextMenuItems`? I'm surprised this always passes an empty list to useContextMenuItems.

Edit: Oh I think I see, gtk/WebContextMenuProxyGtk.cpp calls this with a mostly stub WebContextMenuListenerProxy, since WebContextMenuProxyGtk::showContextMenuWithItems does nothing (and which you just removed). I suppose it could be a real WebContextMenuListenerProxy that does the webkitWebViewPopulateContextMenu call instead of happening here in the client? Seems like it would amount to doing the same thing in the end.

[Alex Christensen]

http://trac.webkit.org/r226789

[Radar WebKit Bug Importer]

<rdar://problem/36444419>

[Alex Christensen]

https://trac.webkit.org/changeset/226794/webkit

[WebKit Commit Bot]

Re-opened since this is blocked by bug 181564

[Alex Christensen]

This broke API tests 
  WebKit.ContextMenuImgWithVideo
  _WKDownload.RedirectedDownload

[Alex Christensen]

Comment on attachment 330794 [details]
Patch

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

> Source/WebKit/UIProcess/API/APIContextMenuClient.h:54
> +    virtual void getContextMenuFromProposedMenu(WebKit::WebPageProxy&, Vector<Ref<WebKit::WebContextMenuItem>>&& /* proposedMenu */, WebKit::WebContextMenuListenerProxy& listener, const WebKit::WebHitTestResultData&, API::Object* /* userData */) { listener.useContextMenuItems({ }); }

This broke the tests.  Instead of calling the listener with an empty list, I needed to move proposedMenu into the listener.  Oops.

[Alex Christensen]

Created attachment 331427 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r227001