Bug 39102 - [GTK] eventSender.contextClick() should return the contents of the context menu
Summary: [GTK] eventSender.contextClick() should return the contents of the context menu
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk, LayoutTestFailure
: 45641 (view as bug list)
Depends on:
Blocks: 45021
  Show dependency treegraph
 
Reported: 2010-05-13 19:20 PDT by Tony Chang
Modified: 2012-10-08 13:29 PDT (History)
7 users (show)

See Also:


Attachments
proposed patch (9.61 KB, patch)
2010-09-14 03:23 PDT, Philippe Normand
eric.carlson: review+
Details | Formatted Diff | Diff
proposed patch (9.64 KB, patch)
2010-09-15 23:39 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2010-05-13 19:20:33 PDT
I'm adding this in bug 25639 so we can test the context menu items.  This will need to be added to GTK DRT.
Comment 1 Philippe Normand 2010-09-13 05:31:35 PDT
FWIW the DRT mac patch introducing the feature: http://trac.webkit.org/changeset/59585
Comment 2 Philippe Normand 2010-09-13 05:32:08 PDT
*** Bug 45641 has been marked as a duplicate of this bug. ***
Comment 3 Philippe Normand 2010-09-14 03:23:21 PDT
Created attachment 67534 [details]
proposed patch
Comment 4 Eric Carlson 2010-09-15 07:50:40 PDT
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.
Comment 5 Philippe Normand 2010-09-15 07:59:21 PDT
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 ;)
Comment 6 Eric Carlson 2010-09-15 08:47:34 PDT
(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)));
Comment 7 Philippe Normand 2010-09-15 09:24:00 PDT
Oh, yes indeed!

A g_free() is missing after the array has been updated. Thanks for spotting this!
Comment 8 Philippe Normand 2010-09-15 23:39:10 PDT
Created attachment 67774 [details]
proposed patch
Comment 9 Martin Robinson 2010-09-16 08:32:01 PDT
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.
Comment 10 Philippe Normand 2010-09-16 09:07:57 PDT
Committed r67626: <http://trac.webkit.org/changeset/67626>
Comment 11 Martin Robinson 2010-09-16 09:09:44 PDT
Comment on attachment 67774 [details]
proposed patch

Clearing flags as this landed in r67626.