[webkit-reviews] review denied: [Bug 63445] [GTK] Add back/forward menu to MiniBrowser toolbar : [Attachment 98705] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jun 27 09:11:23 PDT 2011
Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 63445: [GTK] Add back/forward menu to MiniBrowser toolbar
https://bugs.webkit.org/show_bug.cgi?id=63445
Attachment 98705: Patch
https://bugs.webkit.org/attachment.cgi?id=98705&action=review
------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=98705&action=review
Looks good, just have a few suggestions.
> Tools/MiniBrowser/gtk/BrowserWindow.c:294
> + WKURLRef url = WKBackForwardListItemCopyURL(item);
> + gchar *name = WKURLGetCString(url);
> + WKRelease(url);
> +
> + WKStringRef title = WKBackForwardListItemCopyTitle(item);
> + gchar *label = WKStringGetCString(title);
> + WKRelease(title);
> +
> + return gtk_action_new(name, label, 0, 0);
This seems wrong. Either name and label are leaking or you are using them after
they are freed.
> Tools/MiniBrowser/gtk/BrowserWindow.c:325
> + WKBackForwardListItemRef item = WKArrayGetItemAtIndex(list, i);
> + GtkAction *action = createGtkActionFromBackForwardItem(item);
> + if (!action)
> + continue;
> +
> + g_object_set_data_full(G_OBJECT(action), "back-forward-list-item",
(gpointer)WKRetain(item), (GDestroyNotify)WKRelease);
> + g_signal_connect_swapped(action, "activate",
G_CALLBACK(browserWindowHistoryItemActivated), window);
> +
> + GtkWidget *menuItem = gtk_action_create_menu_item(action);
> + g_signal_connect_swapped(menuItem, "select",
G_CALLBACK(browserWindowHistoryItemSelected), window);
> + g_object_unref(action);
> +
> + gtk_menu_shell_prepend(GTK_MENU_SHELL(menu), menuItem);
> + gtk_widget_show(menuItem);
> + hasItems = TRUE;
Please split this out into a helper.
More information about the webkit-reviews
mailing list