[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


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #110491|review?                     |review-
               Flag|                            |




--- Comment #12 from Martin Robinson <mrobinson at webkit.org>  2011-10-12 18:46:15 PST ---
(From update of attachment 110491)
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.

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

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

> 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

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

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

succeeding -> following ?

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

succeeding -> following

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

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

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