[Webkit-unassigned] [Bug 159631] [GTK] Add webkit_context_menu_item_new_from_gaction

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=159631

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #288043|review?                     |review+, commit-queue-
              Flags|                            |

--- Comment #3 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 288043
  --> https://bugs.webkit.org/attachment.cgi?id=288043
Patch

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

Great job!

r+ means review pass, since my comments are all minor. cq- means commit-queue denied. You'll need to re-upload this patch to address my comments, but you can manually write "Reviewed by Michael Catanzaro" and don't need to set the r? flag again, just cq?. Still, since this patch adds new API, it needs to be approved by a second GTK+ reviewer before we can commit it. Carlos Garcia will look at it soon.

Also, I forgot to suggest this earlier, but we should also deprecate webkit_context_menu_item_new(), since it's impossible to use without using deprecated GtkAction. You can look at webkit_web_context_set_disk_cache_directory() for an example of how to deprecate a method; it would look like this:

Deprecated: 2.16. Use webkit_context_menu_item_new_from_gaction() instead.

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:199
> +static void actionDataFree(ActionData* actionData, GObject* object)

I like the documented name of the second parameter: GObject* whereTheObjectWas. It makes it more clear that the object is being finalized and is not safe to use. But actually, you don't need this parameter at all (see my next comment), so you should leave it unnamed to avoid unused variable compiler warnings:

static void actionDataFree(ActionData* actionData, GObject*)

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:202
> +    g_return_if_fail(G_IS_OBJECT(object));

Hm, I'm slightly surprised this check passes, since object is being finalized after all. I would remove it. You don't use object anywhere in this function, and you know it's being finalized, so why check to see if it's valid?

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:205
> +    if (actionData->parameter)
> +        g_variant_unref(actionData->parameter);

This works, but try something more idiomatic: use a GRefPtr<GVariant> in the ActionData struct. Then you can delete these lines. Since you're not allocating the ActionData struct with normal C++ new and the struct would no longer contain only plain-old-data (POD) types, you'll have to manually invoke ActionData's destructor right before freeing the memory:

actionData->~ActionData();
g_slice_free(ActionData, actionData); // or free it some other way as appropriate, see comments below

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:234
> + * Creates a new #WebKitContextMenuItem for the given action using the given @label.

I would write: Creates a new #WebKitContextMenuItem for @action using the label @label.

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:240
> +WebKitContextMenuItem* webkit_context_menu_item_new_from_gaction(GAction* action, GVariant* parameter, const gchar* label)

OK, time for my only serious concern with this patch: what are the ownership semantics of the GAction parameter? Since you do not have any transfer annotation, the default is transfer full. Is that what you intended? Is it the best choice here? Since it is transfer full, you have to free it in actionDataFree, but it looks like you don't do that. If you intended it to be transfer none, you need to add the annotation.

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:245
> +    ActionData* actionData = g_slice_new(ActionData);

Instead of using GLib's "fast" allocator, which isn't very good, it's better to use WebKit's faster one (bmalloc):

ActionData* actionData = static_cast<ActionData*>(fastMalloc(sizeof(ActionData)));

Then you would free it with fastFree(actionData).

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:542
> +class ContextMenuCustomGActionTest: public ContextMenuTest {

Leave a space before the colon. I see you matched the style of the rest of this file, which is normally the right thing to do, but in this case the other examples in this file were wrong. :)

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:549
> +        : m_itemToActivateLabel(0)
> +        , m_activated(false)
> +        , m_toggled(false)

In new code we initialize these inline-style instead of in the constructor. I'll show you down below.

(And be careful to use nullptr for pointers, not 0.)

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:571
> +        return 0;

Do you really need the return for it to compile? GCC isn't smart enough to see it's unreachable? If you have to keep it, return nullptr rather than 0, since the return value is a pointer type.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:587
> +        return FALSE;

return G_SOURCE_REMOVE (readable alias for FALSE)

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:592
> +        m_activated = m_toggled = false;

I think I prefer to write this on two lines.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:615
> +    void setAction(GAction* action, GVariant *parameter, const char* label)
> +    {
> +        m_action = action;

Add another check: assertObjectIsDeletedWhenTestFinishes(G_OBJECT(action)). Does it pass?

Also, careful to write GVariant* parameter rather than GVariant *parameter while we're in WebKit code rather than GNOME code.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:629
> +    const char* m_label;
> +    const char* m_itemToActivateLabel;
> +    bool m_activated;
> +    bool m_toggled;

const char* m_label { nullptr };
const char* m_itemToActivateLabel { nullptr };
bool m_activated { false };
bool m_toggled { false };

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:1146
> +    ContextMenuCustomGActionTest::add("WebKitWebView", "gaction-populate-menu", testContextMenuGActionPopulateMenu);

Hm, how about "populate-menu-with-gaction" and "testContextMenuPopulateMenuWithGAction"?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160906/7de6b1f1/attachment.html>