| Summary: | [GTK] Add a way to expose well known names for items in the default context menu | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||
| Component: | WebKitGTK | Assignee: | Martin Robinson <mrobinson> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | mrobinson | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Linux | ||||||||||
| Bug Depends on: | 49904 | ||||||||||
| Bug Blocks: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Carlos Garcia Campos
2011-09-06 11:22:47 PDT
Created attachment 106450 [details]
Patch
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. (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! 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.
Created attachment 106760 [details]
Patch removing need for const_cast
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 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? I can't land this yet, it depends on bug #49904. Committed r111847: <http://trac.webkit.org/changeset/111847> |