Bug 67660 - [GTK] Add a way to expose well known names for items in the default context menu
Summary: [GTK] Add a way to expose well known names for items in the default context menu
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords:
Depends on: 49904
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-06 11:22 PDT by Carlos Garcia Campos
Modified: 2012-03-23 04:14 PDT (History)
1 user (show)

See Also:


Attachments
Patch (14.40 KB, patch)
2011-09-06 11:28 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (28.59 KB, patch)
2011-09-07 06:45 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch removing need for const_cast (30.21 KB, patch)
2011-09-08 10:39 PDT, Martin Robinson
mrobinson: review+
mrobinson: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2011-09-06 11:22:47 PDT
It would make easier to create a custom context menu by using the default one, instead of creating new one.
Comment 1 Carlos Garcia Campos 2011-09-06 11:28:13 PDT
Created attachment 106450 [details]
Patch
Comment 2 Martin Robinson 2011-09-06 19:46:13 PDT
Comment on attachment 106450 [details]
Patch

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

I like this patch a lot. I think it has the potential to remove a *ton* of code from Epiphany and other WebKitGTK+ clients. I'm giving the first r+ here, but we need one more to approve the new API. Also, I think this same approach should be used for WebKit2.

> Source/WebKit/gtk/ChangeLog:3
> +        Add webkit_context_menu_item_get_action

I think this one is extra...it should be part of the description below.

> Source/WebKit/gtk/WebCoreSupport/ContextMenuClientGtk.cpp:113
> +        g_object_set_data(G_OBJECT(bidiMenuItem.gtkAction()), "gtk-unicode-menu-entry", (gpointer)&bidi_menu_entries[i]);

I'm almost certain you shouldn't need to cast to gpointer here. If you do though, you should use C++ style casts.

> Source/WebKit/gtk/webkit/webkitglobals.cpp:306
> + * can be used to know the items present in the default context menu.

know -> determine

> Source/WebKit/gtk/webkit/webkitglobals.cpp:308
> + * WebKitWebView:context-menu signal:

The second : should be a '.'

