Bug 80600 – [GTK] Add input methods submenu item to the default context menu for editable content

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Like wk1, we should include the input methods menu item in the default content menu when triggered for a content editable.

[Carlos Garcia Campos]

Created attachment 130830 [details]
Patch

[WebKit Review Bot]

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

[Gustavo Noronha (kov)]

Comment on attachment 130830 [details]
Patch

Attachment 130830 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11863379

[Carlos Garcia Campos]

(In reply to comment #3)
> (From update of attachment 130830 [details])
> Attachment 130830 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/11863379

It doesn't build because it depends on bug #80597

[Carlos Garcia Campos]

Btw, this patch implements some of the ideas I mentioned in the public mailing list regarding the context menu API. The idea is that the current Vector<ContextMenuItem> will be replaced by WebKitContextMenu API object that will be passed to the context-menu signal so that the user can customize it. After the signal emission WebKitContextMenu returns the list of items as a Vector of ContextMenuItem and we pupulate the context menu like this patch does.

[Martin Robinson]

Comment on attachment 130830 [details]
Patch

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

Looks good, though I think some of your private methods could be renamed slightly to make things clearer.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:839
> +    GtkSettings* settings = webView ? gtk_widget_get_settings(GTK_WIDGET(webView)) : gtk_settings_get_default();

Hrm. In what situations can webView be NULL? Perhaps this should just be ASSERT(webView).

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:855
> +    if (!imContext)
> +        return;

ASSERT(imContext) perhaps?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:867
> +void webkitWebViewContextMenu(WebKitWebView* webView, WKArrayRef wkProposedMenu, WKHitTestResultRef wkHitTestResult)

I think I would prefer this to be named something with a verb like webkitWebViewPopulateContextMenu.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:588
> +void webkitWebViewBaseSetActiveContextMenu(WebKitWebViewBase* webkitWebViewBase, WebContextMenuProxyGtk* contextMenuProxy)
> +{
> +    webkitWebViewBase->priv->activeContextMenu = contextMenuProxy;
> +}
> +
> +WebContextMenuProxyGtk* webkitWebViewBaseGetActiveContextMenu(WebKitWebViewBase* webkitWebViewBase)
> +{
> +    return webkitWebViewBase->priv->activeContextMenu;
> +}

I think this code might be a lot clearer if these methods were named webkitWebViewBaseSetActiveContextMenuProxy and webkitWebViewBaseGetActiveContextMenu, while the private member could be named activeContextMenuProxy. I was a bit confused for a while because the activeContextMenu is used to build new menus.

> Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:76
> +        ContextMenuItem menuitem = items.at(i).core();
> +        append(menuitem);

This could probably be one line.

[Carlos Garcia Campos]

(In reply to comment #6)
> (From update of attachment 130830 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130830&action=review
> 
> Looks good, though I think some of your private methods could be renamed slightly to make things clearer.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:839
> > +    GtkSettings* settings = webView ? gtk_widget_get_settings(GTK_WIDGET(webView)) : gtk_settings_get_default();
> 
> Hrm. In what situations can webView be NULL? Perhaps this should just be ASSERT(webView).

It simply can't happen, I guess I copied it from wk1 implementation where this made sense, I don't remember.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:855
> > +    if (!imContext)
> > +        return;
> 
> ASSERT(imContext) perhaps?

Ditto. This is impossible, the im context is created in the init func of the web view, so if we have a web view instance, we have an im context.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:867
> > +void webkitWebViewContextMenu(WebKitWebView* webView, WKArrayRef wkProposedMenu, WKHitTestResultRef wkHitTestResult)
> 
> I think I would prefer this to be named something with a verb like webkitWebViewPopulateContextMenu.

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:588
> > +void webkitWebViewBaseSetActiveContextMenu(WebKitWebViewBase* webkitWebViewBase, WebContextMenuProxyGtk* contextMenuProxy)
> > +{
> > +    webkitWebViewBase->priv->activeContextMenu = contextMenuProxy;
> > +}
> > +
> > +WebContextMenuProxyGtk* webkitWebViewBaseGetActiveContextMenu(WebKitWebViewBase* webkitWebViewBase)
> > +{
> > +    return webkitWebViewBase->priv->activeContextMenu;
> > +}
> 
> I think this code might be a lot clearer if these methods were named webkitWebViewBaseSetActiveContextMenuProxy and webkitWebViewBaseGetActiveContextMenu, while the private member could be named activeContextMenuProxy. I was a bit confused for a while because the activeContextMenu is used to build new menus.

Ok.

> > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:76
> > +        ContextMenuItem menuitem = items.at(i).core();
> > +        append(menuitem);
> 
> This could probably be one line.

[Carlos Garcia Campos]

Committed r120305: <http://trac.webkit.org/changeset/120305>