Bug 39102 – [GTK] eventSender.contextClick() should return the contents of the context menu

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Tony Chang]

I'm adding this in bug 25639 so we can test the context menu items.  This will need to be added to GTK DRT.

[Philippe Normand]

FWIW the DRT mac patch introducing the feature: http://trac.webkit.org/changeset/59585

[Philippe Normand]

*** Bug 45641 has been marked as a duplicate of this bug. ***

[Philippe Normand]

Created attachment 67534 [details]
proposed patch

[Eric Carlson]

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.

[Philippe Normand]

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 ;)

[Eric Carlson]

(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)));

[Philippe Normand]

Oh, yes indeed!

A g_free() is missing after the array has been updated. Thanks for spotting this!

[Philippe Normand]

Created attachment 67774 [details]
proposed patch

[Martin Robinson]

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.

[Philippe Normand]

Committed r67626: <http://trac.webkit.org/changeset/67626>

[Martin Robinson]

Comment on attachment 67774 [details]
proposed patch

Clearing flags as this landed in r67626.

[WebKit Review Bot]

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