> Source/WebKit/gtk/webkit/webkitglobals.cpp:340
> + *     GList* items = gtk_container_get_children (GTK_CONTAINER (default_menu));
> + *     GList* l;
> + *     GtkAction *action;
> + *
> + *     for (l = items; l; l = g_list_next (l)) {
> + *         GtkMenuItem* item = (GtkMenuItem *)l->data;
> + *
> + *         if (GTK_IS_SEPARATOR_MENU_ITEM (item)) {
> + *             /* It's  separator, do nothing */
> + *             continue;
> + *         }
> + *
> + *         switch (webkit_context_menu_item_get_action (item)) {
> + *         case WEBKIT_CONTEXT_MENU_ACTION_NO_ACTION:
> + *             /* No action for this item */
> + *             break;
> + *         /* Don't allow to ope links from context menu */
> + *         case WEBKIT_CONTEXT_MENU_ACTION_OPEN_LINK:
> + *         case WEBKIT_CONTEXT_MENU_ACTION_OPEN_LINK_IN_NEW_WINDOW:
> + *             action = gtk_activatable_get_related_action (GTK_ACTIVATABLE (item));
> + *             gtk_action_set_sensitive (action, FALSE);
> + *             break;

Awesome documentation.

> Source/WebKit/gtk/webkit/webkitglobals.cpp:346
> + *             /* Do somethig with submenu */

Hrm. Maybe expand this comment. Can the embedder crawl this menu too?

> Source/WebKit/gtk/webkit/webkitglobals.cpp:362
> +WebKitContextMenuAction webkit_context_menu_item_get_action (GtkMenuItem* item)

Extra space after webkit_context_menu_item_get_action.
Comment 3 Carlos Garcia Campos 2011-09-07 00:13:33 PDT
(In reply to comment #2)
> (From update of attachment 106450 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106450&action=review
> 
> I like this patch a lot. I think it has the potential to remove a *ton* of code from Epiphany and other WebKitGTK+ clients. I'm giving the first r+ here, but we need one more to approve the new API. Also, I think this same approach should be used for WebKit2.
> 
> > Source/WebKit/gtk/ChangeLog:3
> > +        Add webkit_context_menu_item_get_action
> 
> I think this one is extra...it should be part of the description below.

Yes, it's the git commit message that prepare-Changelog leaves there, and I forgot to remove it :-P

> > Source/WebKit/gtk/WebCoreSupport/ContextMenuClientGtk.cpp:113
> > +        g_object_set_data(G_OBJECT(bidiMenuItem.gtkAction()), "gtk-unicode-menu-entry", (gpointer)&bidi_menu_entries[i]);
> 
> I'm almost certain you shouldn't need to cast to gpointer here. If you do though, you should use C++ style casts.

This code was already there, I'll check if we can avoid the cast or use a C++ style cast.

> > Source/WebKit/gtk/webkit/webkitglobals.cpp:306
> > + * can be used to know the items present in the default context menu.
> 
> know -> determine

Ok.

> > Source/WebKit/gtk/webkit/webkitglobals.cpp:308
> > + * WebKitWebView:context-menu signal:
> 
> The second : should be a '.'

Ok.

> > Source/WebKit/gtk/webkit/webkitglobals.cpp:340
> > + *     GList* items = gtk_container_get_children (GTK_CONTAINER (default_menu));
> > + *     GList* l;
> > + *     GtkAction *action;
> > + *
> > + *     for (l = items; l; l = g_list_next (l)) {
> > + *         GtkMenuItem* item = (GtkMenuItem *)l->data;
> > + *
> > + *         if (GTK_IS_SEPARATOR_MENU_ITEM (item)) {
> > + *             /* It's  separator, do nothing */
> > + *             continue;
> > + *         }
> > + *
> > + *         switch (webkit_context_menu_item_get_action (item)) {
> > + *         case WEBKIT_CONTEXT_MENU_ACTION_NO_ACTION:
> > + *             /* No action for this item */
> > + *             break;
> > + *         /* Don't allow to ope links from context menu */
> > + *         case WEBKIT_CONTEXT_MENU_ACTION_OPEN_LINK:
> > + *         case WEBKIT_CONTEXT_MENU_ACTION_OPEN_LINK_IN_NEW_WINDOW:
> > + *             action = gtk_activatable_get_related_action (GTK_ACTIVATABLE (item));
> > + *             gtk_action_set_sensitive (action, FALSE);
> > + *             break;
> 
> Awesome documentation.
> 
> > Source/WebKit/gtk/webkit/webkitglobals.cpp:346
> > + *             /* Do somethig with submenu */
> 
> Hrm. Maybe expand this comment. Can the embedder crawl this menu too?

Right, I'll add an example of how to add a new menu item.

> > Source/WebKit/gtk/webkit/webkitglobals.cpp:362
> > +WebKitContextMenuAction webkit_context_menu_item_get_action (GtkMenuItem* item)
> 
> Extra space after webkit_context_menu_item_get_action.

Ok.

Thanks!
Comment 4 Carlos Garcia Campos 2011-09-07 06:45:18 PDT
Created attachment 106579 [details]
Updated patch

It fixes the issues pointed out by Martin (expect the cast, after fighting with C++ casts without any sucess I finally gave up), and it also adds a tests case for the context menu using the new api.
Comment 5 Martin Robinson 2011-09-08 10:39:06 PDT
Created attachment 106760 [details]
Patch removing need for const_cast
Comment 6 Gustavo Noronha (kov) 2012-02-16 14:54:48 PST
Comment on attachment 106760 [details]
Patch removing need for const_cast

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

You have my nod.

> Source/WebKit/gtk/webkit/webkitglobals.cpp:336
> + *         /* Don't allow to ope links from context menu */

s/ope/open/

> Source/WebKit/gtk/webkit/webkitglobals.h:59
> + * Since: 1.5.3

Will have to update the since tag =)
Comment 7 Martin Robinson 2012-02-16 15:33:34 PST
Comment on attachment 106760 [details]
Patch removing need for const_cast

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

Looks good! Let's do something like this for WebKit2 as well.

> Source/WebKit/gtk/tests/testcontextmenu.c:36
> +static TestInfo* testInfoNew(const char* data, guint flag)

Many of the asterisks are in the wrong place in this file. Can you just fix all those before landing?

> Source/WebKit/gtk/tests/testcontextmenu.c:40
> +    TestInfo* info;
> +
> +    info = g_slice_new(TestInfo);

Collapse this into one line.

> Source/WebKit/gtk/tests/testcontextmenu.c:102
> +    guint context;
> +    GList* items;
> +    GList* iter;

I would just define these where you first use them.

> Source/WebKit/gtk/tests/testcontextmenu.c:133
> +        break;
> +
> +    case WEBKIT_HIT_TEST_RESULT_CONTEXT_EDITABLE:

Extra newline here?
Comment 8 Carlos Garcia Campos 2012-02-17 02:49:08 PST
I can't land this yet, it depends on bug #49904.
Comment 9 Carlos Garcia Campos 2012-03-23 04:14:33 PDT
Committed r111847: <http://trac.webkit.org/changeset/111847>