I'm adding this in bug 25639 so we can test the context menu items. This will need to be added to GTK DRT.
FWIW the DRT mac patch introducing the feature: http://trac.webkit.org/changeset/59585
*** Bug 45641 has been marked as a duplicate of this bug. ***
Created attachment 67534 [details] proposed patch
Comment on attachment 67534 [details] proposed patch > + JSValueRef valueRef = JSObjectMakeArray(context, 0, NULL, NULL); > + WebKitWebView* view = webkit_web_frame_get_web_view(mainFrame); > + GtkMenu* gtkMenu = webkit_web_view_get_context_menu(view); > + if (gtkMenu) { > + GList* items = gtk_container_get_children(GTK_CONTAINER(gtkMenu)); > + JSValueRef arrayValues[g_list_length(items)]; > + int index = 0; > + for (GList* item = g_list_first(items); item; item = g_list_next(item)) { > + const gchar* label; > + if (GTK_IS_SEPARATOR_MENU_ITEM(item->data)) > + label = g_strdup("<separator>"); > + else > + label = g_strdup(gtk_menu_item_get_label(GTK_MENU_ITEM(item->data))); > + > + arrayValues[index] = JSValueMakeString(context, JSStringCreateWithUTF8CString(label)); > + index++; > + } > + if (index) > + valueRef = JSObjectMakeArray(context, index - 1, arrayValues, NULL); Aren't you leaking the label string allocated for arrayValues? I am definitely not a GTK expert so you might want to get another opinion, but this looks correct to me.
Well I don't think it's leaked but I'm very new to this JSValue stuff :) /*! @function @abstract Creates a JavaScript value of the string type. @param ctx The execution context to use. @param string The JSString to assign to the newly created JSValue. The newly created JSValue retains string, and releases it upon garbage collection. @result A JSValue of the string type, representing the value of string. */ JS_EXPORT JSValueRef JSValueMakeString(JSContextRef ctx, JSStringRef string); Xan, when you get time can you have a look at the patch please? A second eye on the patch of bug 45021 would also be very appreciated please ;)
(In reply to comment #5) > Well I don't think it's leaked but I'm very new to this JSValue stuff :) > > /*! > @function > @abstract Creates a JavaScript value of the string type. > @param ctx The execution context to use. > @param string The JSString to assign to the newly created JSValue. The > newly created JSValue retains string, and releases it upon garbage collection. > @result A JSValue of the string type, representing the value of string. > */ > JS_EXPORT JSValueRef JSValueMakeString(JSContextRef ctx, JSStringRef string); > Right, I mean the array of gchar*s you allocate with g_strdup: > + label = g_strdup("<separator>"); > + label = g_strdup(gtk_menu_item_get_label(GTK_MENU_ITEM(item->data)));
Oh, yes indeed! A g_free() is missing after the array has been updated. Thanks for spotting this!
Created attachment 67774 [details] proposed patch
Comment on attachment 67774 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=67774&action=prettypatch Thank you, thank you, thank you for implementing this missing DRT feature. I just have a few small issues. > WebKit/gtk/webkit/webkitwebview.cpp:4812 > + g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), NULL); This should be a 0. > WebKit/gtk/webkit/webkitwebview.cpp:4817 > + ContextMenu* menu = menuController->contextMenu(); I think it will be a lot cleaner to not cache these: ContextMenu* menu = page(webView)->menuController(); > WebKit/gtk/webkit/webkitwebview.cpp:4822 > + return NULL; This should be a 0. > WebKitTools/DumpRenderTree/gtk/EventSender.cpp:154 > + return JSObjectMakeArray(context, 0, NULL, NULL); These should be 0 per the style guidelines. > WebKitTools/DumpRenderTree/gtk/EventSender.cpp:162 > + if (gtkMenu) { I think this deserves to be it's own function. We can use an early return that way as well. > WebKitTools/DumpRenderTree/gtk/EventSender.cpp:167 > + gchar* label; I think we should use CString here, if we can. If not GOwnPtr. > WebKitTools/DumpRenderTree/gtk/EventSender.cpp:178 > + valueRef = JSObjectMakeArray(context, index - 1, arrayValues, NULL); Also should be 0.
Committed r67626: <http://trac.webkit.org/changeset/67626>
Comment on attachment 67774 [details] proposed patch Clearing flags as this landed in r67626.
http://trac.webkit.org/changeset/67626 might have broken Chromium Win Release The following changes are on the blame list: http://trac.webkit.org/changeset/67626 http://trac.webkit.org/changeset/67627 http://trac.webkit.org/changeset/67628 http://trac.webkit.org/changeset/67629 http://trac.webkit.org/changeset/67630 http://trac.webkit.org/changeset/67631