[Webkit-unassigned] [Bug 69343] [GTK] Initial implementation of back forward list for WebKit2 GTK+ API

[bugzilla-daemon at webkit.org]

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





--- Comment #13 from Carlos Garcia Campos <cgarcia at igalia.com>  2011-10-13 03:32:13 PST ---
(In reply to comment #12)
> (From update of attachment 110491 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110491&action=review
> 
> Looks good. Only giving r- because of the memory leak. I guess you'll need to convert the unit test. Let me know if you want me to do that -- perfectly willing to since I created this mess.
>
> > Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:30
> > + * @Short_description: List of visited pages
> > + * @Title: WebKitBackForwardList
> > + * @See_also: #WebKitWebView, #WebKitBackForwardListItem
> 
> In our old documentation the character after '@' is not capitalized. I'm not sure if this matter, but for the sake of consistency, why don't we keep it a small character.

No, it doesn't matter, I'll change it anyway.

> > Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:33
> > + * go back and forward to the most recent page. Items are inserted
> 
> go back and forward to the most recent page -> used to navigate to recent pages ?

OK.

> > Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:42
> > + * specified item. All other methods returning #WebKitBackForwardListItem<!-- -->s
> 
> Does '<!-- -->' add a space? If so might be better to just write "one or more" before it instead of having a space and then an 's'

Of course not, it's needed to not break the link.

> > Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:108
> > + * Return value: (transfer none): a #WebKitWebHistoryItem,
> > + *    or %NULL if @back_forward_list is empty.
> 
> No comma necessary before or

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:124
> > + * Returns: (transfer none): the #WebKitBackForwardListItem
> > + *    preceding the current item, or %NULL.
> 
> Ditto. Please make this change below as well.

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:216
> > + *    items succeeding the current item
> 
> succeeding -> following ?

OK.

> > Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:232
> > + *    items succeeding the current item, limited by @limit
> 
> succeeding -> following

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardListItem.cpp:78
> > +typedef HashMap<WKBackForwardListItemRef, GRefPtr<WebKitBackForwardListItem> > HistoryItemsMap;
> > +
> > +WebKitBackForwardListItem* webkitBackForwardListItemGetOrCreate(WKBackForwardListItemRef wkListItem)
> > +{
> > +    DEFINE_STATIC_LOCAL(HistoryItemsMap, itemsMap, ());
> > +
> > +    if (!wkListItem)
> > +        return 0;
> > +
> > +    GRefPtr<WebKitBackForwardListItem> listItem = itemsMap.get(wkListItem);
> > +    if (listItem)
> > +        return listItem.get();
> > +
> > +    listItem = adoptGRef(WEBKIT_BACK_FORWARD_LIST_ITEM(g_object_new(WEBKIT_TYPE_BACK_FORWARD_LIST_ITEM, NULL)));
> > +    listItem->priv->wkListItem = wkListItem;
> > +    itemsMap.set(wkListItem, listItem);
> 
> This means that the items will leak indefinitely because there's a reference to the GObject in the HashMap and a reference to the WKItem in the GObject.

Yes, it's the same we are doing in webkit1.

> It's very common to to access the back forward list (for instance via the back and forward buttons in the browser). Perhaps there's a way not to leak memory here?

We are not leaking memory when returning items, since we don't transfer ownership of items to the caller, the only reference it's owned by the hashmap. 

> I wonder if it's useful to have a pile of GObjects that are g_object_unref'd in an idle? This could be quite useful across the entire library.

I don't see the point, we are caching list items, it's not a memory leak, we might limit the cache size, though. Items are cached by WebProcessProxy too, so at least the memory of the WK items won't be freed in any case.

> > Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardListItem.cpp:131
> > + * webkit_back_forward_list_item_get_original_uri:
> > + * @back_forward_list_item: a #WebKitWebHistoryItem
> > + *
> > + * Returns: the original URI of @back_forward_list_item
> 
> Do you mind explaining a bit more how the original URI differs from the URI?

Added a comment in get_uri() documentation, pointing to this one.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.