Bug 63445 - [GTK] Add back/forward menu to MiniBrowser toolbar
Summary: [GTK] Add back/forward menu to MiniBrowser toolbar
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2011-06-27 05:14 PDT by Carlos Garcia Campos
Modified: 2011-06-30 06:03 PDT (History)
0 users

See Also:


Attachments
Patch (7.44 KB, patch)
2011-06-27 05:19 PDT, Carlos Garcia Campos
mrobinson: review-
Details | Formatted Diff | Diff
Updated patch according to review (7.88 KB, patch)
2011-06-27 09:43 PDT, Carlos Garcia Campos
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2011-06-27 05:14:07 PDT
We can use GtkMenuToolButton for the navigation toolbar widgets and attach a menu with the history items.
Comment 1 Carlos Garcia Campos 2011-06-27 05:19:01 PDT
Created attachment 98705 [details]
Patch
Comment 2 Martin Robinson 2011-06-27 09:11:24 PDT
Comment on attachment 98705 [details]
Patch

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.
Comment 3 Carlos Garcia Campos 2011-06-27 09:17:41 PDT
(In reply to comment #2)
> (From update of attachment 98705 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98705&action=review
> 
> Looks good, just have a few suggestions.

Thanks for reviewing!

> > 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.

They are indeed leaking :-P

> > 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.

Ok. Thanks.
Comment 4 Carlos Garcia Campos 2011-06-27 09:43:38 PDT
Created attachment 98739 [details]
Updated patch according to review
Comment 5 Martin Robinson 2011-06-28 09:35:15 PDT
Comment on attachment 98739 [details]
Updated patch according to review

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

> Tools/MiniBrowser/gtk/BrowserWindow.c:298
> +    gchar *name = WKURLGetCString(url);
> +    WKRelease(url);
> +
> +    WKStringRef title = WKBackForwardListItemCopyTitle(item);
> +    gchar *label = WKStringGetCString(title);
> +    WKRelease(title);
> +
> +    GtkAction *action = gtk_action_new(name, label, 0, 0);
> +    g_free(name);
> +    g_free(label);
> +
> +    return action;

I think it's more appropriate to use char and bare free here since these types aren't allocated by GLib.

> Tools/MiniBrowser/gtk/BrowserWindow.c:330
> +    guint i;
> +
> +    if (!list)
> +        return 0;
> +
> +    guint listCount = WKArrayGetSize(list);
> +    if (!listCount)
> +        return 0;
> +
> +    GtkWidget *menu = gtk_menu_new();
> +    gboolean hasItems = FALSE;
> +    for (i = 0; i < listCount; i++) {

Please declare i just above the for for loop instead of at the top.
Comment 6 Carlos Garcia Campos 2011-06-30 06:01:32 PDT
(In reply to comment #5)
> (From update of attachment 98739 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98739&action=review
> 
> > Tools/MiniBrowser/gtk/BrowserWindow.c:298
> > +    gchar *name = WKURLGetCString(url);
> > +    WKRelease(url);
> > +
> > +    WKStringRef title = WKBackForwardListItemCopyTitle(item);
> > +    gchar *label = WKStringGetCString(title);
> > +    WKRelease(title);
> > +
> > +    GtkAction *action = gtk_action_new(name, label, 0, 0);
> > +    g_free(name);
> > +    g_free(label);
> > +
> > +    return action;
> 
> I think it's more appropriate to use char and bare free here since these types aren't allocated by GLib.

Yes, they are, see WKStringGetCString() it uses g_malloc()

> > Tools/MiniBrowser/gtk/BrowserWindow.c:330
> > +    guint i;
> > +
> > +    if (!list)
> > +        return 0;
> > +
> > +    guint listCount = WKArrayGetSize(list);
> > +    if (!listCount)
> > +        return 0;
> > +
> > +    GtkWidget *menu = gtk_menu_new();
> > +    gboolean hasItems = FALSE;
> > +    for (i = 0; i < listCount; i++) {
> 
> Please declare i just above the for for loop instead of at the top.

Ok
Comment 7 Carlos Garcia Campos 2011-06-30 06:03:03 PDT
Committed r90109: <http://trac.webkit.org/changeset/90109>