<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:mcatanzaro@igalia.com" title="Michael Catanzaro <mcatanzaro@igalia.com>"> <span class="fn">Michael Catanzaro</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Add webkit_context_menu_item_new_from_gaction"
href="https://bugs.webkit.org/show_bug.cgi?id=159631">bug 159631</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #288043 Flags</td>
<td>review?
</td>
<td>review+, commit-queue-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Add webkit_context_menu_item_new_from_gaction"
href="https://bugs.webkit.org/show_bug.cgi?id=159631#c3">Comment # 3</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Add webkit_context_menu_item_new_from_gaction"
href="https://bugs.webkit.org/show_bug.cgi?id=159631">bug 159631</a>
from <span class="vcard"><a class="email" href="mailto:mcatanzaro@igalia.com" title="Michael Catanzaro <mcatanzaro@igalia.com>"> <span class="fn">Michael Catanzaro</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=288043&action=diff" name="attach_288043" title="Patch">attachment 288043</a> <a href="attachment.cgi?id=288043&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=288043&action=review">https://bugs.webkit.org/attachment.cgi?id=288043&action=review</a>
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.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:199
> +static void actionDataFree(ActionData* actionData, GObject* object)</span >
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*)
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:202
> + g_return_if_fail(G_IS_OBJECT(object));</span >
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?
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:205
> + if (actionData->parameter)
> + g_variant_unref(actionData->parameter);</span >
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
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:234
> + * Creates a new #WebKitContextMenuItem for the given action using the given @label.</span >
I would write: Creates a new #WebKitContextMenuItem for @action using the label @label.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:240
> +WebKitContextMenuItem* webkit_context_menu_item_new_from_gaction(GAction* action, GVariant* parameter, const gchar* label)</span >
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.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:245
> + ActionData* actionData = g_slice_new(ActionData);</span >
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).
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:542
> +class ContextMenuCustomGActionTest: public ContextMenuTest {</span >
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. :)
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:549
> + : m_itemToActivateLabel(0)
> + , m_activated(false)
> + , m_toggled(false)</span >
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.)
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:571
> + return 0;</span >
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.
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:587
> + return FALSE;</span >
return G_SOURCE_REMOVE (readable alias for FALSE)
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:592
> + m_activated = m_toggled = false;</span >
I think I prefer to write this on two lines.
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:615
> + void setAction(GAction* action, GVariant *parameter, const char* label)
> + {
> + m_action = action;</span >
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.
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:629
> + const char* m_label;
> + const char* m_itemToActivateLabel;
> + bool m_activated;
> + bool m_toggled;</span >
const char* m_label { nullptr };
const char* m_itemToActivateLabel { nullptr };
bool m_activated { false };
bool m_toggled { false };
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:1146
> + ContextMenuCustomGActionTest::add("WebKitWebView", "gaction-populate-menu", testContextMenuGActionPopulateMenu);</span >
Hm, how about "populate-menu-with-gaction" and "testContextMenuPopulateMenuWithGAction"?</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>