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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=80600





--- Comment #7 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-06-14 03:05:22 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.