[Webkit-unassigned] [Bug 80600] [GTK] Add input methods submenu item to the default context menu for editable content
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jun 8 16:28:50 PDT 2012
https://bugs.webkit.org/show_bug.cgi?id=80600
Martin Robinson <mrobinson at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #130830|review? |review+
Flag| |
--- Comment #6 from Martin Robinson <mrobinson at webkit.org> 2012-06-08 16:28:50 PST ---
(From update of attachment 130830)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list